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.

Sign up for GitHub

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

Jump to bottom

fix default of dict.get #13222

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix default of dict.get #13222

wants to merge 3 commits into from
+2 −3

Conversation

tungol
Copy link
Contributor

@tungol tungol commented Dec 8, 2024

This seems like a straightforward fix for a relatively important class/method, so I'm wondering if I'm missing something.

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@tungol
Copy link
Contributor Author

tungol commented Dec 8, 2024

Seems like I was, in fact, missing something. That's quite a lot of noise from mypy primer.

@tungol tungol marked this pull request as draft December 8, 2024 11:15
@tungol
Copy link
Contributor Author

tungol commented Dec 8, 2024

After a little initial investigation of the diff from Alerta:

mydict: dict[str, Any] = {}
mydict.get("foo", None)

Without this change this returns Any. With this change, this returns Any | None. That seems basically correct, but a) I don't understand why it was returning just Any in the first place and b) I need to check more of the diffs to see if anything else is going on.

@tungol
Copy link
Contributor Author

tungol commented Dec 8, 2024

Just noticed that one of the diffs is from mypy itself, so that's something.

@tungol
Copy link
Contributor Author

tungol commented Dec 8, 2024

I figured out why dict.get('str', None) on dict[str, Any] right now.

The current definition is this:

@overload
def get(self, key: _KT, /) -> _VT | None: ...
@overload
def get(self, key: _KT, default: _VT, /) -> _VT: ...
@overload
def get(self, key: _KT, default: _T, /) -> _VT | _T: ...

with dict.get("str") we'll hit the first branch and return Any | None as expected. With dict.get("str", None), None is compatible with the _VT of Any, so we match the second branch and return Any. At first glance I expected it to reach the third branch and return Any | None, but that's not the case. A dict with a _VT of Any will never reach the third branch.

I still need to work through all the diffs from mypy primer, but for right now I'm making the assumption that this is the underlying cause for all of them. If that's the case, and we want to keep the current behavior, I think we can make this change and keep the current behavior by re-ordering the overload:

@overload
def get(self, key: _KT, default: _VT, /) -> _VT: ...
@overload
def get(self, key: _KT, default: None = None, /) -> _VT | None: ...
@overload
def get(self, key: _KT, default: _T, /) -> _VT | _T: ...

This would keep dict.get("str", None) -> Any. I'd need to see mypy-primer output, but I suspect this wouldn't change much else. At least, it doesn't change things for the dict[str, Any] case. That said, I don't know that we do want to keep the current behavior. It's a bit unexpected that:

d: dict[str, Any]
d.get("str") -> Any | None
d.get("str", None) -> Any

I don't think anyone expects that explicitly adding None as the default will cause the return type to not include None. That's pretty counterintuitive. I'll comment again once I've gone through all the mypy-primer results, but I think we might want to bite the bullet on this one.

@tungol
Copy link
Contributor Author

tungol commented Dec 8, 2024 •
edited
Loading

I'm not quite done with going through the mypy-primer results, and I'll have a full write-up later, but this is some really weird behavior:

Without this change:

from typing import Any
type ConfigType = dict[str, Any]
def foo(config: ConfigType) -> None:
    reveal_type(config)  # note: Revealed type is "builtins.dict[builtins.str, Any]"

    a = config["str"]
    reveal_type(a)  # note: Revealed type is "Any"
    
    b = config.get("str", a)
    reveal_type(b)  # note: Revealed type is "Any"

With this change:

from typing import Any
type ConfigType = dict[str, Any]
def foo(config: ConfigType) -> None:
    reveal_type(config)  # note: Revealed type is "builtins.dict[builtins.str, Any]"

    a = config["str"]
    reveal_type(a)  # note: Revealed type is "Any"
    
    b = config.get("str", a)
    reveal_type(b)  # note: Revealed type is "Union[Any, None]"

But with or without this change:

from typing import Any
type ConfigType = dict[str, Any]
def foo(config: ConfigType) -> None:
    reveal_type(config)  # note: Revealed type is "builtins.dict[builtins.str, Any]"

    a: Any = config["str"]
    reveal_type(a)  # note: Revealed type is "Any"
    
    b = config.get("str", a)
    reveal_type(b)  # note: Revealed type is "Any"

Also with or without this change:

