KEMBAR78
Use fully qualified names for ambiguous class names resembling builtins. by Muks14x · Pull Request #8425 · python/mypy · GitHub
Skip to content

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