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

Add recommendation to use string literal escaping #7707

Merged
merged 10 commits into from Oct 21, 2019

Conversation

@Coronon
Copy link
Contributor

Coronon commented Oct 13, 2019

This commit introduces a recommendation for classes that don't support indexing at runtime to be escaped as string literals on missing required generic type args (BARE_GENERIC) error.

It should resolve #7539

This is my first time contributing here, so please let me know if there is anything I could have done better. :)

Coronon added 3 commits Oct 13, 2019
@@ -907,8 +911,13 @@ def get_omitted_any(disallow_any: bool, fail: FailCallback,
typ = unexpanded_type or typ
type_str = typ.name if isinstance(typ, UnboundType) else format_type_bare(typ)

fail(message_registry.BARE_GENERIC.format(quote_type_string(type_str)), typ,
code=codes.TYPE_ARG)
if type_str in GENERIC_STUB_NOT_AT_RUNTIME_TYPES:

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum Oct 13, 2019

Member

I'm not sure I understand what's special about OrderedDict. The same thing would happen with any type that's defined both in collections and in typing. And in general I'm not sure that this particular hint belongs in the error message.

This comment has been minimized.

Copy link
@Coronon

Coronon Oct 14, 2019

Author Contributor

@JukkaL wanted this little reminder in the error message:
#7539 (comment)

Indeed OrderedDicts are not unique that way, Queue would also have to go into the list.
This error message is useful because OderedDict[Any, Any] would pass mypy checks atm. but wouldn’t run. Many users would probably have difficulties understanding the problem and finding a fix.

So this is purely for usability.

@Coronon

This comment has been minimized.

Copy link
Contributor Author

Coronon commented Oct 14, 2019

Couldn’t we just check this dynamically with

getattr(typ, "__getitem__", None)
@gvanrossum

This comment has been minimized.

Copy link
Member

gvanrossum commented Oct 14, 2019

But I believe you can use ‘from typing import OrderedDict’ and the problem will go away. Is that not so?

@Coronon

This comment has been minimized.

Copy link
Contributor Author

Coronon commented Oct 14, 2019

No, this would indeed run(runtime), but raises a mypy error:
Your suggested code (I think):

from typing import OrderedDict
from typing import Any

def name(value: OrderedDict[Any, Any]) -> None:
    pass

The error it raises when mypy checked:

error: Variable "typing.OrderedDict" is not valid as a type
Found 1 error in 1 file (checked 1 source file)
@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Oct 14, 2019

Variable "typing.OrderedDict" is not valid as a type

This is just a bug that should be fixed by updating mypy.nodes.type_aliases and mypy.nodes.type_aliases_target_versions (as you can see OrderedDict is not there yet).

@Coronon

This comment has been minimized.

Copy link
Contributor Author

Coronon commented Oct 14, 2019

Should this little reminder still be added or not, I just saw the 'good-first-issue' by JukkaL and wanted to contribute. I think what this boils down to is just usability for new users, like me.

Variable "typing.OrderedDict" is not valid as a type

This is just a bug that should be fixed by updating mypy.nodes.type_aliases and mypy.nodes.type_aliases_target_versions (as you can see OrderedDict is not there yet).

I could make a new pr with this fix, or just add it to this one :)

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Oct 14, 2019

Should this little reminder still be added or not, I just saw the 'good-first-issue' by JukkaL and wanted to contribute. I think what this boils down to is just usability for new users, like me.

I think it still makes sense, but:

  • This should be made a note on a separate line saying something like Subscripting classes that are not generic at runtime may require escaping, see https://mypy.readthedocs.io/en/latest/common_issues.html#using-classes-that-are-generic-in-stubs-but-not-at-runtime. Not an alternative error message.
  • We should probably update the docs to make this link shorter
  • The note should be shown only for classes in a blacklist that are known to cause problems, and OrderedDict should not be in this list.
@Coronon

This comment has been minimized.

Copy link
Contributor Author

Coronon commented Oct 14, 2019

* We should probably update the docs to make this link shorter

Would you add a new doc page or just make the name shorter?

* The note should be shown only for classes in a blacklist that are known to cause problems, and `OrderedDict` should not be in this list.

