-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Special case assignment to '_': always infer 'Any' #4833
Conversation
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 would add at least one of the two safety rules:
- check that the variable is never read at the same scope (this is free from problems I mentioned in the issue)
- only allow this at a local (function or method) scope
test-data/unit/check-inference.test
Outdated
-- ---------------------------------- | ||
|
||
[case testUnusedTargetSingle] | ||
def foo() -> None: |
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 do you have only tests at the function scope? Is it something intentional?
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.
Oops, I thought it would all be treated the same, but it isn't. Will fix. (Since I've seen a fair number of global uses of this idiom.)
test-data/unit/check-inference.test
Outdated
[case testUnusedTargetNotDef] | ||
def _() -> int: | ||
pass | ||
_() + 0 |
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 would add _() + 'hi'
to check that this generates an error.
I still am hesitant about the safety rules -- I suspect there are too many different places. I'd almost rather add it behind a flag. |
Maybe now I am missing something, but I still don't see the tests updates in this PR. |
I'm guessing I committed but didn't push (or the push failed and I didn't notice). I pushed again, we now have 67d84aa and the tests for that are running. |
Recording a few ideas from Jukka:
Also need tests:
|
Some thoughts about these ideas (not sure if I should comment these, but just in case).
I am not sure I understand the benefit of this.
I would say it is a bad idea to have two variables with the same name.
I like this idea, this is similar to what I was thinking about this. |
This is ready for another review. I moved the special-casing to semanal.py, where it seems to work fine, and limited it to locals only. There's one quirk: |
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.
Looks good!
mypy/semanal.py
Outdated
@@ -1826,6 +1826,10 @@ def analyze_lvalue(self, lval: Lvalue, nested: bool = False, | |||
lval.kind = LDEF | |||
lval.fullname = lval.name | |||
self.add_local(v, lval) | |||
if isinstance(lval, NameExpr) and lval.name == '_' and self.is_func_scope(): |
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.
First isinstance()
should be unnecessary, since we are already in a huge if isinstance(lval, NameExpr)
block. Also the last check self.is_func_scope()
is not needed, because it is equivalent to self.locals[-1] is not None
where we are now. But it is up to you.
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.
OK, I'll remove the redundant checks. If the tests pass I'll merge.
Fixes #465. Also see discussion there about the exact specification and concerns.