from typing import Any
def foo(config: dict[str, Any]) -> None:
    reveal_type(config)  # note: Revealed type is "builtins.dict[builtins.str, Any]"

    a = config["str"]
    reveal_type(a)  # note: Revealed type is "Any"
    
    b = config.get("str", a)
    reveal_type(b)  # note: Revealed type is "Any"

I haven't explored the underlying cause of this yet, but it certainly feels bad.

@tungol tungol mentioned this pull request Dec 8, 2024
Closed
@tungol
Copy link
Contributor Author

tungol commented Dec 9, 2024

I created a python file which reproduces the mypy-primer diff for this change. The primer results are inline as comments. Since it's 540 lines long, I uploaded it as a gist instead of putting it here: https://gist.github.com/tungol/e792212afa4b89153b6479baa0fad3b2

Any | None where it was Any before

The bulk of the mypy-primer results are from getting Any | None somewhere that
was previously getting Any. The results from alerta, pylint, werkzeug,
spark, pandas, freqtrade, operator, colour, kornia, ignite, httpx_caching,
sphinx, dd-trace-py, mitmproxy, and cloud_init fall into this category.

Alerta is a clear example of the pattern. All of it's new errors are basically versions of this:

def foo(doc: dict[str, Any]):
    Note(
        text=doc.get("text", None),
        user=doc.get("user", None),
        note_type=doc.get("type", None),
    )
  • mitmproxy:

Nothing more to say about the first new error. The second new error is the
same issue, but with the twist
that they actually check that the result is the desired type, but then they
only issue a warning and keep going anyway.

  • cloud_init:

These are straightforward, with the extra detail that they're actually perfectly
safe, but mypy can't follow it. They're looking for either one of two keys
in the dict, and they do validate that at least one of them is present before
accessing them, but the check is too complicated for static analysis to follow.
It's more or less this:

if "ca-certs" in cfg:
    # issue a deprecation warning
elif "ca_certs" not in cfg:
    return

ca_cert_cfg = cfg.get("ca_certs", cfg.get("ca-certs"))

# do a bunch of stuff with ca_cert_cfg

Custom subclasses

The second category of new errors come from custom subclasses of dict, where
the new overload either doesn't match where it did before, or it didn't match
before but now it doesn't match in a slight differnt way. This covers the
results from steam.py, django-stubs, and ibis.

  • ibis:

A little interesting because Ibis duplicates classes from collections.abc as
non-abcs for performance reasons.
The "Mapping" referenced there is actually their copy of Mapping, not abc.collections.Mapping.

Other stuff

  • prefect:

Each of these is from a line like _locks.get(key, {}).get("lock"). The
message itself is a warning that no overload matched, and the diff is just
a change in the available overloads that we're not matching.

  • streamlit:

The diff is a new Returning Any from function declared to return "str" error.
My reproduction for this one is:

@dataclass
class KeyIdMapper:
    _key_id_mapping: dict[str, str] = field(default_factory=dict)

    def get_id_from_key(self, key: str, default: Any = None) -> str:
        return self._key_id_mapping.get(key, default)

I'm not sure yet why this is a new error for this MR, but it differs from
the first category because the default being passed is Any instead of None.

  • beartype:

This one is interesting becaue of the new "unused ignore" error.

Here's the reproduction:

class HintSign: ...
class ViolationCause: ...

hint_sign: HintSign = HintSign()
cause_finder: Callable[[ViolationCause], ViolationCause] = None
HINT_SIGN_TO_GET_CAUSE_FUNC: dict[
    HintSign, Callable[[ViolationCause], ViolationCause]
] = {}

cause_finder = HINT_SIGN_TO_GET_CAUSE_FUNC.get(
    hint_sign, None)  # type: ignore[arg-type]

Which, without this change, and if it wasn't ignored, would generate:

error: Argument 2 to "get" of "dict" has incompatible type "None"; expected "Callable[[ViolationCause], ViolationCause]" [arg-type]

With this change, we get this instead:

error: Incompatible types in assignment (expression has type "Callable[[ViolationCause], ViolationCause] | None", variable has type "Callable[[ViolationCause], ViolationCause]") [assignment]

Essentially, this change transfers the error from the method call to the return type, here.

  • mypy:

This one doesn't actually reproduce in the linked gist, because it's a mypy-specific warning. It's a new error in mypy.partially_defined.PossiblyUndefinedVariableVisitor.visit_expression_stmt:

