Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
avanwinkle opened this issue Nov 1, 2020 · 7 comments
Open

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

avanwinkle opened this issue Nov 1, 2020 · 7 comments

Comments

@avanwinkle
Copy link
Collaborator

avanwinkle commented Nov 1, 2020

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
Copy link
Collaborator

jabdoa2 commented Nov 1, 2020

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
Copy link
Collaborator

jabdoa2 commented Nov 1, 2020

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator

jabdoa2 commented Dec 9, 2020

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
Copy link
Collaborator Author

avanwinkle commented Dec 10, 2020

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.

Screen Shot 2020-12-09 at 4 44 05 PM

@jabdoa2
Copy link
Collaborator

jabdoa2 commented Dec 10, 2020

I will have another look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants