-
-
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
New semantic analyzer: Support multiple assignments to underscore #6450
New semantic analyzer: Support multiple assignments to underscore #6450
Conversation
The Travis CI build is failing. |
I fixed the build, unfortunately with explicit |
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, just one question.
@@ -2315,11 +2316,19 @@ def analyze_name_lvalue(self, | |||
added = self.add_symbol(name, var, lvalue) | |||
# Only bind expression if we successfully added name to symbol table. | |||
if added: | |||
lvalue.is_new_def = True | |||
lvalue.is_inferred_def = True |
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.
Should this depend on explicit_type
(presence of type annotation)?
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.
FWIW, I just copied the logic from make_name_lvalue_var()
. My understanding is that process_type_annotation()
which is called after analyze_lvalues()
will set it to False (depending on annotation).
TBH, the logic is quite opaque here, and should be clean-up. I would however not do this because analyzers are merged, because it is very easy to break checker. I will create a follow-up issue.
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.
Opened #6458
I also enable the existing test file with existing tests for this feature. I discovered a bug in
make_name_lvalue_var()
that caused some existing tests to fail, so I fixed it.I skip three tests (they already have issues). I also updated one test case because the behavior is more consistent in new analyzer (it always shows
Cannot determine type
, while old one shows this error only at global scope).