Merge branch 'verifiers' into 'master'

Complain about fishy local variable names, improve editor effect list handling

See merge request OpenMW/openmw!4160

apps/opencs/CMakeLists.txt

@@ -42,7 +42,7 @@ opencs_units (model/tools
     mandatoryid skillcheck classcheck factioncheck racecheck soundcheck regioncheck
     birthsigncheck spellcheck referencecheck referenceablecheck scriptcheck bodypartcheck
     startscriptcheck search searchoperation searchstage pathgridcheck soundgencheck magiceffectcheck
-    mergestages gmstcheck topicinfocheck journalcheck enchantmentcheck
+    mergestages gmstcheck topicinfocheck journalcheck enchantmentcheck effectlistcheck
     )
 
 opencs_hdrs (model/tools

apps/opencs/model/tools/effectlistcheck.cpp

@@ -0,0 +1,88 @@
+#include "effectlistcheck.hpp"
+
+#include <components/esm/attr.hpp>
+#include <components/esm3/effectlist.hpp>
+#include <components/esm3/loadingr.hpp>
+#include <components/esm3/loadmgef.hpp>
+#include <components/esm3/loadskil.hpp>
+
+#include <apps/opencs/model/doc/messages.hpp>
+#include <apps/opencs/model/world/universalid.hpp>
+
+namespace CSMTools
+{
+    void effectListCheck(
+        const std::vector<ESM::IndexedENAMstruct>& list, CSMDoc::Messages& messages, const CSMWorld::UniversalId& id)
+    {
+        if (list.empty())
+        {
+            messages.add(id, "No magic effects", "", CSMDoc::Message::Severity_Warning);
+            return;
+        }
+
+        size_t i = 1;
+        for (const ESM::IndexedENAMstruct& effect : list)
+        {
+            const std::string number = std::to_string(i);
+
+            // At the time of writing this effects, attributes and skills are mostly hardcoded
+            if (effect.mData.mEffectID < 0 || effect.mData.mEffectID >= ESM::MagicEffect::Length)
+                messages.add(id, "Effect #" + number + ": invalid effect ID", "", CSMDoc::Message::Severity_Error);
+            if (effect.mData.mSkill < -1 || effect.mData.mSkill >= ESM::Skill::Length)
+                messages.add(id, "Effect #" + number + ": invalid skill", "", CSMDoc::Message::Severity_Error);
+            if (effect.mData.mAttribute < -1 || effect.mData.mAttribute >= ESM::Attribute::Length)
+                messages.add(id, "Effect #" + number + ": invalid attribute", "", CSMDoc::Message::Severity_Error);
+
+            if (effect.mData.mRange < ESM::RT_Self || effect.mData.mRange > ESM::RT_Target)
+                messages.add(id, "Effect #" + number + ": invalid range", "", CSMDoc::Message::Severity_Error);
+
+            if (effect.mData.mArea < 0)
+                messages.add(id, "Effect #" + number + ": negative area", "", CSMDoc::Message::Severity_Error);
+
+            if (effect.mData.mDuration < 0)
+                messages.add(id, "Effect #" + number + ": negative duration", "", CSMDoc::Message::Severity_Error);
+
+            if (effect.mData.mMagnMin < 0)
+                messages.add(
+                    id, "Effect #" + number + ": negative minimum magnitude", "", CSMDoc::Message::Severity_Error);
+
+            if (effect.mData.mMagnMax < 0)
+                messages.add(
+                    id, "Effect #" + number + ": negative maximum magnitude", "", CSMDoc::Message::Severity_Error);
+            else if (effect.mData.mMagnMax == 0)
+                messages.add(
+                    id, "Effect #" + number + ": zero maximum magnitude", "", CSMDoc::Message::Severity_Warning);
+
+            if (effect.mData.mMagnMin > effect.mData.mMagnMax)
+                messages.add(id, "Effect #" + number + ": minimum magnitude is higher than maximum magnitude", "",
+                    CSMDoc::Message::Severity_Error);
+
+            ++i;
+        }
+    }
+
+    void ingredientEffectListCheck(
+        const ESM::Ingredient& ingredient, CSMDoc::Messages& messages, const CSMWorld::UniversalId& id)
+    {
+        bool hasEffects = false;
+
+        for (size_t i = 0; i < 4; i++)
+        {
+            if (ingredient.mData.mEffectID[i] == -1)
+                continue;
+
+            hasEffects = true;
+
+            const std::string number = std::to_string(i + 1);
+            if (ingredient.mData.mEffectID[i] < -1 || ingredient.mData.mEffectID[i] >= ESM::MagicEffect::Length)
+                messages.add(id, "Effect #" + number + ": invalid effect ID", "", CSMDoc::Message::Severity_Error);
+            if (ingredient.mData.mSkills[i] < -1 || ingredient.mData.mSkills[i] >= ESM::Skill::Length)
+                messages.add(id, "Effect #" + number + ": invalid skill", "", CSMDoc::Message::Severity_Error);
+            if (ingredient.mData.mAttributes[i] < -1 || ingredient.mData.mAttributes[i] >= ESM::Attribute::Length)
+                messages.add(id, "Effect #" + number + ": invalid attribute", "", CSMDoc::Message::Severity_Error);
+        }
+
+        if (!hasEffects)
+            messages.add(id, "No magic effects", "", CSMDoc::Message::Severity_Warning);
+    }
+}

apps/opencs/model/tools/effectlistcheck.hpp

@@ -0,0 +1,30 @@
+#ifndef CSM_TOOLS_EFFECTLISTCHECK_H
+#define CSM_TOOLS_EFFECTLISTCHECK_H
+
+#include <vector>
+
+namespace ESM
+{
+    struct IndexedENAMstruct;
+    struct Ingredient;
+}
+
+namespace CSMDoc
+{
+    class Messages;
+}
+
+namespace CSMWorld
+{
+    class UniversalId;
+}
+
+namespace CSMTools
+{
+    void effectListCheck(
+        const std::vector<ESM::IndexedENAMstruct>& list, CSMDoc::Messages& messages, const CSMWorld::UniversalId& id);
+    void ingredientEffectListCheck(
+        const ESM::Ingredient& ingredient, CSMDoc::Messages& messages, const CSMWorld::UniversalId& id);
+}
+
+#endif

apps/opencs/model/tools/enchantmentcheck.cpp

@@ -10,13 +10,14 @@
 #include <apps/opencs/model/world/idcollection.hpp>
 #include <apps/opencs/model/world/record.hpp>
 
-#include <components/esm3/effectlist.hpp>
 #include <components/esm3/loadench.hpp>
 
 #include "../prefs/state.hpp"
 
 #include "../world/universalid.hpp"
 
+#include "effectlistcheck.hpp"
+
 CSMTools::EnchantmentCheckStage::EnchantmentCheckStage(const CSMWorld::IdCollection<ESM::Enchantment>& enchantments)
     : mEnchantments(enchantments)
 {
@@ -54,47 +55,5 @@ void CSMTools::EnchantmentCheckStage::perform(int stage, CSMDoc::Messages& messa
     if (enchantment.mData.mCost > enchantment.mData.mCharge)
         messages.add(id, "Cost is higher than charge", "", CSMDoc::Message::Severity_Error);
 
-    if (enchantment.mEffects.mList.empty())
+    effectListCheck(enchantment.mEffects.mList, messages, id);
-    {
-        messages.add(id, "Enchantment doesn't have any magic effects", "", CSMDoc::Message::Severity_Warning);
-    }
-    else
-    {
-        std::vector<ESM::IndexedENAMstruct>::const_iterator effect = enchantment.mEffects.mList.begin();
-
-        for (size_t i = 1; i <= enchantment.mEffects.mList.size(); i++)
-        {
-            const std::string number = std::to_string(i);
-            // At the time of writing this effects, attributes and skills are hardcoded
-            if (effect->mData.mEffectID < 0 || effect->mData.mEffectID > 142)
-            {
-                messages.add(id, "Effect #" + number + " is invalid", "", CSMDoc::Message::Severity_Error);
-                ++effect;
-                continue;
-            }
-
-            if (effect->mData.mSkill < -1 || effect->mData.mSkill > 26)
-                messages.add(
-                    id, "Effect #" + number + " affected skill is invalid", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mAttribute < -1 || effect->mData.mAttribute > 7)
-                messages.add(
-                    id, "Effect #" + number + " affected attribute is invalid", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mRange < 0 || effect->mData.mRange > 2)
-                messages.add(id, "Effect #" + number + " range is invalid", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mArea < 0)
-                messages.add(id, "Effect #" + number + " area is negative", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mDuration < 0)
-                messages.add(id, "Effect #" + number + " duration is negative", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mMagnMin < 0)
-                messages.add(
-                    id, "Effect #" + number + " minimum magnitude is negative", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mMagnMax < 0)
-                messages.add(
-                    id, "Effect #" + number + " maximum magnitude is negative", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mMagnMin > effect->mData.mMagnMax)
-                messages.add(id, "Effect #" + number + " minimum magnitude is higher than maximum magnitude", "",
-                    CSMDoc::Message::Severity_Error);
-            ++effect;
-        }
-    }
 }

apps/opencs/model/tools/referenceablecheck.cpp

@@ -38,6 +38,8 @@
 #include "../world/record.hpp"
 #include "../world/universalid.hpp"
 
+#include "effectlistcheck.hpp"
+
 namespace ESM
 {
     class Script;
@@ -330,7 +332,8 @@ void CSMTools::ReferenceableCheckStage::potionCheck(
     CSMWorld::UniversalId id(CSMWorld::UniversalId::Type_Potion, potion.mId);
 
     inventoryItemCheck<ESM::Potion>(potion, messages, id.toString());
-    /// \todo Check magic effects for validity
+
+    effectListCheck(potion.mEffects.mList, messages, id);
 
     // Check that mentioned scripts exist
     scriptCheck<ESM::Potion>(potion, messages, id.toString());
@@ -565,6 +568,8 @@ void CSMTools::ReferenceableCheckStage::ingredientCheck(
 
     // Check that mentioned scripts exist
     scriptCheck<ESM::Ingredient>(ingredient, messages, id.toString());
+
+    ingredientEffectListCheck(ingredient, messages, id);
 }
 
 void CSMTools::ReferenceableCheckStage::creaturesLevListCheck(

apps/opencs/model/tools/spellcheck.cpp

@@ -13,6 +13,8 @@
 
 #include "../prefs/state.hpp"
 
+#include "effectlistcheck.hpp"
+
 CSMTools::SpellCheckStage::SpellCheckStage(const CSMWorld::IdCollection<ESM::Spell>& spells)
     : mSpells(spells)
 {
@@ -46,5 +48,5 @@ void CSMTools::SpellCheckStage::perform(int stage, CSMDoc::Messages& messages)
     if (spell.mData.mCost < 0)
         messages.add(id, "Spell cost is negative", "", CSMDoc::Message::Severity_Error);
 
-    /// \todo check data members that can't be edited in the table view
+    effectListCheck(spell.mEffects.mList, messages, id);
 }

apps/opencs/model/world/nestedcoladapterimp.hpp

@@ -319,12 +319,7 @@ namespace CSMWorld
             switch (subColIndex)
             {
                 case 0:
-                {
+                    return effect.mEffectID;
-                    if (effect.mEffectID >= 0 && effect.mEffectID < ESM::MagicEffect::Length)
-                        return effect.mEffectID;
-                    else
-                        throw std::runtime_error("Magic effects ID unexpected value");
-                }
                 case 1:
                 {
                     switch (effect.mEffectID)
@@ -354,12 +349,7 @@ namespace CSMWorld
                     }
                 }
                 case 3:
-                {
+                    return effect.mRange;
-                    if (effect.mRange >= 0 && effect.mRange <= 2)
-                        return effect.mRange;
-                    else
-                        throw std::runtime_error("Magic effects range unexpected value");
-                }
                 case 4:
                     return effect.mArea;
                 case 5:

apps/opencs/model/world/refidadapterimp.cpp

@@ -194,6 +194,26 @@ void CSMWorld::IngredEffectRefIdAdapter::setNestedData(
     {
         case 0:
             ingredient.mData.mEffectID[subRowIndex] = value.toInt();
+            switch (ingredient.mData.mEffectID[subRowIndex])
+            {
+                case ESM::MagicEffect::DrainSkill:
+                case ESM::MagicEffect::DamageSkill:
+                case ESM::MagicEffect::RestoreSkill:
+                case ESM::MagicEffect::FortifySkill:
+                case ESM::MagicEffect::AbsorbSkill:
+                    ingredient.mData.mAttributes[subRowIndex] = -1;
+                    break;
+                case ESM::MagicEffect::DrainAttribute:
+                case ESM::MagicEffect::DamageAttribute:
+                case ESM::MagicEffect::RestoreAttribute:
+                case ESM::MagicEffect::FortifyAttribute:
+                case ESM::MagicEffect::AbsorbAttribute:
+                    ingredient.mData.mSkills[subRowIndex] = -1;
+                    break;
+                default:
+                    ingredient.mData.mSkills[subRowIndex] = -1;
+                    ingredient.mData.mAttributes[subRowIndex] = -1;
+            }
             break;
         case 1:
             ingredient.mData.mSkills[subRowIndex] = value.toInt();

components/compiler/declarationparser.cpp

@@ -2,7 +2,9 @@
 
 #include <components/misc/strings/lower.hpp>
 
+#include "context.hpp"
 #include "errorhandler.hpp"
+#include "extensions.hpp"
 #include "locals.hpp"
 #include "scanner.hpp"
 #include "skipparser.hpp"
@@ -70,6 +72,15 @@ bool Compiler::DeclarationParser::parseKeyword(int keyword, const TokenLoc& loc,
     else if (mState == State_Name)
     {
         // allow keywords to be used as local variable names. MW script compiler, you suck!
+        if (const Extensions* extensions = getContext().getExtensions())
+        {
+            std::string argumentType;
+            bool hasExplicit = false;
+            char returnType;
+            if (extensions->isFunction(keyword, returnType, argumentType, hasExplicit))
+                getErrorHandler().warning("Local variable declaration shadows a function", loc);
+        }
+
         return parseName(loc.mLiteral, loc, scanner);
     }
     else if (mState == State_End)
@@ -104,6 +115,7 @@ bool Compiler::DeclarationParser::parseInt(int value, const TokenLoc& loc, Scann
     if (mState == State_Name)
     {
         // Allow integers to be used as variable names
+        getErrorHandler().warning("Integer is used as a local variable name", loc);
         return parseName(loc.mLiteral, loc, scanner);
     }
     return Parser::parseInt(value, loc, scanner);