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

move dict_keys, dict_items, and dict_values to builtins #12989

Closed
wants to merge 2 commits into from

Conversation

tungol
Copy link
Contributor

@tungol tungol commented Nov 10, 2024

these types call themselves builtins.* which is probably correct, even if they're not exposed there. Maybe this is a problematic idea? I'm curious to see what mypy-primer thinks.

Within _collections_abc.pyi, using dict_keys = _dict_keys instead of import dict_keys as dict_keys is to work around the fact that builtins.dict_keys is marked as type_check_only.

This comment has been minimized.

Copy link
Contributor

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

prefect (https://github.com/PrefectHQ/prefect)
+ src/prefect/cli/deploy.py:1134: error: Invalid index type "str | None" for "dict[str, str]"; expected type "str"  [index]
+ src/prefect/cli/deploy.py:1140: error: Argument "storage_provider" to "prompt_select_blob_storage_credentials" has incompatible type "str | None"; expected "str"  [arg-type]

spark (https://github.com/apache/spark)
+ python/pyspark/sql/types.py:554: error: Invalid index type "int | None" for "dict[int, str]"; expected type "int"  [index]
+ python/pyspark/sql/types.py:555: error: Invalid index type "int | None" for "dict[int, str]"; expected type "int"  [index]
+ python/pyspark/sql/types.py:566: error: Incompatible types in string interpolation (expression has type "int | None", placeholder has type "int | float | SupportsInt")  [str-format]
+ python/pyspark/sql/types.py:617: error: Invalid index type "int | None" for "dict[int, str]"; expected type "int"  [index]
+ python/pyspark/sql/types.py:618: error: Invalid index type "int | None" for "dict[int, str]"; expected type "int"  [index]
+ python/pyspark/sql/types.py:647: error: Incompatible types in string interpolation (expression has type "int | None", placeholder has type "int | float | SupportsInt")  [str-format]
+ python/pyspark/sql/connect/types.py:159: error: Incompatible types in assignment (expression has type "int | None", variable has type "int")  [assignment]
+ python/pyspark/sql/connect/types.py:160: error: Incompatible types in assignment (expression has type "int | None", variable has type "int")  [assignment]
+ python/pyspark/sql/connect/types.py:162: error: Incompatible types in assignment (expression has type "int | None", variable has type "int")  [assignment]
+ python/pyspark/sql/connect/types.py:163: error: Incompatible types in assignment (expression has type "int | None", variable has type "int")  [assignment]

Tanjun (https://github.com/FasterSpeeding/Tanjun)
+ tanjun/clients.py:451: error: Non-overlapping equality check (left operand type: "dict_keys[MessageAcceptsEnum, Optional[type[MessageCreateEvent]]]", right operand type: "set[MessageAcceptsEnum]")  [comparison-overlap]

optuna (https://github.com/optuna/optuna)
+ optuna/samplers/_brute_force.py:50: error: Non-overlapping equality check (left operand type: "dict_keys[float, _TreeNode]", right operand type: "set[float]")  [comparison-overlap]

mypy (https://github.com/python/mypy)
+ mypy/semanal_typeddict.py:348: error: Non-overlapping equality check (left operand type: "dict_keys[str, Expression]", right operand type: "Set[str]")  [comparison-overlap]
+ mypy/semanal_typeddict.py:348: note: See https://mypy.rtfd.io/en/stable/_refs.html#code-comparison-overlap for more info
+ mypy/checkpattern.py:183: error: Non-overlapping equality check (left operand type: "dict_keys[Var, List[Tuple[Expression, Type]]]", right operand type: "Set[Var]")  [comparison-overlap]

@tungol
Copy link
Contributor Author

tungol commented Nov 10, 2024

I think all the diffs in mypy-primer are from bypassing special handling of narrowing against dict.keys() or dict.items().

The pytype errors look like they're a result of pytype maintaining a separate set of stubs for builtins which include dict_keys/values already; those definitions are only singly generic.

Neither should be too hard to fix, so this is probably workable.

@tungol tungol marked this pull request as draft November 10, 2024 02:57
@JelleZijlstra
Copy link
Member

I'd rather keep builtins.pyi free of things that aren't builtins.

@tungol
Copy link
Contributor Author

tungol commented Nov 10, 2024

Can you say more about your reasoning in this case? It's not actually exposed in the builtins module, but e.g. dict_keys is closely tied to dict and it's not officially exposed anywhere else. I'd think that conceptually it is in builtins. @type_check_only takes care of the part where it's not exposed there.

>>> import _collections_abc
>>> _collections_abc.dict_keys.__module__
'builtins'

I realize that's kind of just an implementation detail of it not having any other module name specified. But since it's presence in _collections_abc is undocumented and it's closely tied to builtins.dict, that seems to me to be more correct than _collections_abc.dict_keys.

This is kind of related to the discuss thread I started recently about naming consistency. I didn't mention _collections_abc.dict_keys / _collections_abc.dict_items / _collections_abc.dict_values in my accounting there since those aren't documented exposures, but they do show up in the same modified version of stubtest that I've been playing with. If these had been added to types, that'd be a different situation.

I could flip all the imports back to using _collections_abc.dict_keys - it makes sense to keep importing from there since that's what's possible at runtime. Would that be any better, or is your central objection to having the classes defined in builtins.pyi at all?

@tungol
Copy link
Contributor Author

tungol commented Nov 10, 2024

Upon additional reflection, I suspect it's more about the fact that everything in builtins.pyi is treated as available without an import? That would make sense and an angle I was failing to consider.

@JelleZijlstra
Copy link
Member

Yes, the builtins module is special, and putting things in there when they are not builtins at runtime is confusing. (We also have a few other types in builtins.pyi that shouldn't be there, like builtins.function. However, that's something we should eventually fix.)

As you've noted, the module attribute of various types is often wrong.

@tungol tungol closed this Nov 10, 2024
@tungol tungol deleted the dict branch November 11, 2024 04:07
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.

2 participants