error: Never apply isinstance() to unexpanded types; use mypy.types.get_proper_type() first

def visit_expression_stmt(self, o: ExpressionStmt) -> None:
    if isinstance(self.type_map.get(o.expr, None), (UninhabitedType, type(None))):
        self.tracker.skip_branch()
    super().visit_expression_stmt(o)

I haven't dug deeply into why this is a new error here with this change; I'm not
familiar with how this check works.

  • core / homeassistant

Most of these are the Any to Any | None issue. but one of them is the one
I highlighted in my earlier comment: #13222 (comment)

Something deeply hinky is going on with that one.

@tungol
Copy link
Contributor Author

tungol commented Dec 10, 2024 •
edited
Loading

The underlying cause of the weird behavior with a TypeAlias is that for dict[str, Any] the type of any for the Any is explicit. For type ConfigType = dict[str, Any], the type of any for the Any in there is special_form. The latter is not considered a real Any in this context, so selecting the first matching overload is allowed rather than checking all the overloads for consistency in the face of Any. This seems a little questionable?

That is:

from typing import Any, reveal_type

type MyType = dict[str, Any]
d: MyType = {}
d2: dict[str, Any] = {}

a = d["str"]  # this is a "special_form" Any
b = d2["str"]  # this is an "explicit" Any

This issue came up in python/mypy#15497 previously

@github-actions GitHub Actions
Copy link
Contributor

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

