PEP 702 (@deprecated): improve the handling of overloaded functions and methods

[tyralla]

Fixes#18323

Mypy's current approach to detecting deprecated overloads of functions and methods that I implemented has many drawbacks. For example, it does not handle some kinds of unions (see the discussion in #18477) and generic types (#18323). The complicating matter is that we made deprecated a symbol property instead of a type property (as we decided in #17476), but the selection of the relevant overload happens when only the types are reliably available. My first attempt was to search for possible deprecations at places where the symbols and types of all overloads and the type of the relevant overload are known. The naive assumption was that one could compare the relevant overload type with all available overload types to find the relevant overload symbol. However, the relevant overload type is often a modification of one or multiple of the original overload types. Hence, one would have to repeat all these possible modifications. #18323 would improve the current algorithm by adding one more of these modifications, but it seems worth trying a completely different way, which is what this PR is about.

The proposed solution is to handle the situation at the place where the relevant overload or overloads are detected by querying the relevant symbols from the available modules and assigning them to the overload types beforehand. Therefore, I use the already available but "unreliable" definition attribute. This approach requires including the deprecated attribute into symbol snapshots (relevant for caching), which might have been necessary anyhow (I have now added corresponding tests to the fine-grained test suite). It seems the approach works, but if it is risky in some way, I could soften it by resetting the previous definition values, updating only the deprecated subattributes instead of the complete definition attributes, adding a special-purpose attribute, or something like that.

