Error in CvTeamAI::AI_chooseElection?

[Leoreth]

I have run into a debug assertion warning, and it seems like the code is incorrect. Here is the function in its entirety:

int CvTeamAI::AI_chooseElection(const VoteSelectionData& kVoteSelectionData) const
{
   VoteSourceTypes eVoteSource = kVoteSelectionData.eVoteSource;

   FAssert(!isHuman());
   FAssert(GC.getGameINLINE().getSecretaryGeneral(eVoteSource) == getID());

   int iBestVote = -1;
   int iBestValue = 0;

   for (int iI = 0; iI < (int)kVoteSelectionData.aVoteOptions.size(); ++iI)
   {
       VoteTypes eVote = kVoteSelectionData.aVoteOptions[iI].eVote;
       CvVoteInfo& kVoteInfo = GC.getVoteInfo(eVote);

       FAssert(kVoteInfo.isVoteSourceType(eVoteSource));

       FAssert(GC.getGameINLINE().isChooseElection(eVote));
       bool bValid = true;

       if (!GC.getGameINLINE().isTeamVote(eVote))
       {
           for (int iJ = 0; iJ < MAX_PLAYERS; iJ++)
           {
               if (GET_PLAYER((PlayerTypes)iJ).isAlive())
               {
                   if (GET_PLAYER((PlayerTypes)iJ).getTeam() == getID())
                   {
                       PlayerVoteTypes eVote = GET_PLAYER((PlayerTypes)iJ).AI_diploVote(kVoteSelectionData.aVoteOptions[iI], eVoteSource, true);

                       if (eVote != PLAYER_VOTE_YES || eVote == GC.getGameINLINE().getVoteOutcome((VoteTypes)iI))
                       {
                           bValid = false;
                           break;
                       }
                   }
               }
           }
       }

       if (bValid)
       {
           int iValue = (1 + GC.getGameINLINE().getSorenRandNum(10000, "AI Choose Vote"));

           if (iValue > iBestValue)
           {
               iBestValue = iValue;
               iBestVote = iI;
           }
       }
   }

   return iBestVote;
}

The line in question is

if (eVote != PLAYER_VOTE_YES || eVote == GC.getGameINLINE().getVoteOutcome((VoteTypes)iI))

The violated assertion was in getVoteOutcome, which is a simple getter with trivial implementation. The issue was that the value of iI exceeded the number of VoteInfos defined in the DLL (28 > 27 in my case). It happened in my mod, but I checked the code against BtS and it is unchanged.

This seems wrong to me. First of all, iI is just the index of a struct in the kVoteSelectionData list, which is then carelessly cast to VoteTypes. The actual VoteType of that struct is eVote. There can easily be more entries in kVoteSelectionData than there are VoteTypes, because some vote types can be selected multiple types. For example "declare war against X" can have multiple votes with different targets. This is a clear error.

But that raised further questions for me. eVote is a VoteType, which is just an ID for a VoteInfo, i.e. the description of a UN/AP resolution. It cannot be PLAYER_VOTE_YES. Instead, PLAYER_VOTE_YES is a value of the enum PlayerVoteTypes, which describes the decision a player has made on a given resolution that has been put to a vote. The compiler lets this happen because enums can always be compared on their integer values (really unfortunate choice of a naming convention if you ask me). Likewise, it turns out CvGame::getVoteOutcome also returns a PlayerVoteTypes enum, but its returned value is compared to eVote, which is a VoteTypes enum!

Okay great. So basically everything in this line is incorrect. But that leaves the question, what is the intent of this line? Why does this method still mostly fulfill its function and this error goes unnoticed in practice?

 

[Leoreth]

Oh, right after posting this thread I saw the error. eVote is redeclared as a variable of type PlayerVoteTypes deeper in the cascading if clause. Why does that even compile? Whoever went on to code the line I was talking about then noticed they had no access to the previous eVote anymore and foolishly used iI as if its value was identical.

Still surprised that doesn't lead to further trouble for the game but at least I understand what is happening and the likely original intent.

 

[Nightinggale]

I'm not surprised that there are questionable code like this. The frequent usage of typecasting and using int instead of enum types in vanilla makes it prone to bugs. If coded correctly, the compiler shouldn't accept mixing VoteTypes and PlayerVoteTypes. A classic error I have seen modders do more than once is mixing UnitTypes with UnitClassTypes. Kind of the same issue.

