Fix crash involving explicit any flag and Required #12039

hmc-cs-mdrissi · 2022-01-22T06:38:56Z

Description

Fixes #12038. This adds an accept method to Required that delegates to the inner type. @davidfstr

Test Plan

I ran the tests locally and they passed. I added two small test cases to check-typeddict.test that correspond to fixed crash.

A5rocks

I can't approve the workflow runs cause I'm not a mypy maintainer. (By the way, don't take these comments too seriously, I don't really know too much about mypy internals)

A5rocks · 2022-01-22T07:45:09Z

test-data/unit/fixtures/typing-typeddict.pyi

@@ -43,7 +43,7 @@ class Iterator(Iterable[T_co], Protocol):
    def __next__(self) -> T_co: pass

 class Sequence(Iterable[T_co]):
-    def __getitem__(self, n: Any) -> T_co: pass
+    def __getitem__(self, n: Any) -> T_co: pass # type: ignore


Why is this # type ignored? Perhaps leave a note for future viewers?

It has an Any and my new test case checks for explicit Any with typeddict stubs. Alternatively I could add silent import flag to the test case.

What you now makes sense then, I suppose putting a specific code on the type: ignore (like, if you run with --show-error-codes it will tell you what code to put) would be appreciated. But I don't know if that's the style tests normally take so...

I suppose putting a specific code on the type: ignore [...] would be appreciated

+1

I've added the error code. The code is vague [misc] so I included a comment too on which error it was.

A5rocks · 2022-01-22T07:48:07Z

mypy/types.py

@@ -360,6 +360,9 @@ def __repr__(self) -> str:
        else:
            return "NotRequired[{}]".format(self.item)

+    def accept(self, visitor: 'TypeVisitor[T]') -> T:


This feels icky, but I presume it's needed?

