gh-104504: Cases generator: enable mypy's possibly-undefined error code

[AlexWaygood]

@gvanrossum: This fixes the new pyright complaints you reported in #108112 (comment), and enables the equivalent mypy lint so that this kind of thing is checked in CI in the future.

The maybe_* variable names I've chosen aren't great, but they were the best I could think of -- feel free to suggest better ones! (The fact that I couldn't think of great variable names was the reason why I didn't initially go with this approach in #108112, FWIW. I'm still not entirely sure that this improves the code, really, but it does make the type checkers happier. Maybe it's worth it.)

• Issue: Run mypy on cases_generator #104504

[gvanrossum]

Hm. When I manually run

mypy --enable-error-code=possibly-undefined --strict Tools/cases_generator/

I still get an error for this line:

Tools/cases_generator/generate_cases.py:403: error: Name "format" may be undefined  [possibly-undefined]

(Also for popped, but I'm using format as the simpler example to get right first.)

FWIW maybe the variable could be named fmt? IMO maybe_format is a bit unwieldy and not all that helpful. Using an abbreviation intuitively conveys the idea that this is a more local use than format.

Not sure how to fix that error I get -- maybe it wants an assignment to format in the unreachable case _ clause, or maybe we need to just add format: str | None = None at the top (just before match) and just add assert format is not None vefore adding it to the set? Then we can get rid of maybe_format again.

[AlexWaygood]

Hm. When I manually run

mypy --enable-error-code=possibly-undefined --strict Tools/cases_generator/

I still get an error for this line:

That's strange. I can reproduce your results locally if I use that command, but not if I run mypy --config-file Tools/cases_generator/mypy.ini (which is the command we're running in CI). And I know that before I made the code changes, mypy --config-file Tools/cases_generator/mypy.ini was resulting in possibly-undefined errors on these lines.

I wonder why the two commands are leading to different results? It seems very odd.

[AlexWaygood]

@hauntsaninja, any idea what might be going on here? :)

[AlexWaygood]

This diff would fix the error you're seeing when you're running mypy with the different invocation, but I still don't understand why the two different mypy invocations are producing different results (and the errors you're seeing with the different mypy invocation feel spurious, since mypy's well aware that these match statements are exhaustive):

Possible fix:

diff --git a/Tools/cases_generator/generate_cases.py b/Tools/cases_generator/generate_cases.py
index 41eb5219d7..63a3db5c74 100644
--- a/Tools/cases_generator/generate_cases.py
+++ b/Tools/cases_generator/generate_cases.py
@@ -156,6 +156,8 @@ def effect_str(effects: list[StackEffect]) -> str:
             return str(n_effect)

         instr: AnyInstruction | None
+        popped: str | None = None
+        pushed: str | None = None
         match thing:
             case parsing.InstDef():
                 if thing.kind != "op" or self.instrs[thing.name].is_viable_uop():
@@ -173,8 +175,6 @@ def effect_str(effects: list[StackEffect]) -> str:
                 instr = self.pseudo_instrs[thing.name]
                 # Calculate stack effect, and check that it's the the same
                 # for all targets.
-                maybe_popped: str | None = None
-                maybe_pushed: str | None = None
                 for target in self.pseudos[thing.name].targets:
                     target_instr = self.instrs.get(target)
                     # Currently target is always an instr. This could change
@@ -183,15 +183,14 @@ def effect_str(effects: list[StackEffect]) -> str:
                     assert target_instr
                     target_popped = effect_str(target_instr.input_effects)
                     target_pushed = effect_str(target_instr.output_effects)
-                    if maybe_popped is None:
-                        maybe_popped, maybe_pushed = target_popped, target_pushed
+                    if popped is None:
+                        popped, pushed = target_popped, target_pushed
                     else:
-                        assert maybe_popped == target_popped
-                        assert maybe_pushed == target_pushed
-                assert maybe_popped is not None and maybe_pushed is not None
-                popped, pushed = maybe_popped, maybe_pushed
+                        assert popped == target_popped
+                        assert pushed == target_pushed
             case _:
                 assert_never(thing)
+        assert popped is not None and pushed is not None
         return instr, popped, pushed

     @contextlib.contextmanager
@@ -380,6 +379,7 @@ def write_metadata(self, metadata_filename: str, pymetadata_filename: str) -> No
         # Compute the set of all instruction formats.
         all_formats: set[str] = set()
         for thing in self.everything:
+            format: str | None = None
             match thing:
                 case OverriddenInstructionPlaceHolder():
                     continue
@@ -388,18 +388,16 @@ def write_metadata(self, metadata_filename: str, pymetadata_filename: str) -> No
                 case parsing.Macro():
                     format = self.macro_instrs[thing.name].instr_fmt
                 case parsing.Pseudo():
-                    fmt: str | None = None
                     for target in self.pseudos[thing.name].targets:
                         target_instr = self.instrs.get(target)
                         assert target_instr
-                        if fmt is None:
-                            fmt = target_instr.instr_fmt
+                        if format is None:
+                            format = target_instr.instr_fmt
                         else:
-                            assert fmt == target_instr.instr_fmt
-                    assert fmt is not None
-                    format = fmt
+                            assert format == target_instr.instr_fmt
                 case _:
                     assert_never(thing)
+            assert format is not None
             all_formats.add(format)

         # Turn it into a sorted list of enum values.

[gvanrossum]

If I comment out warn_unreachable = True in the mypy.ini file, it produces those warnings. So that flag may not mean what we think it means.

[AlexWaygood]

Looks like we simultaneously figured this out here 😄

[hauntsaninja]

Hmm that's weird. Not familiar with the undefined stuff, let me go get pdb out from my closet

[AlexWaygood]

ISTM that there are probably two mypy bugs we've stumbled upon here (☹️):

1. --warn-unreachable should never be making errors go away: if mypy reports these errors without --warn-unreachable, it should probably report them with --warn-unreachable as well
2. Mypy shouldn't be emitting these possibly-undefined errors with or without --warn-unreachable specified, since mypy's well aware that these match statements are exhaustive, so it's not possible to get to the end of the match thing statement without format being assigned to.

[hauntsaninja]

Sorry for the comment earlier that was a repeat of what you already found, my Github was stale.

I need to get back to work, but it looks like there's some sort of side-effect-y message filtering that's going on in a place that to me seems like it absolutely should not have side effects. This diff makes mypy consistently not error:

diff --git a/mypy/checker.py b/mypy/checker.py
index 87dff9175..2ab9ae55d 100644
--- a/mypy/checker.py
+++ b/mypy/checker.py
@@ -2746,6 +2746,10 @@ class TypeChecker(NodeVisitor[None], CheckerPluginInterface):
         for s in b.body:
             if self.binder.is_unreachable():
                 if not self.should_report_unreachable_issues():
+
+                    # this does some side effect
+                    # adding it here ensures the side effect happens both with and without --warn-unreachable
+                    self.is_noop_for_reachability(s)
+
                     break
                 if not self.is_noop_for_reachability(s):
                     self.msg.unreachable_statement(s)

[hauntsaninja]

Found some more time to look at this, I think python/mypy#15995 should fix

[AlexWaygood]

I've filed python/mypy#15958 so that we can track the mypy bugs on the mypy issue tracker. For now, I guess I'll switch to the diff I suggested in #108454 (comment), so that we get consistent mypy behaviour regardless of whether you specify --warn-unreachable or not on the command line.

[gvanrossum]

Thanks! This was an interesting detour…