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

Fix error reporting context for missing generic type arguments #7100

Merged
merged 5 commits into from Jul 3, 2019

Conversation

@ilevkivskyi
Copy link
Collaborator

commented Jun 28, 2019

Fixes #7077

The logic of the fix is quite straightforward: emit the error immediately when we fix an instance type, and not in a traversal after the main pass. Note that I fix this only for the new analyser, because the old one will be anyway deprecated soon.

This causes a bit of code churn because we need to pass the flags to several functions, but the logic should not change much. I didn't add many new tests, since we have a bunch of existing tests. It would probably make sense to play with this, suggestions for more tests are very welcome.

@ilevkivskyi ilevkivskyi requested a review from msullivan Jun 28, 2019

@msullivan

This comment has been minimized.

Copy link
Collaborator

commented Jun 28, 2019

I wasn't able to fully test this out internally because I ran into some other issues but one thing I noticed is this makes aliases like Foo = List illegal. (Even though if you # type: ignore the error they do work correctly)

@msullivan
Copy link
Collaborator

left a comment

This looks reasonable to me, modulo my concern about Foo = List style aliases

@@ -2369,7 +2369,9 @@ def check_and_set_up_type_alias(self, s: AssignmentStmt) -> bool:
# so we need to replace it with non-explicit Anys.
res = make_any_non_explicit(res)
no_args = isinstance(res, Instance) and not res.args
fix_instance_types(res, self.fail)
fix_instance_types(res, self.fail,

This comment has been minimized.

Copy link
@msullivan

msullivan Jun 28, 2019

Collaborator

I think this needs to be done conditionally only when no_args, maybe?

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Jul 1, 2019

Author Collaborator

Yes, aliases like A = List should be allowed even with the flag, will fix it now.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 1, 2019

@msullivan I fixed the errors you reported, could you please try again?

@msullivan
Copy link
Collaborator

left a comment

Looks great. Thanks for this.

@ilevkivskyi ilevkivskyi merged commit 3549c0e into python:master Jul 3, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ilevkivskyi ilevkivskyi deleted the ilevkivskyi:newan-fix-disallow-any branch Jul 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.