I think I would have preferred fixing this one layer up -- whatever is trying to figure out if this is a Required should maybe know (this only works if that one layer up is related to TypedDicts which it probably isn't).

The caller is this function, https://github.com/python/mypy/blob/master/mypy/typeanal.py#L1304. It doesn't feel like that function should need to have knowledge of typeddicts/required. This function only wants to check does an Annotation include Any somewhere in it.

The caller above that is https://github.com/python/mypy/blob/master/mypy/semanal_typeddict.py#L305. This does look like a reasonable place. If you would prefer I have the typeddict semantic analyzer strip Required/NotRequired before calling explicit any check I can do that.

Also I appreciate the comments. This is my first pr/time looking at mypy's actual code. I've been using it for a while, but have never had a bug that looked small enough I could just do and this made me explore mypy code.

I think either approach works:

Your current approach is more broad and will generally fix the problem with accepting a Required anywhere, as long as just having the item accept it is what is always meant (it sounds like it might be).

Just changing the function above is less broad but also has less chance of accidentally introducing subtly wrong behavior.

I think the "subtly wrong behavior" is so unlikely that whatever approach is easier will work.

davidfstr · 2022-01-23T17:05:38Z

Thanks for the PR @hmc-cs-mdrissi !

Looks like the underlying problem fixed here is that RequiredType (directly) extends Type but did not previously implement the abstract method Type.accept. @hmc-cs-mdrissi I think defining the accept method to directly enter the enclosed item, as you have done, is the right call here. 👍

davidfstr · 2022-01-23T17:10:46Z

I notice that TypeGuardedType also directly extends Type and does not implement Type.accept, so there's probably a similar crash issue with TypeGuardedType.

CC @erictraut - TypeGuard PEP author
CC @hauntsaninja - most recent contributor to make a major change to TypeGuardedType
CC @gvanrossum - original contributor for TypeGuardType

davidfstr · 2022-01-23T17:18:38Z

It would be nice if running mypy on itself would highlight that RequiredType (and TypeGuardedType) fail to implement abstract method Type.accept, rather than that error being discovered at runtime.

A cursory inspection of the mypy documentation suggests that mypy understands an "abstract method" in the context of explicitly-defined Abstract Base Classes. So it might be possible to mark Type.accept with @abc.abstractmethod to get mypy to flag concrete classes like TypeGuardedType which fail to implement accept. However I'm not familar with ABCs so I don't know if that would actually work.

gvanrossum · 2022-01-23T21:37:41Z

Sadly I no longer remember enough of mypy internals to be useful here. There are several new maintainers though!

hmc-cs-mdrissi · 2022-01-24T01:32:37Z

Typeguard doesn't crash, but that's because it doesn't even check for explicit Any. This code,

def foo(x: object) -> TypeGuard[Any]:
  ...

when run with mypy --disallow-any-explicit produces no type errors. So looks like typeguard probably needs an accept and also to check for any.

As for abstract method issue, that works. If I add abstractmethod + ABC to Type class it produces a type error for TypeGuardType.

I'll work on a small separate pr for abstract class/method for accept.

edit: The typeguard issue is less clear to me. Some debugging the reason typeguard explicit any checks does nothing is return type is normalized to bool before explicit Any check happens. I'll make a separate bug ticket for it and may take another look later.

davidfstr · 2022-01-26T16:39:50Z

LGTM, with the new changes.

I cannot merge this PR myself. @JukkaL ?

Mehdi Drissi added 3 commits January 21, 2022 22:12

Fix crash involving explicit any flag and Required

81085b6

Add unit test for explicit Any Required.

30432b0

fix unit test.

ebdd6c0

A5rocks reviewed Jan 22, 2022

View reviewed changes

Add error code to type ignore for explicitness

ce1883f

This was referenced Jan 24, 2022

Explicit Any check for TypeGuard #12049

Open

Fix TypeGuard Explicit Any Check #12050

Open

JelleZijlstra merged commit d5d077b into python:master Jan 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crash involving explicit any flag and Required #12039

Fix crash involving explicit any flag and Required #12039

hmc-cs-mdrissi commented Jan 22, 2022 •

edited

Loading

A5rocks left a comment

A5rocks Jan 22, 2022

hmc-cs-mdrissi Jan 22, 2022

A5rocks Jan 22, 2022

davidfstr Jan 23, 2022

hmc-cs-mdrissi Jan 24, 2022

A5rocks Jan 22, 2022

hmc-cs-mdrissi Jan 22, 2022 •

edited

Loading

A5rocks Jan 22, 2022

davidfstr commented Jan 23, 2022

davidfstr commented Jan 23, 2022

davidfstr commented Jan 23, 2022 •

edited

Loading

gvanrossum commented Jan 23, 2022

hmc-cs-mdrissi commented Jan 24, 2022 •

edited

Loading

davidfstr commented Jan 26, 2022

Fix crash involving explicit any flag and Required #12039

Fix crash involving explicit any flag and Required #12039

Conversation

hmc-cs-mdrissi commented Jan 22, 2022 • edited Loading

Description

Test Plan

A5rocks left a comment

Choose a reason for hiding this comment

A5rocks Jan 22, 2022

Choose a reason for hiding this comment

hmc-cs-mdrissi Jan 22, 2022

Choose a reason for hiding this comment

A5rocks Jan 22, 2022

Choose a reason for hiding this comment

davidfstr Jan 23, 2022

Choose a reason for hiding this comment

hmc-cs-mdrissi Jan 24, 2022

Choose a reason for hiding this comment

A5rocks Jan 22, 2022

Choose a reason for hiding this comment

hmc-cs-mdrissi Jan 22, 2022 • edited Loading

Choose a reason for hiding this comment

A5rocks Jan 22, 2022

Choose a reason for hiding this comment

davidfstr commented Jan 23, 2022

davidfstr commented Jan 23, 2022

davidfstr commented Jan 23, 2022 • edited Loading

gvanrossum commented Jan 23, 2022

hmc-cs-mdrissi commented Jan 24, 2022 • edited Loading

davidfstr commented Jan 26, 2022

hmc-cs-mdrissi commented Jan 22, 2022 •

edited

Loading

hmc-cs-mdrissi Jan 22, 2022 •

edited

Loading

davidfstr commented Jan 23, 2022 •

edited

Loading

hmc-cs-mdrissi commented Jan 24, 2022 •

edited

Loading