// overloaded ++ for enum types

// prefix
template <class T>
static inline T& operator++(T& c)
{
   c = (T)(c + 1);
   return c;
}

// postfix
template <class T>
static inline T operator++(T& c, int)
{
   T cache = c;
   c = (T)(c + 1);
   return cache;
}

// overloaded -- for enum types

// prefix
template <class T>
static inline T& operator--(T& c)
{
   c = (T)(c - 1);
   return c;
}

// postfix
template <class T>
static inline T operator--(T& c, int)
{
   T cache = c;
   c = (T)(c - 1);
   return cache;
}

Code I added to the end of CvEnums.h, which allows using ++ and -- on enum types, hence allow clean code to loop enum types. I wonder if == should be overloaded to only accept the same enum type

None of my reply is for your problem specifically. It's more like coding style to avoid the type of ugly code, which increases the risk of bugs of this nature.

 

[Leoreth]

You're right, and that's pretty useful code. I think it would be much better if you could not compare enums of different types, although if you also disallow comparing them to int it will probably require heavy refactoring before the DLL compiles again.

Thinking about my question a bit more, I dimly remember talk about UN resolutions sometimes being canceled for no discernible reason. Could this be the reason?

 

[f1rpo]

@DarkLunaPhantom fixed this bug a couple of years ago in Kek-Mod: Git commit

Compiler warnings about shadowed variables (C4456-4459) were only added in VS2015 (source). And they're level-4 warnings, so I don't think even a recent version of the IDE will flag such potential errors. I run Cppcheck from time to time, which warns about shadowing. Maybe not the greatest code analysis tool – I don't think it can warn about comparisons between different enum types – but pretty easy to set up. Though, the first time I ran it, I was inundated with warnings, and there are still a few dozen warnings left in my codebase that I don't want to act upon and that Cppcheck can't be told to ignore. (I think it can ignore particular line numbers, but line numbers can easily change.)

[...] I think it would be much better if you could not compare enums of different types, although if you also disallow comparing them to int it will probably require heavy refactoring before the DLL compiles again.

An operator== template would also prohibit comparisons between e.g. int and short if I'm not mistaken. I think one has to overload the operator for every pair of enum types. I've done it only for pairs that are easily mixed up. (VoteTypes - VoteSourceTypes is in my "blocklist," but I actually forgot PlayerVoteTypes - VoteTypes.) If the CvInfo functions were properly typed (e.g. CvSpecialistInfo::getGreatPeopleUnitClass returning UnitClassTypes instead of int), then rooting out int-enum comparisons might be feasible ...

Thinking about my question a bit more, I dimly remember talk about UN resolutions sometimes being canceled for no discernible reason. Could this be the reason?

It does look like the bug could cause an AI civ to propose repealing a resolution that the AI civ actually likes. And the AI might sometimes fail to consider a valid resolution.

Edit: GitHub link fixed

 

[Nightinggale]

An operator== template would also prohibit comparisons between e.g. int and short if I'm not mistaken. I think one has to overload the operator for every pair of enum types. I've done it only for pairs that are easily mixed up. (VoteTypes - VoteSourceTypes is in my "blocklist," but I actually forgot PlayerVoteTypes - VoteTypes.) If the CvInfo functions were properly typed (e.g. CvSpecialistInfo::getGreatPeopleUnitClass returning UnitClassTypes instead of int), then rooting out int-enum comparisons might be feasible ...

I suppose we are talking about DO_FOR_EACH_FALSE_FRIEND (starting at line 2279 at the time of posting)

Looks interesting and it looks like a setup, which can be scripted to become complete. It is however likely that it can be reduced to disallowing int vs enum as enums can be converted to ints without being typecast. This way UnitWarrior == UnitClassWarrior would likely be compiled as int == int.

Yes it will require a bit of refactoring of the code, but I want that anyway. I want to replace the ints with enums whenever possible. Not only will it reduce the need for typecasting and reduce the number of bugs, debugging enums reveal the int value and enum string (at least in MSVS 2017). I scripted autogeneration of the enums from xml files, meaning heavy usage of enums makes the code much more readable in the debugger.

However for the time being, hunting network desyncs will be a priority. I likely won't get around to look into this this year.