Skip to content

Conversation

@ilevkivskyi
Copy link
Member

This used to be a source of code duplication, and also using the "native" visitor will allow us to implement new features for type aliases easily (e.g. variadic type aliases, that I am going to do in next PR).

@github-actions

This comment has been minimized.

@JelleZijlstra
Copy link
Member

Looking at mypy-primer:

  • The pytorch/vision error is here. The new error is correct; x[1] is a module and ModuleType.__file__ is str | None. Mypy got better at type inference here.
  • The pandas hits are here. Very similar situation where mypy now infers a more precise type for an inspect.getmembers call.
  • steam.py is here, another inspect.getmembers call. The code is buggy and the mypy error seems like a false positive, but again it looks like this PR infers more precise types. The second steam.py hit looks similar.

In conclusion, the PR does a better job of inferring types around inspect.getmembers.

@AlexWaygood
Copy link
Member

AlexWaygood commented May 8, 2023

For context, the more precise types we currently have for inspect.getmembers() in typeshed were added quite recently, in python/typeshed#9891 (by @Gobot1234, the author of steam.py). I noted in that PR thread that mypy didn't seem able to properly understand the new types we were adding (but merged the PR anyway, as it didn't seem to produce any mypy regressions, and was a clear improvement for pyright). It's great that it now can!

Could we add a test case for functions where a parameter is annotated with an alias to a generic TypeGuard?

@ilevkivskyi
Copy link
Member Author

Yeah, it looks like a bug (or maybe not) in TypeTranslator, it doesn't translate the .type_guard in visit_callable_type(). Since ExpandTypeVisitor overrides it with some non-trivial logic that also includes .type_guard.accept(self) it worked there, but InstantiateAliasVisitor simply inherited the method before.

For now I will just add a test.

@github-actions
Copy link
Contributor

github-actions bot commented May 8, 2023

Diff from mypy_primer, showing the effect of this PR on open source code:

vision (https://github.com/pytorch/vision) got 1.25x slower (42.3s -> 52.9s)
+ torchvision/models/_api.py:125: error: Item "None" of "str | None" has no attribute "endswith"  [union-attr]

steam.py (https://github.com/Gobot1234/steam.py)
- steam/state.py:2696: error: Need type annotation for "func"  [var-annotated]
- steam/state.py:2696: error: Argument 2 to "getmembers" has incompatible type "Callable[[Any], Any | bool]"; expected "Callable[[Any], TypeGuard[_T]]"  [arg-type]
+ steam/state.py:2696: error: Argument 2 to "getmembers" has incompatible type "Callable[[Any], Any | bool]"; expected "Callable[[Any], TypeGuard[Any | bool]]"  [arg-type]
+ steam/state.py:2697: error: Argument 1 to "get_annotations" has incompatible type "Any | bool"; expected "Callable[..., object] | type[Any] | Module"  [arg-type]
+ steam/state.py:2699: error: Incompatible types in assignment (expression has type "Any | bool", target has type "ParserCallback[ConnectionState, ProtobufMessage | Message | GCProtobufMessage | GCMessage]")  [assignment]
+ steam/state.py:2701: error: Incompatible types in assignment (expression has type "Any | bool", target has type "ParserCallback[ConnectionState, ProtobufMessage | Message | GCProtobufMessage | GCMessage]")  [assignment]
- steam/_gc/state.py:82: error: Need type annotation for "func"  [var-annotated]
- steam/_gc/state.py:82: error: Argument 2 to "getmembers" has incompatible type "Callable[[Any], bool]"; expected "Callable[[Any], TypeGuard[_T]]"  [arg-type]
+ steam/_gc/state.py:82: error: Argument 2 to "getmembers" has incompatible type "Callable[[Any], bool]"; expected "Callable[[Any], TypeGuard[bool]]"  [arg-type]
+ steam/_gc/state.py:83: error: Argument 1 to "get_annotations" has incompatible type "bool"; expected "Callable[..., object] | type[Any] | Module"  [arg-type]
+ steam/_gc/state.py:87: error: Incompatible types in assignment (expression has type "bool", target has type "ParserCallback[GCState[Any], GCProtobufMessage | GCMessage]")  [assignment]

spark (https://github.com/apache/spark)
+ python/pyspark/pandas/supported_api_gen.py:160: error: Argument 2 to "_organize_by_implementation_status" has incompatible type "dict[str, FunctionType]"; expected "dict[str, Callable[..., Any]]"  [arg-type]
+ python/pyspark/pandas/supported_api_gen.py:160: note: "Dict" is invariant -- see https://mypy.readthedocs.io/en/stable/common_issues.html#variance
+ python/pyspark/pandas/supported_api_gen.py:160: note: Consider using "Mapping" instead, which is covariant in the value type
+ python/pyspark/pandas/supported_api_gen.py:160: error: Argument 3 to "_organize_by_implementation_status" has incompatible type "dict[str, FunctionType]"; expected "dict[str, Callable[..., Any]]"  [arg-type]

materialize (https://github.com/MaterializeInc/materialize) got 1.11x faster (80.3s -> 72.5s)

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thank you! And thanks Jelle and Alex for looking through the primer diff

@hauntsaninja hauntsaninja merged commit 35acb1b into python:master May 9, 2023
@ilevkivskyi ilevkivskyi deleted the use-native-expand branch May 9, 2023 09:25
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.

4 participants