Add recommendation to use string literal escaping #7707

Coronon · 2019-10-13T18:11:54Z

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

gvanrossum · 2019-10-13T23:14:59Z

mypy/typeanal.py

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


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.

@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 · 2019-10-14T08:31:50Z

Couldn’t we just check this dynamically with

getattr(typ, "__getitem__", None)

gvanrossum · 2019-10-14T15:53:13Z

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

Coronon · 2019-10-14T16:11:19Z

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 · 2019-10-14T16:17:59Z

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 · 2019-10-14T16:44:24Z

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 · 2019-10-14T16:58:28Z

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 · 2019-10-14T17:17:06Z

* 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 · 2019-10-14T17:55:45Z

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 · 2019-10-14T19:25:37Z

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

ilevkivskyi

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

ilevkivskyi · 2019-10-16T14:36:15Z

docs/source/common_issues.rst

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


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

ilevkivskyi · 2019-10-16T14:37:18Z

mypy/typeanal.py

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

+GENERIC_STUB_NOT_AT_RUNTIME_TYPES = {
+    'Queue'


I would also add os.PathLike here.

ilevkivskyi · 2019-10-16T14:38:43Z

mypy/typeanal.py

@@ -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 alias is identical to the above one. You can rename it to MsgCallback and use it to annotate both fail and note.

ilevkivskyi · 2019-10-16T14:44:07Z

mypy/typeanal.py

+                typ,
+                code=codes.TYPE_ARG)
+
+            if type_str in GENERIC_STUB_NOT_AT_RUNTIME_TYPES:


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

Coronon · 2019-10-16T19:42:43Z

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

Implemented all things as requested (hopefully) :)

ilevkivskyi · 2019-10-16T20:55:58Z

mypy/typeanal.py

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


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.

ilevkivskyi

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

ilevkivskyi · 2019-10-25T15:50:55Z

@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 · 2019-10-25T20:31:41Z

I can make it tomorrow if you want :)

ilevkivskyi · 2019-10-25T20:59:25Z

I can make it tomorrow if you want :)

OK, good.

Coronon added 3 commits October 13, 2019 22:04

Add recommendation to use string literal escaping

e30bea6

PEP8 compliant

3983f50

PEP8 compliant2

56de035

gvanrossum reviewed Oct 13, 2019

View reviewed changes

Changed recom to note and added shorter doc anchor

5455ea4

fixed multiple values for argument 'note' + PEP8

75ff95d

ilevkivskyi reviewed Oct 16, 2019

View reviewed changes

annotations, MsgCallback, os.Pathlike, fullname

45e12fb

ilevkivskyi reviewed Oct 16, 2019

View reviewed changes

Coronon and others added 3 commits October 17, 2019 12:08

moved annotations to docs

3ededa1

Tweak docs

98697b8

Add test (and fix it)

e7ea063

ilevkivskyi approved these changes Oct 21, 2019

View reviewed changes

Fix test on Python 3.5

5a1245d

ilevkivskyi merged commit 6db1663 into python:master Oct 21, 2019

septatrix mentioned this pull request Aug 31, 2020

Add typing.OrderedDict to type_aliases #9389

Merged

Uh oh!

Add recommendation to use string literal escaping #7707

Add recommendation to use string literal escaping #7707

Uh oh!

Conversation

Coronon commented Oct 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum Oct 13, 2019

Choose a reason for hiding this comment

Uh oh!

Coronon Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Coronon commented Oct 14, 2019

Uh oh!

gvanrossum commented Oct 14, 2019

Uh oh!

Coronon commented Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilevkivskyi commented Oct 14, 2019

Uh oh!

Coronon commented Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilevkivskyi commented Oct 14, 2019

Uh oh!

Coronon commented Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilevkivskyi commented Oct 14, 2019

Uh oh!

Coronon commented Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

Coronon commented Oct 16, 2019

Uh oh!

ilevkivskyi Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi commented Oct 25, 2019

Uh oh!

Coronon commented Oct 25, 2019

Uh oh!

ilevkivskyi commented Oct 25, 2019

Uh oh!

Uh oh!

Coronon commented Oct 13, 2019 •

edited

Loading

Coronon Oct 14, 2019 •

edited

Loading

Coronon commented Oct 14, 2019 •

edited

Loading

Coronon commented Oct 14, 2019 •

edited

Loading

Coronon commented Oct 14, 2019 •

edited

Loading

Coronon commented Oct 14, 2019 •

edited

Loading