Skip to content
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

Merged
merged 9 commits into from
Apr 16, 2018

Conversation

gvanrossum
Copy link
Member

Fixes #465. Also see discussion there about the exact specification and concerns.

@gvanrossum gvanrossum requested a review from JukkaL March 31, 2018 04:18
Copy link
Member

@ilevkivskyi ilevkivskyi left a 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

-- ----------------------------------

[case testUnusedTargetSingle]
def foo() -> None:
Copy link
Member

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?

Copy link
Member Author

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.)

[case testUnusedTargetNotDef]
def _() -> int:
pass
_() + 0
Copy link
Member

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.

@gvanrossum
Copy link
Member Author

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.

@ilevkivskyi
Copy link
Member

Oh, pilot error, pushed the test updates (I hope).

Maybe now I am missing something, but I still don't see the tests updates in this PR.

@gvanrossum
Copy link
Member Author

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.

@gvanrossum
Copy link
Member Author

gvanrossum commented Apr 4, 2018

Recording a few ideas from Jukka:

  • only do it on the second assignment?
  • create a new Var on each assignment?
  • do this analysis in semanal.py, using is_func_scope()?

Also need tests:

  • multiple assignments to _ on same line (e.g. _, _, x = <tuple>)
  • make sure import ... as _ doesn't trigger this
  • make sure _: <type> = <expr> overrides

@ilevkivskyi
Copy link
Member

Some thoughts about these ideas (not sure if I should comment these, but just in case).

only do it on the second assignment?

I am not sure I understand the benefit of this.

create a new Var on each assignment?

I would say it is a bad idea to have two variables with the same name.

do this analysis in semanal.py, using is_func_scope()?

I like this idea, this is similar to what I was thinking about this.

@gvanrossum
Copy link
Member Author

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: except Exception as _: doesn't treat _ as Any because the checker overrides the type. The code in the checker looks a bit complex so I'd rather keep this as a limitation rather than trying to fix it. As the tests show, it works fine with the target in a for loop or in a with statement.

Copy link
Member

@ilevkivskyi ilevkivskyi left a 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():
Copy link
Member

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.

Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants