-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Avoid false unreachable
and redundant-expr
warnings in loops more robustly and efficiently, and avoid multiple revealed type
notes for the same line.
#19118
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
Open
tyralla
wants to merge
6
commits into
python:master
Choose a base branch
from
tyralla:fix/reachability_regression
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+139
−33
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
229e4a6
Avoid false `unreachable` and `redundant-expr` warnings in loops more…
tyralla c4bea4e
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] c24c950
report all revealed types, not only those of the last iterations step
tyralla b6930e7
restart CI
tyralla 01b6140
Revert "restart CI"
tyralla 15f5740
Add the test case `testPersistentUnreachableLinesNestedInInpersistent…
tyralla File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't this just use the last round of unreachability warnings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good question. I initially implemented it so in 229e4a6. However, in the last function definition of test case
testNewRedefineTryStatement
, the first response toreveal_type
does provide complete information, but the second (and final) response does not:I have no experience with the new
--allow-redefinition-new
option so far, and have not tried to investigate why this happens. I just extended my approach in c24c950.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't that case also allow something not allowed for
str
? Eg does that error for:Above the
reveal_type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, after re-reading, it seems that I have not hit the point of your question. Another try: There might be some scope for optimisations. However, I am unsure what can possibly happen when running Mypy with different configurations (like in the
f4
example discussed above), so I preferred to implement this part of the approach as robustly as possible.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand it's possible to be too robust: mypy only prints the errors for the last round right? So it's possible (through some bug elsewhere, like forgetting to propagate line numbers maybe?) to have unreachable code in the last run that isn't marked -- and so have no signal that the code there isn't checked.
On the other other hand, just using the last iteration's unreachability errors might be wrong in presence of bugs like in
f4
, but it's simpler (and implementation is trivial to get right) and could never mislead the user about what code could get errors.Basically I think it's better to match the semantics of how error reporting happens (ie during the last round of checking) then to have only one type of error be correct in face of bugs.
Edit: actually I might be wrong about my assumptions here, let me play with your PR a bit and I'll update this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well there seems to be a bug in your logic anyways (I can't see where, it looks right to me):
Anyways, turns out I was wrong about mypy only erroring in the final cycle. But the above bug should be enough for now and IMO a type becoming a subtype at the same place in the code seems like a bug.
@JukkaL since maybe some of my other assumptions are wrong. Do you think that looping over code multiple times but with subtypes should be considered a bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you certain? This is what I get:
(I had to add
isinstance
toexception.pyi
to get this running.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just added that to a
repro.py
file and ranmypy repro.py --allow-redefinition-new --local-partial-types --warn-unreachable
but maybe I checked your branch out incorrectly?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad; the
-warn-unreachable
flag was missing in the test case. Now I get bothStatement is unreachable
andRevealed type is "builtins.str"
. However, the same happens when I check the modified test with Mypy-Master. Therefore, I don't know if this actually points to a flaw in my current approach. I will investigate tomorrow....