Regarding the Mypy primer change for pydantic, I did not investigate why this change happens, but it seems like an improvement as the old notes seem really buggy ( pydantic/fields.py:542: note: def Field(default, default: EllipsisType, *...).

There is one use mypy.types.get_proper_type() error in both "Type check your own code" test suits. Either there is a bug, or I guess I am missing something very obvious...

I am interested to hear what you think about it!

Please note that I took the extension of the testDeprecatedDescriptor test case from #18333 by @Viicos.

[tyralla]

Oh no, I lost a few extensions of the check-deprecated test suite. I will re-add them later...

[tyralla]

Oh no, I lost a few extensions of the check-deprecated test suite. I will re-add them later...

fixed

[sobolevn]

This looks like a bug in mypy's plugin / source: if isinstance(c := get_proper_type(inferred_type), CallableType): is not detected properly by our proper_type plugin

[tyralla]

This looks like a bug in mypy's plugin / source: if isinstance(c := get_proper_type(inferred_type), CallableType): is not detected properly by our proper_type plugin

I was afraid of something like that. I have not dealt with this plugin before, but it does not look that complicated. I will try to fix it later.

[tyralla]

This looks like a bug in mypy's plugin / source: if isinstance(c := get_proper_type(inferred_type), CallableType): is not detected properly by our proper_type plugin

I was afraid of something like that. I have not dealt with this plugin before, but it does not look that complicated. I will try to fix it later.

I've got it. Mypy does not understand the following usage of zip:

mypy/mypy/checkexpr.py

Line 2696 in 5202c98

returns, inferred_types=zip(*unioned_return)

So, inferred_types (and returns) become Any, which seems to confuse the proper_plugin. Supporting this usage of zip and improving proper_plugin (provide a more useful error message) would be good, but for this PR I will just change the problematic line in the next commit.

[github-actions]

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

pydantic (https://github.com/pydantic/pydantic)
- pydantic/fields.py:616: note: def Field(default, default: EllipsisType, *, alias: str | None = ..., alias_priority: int | None = ..., validation_alias: str | AliasPath | AliasChoices | None = ..., serialization_alias: str | None = ..., title: str | None = ..., field_title_generator: Callable[[str, FieldInfo], str] | None = ..., description: str | None = ..., examples: list[Any] | None = ..., exclude: bool | None = ..., discriminator: str | Discriminator | None = ..., deprecated: deprecated | str | bool | None = ..., json_schema_extra: JsonDict | Callable[[JsonDict], None] | None = ..., frozen: bool | None = ..., validate_default: bool | None = ..., repr: bool = ..., init: bool | None = ..., init_var: bool | None = ..., kw_only: bool | None = ..., pattern: str | Pattern[str] | None = ..., strict: bool | None = ..., coerce_numbers_to_str: bool | None = ..., gt: SupportsGt | None = ..., ge: SupportsGe | None = ..., lt: SupportsLt | None = ..., le: SupportsLe | None = ..., multiple_of: float | None = ..., allow_inf_nan: bool | None = ..., max_digits: int | None = ..., decimal_places: int | None = ..., min_length: int | None = ..., max_length: int | None = ..., union_mode: Literal['smart', 'left_to_right'] = ..., fail_fast: bool | None = ...) -> Any
+ pydantic/fields.py:616: note: def Field(default: EllipsisType, *, alias: str | None = ..., alias_priority: int | None = ..., validation_alias: str | AliasPath | AliasChoices | None = ..., serialization_alias: str | None = ..., title: str | None = ..., field_title_generator: Callable[[str, FieldInfo], str] | None = ..., description: str | None = ..., examples: list[Any] | None = ..., exclude: bool | None = ..., discriminator: str | Discriminator | None = ..., deprecated: deprecated | str | bool | None = ..., json_schema_extra: JsonDict | Callable[[JsonDict], None] | None = ..., frozen: bool | None = ..., validate_default: bool | None = ..., repr: bool = ..., init: bool | None = ..., init_var: bool | None = ..., kw_only: bool | None = ..., pattern: str | Pattern[str] | None = ..., strict: bool | None = ..., coerce_numbers_to_str: bool | None = ..., gt: SupportsGt | None = ..., ge: SupportsGe | None = ..., lt: SupportsLt | None = ..., le: SupportsLe | None = ..., multiple_of: float | None = ..., allow_inf_nan: bool | None = ..., max_digits: int | None = ..., decimal_places: int | None = ..., min_length: int | None = ..., max_length: int | None = ..., union_mode: Literal['smart', 'left_to_right'] = ..., fail_fast: bool | None = ...) -> Any
- pydantic/fields.py:616: note: def [_T] Field(default, default: _T, *, alias: str | None = ..., alias_priority: int | None = ..., validation_alias: str | AliasPath | AliasChoices | None = ..., serialization_alias: str | None = ..., title: str | None = ..., field_title_generator: Callable[[str, FieldInfo], str] | None = ..., description: str | None = ..., examples: list[Any] | None = ..., exclude: bool | None = ..., discriminator: str | Discriminator | None = ..., deprecated: deprecated | str | bool | None = ..., json_schema_extra: JsonDict | Callable[[JsonDict], None] | None = ..., frozen: bool | None = ..., validate_default: bool | None = ..., repr: bool = ..., init: bool | None = ..., init_var: bool | None = ..., kw_only: bool | None = ..., pattern: str | Pattern[str] | None = ..., strict: bool | None = ..., coerce_numbers_to_str: bool | None = ..., gt: SupportsGt | None = ..., ge: SupportsGe | None = ..., lt: SupportsLt | None = ..., le: SupportsLe | None = ..., multiple_of: float | None = ..., allow_inf_nan: bool | None = ..., max_digits: int | None = ..., decimal_places: int | None = ..., min_length: int | None = ..., max_length: int | None = ..., union_mode: Literal['smart', 'left_to_right'] = ..., fail_fast: bool | None = ...) -> _T
+ pydantic/fields.py:616: note: def [_T] Field(default: _T, *, alias: str | None = ..., alias_priority: int | None = ..., validation_alias: str | AliasPath | AliasChoices | None = ..., serialization_alias: str | None = ..., title: str | None = ..., field_title_generator: Callable[[str, FieldInfo], str] | None = ..., description: str | None = ..., examples: list[Any] | None = ..., exclude: bool | None = ..., discriminator: str | Discriminator | None = ..., deprecated: deprecated | str | bool | None = ..., json_schema_extra: JsonDict | Callable[[JsonDict], None] | None = ..., frozen: bool | None = ..., validate_default: bool | None = ..., repr: bool = ..., init: bool | None = ..., init_var: bool | None = ..., kw_only: bool | None = ..., pattern: str | Pattern[str] | None = ..., strict: bool | None = ..., coerce_numbers_to_str: bool | None = ..., gt: SupportsGt | None = ..., ge: SupportsGe | None = ..., lt: SupportsLt | None = ..., le: SupportsLe | None = ..., multiple_of: float | None = ..., allow_inf_nan: bool | None = ..., max_digits: int | None = ..., decimal_places: int | None = ..., min_length: int | None = ..., max_length: int | None = ..., union_mode: Literal['smart', 'left_to_right'] = ..., fail_fast: bool | None = ...) -> _T
- pydantic/fields.py:616: note: def [_T] Field(default_factory, *, default_factory: Callable[[], _T] | Callable[[dict[str, Any]], _T], alias: str | None = ..., alias_priority: int | None = ..., validation_alias: str | AliasPath | AliasChoices | None = ..., serialization_alias: str | None = ..., title: str | None = ..., field_title_generator: Callable[[str, FieldInfo], str] | None = ..., description: str | None = ..., examples: list[Any] | None = ..., exclude: bool | None = ..., discriminator: str | Discriminator | None = ..., deprecated: deprecated | str | bool | None = ..., json_schema_extra: JsonDict | Callable[[JsonDict], None] | None = ..., frozen: bool | None = ..., validate_default: bool | None = ..., repr: bool = ..., init: bool | None = ..., init_var: bool | None = ..., kw_only: bool | None = ..., pattern: str | Pattern[str] | None = ..., strict: bool | None = ..., coerce_numbers_to_str: bool | None = ..., gt: SupportsGt | None = ..., ge: SupportsGe | None = ..., lt: SupportsLt | None = ..., le: SupportsLe | None = ..., multiple_of: float | None = ..., allow_inf_nan: bool | None = ..., max_digits: int | None = ..., decimal_places: int | None = ..., min_length: int | None = ..., max_length: int | None = ..., union_mode: Literal['smart', 'left_to_right'] = ..., fail_fast: bool | None = ...) -> _T
+ pydantic/fields.py:616: note: def [_T] Field(*, default_factory: Callable[[], _T] | Callable[[dict[str, Any]], _T], alias: str | None = ..., alias_priority: int | None = ..., validation_alias: str | AliasPath | AliasChoices | None = ..., serialization_alias: str | None = ..., title: str | None = ..., field_title_generator: Callable[[str, FieldInfo], str] | None = ..., description: str | None = ..., examples: list[Any] | None = ..., exclude: bool | None = ..., discriminator: str | Discriminator | None = ..., deprecated: deprecated | str | bool | None = ..., json_schema_extra: JsonDict | Callable[[JsonDict], None] | None = ..., frozen: bool | None = ..., validate_default: bool | None = ..., repr: bool = ..., init: bool | None = ..., init_var: bool | None = ..., kw_only: bool | None = ..., pattern: str | Pattern[str] | None = ..., strict: bool | None = ..., coerce_numbers_to_str: bool | None = ..., gt: SupportsGt | None = ..., ge: SupportsGe | None = ..., lt: SupportsLt | None = ..., le: SupportsLe | None = ..., multiple_of: float | None = ..., allow_inf_nan: bool | None = ..., max_digits: int | None = ..., decimal_places: int | None = ..., min_length: int | None = ..., max_length: int | None = ..., union_mode: Literal['smart', 'left_to_right'] = ..., fail_fast: bool | None = ...) -> _T
- pydantic/fields.py:616: note: def Field(alias, *, alias: str | None = ..., alias_priority: int | None = ..., validation_alias: str | AliasPath | AliasChoices | None = ..., serialization_alias: str | None = ..., title: str | None = ..., field_title_generator: Callable[[str, FieldInfo], str] | None = ..., description: str | None = ..., examples: list[Any] | None = ..., exclude: bool | None = ..., discriminator: str | Discriminator | None = ..., deprecated: deprecated | str | bool | None = ..., json_schema_extra: JsonDict | Callable[[JsonDict], None] | None = ..., frozen: bool | None = ..., validate_default: bool | None = ..., repr: bool = ..., init: bool | None = ..., init_var: bool | None = ..., kw_only: bool | None = ..., pattern: str | Pattern[str] | None = ..., strict: bool | None = ..., coerce_numbers_to_str: bool | None = ..., gt: SupportsGt | None = ..., ge: SupportsGe | None = ..., lt: SupportsLt | None = ..., le: SupportsLe | None = ..., multiple_of: float | None = ..., allow_inf_nan: bool | None = ..., max_digits: int | None = ..., decimal_places: int | None = ..., min_length: int | None = ..., max_length: int | None = ..., union_mode: Literal['smart', 'left_to_right'] = ..., fail_fast: bool | None = ...) -> Any
+ pydantic/fields.py:616: note: def Field(*, alias: str | None = ..., alias_priority: int | None = ..., validation_alias: str | AliasPath | AliasChoices | None = ..., serialization_alias: str | None = ..., title: str | None = ..., field_title_generator: Callable[[str, FieldInfo], str] | None = ..., description: str | None = ..., examples: list[Any] | None = ..., exclude: bool | None = ..., discriminator: str | Discriminator | None = ..., deprecated: deprecated | str | bool | None = ..., json_schema_extra: JsonDict | Callable[[JsonDict], None] | None = ..., frozen: bool | None = ..., validate_default: bool | None = ..., repr: bool = ..., init: bool | None = ..., init_var: bool | None = ..., kw_only: bool | None = ..., pattern: str | Pattern[str] | None = ..., strict: bool | None = ..., coerce_numbers_to_str: bool | None = ..., gt: SupportsGt | None = ..., ge: SupportsGe | None = ..., lt: SupportsLt | None = ..., le: SupportsLe | None = ..., multiple_of: float | None = ..., allow_inf_nan: bool | None = ..., max_digits: int | None = ..., decimal_places: int | None = ..., min_length: int | None = ..., max_length: int | None = ..., union_mode: Literal['smart', 'left_to_right'] = ..., fail_fast: bool | None = ...) -> Any

[tyralla]

I had to adjust a few things to @ilevkivskyi's check member refactoring. Due to this refactoring, the code in this PR appears a little handier, but the definition attribute trick remains, of course.

[ilevkivskyi]

@tyralla Oh, I completely forgot about this PR and implemented something similar as part of one of my performance optimization PRs (apparently previous approach, was not just fragile, but also slow). Could you please check if anything in this PR is still necessary? In any case, I think it is worth at least merging the tests (I didn't really add any new tests in my PRs)

[Viicos]

Thanks @ilevkivskyi, can confirm my MRE in the issue is now working as expected on main, so I guess only tests can be included.

[tyralla]

This PR covers a few more cases than the one reported in #18323. I try to find some time later this week to check if all this is now already fixed by #19556.

After a short look at #19556: Do I understand correctly that, due to the "fixup" func.type.definition = func, the definition attribute can now always be reliably used? This would make the major change of my PR unnecessary, of course. However, code comments suggest this attribute should still be (only) used for error messages:

mypy/mypy/types.py

Line 1824 in 3387d6f

"definition", # For error messages. May be None.

mypy/mypy/types.py

Line 2259 in 3387d6f

# We don't serialize the definition (only used for error messages).

[tyralla]

It seems two things about this PR are still of value.

First, Mypy still emits the following false positive:

fromtypingimportAny, overload, Unionfromtyping_extensionsimportdeprecatedint_or_str: Union[int, str]
@overloaddefj(x: int) ->int: ...
@overloaddefj(x: str) ->str: ...
@overload@deprecated("work with int or str instead")defj(x: Any) ->Any: ...
defj(x: Union[int, str]) ->Union[int, str]: ...
jj(1)
j("x")
z=j(int_or_str) # E: overload def (x: Any) -> Any of function __main__.j is deprecated: work with int or str insteadreveal_type(z) # N: Revealed type is "Union[builtins.int, builtins.str]"

Second, some fine-grained test cases fail (those with "indirect imports").

I will create two new PRs for these different problems later and close this one then.

[ilevkivskyi]

Do I understand correctly that, due to the "fixup" func.type.definition = func, the definition attribute can now always be reliably used?

Yes, it can be reliably used, but the FuncDef itself is very limited version of the original node (essentially, name/fullname, few flags, type, and that's it).

First, Mypy still emits the following false positive:

Oh, right I noticed this when working on my PR. Thes problem is much broader actually (not union specific), for example:

@deprecated("no int")@overloaddeff(x: int) ->int: ...
@overloaddeff(x: object) ->object: ...

I think we should not emit the warning in this case either. Essentially, the logic should be like this: if we detected that we have actually matched a deprecated overload (in the two places I added), then we should try checking the call to function again with all deprecated overload items removed, and if the call still succeeds, stay silent.

Second, some fine-grained test cases fail (those with "indirect imports").

Oh, I see, your solution makes sense, btw, I think you can try writing simply

deprecated: str|list[str|None] |None=Noneifisinstance(node, FuncDef):
deprecated=node.deprecatedelifisinstance(node, OverloadedFuncDef):
deprecated= [node.deprecated] + [
i.func.deprecatedforiinnode.itemsifisinstance(i, Decorator)
]

(symbol snapshots can be heterogeneous).

[tyralla]

Thanks for the information and thoughts. I will try your suggestions. Cool that func.type.definition is now more usable.

[tyralla]

Regarding the false positive, I think the two discussed problems are somewhat different. In the first case, Mypy's current behaviour seems just buggy (I repeat it without Any for simplicity):

classC: ...
classA(C): ...
classB(C): ...
a_or_b: A|B@overloaddeff(x: A) ->A: ...
@overloaddeff(x: B) ->B: ...
@overload@deprecated("A or B, please")deff(x: C) ->C: ...
deff(x: A|B|C) ->A|B|C: returnxf(a_or_b) # E: overload def (x: __main__.C) -> __main__.C of function __main__.f is deprecated: A or B, please

The second case seems to be more about finding the right balance:

classA: ...
classB(A): ...
@overload@deprecated("outdated")deff(x: B) ->B: ...
@overloaddeff(x: A) ->A: ...
deff(x: A|B) ->A|B:
returnxx: Bx=f(x) # overload def (x: __main__.B) -> __main__.B of function __main__.f is deprecated: outdated ???_______________________________________deff(x: A) ->A: returnxx: Bx=f(x) # Incompatible types in assignment (expression has type "A", variable has type "B") !!!

If we prefer to miss some helpful deprecation notes in favour of avoiding unnecessary ones, Ivan's suggested strategy should likely solve both cases. Otherwise, the approach implemented in this PR could be of more help.

@JelleZijlstra: It would be interesting to hear your opinion on this (I guess you share Ivan's view, but I want to get sure before starting to work on it - I don't have a preference myself).

[ilevkivskyi]

@tyralla Yeah, I understand these are not the same, but IMO they may be equally annoying for a user. FWIW I would stay on the side of only showing the deprecation warning if we are sure it is helpful.

[sterliakov]

Essentially, the logic should be like this: if we detected that we have actually matched a deprecated overload (in the two places I added), then we should try checking the call to function again with all deprecated overload items removed, and if the call still succeeds, stay silent.

I have to disagree here. While you're correct from the pure checking POV, @deprecated is actually my favorite way of implementing "all types except" for overloads. Another option is returning Never, but it relies more on reachability analysis (and doesn't immediately point to the problem source, blaming the next line instead). See this discourse thread. Consider the following snippet with a (simplified) definition of builtins.sum that takes "sum-vs-str.join" quirk into account (very similar to your example):

fromcollections.abcimportIterablefromtypingimportTYPE_CHECKING, Any, Never, TypeVar, overloadfromwarningsimportdeprecatedifTYPE_CHECKING:
from_typeshedimportSupportsAdd_A=TypeVar("_A", bound='SupportsAdd[Any, Any]')
@overload@deprecated("No, this raises at runtime")defsum(items: Iterable[str], /, default: str) ->Never: ...
@overloaddefsum(items: Iterable[_A], /, default: _A) ->_A: ...
defsum(items: Iterable[_A], /, default: _A) ->_A: ...
sum(['a', 'b'], '')

Note that --warn-unreachable doesn't fire here because the call is already trailing. Deprecation, however, does, and it works as an equivalent of a hypothetical @type_error, @nomatch or -> NoMatch annotation fir everyone with [deprecated] code enabled.

[ilevkivskyi]

@sterliakov Oh wow, what a hack! But Idk, if is useful for someone we may actually keep this behavior (it is not someone already complained about the current behavior).

[tyralla]

Okay, thanks for adding this perspective, Stanislav. As the decision is settled, I'll suggest a fix for the remaining issue in a new PR soon.