Cache failed compilation when getting locals

CHANGELOG.md

@@ -9,6 +9,7 @@
     Bug #6066: addtopic "return" does not work from within script. No errors thrown
     Bug #6101: Disarming trapped unlocked owned objects isn't considered a crime
     Bug #6115: Showmap overzealous matching
+    Bug #6123: NPC with broken script freezes the game on hello
     Bug #6129: Player avatar not displayed correctly for large window sizes when GUI scaling active
     Bug #6131: Item selection in the avatar window not working correctly for large window sizes
     Bug #6133: Cannot reliably sneak or steal in the sight of the NPCs siding with player

apps/openmw/mwscript/locals.cpp

@@ -6,7 +6,6 @@
 #include <components/esm/locals.hpp>
 #include <components/debug/debuglog.hpp>
 #include <components/compiler/locals.hpp>
-#include <components/compiler/exception.hpp>
 
 #include "../mwbase/environment.hpp"
 #include "../mwbase/scriptmanager.hpp"
@@ -65,19 +64,12 @@ namespace MWScript
 
     bool Locals::hasVar(const std::string &script, const std::string &var)
     {
-        try
+        ensure (script);
-        {
-            ensure (script);
 
-            const Compiler::Locals& locals =
+        const Compiler::Locals& locals =
-                MWBase::Environment::get().getScriptManager()->getLocals(script);
+            MWBase::Environment::get().getScriptManager()->getLocals(script);
-            int index = locals.getIndex(var);
+        int index = locals.getIndex(var);
-            return (index != -1);
+        return (index != -1);
-        }
-        catch (const Compiler::SourceException&)
-        {
-            return false;
-        }
     }
 
     int Locals::getIntVar(const std::string &script, const std::string &var)
@@ -162,42 +154,36 @@ namespace MWScript
         if (!mInitialised)
             return false;
 
-        try
+        const Compiler::Locals& declarations =
-        {
+            MWBase::Environment::get().getScriptManager()->getLocals(script);
-            const Compiler::Locals& declarations =
-                MWBase::Environment::get().getScriptManager()->getLocals(script);
 
-            for (int i=0; i<3; ++i)
+        for (int i=0; i<3; ++i)
+        {
+            char type = 0;
+
+            switch (i)
             {
-                char type = 0;
+                case 0: type = 's'; break;
+                case 1: type = 'l'; break;
+                case 2: type = 'f'; break;
+            }
+
+            const std::vector<std::string>& names = declarations.get (type);
+
+            for (int i2=0; i2<static_cast<int> (names.size()); ++i2)
+            {
+                ESM::Variant value;
 
                 switch (i)
                 {
-                    case 0: type = 's'; break;
+                    case 0: value.setType (ESM::VT_Int); value.setInteger (mShorts.at (i2)); break;
-                    case 1: type = 'l'; break;
+                    case 1: value.setType (ESM::VT_Int); value.setInteger (mLongs.at (i2)); break;
-                    case 2: type = 'f'; break;
+                    case 2: value.setType (ESM::VT_Float); value.setFloat (mFloats.at (i2)); break;
                 }
 
-                const std::vector<std::string>& names = declarations.get (type);
+                locals.mVariables.emplace_back (names[i2], value);
-
-                for (int i2=0; i2<static_cast<int> (names.size()); ++i2)
-                {
-                    ESM::Variant value;
-
-                    switch (i)
-                    {
-                        case 0: value.setType (ESM::VT_Int); value.setInteger (mShorts.at (i2)); break;
-                        case 1: value.setType (ESM::VT_Int); value.setInteger (mLongs.at (i2)); break;
-                        case 2: value.setType (ESM::VT_Float); value.setFloat (mFloats.at (i2)); break;
-                    }
-
-                    locals.mVariables.emplace_back (names[i2], value);
-                }
             }
         }
-        catch (const Compiler::SourceException&)
-        {
-        }
 
         return true;
     }