My PR introduces such a list, I will remove OD`s

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Oct 14, 2019

I could make a new pr with this fix, or just add it to this one :)

I think a separate PR is better

Would you add a new doc page or just make the name shorter?

I am not a big expert in Sphinx, so I hope there is a way to generate a shorter link without changing the name. If not then you can try finding a better shorter title for that section.

@Coronon

This comment has been minimized.

Copy link
Contributor Author

Coronon commented Oct 14, 2019

Should this little reminder still be added or not, I just saw the 'good-first-issue' by JukkaL and wanted to contribute. I think what this boils down to is just usability for new users, like me.

I think it still makes sense, but:

* This should be made a note on a separate line saying something like `Subscripting classes that are not generic at runtime may require escaping, see https://mypy.readthedocs.io/en/latest/common_issues.html#using-classes-that-are-generic-in-stubs-but-not-at-runtime`. Not an alternative error message.

* We should probably update the docs to make this link shorter

This has the note working and a shorter link to the doc https://mypy.readthedocs.io/en/latest/common_issues.html#not-generic-runtime

I have tested this with OrderedDict and then removed it from the blacklist, the note is displayed after the error with the note function I routet to mypy.typeanal.get_omitted_any from all callers I could find

Copy link
Collaborator

ilevkivskyi left a comment

Thanks for the updates! This looks almost ready, I have few more comments.

@@ -533,6 +533,7 @@ Here's the above example modified to use ``MYPY``:
def listify(arg: 'bar.BarClass') -> 'List[bar.BarClass]':
return [arg]
.. _not-generic-runtime:

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Oct 16, 2019

Collaborator

Reading now through this section I think it makes sense to add couple sentences at the end about using from __future__ import annotations as a (better) alternative to string quotes on Python 3.7+.

@@ -55,6 +55,10 @@
'mypy_extensions.KwArg': ARG_STAR2,
} # type: Final

GENERIC_STUB_NOT_AT_RUNTIME_TYPES = {
'Queue'

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Oct 16, 2019

Collaborator

I would also add os.PathLike here.

@@ -892,9 +902,10 @@ def tuple_type(self, items: List[Type]) -> TupleType:

# Mypyc doesn't support callback protocols yet.
FailCallback = Callable[[str, Context, DefaultNamedArg(Optional[ErrorCode], 'code')], None]
NoteCallback = Callable[[str, Context, DefaultNamedArg(Optional[ErrorCode], 'code')], None]

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Oct 16, 2019

Collaborator

This alias is identical to the above one. You can rename it to MsgCallback and use it to annotate both fail and note.

typ,
code=codes.TYPE_ARG)

if type_str in GENERIC_STUB_NOT_AT_RUNTIME_TYPES:

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Oct 16, 2019

Collaborator

It is not safe to make this decision using type_str, you should use fullname like few lines above.

@Coronon

This comment has been minimized.

Copy link
Contributor Author

Coronon commented Oct 16, 2019

Thanks for the updates! This looks almost ready, I have few more comments.

Implemented all things as requested (hopefully) :)

note(
"Subscripting classes that are not generic at runtime may require "
"escaping. If you are running Python 3.7+ you can simply use "
"`from __future__ import annotations`, otherwise please "

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Oct 16, 2019

Collaborator

I actually wanted to add this to the docs themselves, not here. Please keep the note as it was, and instead add this sentence (plus maybe also a link to PEP 563) at the end of the docs section.

Coronon and others added 3 commits Oct 17, 2019
Ivan Levkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Thanks for updates! I pushed couple small tweaks, will merge this soon.

Ivan Levkivskyi
@ilevkivskyi ilevkivskyi merged commit 6db1663 into python:master Oct 21, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Oct 25, 2019

@Coronon

I could make a new pr with this fix, or just add it to this one :)

Do you still want to make a PR for the Variable "typing.OrderedDict" is not valid as a type issue?

@Coronon

This comment has been minimized.

Copy link
Contributor Author

Coronon commented Oct 25, 2019

I can make it tomorrow if you want :)

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Oct 25, 2019

I can make it tomorrow if you want :)

OK, good.

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