ArchipelagoMW / Archipelago

Archipelago Multi-Game Randomizer and Server
https://archipelago.gg
Other
528 stars 693 forks source link

Bug: OoT Rule_AST_Transformer.parse_rule uses deprecated NameConstant in strings, resulting in some code never being run #3993

Open Mysteryem opened 2 months ago

Mysteryem commented 2 months ago

What happened?

ast.NameConstant was deprecated in Python 3.8 such that attempting to create an ast.NameConstant creates an ast.Constant instead (see https://docs.python.org/3/library/ast.html#ast.AST), this means that all checks against the string dump of a node being 'NameConstant(True)' or 'NameConstant(False)' are never hit, because the string dumps will instead be 'Constant(True)' or 'Constant(False)'.

To check, run the following:

import ast
# RuleParser.Rule_AST_Transformer.make_access_rule uses ast.dump(body, False)
print(ast.dump(ast.NameConstant(True), False))

I would have submitted a PR to make this change, but doing so causes generation with entrance randomization enabled to fail (yaml attached in a zip) and I have no idea why.

Changing these to use 'Constant(False)'/'Constant(True)' seems to work: https://github.com/ArchipelagoMW/Archipelago/blob/f06d4503d83209b8fae6897eef500493d57826e8/worlds/oot/RuleParser.py#L388 https://github.com/ArchipelagoMW/Archipelago/blob/f06d4503d83209b8fae6897eef500493d57826e8/worlds/oot/RuleParser.py#L393

But changing these to use 'Constant(False)'/'Constant(True)' causes generation with entrance randomization to fail: https://github.com/ArchipelagoMW/Archipelago/blob/f06d4503d83209b8fae6897eef500493d57826e8/worlds/oot/RuleParser.py#L489 https://github.com/ArchipelagoMW/Archipelago/blob/f06d4503d83209b8fae6897eef500493d57826e8/worlds/oot/RuleParser.py#L491

So whatever spot.never = True or spot.always = True did in the past seems to no longer work correctly:

Calculating Access Rules.
Uncaught exception
Traceback (most recent call last):
  File "G:\git_Repos_other\Archipelago\Generate.py", line 531, in <module>
    multiworld = ERmain(erargs, seed)
                 ^^^^^^^^^^^^^^^^^^^^
  File "G:\git_Repos_other\Archipelago\Main.py", line 150, in main
    AutoWorld.call_all(multiworld, "set_rules")
  File "G:\git_Repos_other\Archipelago\worlds\AutoWorld.py", line 181, in call_all
    call_single(multiworld, method_name, player, *args)
  File "G:\git_Repos_other\Archipelago\worlds\AutoWorld.py", line 171, in call_single
    raise e
  File "G:\git_Repos_other\Archipelago\worlds\AutoWorld.py", line 164, in call_single
    ret = _timed_call(method, *args, multiworld=multiworld, player=player)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "G:\git_Repos_other\Archipelago\worlds\AutoWorld.py", line 150, in _timed_call
    ret = method(*args)
          ^^^^^^^^^^^^^
  File "G:\git_Repos_other\Archipelago\worlds\oot\__init__.py", line 818, in set_rules
    shuffle_random_entrances(self)
  File "G:\git_Repos_other\Archipelago\worlds\oot\EntranceShuffle.py", line 452, in shuffle_random_entrances
    set_all_entrances_data(multiworld, player)
  File "G:\git_Repos_other\Archipelago\worlds\oot\EntranceShuffle.py", line 21, in set_all_entrances_data
    return_entrance = world.get_entrance(return_entry[0], player)
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "G:\git_Repos_other\Archipelago\BaseClasses.py", line 424, in get_entrance
    return self.regions.entrance_cache[player][entrance_name]
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^
KeyError: 'Barinade Boss Room -> Jabu Jabus Belly Boss Door'
Exception in <bound method OOTWorld.set_rules of <worlds.oot.OOTWorld object at 0x00000213B0007650>> for player 1, named Player1.

Ocarina of Time.yaml.zip

What were the expected results?

There should not be conditional branches than can never be hit.

Software

Local generation

Zannick commented 1 month ago

The python 3.8 issue was fixed in https://github.com/OoTRandomizer/OoT-Randomizer/commit/0eb85cb8be443036a9d91b6f21e9be63d6125fa3 to just add the Constant(False) etc options as alternatives.

I don't know why the crash occurs with the fix; the purpose of self.never was essentially to let us discard entrances and locations that could never be used, in order to streamline searches. In practice, it looks like OoTR now mostly uses them to simplify rules editing (i.e. a hinted location has a rule that its hint must be accessible first).

AP, however, still drops entrances here: https://github.com/ArchipelagoMW/Archipelago/blob/6287bc27a68ace32679fe8a41a580df7180cd9d8/worlds/oot/__init__.py#L583-L586

whereas that was removed from OoTR in https://github.com/OoTRandomizer/OoT-Randomizer/commit/6a8967cfa25060edd6793c3b9699fb33b195df4b#diff-bfdf4e3e2ed58a0618ffff5f14862b7a0b5427b9ce41ce7fc8edb19224e1862c

(Behavior for item locations and event locations seems to be same as current OoTR.)