@@ -206,72 +192,66 @@ namespace MWScript
     {
         ensure (script);
 
-        try
+        const Compiler::Locals& declarations =
+            MWBase::Environment::get().getScriptManager()->getLocals(script);
+
+        int index = 0, numshorts = 0, numlongs = 0;
+        for (unsigned int v=0; v<locals.mVariables.size();++v)
         {
-            const Compiler::Locals& declarations =
+            ESM::VarType type = locals.mVariables[v].second.getType();
-                MWBase::Environment::get().getScriptManager()->getLocals(script);
+            if (type == ESM::VT_Short)
-
+                ++numshorts;
-            int index = 0, numshorts = 0, numlongs = 0;
+            else if (type == ESM::VT_Int)
-            for (unsigned int v=0; v<locals.mVariables.size();++v)
+                ++numlongs;
-            {
-                ESM::VarType type = locals.mVariables[v].second.getType();
-                if (type == ESM::VT_Short)
-                    ++numshorts;
-                else if (type == ESM::VT_Int)
-                    ++numlongs;
-            }
-
-            for (std::vector<std::pair<std::string, ESM::Variant> >::const_iterator iter
-                = locals.mVariables.begin(); iter!=locals.mVariables.end(); ++iter,++index)
-            {
-                if (iter->first.empty())
-                {
-                    // no variable names available (this will happen for legacy, i.e. ESS-imported savegames only)
-                    try
-                    {
-                        if (index >= numshorts+numlongs)
-                            mFloats.at(index - (numshorts+numlongs)) = iter->second.getFloat();
-                        else if (index >= numshorts)
-                            mLongs.at(index - numshorts) = iter->second.getInteger();
-                        else
-                            mShorts.at(index) = iter->second.getInteger();
-                    }
-                    catch (std::exception& e)
-                    {
-                        Log(Debug::Error) << "Failed to read local variable state for script '"
-                                          << script << "' (legacy format): " << e.what()
-                                          << "\nNum shorts: " << numshorts << " / " << mShorts.size()
-                                          << " Num longs: " << numlongs << " / " << mLongs.size();
-                    }
-                }
-                else
-                {
-                    char type =  declarations.getType (iter->first);
-                    int index2 = declarations.getIndex (iter->first);
-
-                    // silently ignore locals that don't exist anymore
-                    if (type == ' ' || index2 == -1)
-                        continue;
-
-                    try
-                    {
-                        switch (type)
-                        {
-                            case 's': mShorts.at (index2) = iter->second.getInteger(); break;
-                            case 'l': mLongs.at (index2) = iter->second.getInteger(); break;
-                            case 'f': mFloats.at (index2) = iter->second.getFloat(); break;
-                        }
-                    }
-                    catch (...)
-                    {
-                        // ignore type changes
-                        /// \todo write to log
-                    }
-                }
-            }
         }
-        catch (const Compiler::SourceException&)
+
+        for (std::vector<std::pair<std::string, ESM::Variant> >::const_iterator iter
+            = locals.mVariables.begin(); iter!=locals.mVariables.end(); ++iter,++index)
         {
+            if (iter->first.empty())
+            {
+                // no variable names available (this will happen for legacy, i.e. ESS-imported savegames only)
+                try
+                {
+                    if (index >= numshorts+numlongs)
+                        mFloats.at(index - (numshorts+numlongs)) = iter->second.getFloat();
+                    else if (index >= numshorts)
+                        mLongs.at(index - numshorts) = iter->second.getInteger();
+                    else
+                        mShorts.at(index) = iter->second.getInteger();
+                }
+                catch (std::exception& e)
+                {
+                    Log(Debug::Error) << "Failed to read local variable state for script '"
+                                        << script << "' (legacy format): " << e.what()
+                                        << "\nNum shorts: " << numshorts << " / " << mShorts.size()
+                                        << " Num longs: " << numlongs << " / " << mLongs.size();
+                }
+            }
+            else
+            {
+                char type =  declarations.getType (iter->first);
+                int index2 = declarations.getIndex (iter->first);
+
+                // silently ignore locals that don't exist anymore
+                if (type == ' ' || index2 == -1)
+                    continue;
+
+                try
+                {
+                    switch (type)
+                    {
+                        case 's': mShorts.at (index2) = iter->second.getInteger(); break;
+                        case 'l': mLongs.at (index2) = iter->second.getInteger(); break;
+                        case 'f': mFloats.at (index2) = iter->second.getFloat(); break;
+                    }
+                }
+                catch (...)
+                {
+                    // ignore type changes
+                    /// \todo write to log
+                }
+            }
         }
     }
 }

apps/openmw/mwscript/scriptmanagerimp.cpp

@@ -170,14 +170,14 @@ namespace MWScript
         std::string name2 = Misc::StringUtils::lowerCase (name);
 
         {
-            ScriptCollection::iterator iter = mScripts.find (name2);
+            auto iter = mScripts.find (name2);
 
             if (iter!=mScripts.end())
                 return iter->second.mLocals;
         }
 
         {
-            std::map<std::string, Compiler::Locals>::iterator iter = mOtherLocals.find (name2);
+            auto iter = mOtherLocals.find (name2);
 
             if (iter!=mOtherLocals.end())
                 return iter->second;
@@ -192,10 +192,22 @@ namespace MWScript
             std::istringstream stream (script->mScriptText);
             Compiler::QuickFileParser parser (mErrorHandler, mCompilerContext, locals);
             Compiler::Scanner scanner (mErrorHandler, stream, mCompilerContext.getExtensions());
-            scanner.scan (parser);
+            try
+            {
+                scanner.scan (parser);
+            }
+            catch (const Compiler::SourceException&)
+            {
+                // error has already been reported via error handler
+                locals.clear();
+            }
+            catch (const std::exception& error)
+            {
+                Log(Debug::Error) << "Error: An exception has been thrown: " << error.what();
+                locals.clear();
+            }
 
-            std::map<std::string, Compiler::Locals>::iterator iter =
+            auto iter = mOtherLocals.emplace(name2, locals).first;
-                mOtherLocals.emplace(name2, locals).first;
 
             return iter->second;
         }