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

Use fully qualified names for ambiguous class names resembling builtins. #8425

Merged
merged 5 commits into from Feb 27, 2020

Conversation

@Muks14x
Copy link
Contributor

Muks14x commented Feb 21, 2020

Resolves #8372.

Is there a better way to get the types from typing and builtins?

@JelleZijlstra

This comment has been minimized.

Copy link
Collaborator

JelleZijlstra commented Feb 21, 2020

Is there a better way to get the types from typing and builtins?

Probably we should get them from the stubs for the appropriate Python version mypy is targeting, but I don't know off hand how to do that.

Copy link
Collaborator

msullivan left a comment

This seems to break a bunch of existing cases.

To figure out what names to worry about, I'd just use the list from TYPES_FOR_UNIMPORTED_HINTS in semanal.py (which can be moved out of it).

Please also make sure to add tests.

Thanks!

@Muks14x

This comment has been minimized.

Copy link
Contributor Author

Muks14x commented Feb 21, 2020

@msullivan Thanks for the suggestions! I've updated some older outputs that needed changing, I'll try and add a couple of new test cases too.

Probably we should get them from the stubs for the appropriate Python version mypy is targeting, but I don't know off hand how to do that.

@JelleZijlstra Thank you! I don't know how to load version-specific stubs though, could you tell me where to start looking?

@Muks14x

This comment has been minimized.

Copy link
Contributor Author

Muks14x commented Feb 21, 2020

To figure out what names to worry about, I'd just use the list from TYPES_FOR_UNIMPORTED_HINTS in semanal.py (which can be moved out of it).

Oh, sorry, I overlooked this. I'll look for the names there, thanks!

@Muks14x Muks14x force-pushed the Muks14x:master branch from 4bfbf9d to ae306f4 Feb 22, 2020
@Muks14x Muks14x force-pushed the Muks14x:master branch from ae306f4 to e698eb1 Feb 22, 2020
@Muks14x

This comment has been minimized.

Copy link
Contributor Author

Muks14x commented Feb 22, 2020

I've added a test case, but I think I might've added it to the wrong place. Does it belong somewhere else?

for type in types:
for inst in collect_all_instances(type):
d.setdefault(inst.type.name, set()).add(inst.type.fullname)
for shortname in d.keys():
if shortname in builtin_types:

This comment has been minimized.

Copy link
@msullivan

msullivan Feb 26, 2020

Collaborator

I would drop the builtins_types part of this

@msullivan

This comment has been minimized.

Copy link
Collaborator

msullivan commented Feb 26, 2020

Test is in the right place!

@Muks14x

This comment has been minimized.

Copy link
Contributor Author

Muks14x commented Feb 26, 2020

I've removed the overlap-check for builtins. Is there a better way to do the check? I guess dir(builtins) is hacky and could cause incorrect messages if the target python version is very different.

Thanks for the help!

class Any: pass

x = Any()
x = 1 # E: Incompatible types in assignment (expression has type "int", variable has type "__main__.Any")

This comment has been minimized.

Copy link
@JukkaL

JukkaL Feb 26, 2020

Collaborator

It would be good to test a few other type names as well, such as list / List. (Lower-case list might not work, since it's not in the TYPES_FOR_UNIMPORTED_HINTS set.)

This comment has been minimized.

Copy link
@Muks14x

Muks14x Feb 26, 2020

Author Contributor

I've added a few more types from TYPES_FOR_UNIMPORTED_HINTS.

Copy link
Collaborator

msullivan left a comment

Great!

@msullivan msullivan merged commit ef0b0df into python:master Feb 27, 2020
4 checks passed
4 checks passed
build (windows-py37-32)
Details
build (windows-py37-64)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.