Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Aug 25, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bec3fb7
Cases generator: enable mypy's `possibly-undefined` error code
AlexWaygood Aug 24, 2023
cda5499
Better variable name
AlexWaygood Aug 24, 2023
163167b
Work around the mypy bugs
AlexWaygood Aug 25, 2023
d7e36f0
Merge branch 'main' into possibly-undefined-cases
AlexWaygood Aug 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions Tools/cases_generator/generate_cases.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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():
Expand All @@ -173,21 +175,22 @@ 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.
for idx, target in enumerate(self.pseudos[thing.name].targets):
for target in self.pseudos[thing.name].targets:
target_instr = self.instrs.get(target)
# Currently target is always an instr. This could change
# in the future, e.g., if we have a pseudo targetting a
# macro instruction.
assert target_instr
target_popped = effect_str(target_instr.input_effects)
target_pushed = effect_str(target_instr.output_effects)
if idx == 0:
if popped is None:
popped, pushed = target_popped, target_pushed
else:
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
Expand Down Expand Up @@ -376,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
Expand All @@ -384,15 +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():
for idx, target in enumerate(self.pseudos[thing.name].targets):
for target in self.pseudos[thing.name].targets:
target_instr = self.instrs.get(target)
assert target_instr
if idx == 0:
if format is None:
format = target_instr.instr_fmt
else:
assert format == target_instr.instr_fmt
case _:
assert_never(thing)
assert format is not None
all_formats.add(format)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@AlexWaygood AlexWaygood Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@AlexWaygood AlexWaygood Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we simultaneously figured this out here 😄

Copy link
Contributor

@hauntsaninja hauntsaninja Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@AlexWaygood AlexWaygood Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@hauntsaninja hauntsaninja Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


# Turn it into a sorted list of enum values.
Expand Down
2 changes: 1 addition & 1 deletion Tools/cases_generator/mypy.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ python_version = 3.10
# ...And be strict:
strict = True
strict_concatenate = True
enable_error_code = ignore-without-code,redundant-expr,truthy-bool
enable_error_code = ignore-without-code,redundant-expr,truthy-bool,possibly-undefined
warn_unreachable = True