MSUTeam / MSU

Modding Standards and Utilities for Battle Brothers
21 stars 4 forks source link

[BUG] Accessing an ordered map doesn't work as intended when a key accidentally mirrors a function name of ordered map #361

Closed OberKnirps closed 1 month ago

OberKnirps commented 3 months ago

Versions

Describe the bug When adding an entry to the map and using "filter" (or any other function name of ordered map) as a key, accessing that entry results in returning the function address of the filter() function.

To Reproduce

local orderedMap = ::MSU.Class.OrderedMap();
orderedMap.key1 <- 30;
orderedMap["filter"] <- 20;

foreach(key, value in orderedMap)
{
    this.logInfo(key + " | " + value);
}

prints

key1 | 30
filter | (function : 0x1D6791B8)

but when accessing directly

this.logInfo("filter" + " | " + orderedMap.Table["filter"]);

it prints the proper result

filter | 20

Additional context I noticed the bug while adding settings for my mod and used "filter" as an ID. At first I wanted to report it as a bug related to settings_page.nut, but after a bit of digging it turned out to be a bug in ordered map. Therefore I'm adding my original bug report, but following bug should result in the problem above:

Describe the bug Adding a mod setting to a page and using filter as an ID for that setting breaks the mod settings screen by causing an error within getUIData(). It seams to be the case for all IDs that are also functions of ordered map, except the ones starting with an underscore.

To Reproduce

  1. Setup your mod settings with MSU
  2. add a setting to a page with an ID mirroring a function from ordered map e.g. testPage.addStringSetting("filter", "", "test");
  3. Run the game with that mod
  4. Trying to open 'Mod Options' produces the error in the log

Expected behavior When calling getUIData( ) in settings_page.nut the foreach-loop should access this.Settings["filter"] which should be equal to this.Settings.Table["filter"], because Settings is an ordered map and the [] operator gets overloaded there. Instead this.Settings["filter"] seams to be treated as this.Settings.filter, returning a function and therefore resulting in an error when calling .getUIData(_flags) in settings_page.nut:171.

Log image

Enduriel commented 3 months ago

Thanks for this thorough report. Unfortunately this is a squirrel issue mostly, the best we could do is throw the error when trying to set a key with a value that is already a function in the ordered map :/

Enduriel commented 3 months ago

I don't think there's an alternative complete solution here because you should be able to access the ordered map via dot notation Map.Value vs Map["Value"], and you could not do that with something that is already a function on the ordered map.

OberKnirps commented 3 months ago

I thought as much. An error would be good though, because finding out what the actual cause was, was a bit convoluted.

Other solutions I could think of were:

  1. moving the properties and the overloaded functions into a table e.g. m within the ordered map and than accessing it with ordered_map.m["filter"], but it then breaks everything that uses ordered maps and kind of defeats the point of how they were written in the first place.
  2. extracting the functions and make them part of the MSU class or something and use it like ::MSU.Functions.OrderedMap.filter(myMap, filterFunction); which also breaks things and shouldn't be that convoluted.

Also a disclaimer/info in the wiki could be helpful, but it's also pretty uncommon problem to run into.

LordMidas commented 3 months ago

I'd say we disallow adding anything to the container which is a key already present in the container class and we throw it at that point. This way the modder knows that they must choose a different name for their item.

function _newslot( _key, _value )
{
    if (_key in ::MSU.Class.OrderedMap)
        throw _key + " is a reserved keyword in class";

    if (!(_key in this.Table))
    {
        this.Array.push(_key);
    }
    this.Table[_key] <- _value;
}
Enduriel commented 2 months ago

@LordMidas yeah your solution is probably best