Util.dict_merge attempting to access key '0' on dict

[avanwinkle]

There's a bug in Util.dict_merge() that I uncovered when using the MPF Language Server. The merge method attempts to access a key 0 of the dict, which is often not found, and the file validation fails.

2020-11-01 12:18:46,454 UTC - DEBUG - pyls_jsonrpc.endpoint - Sending notification: 
textDocument/publishDiagnostics {
    'message': 'Internal error while verifying: 0 Traceback (most recent call last):
      File "/Users/Anthony/Git/mpf-ls/mpfls/mpf_ls.py", line 1252, in lint
        diagnostics = self._walk_diagnostics_root(document, document.config_roundtrip)
      File "/Users/Anthony/Git/mpf-ls/mpfls/mpf_ls.py", line 962, in _walk_diagnostics_root
        diagnostics.extend(self._walk_diagnostics_devices(document, config[key], root_spec, key, lc))
      File "/Users/Anthony/Git/mpf-ls/mpfls/mpf_ls.py", line 1007, in _walk_diagnostics_devices
        diagnostics.extend(self._walk_diagnostics(document, config[key], [root_key], root_key, lc_child))
      File "/Users/Anthony/Git/mpf-ls/mpfls/mpf_ls.py", line 1160, in _walk_diagnostics
        key_spec[1], element_lc))
      File "/Users/Anthony/Git/mpf-ls/mpfls/mpf_ls.py", line 1205, in _walk_diagnostics_value
        found = self._get_definitions(device_type, config)
      File "/Users/Anthony/Git/mpf-ls/mpfls/mpf_ls.py", line 343, in _get_definitions
        config = self.workspace.get_complete_config()
      File "/Users/Anthony/Git/mpf-ls/mpfls/workspace.py", line 82, in get_complete_config
        config = Util.dict_merge(config, mode_config)
      File "/Users/Anthony/Git/mpf/mpf/core/utility_functions.py", line 243, in dict_merge
        result[k] = Util.dict_merge(result[k], v, combine_lists)
      File "/Users/Anthony/Git/mpf/mpf/core/utility_functions.py", line 246, in dict_merge
        if isinstance(v, dict) and v[0] == dict(_overwrite=True):
    KeyError: 0
    ', 'severity': 1}]}

The code block in question is the following iteration that merges two dicts, a and b:

for k, v in b.items():
    if v is None:
        continue
    if isinstance(v, dict) and '_overwrite' in v:
        result[k] = v
        del result[k]['_overwrite']
    elif isinstance(v, dict) and '_delete' in v:
        if k in result:
            del result[k]
    elif k in result and isinstance(result[k], dict):
        result[k] = Util.dict_merge(result[k], v, combine_lists)
    elif k in result and isinstance(result[k], list):
        if isinstance(v, dict) and v[0] == dict(_overwrite=True):   ## <<<< This is the failing line
            result[k] = v[1:]
        elif isinstance(v, list) and combine_lists:
            result[k].extend(v)
        else:
            result[k] = deepcopy(v)
    else:
        result[k] = deepcopy(v)

From digging into the events of mine that trigger this, the culprit seems to be event players where the events are a combination of strings, lists, and dicts. I'm having trouble teasing out exactly how the dict merge is working, but I think it might be as simple as a typo on the failing line.

elif k in result and isinstance(result[k], list):
    if isinstance(v, dict) and v[0] == dict(_overwrite=True):  ## <<< Is there a typo here?
        result[k] = v[1:]
    elif isinstance(v, list) and combine_lists:
        result[k].extend(v)

All that logic is about v being a list, but the line says if isinstance(v, dict) and v[0]. When I change that line to read if isinstance(v, list) and v[0] the error goes away and the language server works properly.

I'm assuming that this doesn't show in normal playback because the callers of the merge are wrapping the error, but it could also be that I'm misreading the code and my change is actually breaking functionality. Merely changing the instance type from dict to list causes a million test failures because of dicts with empty lists. Previously those empty lists failed the dict type so the v[0] was never evaulated, but calling v[0] on an empty list will throw. So I had to make the final line look like this:

if isinstance(v, list) and v and v[0] == dict(_overwrite=True):

With that, all of the tests pass again.

So... am I on the right track here? Or am I overlooking something?

[jabdoa2]

Do you have an example which triggers this in LS? I would like to add an unit or integration test for that. Any way to reproduce this would help.

[jabdoa2]

Your fix makes sense in general. Guess this is related to usage of "_overwrite". We could just add your fix but it might break again in the future. To prevent this I would like a failing test first which is then fixed by your change.

[avanwinkle]

Here's a block from my code that triggers the crash:

event_player:
  timer_reaper_charging_complete: fire_reapercannon
  timer_reaper_firing_complete:
    - flippers_off
    - restart_reaper|4s
  restart_reaper:
    - flippers_on
    - enable_random_reapershot
  player_reaper_hp{player_num==current_player.number}:
    - humanreaper_failed{value>0 and current_player.reaper_rounds<4}
    - humanreaper_complete{value<=0 or current_player.reaper_rounds>=4}
  ball_save_reaper_squadmate_save_saving_ball:
    kill_squadmate:
      squadmate: random

Specifically, it's the final event with the dict-type declaration that causes the error. If I change that to be just a simple string or list, the error goes away.

[avanwinkle]

I suppose if I were to convert that last item to be a list of dicts, it would also pass. So maybe the fix would be to coerce the children of event_player to be a list, even if they are a dict? Converting strings to list works well enough.

And maybe I should have just had it like this the whole time?

ball_save_reaper_squadmate_save_saving_ball:
  - kill_squadmate:
      squadmate: random

[jabdoa2]

I tried to reproduce your issue today but the example seems to work for me. As an example for mpf test:

event_player:
  timer_reaper_charging_complete: fire_reapercannon
  timer_reaper_firing_complete:
    - flippers_off
    - restart_reaper|4s
  restart_reaper:
    - flippers_on
    - enable_random_reapershot
  player_reaper_hp{player_num==current_player.number}:
    - humanreaper_failed{value>0 and current_player.reaper_rounds<4}
    - humanreaper_complete{value<=0 or current_player.reaper_rounds>=4}
  ball_save_reaper_squadmate_save_saving_ball:
    kill_squadmate:
      squadmate: random
##! test
#! start_game
#! post ball_save_reaper_squadmate_save_saving_ball

This passes in dev. Also if I add it to a machine this seems to work fine in mpf-ls. Did you fix this already? Or did I miss something?

[avanwinkle]

I knew I'd seen this before! I threw a quick fix into this PR (#1541 (review)) before seeing that it broke all the tests, so I reverted it.

I do still see the MPF-LS error when I open this file in VSCode, though. It doesn't hurt anything MPF (that I can find or remember), but the language server isn't happy. Have the latest dev pulls of both MPF and LS.

[jabdoa2]

I will have another look