alerta (https://github.com/alerta/alerta)
+ alerta/models/permission.py:29: error: Argument "match" to "Permission" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/permission.py:49: error: Argument "match" to "Permission" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/note.py:34: error: Argument "text" to "Note" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/note.py:35: error: Argument "user" to "Note" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/note.py:37: error: Argument "note_type" to "Note" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/note.py:76: error: Argument "text" to "Note" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/note.py:77: error: Argument "user" to "Note" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/note.py:79: error: Argument "note_type" to "Note" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/key.py:51: error: Argument "user" to "ApiKey" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/key.py:53: error: Argument "text" to "ApiKey" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/key.py:55: error: Argument "customer" to "ApiKey" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/key.py:89: error: Argument "user" to "ApiKey" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/key.py:91: error: Argument 1 to "type_to_scopes" of "ApiKeyHelper" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/key.py:91: error: Argument 2 to "type_to_scopes" of "ApiKeyHelper" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/key.py:92: error: Argument "text" to "ApiKey" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/key.py:93: error: Argument "expire_time" to "ApiKey" has incompatible type "Any | None"; expected "datetime"  [arg-type]
+ alerta/models/key.py:96: error: Argument "customer" to "ApiKey" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/heartbeat.py:85: error: Argument "origin" to "Heartbeat" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/heartbeat.py:89: error: Argument "timeout" to "Heartbeat" has incompatible type "Any | None"; expected "int"  [arg-type]
+ alerta/models/heartbeat.py:90: error: Argument "customer" to "Heartbeat" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/heartbeat.py:120: error: Argument "origin" to "Heartbeat" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/heartbeat.py:124: error: Argument "create_time" to "Heartbeat" has incompatible type "Any | None"; expected "datetime"  [arg-type]
+ alerta/models/heartbeat.py:125: error: Argument "timeout" to "Heartbeat" has incompatible type "Any | None"; expected "int"  [arg-type]
+ alerta/models/heartbeat.py:129: error: Argument "customer" to "Heartbeat" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/group.py:32: error: Argument "id" to "GroupUser" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/group.py:33: error: Argument "name" to "GroupUser" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/group.py:34: error: Argument "login" to "GroupUser" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/group.py:35: error: Argument "status" to "GroupUser" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/group.py:84: error: Argument "name" to "Group" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/group.py:85: error: Argument "text" to "Group" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/group.py:106: error: Argument "name" to "Group" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/group.py:107: error: Argument "text" to "Group" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/customer.py:23: error: Argument "match" to "Customer" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/customer.py:24: error: Argument "customer" to "Customer" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/customer.py:44: error: Argument "match" to "Customer" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/customer.py:45: error: Argument "customer" to "Customer" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/alert.py:98: error: Argument "resource" to "Alert" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/alert.py:99: error: Argument "event" to "Alert" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/alert.py:173: error: Argument "resource" to "Alert" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/alert.py:174: error: Argument "event" to "Alert" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/user.py:66: error: Argument "login" to "User" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/user.py:68: error: Argument "email" to "User" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/user.py:72: error: Argument "text" to "User" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/user.py:107: error: Argument "name" to "User" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/user.py:109: error: Argument "password" to "User" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/user.py:110: error: Argument "email" to "User" has incompatible type "Any | None"; expected "str"  [arg-type]
+ alerta/models/user.py:116: error: Argument "text" to "User" has incompatible type "Any | None"; expected "str"  [arg-type]

pylint (https://github.com/pycqa/pylint)
+ pylint/config/utils.py:66: error: Argument "metavar" to "_CallableArgument" has incompatible type "Any | None"; expected "str"  [arg-type]

prefect (https://github.com/PrefectHQ/prefect)
- src/prefect/locking/memory.py:194: note:     def get(self, Never, /) -> None
+ src/prefect/locking/memory.py:194: note:     def get(self, Never, None = ..., /) -> None
- src/prefect/locking/memory.py:205: note:     def get(self, Never, /) -> None
+ src/prefect/locking/memory.py:205: note:     def get(self, Never, None = ..., /) -> None
- src/prefect/records/memory.py:176: note:     def get(self, Never, /) -> None
+ src/prefect/records/memory.py:176: note:     def get(self, Never, None = ..., /) -> None

werkzeug (https://github.com/pallets/werkzeug)
+ src/werkzeug/wsgi.py:78: error: Argument 1 to "int" has incompatible type "Optional[Any]"; expected "Union[str, Buffer, SupportsInt, SupportsIndex, SupportsTrunc]"  [arg-type]

ibis (https://github.com/ibis-project/ibis)
- ibis/common/collections.py:280: error: Definition of "get" in base class "dict" is incompatible with definition in base class "Mapping"  [misc]

steam.py (https://github.com/Gobot1234/steam.py)
- steam/ext/commands/utils.py:43: note:          def get(self, str, /) -> _VT | None
+ steam/ext/commands/utils.py:43: note:          def get(self, str, None = ..., /) -> _VT | None

streamlit (https://github.com/streamlit/streamlit)
+ lib/streamlit/runtime/state/session_state.py: note: In member "get_id_from_key" of class "KeyIdMapper":
+ lib/streamlit/runtime/state/session_state.py:314:9: error: Returning Any from function declared to return "str"  [no-any-return]

spark (https://github.com/apache/spark)
+ python/pyspark/sql/session.py:322: error: Dict entry 0 has incompatible type "str": "Any | None"; expected "str": "str"  [dict-item]
+ python/pyspark/sql/session.py:323: error: Dict entry 1 has incompatible type "str": "Any | None"; expected "str": "str"  [dict-item]
+ python/pyspark/sql/session.py:574: error: Item "None" of "Any | None" has no attribute "startswith"  [union-attr]

pandas (https://github.com/pandas-dev/pandas)
+ pandas/core/apply.py:1560: error: Dict entry 0 has incompatible type "str": "Any | None"; expected "str": "str | bool"  [dict-item]
+ pandas/core/apply.py:1560: error: Dict entry 1 has incompatible type "str": "Any | None"; expected "str": "str | bool"  [dict-item]

altair (https://github.com/vega/altair)
+ altair/utils/_vegafusion_data.py:230: error: Argument 1 to "handle_row_limit_exceeded" has incompatible type "Any | None"; expected "int"  [arg-type]
+ altair/utils/_vegafusion_data.py:281: error: Argument 1 to "handle_row_limit_exceeded" has incompatible type "Any | None"; expected "int"  [arg-type]

freqtrade (https://github.com/freqtrade/freqtrade)
+ freqtrade/data/entryexitanalysis.py:365: error: Argument 1 to "Path" has incompatible type "Any | None"; expected "str | PathLike[str]"  [arg-type]

operator (https://github.com/canonical/operator)
- ops/_private/harness.py:1193: error: Argument 2 to "get" of "dict" has incompatible type "None"; expected "Mapping[str, str]"  [arg-type]
+ ops/_private/harness.py:1193: error: Incompatible return value type (got "dict[str, str] | None", expected "Mapping[str, str]")  [return-value]

colour (https://github.com/colour-science/colour)
+ colour/utilities/network.py:1430: error: Item "None" of "Any | None" has no attribute "connect"  [union-attr]
+ colour/utilities/network.py:1475: error: Item "None" of "Any | None" has no attribute "disconnect"  [union-attr]

mitmproxy (https://github.com/mitmproxy/mitmproxy)
+ mitmproxy/io/compat.py:510: error: Argument 1 to "tuple" has incompatible type "Any | None"; expected "Iterable[Any]"  [arg-type]
+ mitmproxy/proxy/events.py:97: error: Invalid index type "Any | None" for "dict[Command, type[CommandCompleted]]"; expected type "Command"  [index]

beartype (https://github.com/beartype/beartype)
+ beartype/_check/error/errcause.py:551: error: Incompatible types in assignment (expression has type "Callable[[ViolationCause], ViolationCause] | None", variable has type "Callable[[ViolationCause], ViolationCause]")  [assignment]
+ beartype/_check/error/errcause.py:552: error: Unused "type: ignore" comment  [unused-ignore]

kornia (https://github.com/kornia/kornia)
+ kornia/augmentation/container/augment.py:502: error: Incompatible types in assignment (expression has type "Any | None", variable has type "list[DataKey]")  [assignment]

ignite (https://github.com/pytorch/ignite)
+ ignite/handlers/tqdm_logger.py:216: error: Argument 1 to "_OutputHandler" has incompatible type "Any | None"; expected "str"  [arg-type]

cloud-init (https://github.com/canonical/cloud-init)
+ cloudinit/config/cc_ca_certs.py:256: error: Unsupported right operand type for in ("Any | None")  [operator]
+ cloudinit/config/cc_ca_certs.py:262: error: Item "None" of "Any | None" has no attribute "get"  [union-attr]
+ cloudinit/config/cc_ca_certs.py:263: error: Item "None" of "Any | None" has no attribute "get"  [union-attr]
+ cloudinit/config/cc_ca_certs.py:269: error: Unsupported right operand type for in ("Any | None")  [operator]

httpx-caching (https://github.com/johtso/httpx-caching)
+ httpx_caching/_sync/_cache.py:20: error: Argument 1 to "loads" of "Serializer" has incompatible type "Any | None"; expected "bytes"  [arg-type]
+ httpx_caching/_async/_cache.py:20: error: Argument 1 to "loads" of "Serializer" has incompatible type "Any | None"; expected "bytes"  [arg-type]

openlibrary (https://github.com/internetarchive/openlibrary)
+ openlibrary/solr/updater/edition.py: note: In member "number_of_pages" of class "EditionSolrBuilder":
+ openlibrary/solr/updater/edition.py:167: error: Argument 1 to "int" has incompatible type "Any | None"; expected "str | Buffer | SupportsInt | SupportsIndex | SupportsTrunc"  [arg-type]

sphinx (https://github.com/sphinx-doc/sphinx)
+ sphinx/domains/python/_object.py: note: In member "add_target_and_index" of class "PyObject":
+ sphinx/domains/python/_object.py:349:45: error: Argument 1 to "get_index_text" of "PyObject" has incompatible type "Any | None"; expected "str"  [arg-type]

mypy (https://github.com/python/mypy)
+ mypy/partially_defined.py:510: error: Never apply isinstance() to unexpanded types; use mypy.types.get_proper_type() first  [misc]
+ mypy/partially_defined.py:510: note: If you pass on the original type after the check, always use its unexpanded version

django-stubs (https://github.com/typeddjango/django-stubs)
+ django-stubs/utils/datastructures.pyi:65: error: Signature of "get" incompatible with supertype "dict"  [override]
+ django-stubs/utils/datastructures.pyi:65: note:      Superclass:
+ django-stubs/utils/datastructures.pyi:65: note:          @overload
+ django-stubs/utils/datastructures.pyi:65: note:          def get(self, _K, None = ..., /) -> Optional[_V]
+ django-stubs/utils/datastructures.pyi:65: note:          @overload
+ django-stubs/utils/datastructures.pyi:65: note:          def get(self, _K, _V, /) -> _V
+ django-stubs/utils/datastructures.pyi:65: note:          @overload
+ django-stubs/utils/datastructures.pyi:65: note:          def [_T] get(self, _K, _T, /) -> Union[_V, _T]
+ django-stubs/utils/datastructures.pyi:65: note:      Subclass:
+ django-stubs/utils/datastructures.pyi:65: note:          @overload
+ django-stubs/utils/datastructures.pyi:65: note:          def get(self, key: _K) -> Optional[_V]
+ django-stubs/utils/datastructures.pyi:65: note:          @overload
+ django-stubs/utils/datastructures.pyi:65: note:          def [_Z] get(self, key: _K, default: _Z) -> Union[_V, _Z]

dd-trace-py (https://github.com/DataDog/dd-trace-py)
+ ddtrace/appsec/_iast/taint_sinks/path_traversal.py:31: error: Argument "evidence_value" to "report" of "VulnerabilityBase" has incompatible type "Any | None"; expected "str"  [arg-type]

core (https://github.com/home-assistant/core)
+ homeassistant/components/alexa/capabilities.py:1429: error: Unsupported right operand type for in ("Any | None")  [operator]
+ homeassistant/components/tradfri/config_flow.py:57: error: Argument 2 to "authenticate" has incompatible type "Any | None"; expected "str"  [arg-type]
+ homeassistant/components/fints/sensor.py:86: error: Argument 2 to "FinTsClient" has incompatible type "Any | None"; expected "str"  [arg-type]
+ homeassistant/components/everlights/light.py:123: error: Value of type "Any | None" is not indexable  [index]
+ homeassistant/components/everlights/light.py:123: error: Unsupported operand types for / ("None" and "int")  [operator]
+ homeassistant/components/everlights/light.py:123: note: Left operand is of type "Any | None"
+ homeassistant/components/melcloud/climate.py:229: error: Argument 1 to "_apply_set_hvac_mode" of "AtaDeviceClimate" has incompatible type "Any | None"; expected "HVACMode"  [arg-type]
+ homeassistant/components/home_connect/light.py:204: error: Argument 2 to "brightness_to_value" has incompatible type "Any | None"; expected "int"  [arg-type]

@tungol tungol mentioned this pull request Dec 10, 2024
Open
tungol added a commit to tungol/typeshed that referenced this pull request Dec 11, 2024
@tungol
a different alternate of dict.get
c6449d7 
Pushing this up just to compare the mypy-primer damage as compared
with python#13222. I expect it to be more disruptive, but I'm curious how
much more. It seems similar to but dramatically less disruptive than
an older attempt at changing dict.get in python#10294.
@tungol tungol mentioned this pull request Dec 11, 2024
Closed
@tungol tungol marked this pull request as ready for review December 12, 2024 09:59
@tungol
Copy link
Contributor Author

tungol commented Dec 12, 2024

I'm interested in hearing if anyone has opinions about this. It's a reasonable amount of churn in primer, although I'd encourge you to ignore the alerta diff entirely. After looking at them a little more, I realized that that project is still using implicit optional arguments, and all of that would go away if that were taken into account in primer.

My analysis of some parts of the diff ended up in #13224 because that milder change also triggered some of them.

Ultimately after dwelling on this quite a bit, I think that d_any.get(key, None) -> Any | None is the right thing to do, and 77 out of 86 error lines in the primer diff are exactly that. Only really one of the other 9 lines is particularly notable, which is the new [no-any-return] error for streamlit. Other than that, 4 of the lines are not really new, 2 are subclass related and would happen for any change to the signature, 1 is from a mypy bug, and 1 is specific to mypy itself.

I made a post on discuss.python.org which broke down significant scenarios that this affects here: https://discuss.python.org/t/dict-get-return-values-when-any-involved/73915

It hasn't seen much engagement, and I don't know if it's likely to get more at this point.

Of concern separately from the mypy-primer result is a weird thing this does to pyright's inference in this case:

d_str: dict[str, str] = {}
any_value: Any = object()

reveal_type(d.get("key", any_value))  # With this MR pyright says "str | None"

I asked Eric about it here; He agreed that it's wonky, but isn't planning any changes to how pyright handles overloads until the formal spec is done, which is totally reasonable. If I understand his draft correctly, this would be Any under the draft spec, which matches what mypy already gets here.

Anyway, I know this is getting longwinded. Ultimately I don't know if this makes sense to pursue before the overload spec to gets finalized. I guess what I'd like to know from the typeshed maintainers is:

a) Do you think this is in theory a good idea, or am I going in the wrong direction here?
b) If it's in theory a good idea, do you think that this is possibly an acceptable amount of primer output, or just too much and more work would be needed to get it down before this could be considered?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@tungol