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 Muks14x commented Feb 21, 2020

Resolves #8372.

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

@JelleZijlstra
Copy link
Member

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 msullivan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
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
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
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?

mypy/messages.py Outdated
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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would drop the builtins_types part of this

@msullivan
Copy link
Collaborator

Test is in the right place!

@Muks14x
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")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a few more types from TYPES_FOR_UNIMPORTED_HINTS.

Copy link
Collaborator

@msullivan msullivan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@msullivan msullivan merged commit ef0b0df into python:master Feb 27, 2020
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.

Use fully qualified name for class with confusing name such as "Any"
4 participants