CMakePP / CMakePPLang

Object-oriented extension to the CMake language.
http://cmakepp.github.io/CMakePPLang/
Apache License 2.0
11 stars 4 forks source link

Calling cpp_map_set with a key of "keys" results in corrupted state #48

Closed AutonomicPerfectionist closed 2 years ago

AutonomicPerfectionist commented 2 years ago

Describe the bug Calling cpp_map(SET "${instance}" keys "${blah}") results in overwriting the set of map keys, and calling cpp_map(SET...) with a different key afterward changes the value of the first mapping. This is due to how keys are stored, as seen here: https://github.com/CMakePP/CMakePPLang/blob/b19617ebb542f1042699e8d0913f5f75792f397a/cmake/cmakepp_lang/map/map.cmake#L151-L160

A key is stored in the same way that the list of keys are stored, i.e. if the key is keys then it will resolve to the exact same global as the list of keys.

To Reproduce Steps to reproduce the behavior:

  1. Create a map
  2. Set some mappings
  3. Set a mapping with a key of keys
  4. Set some more mappings
  5. Get the mapping with a key of keys and observe the corrupted behavior

Expected behavior Setting a mapping should not affect other mappings nor should it overwrite the list of keys

Additional context This bug was discovered while fixing the tests/map/test_map.cmake test file, the affected section was the one named keys underneath test_cpp_map.

An easy solution is to add additional separators in the globals. For example, the list of keys can remain the same and a particular key could be stored underneath "${_ms_this}_keys_${_ms_key}" so that a key named keys would resolve to ..._keys_keys and therefore not conflict.

ryanmrichard commented 2 years ago

@AutonomicPerfectionist so if I'm understanding you correctly the proposal is to change line 155 of your code snippet to cpp_set_global("${_ms_this}_keys_${_ms_key}" "${_ms_value}")?

That sounds good to me. Maybe @zachcran can look into this? It should be pretty quick, but may involve tracking down more places where we access keys.

AutonomicPerfectionist commented 2 years ago

Correct, it would also involve changing cpp_map_get and cpp_map_append to point to those new globals as well

zachcran commented 2 years ago

@AutonomicPerfectionist @ryanmrichard I'll take a look and see if I can get a PR in soon!