PEP 702 (@deprecated): consider all possible type positions

[tyralla]

This pull request generalises #17899.

Initially, it started with extending #17899 to function signatures only, as can be seen from the following initial comments and the subsequent discussions.

---

@ilevkivskyi

This is part two of the checker.py chase.

I had to extend InstanceDeprecatedVisitor so that it refrains from reporting that self in the signature of a deprecated class' method refers to this deprecated class. This seems to suggest that a strictly uniform handling for all possible type positions might not be possible. Do you think the same?

However, searching for more central locations to apply InstanceDeprecatedVisitor is certainly a good idea. I can try it in this or the next PR, as you prefer.

[tyralla]

On second thought: InstanceDeprecatedVisitor already knows the necessary context to solve this problem without external help.

    def visit_instance(self, t: Instance) -> None:
        super().visit_instance(t)
        if not (
            isinstance(defn := self.context, FuncDef)
            and defn.info and
            (t.type.fullname == defn.info.fullname)
        ):
            self.typechecker.check_deprecated(t.type, self.context)

[tyralla]

After a bit of experimenting, I think extending TypeArgumentAnalyzer a little could be the way to go. I would try it if you agree.

[sobolevn]

Generally looks good to me. But, maybe we should simplify this to ignore_self? And just ignore first args in class / instance methods?

[ilevkivskyi]

I think the whole visitor should be replaced with a single check in analyze_type_with_type_info() in typeanal.py: simply check if TypeInfo is deprecated and it will cover all cases at once. Conveniently, the current class is available there as self.api.type, so it is very easy to skip self types.

Another thought is that we need to update the TypeInfo branch in snapshot_definition() in astdiff.py and add fine grained test case(s) for deprecating classes.

@tyralla Could you please re-purpose this PR for the above two things?

[ilevkivskyi]

Note btw what I say above may require moving around some existing code for emitting error messages, since you can't import checker.py in typeanal.py.

[tyralla]

Generally looks good to me. But, maybe we should simplify this to ignore_self? And just ignore first args in class / instance methods?

Thanks for the review, @sobolevn. Could you explain why you suggest simplifying? I see now that a reference to the relevant class (TypeInfo) always seems to be available somehow. Or do you expect other problems with my approach?

I think the whole visitor should be replaced with a single check in analyze_type_with_type_info() in typeanal.py: simply check if TypeInfo is deprecated and it will cover all cases at once. Conveniently, the current class is available there as self.api.type, so it is very easy to skip self types.

And thanks for the suggestion, @ilevkivskyi. analyze_type_with_type_info seems, in fact, convenient for doing the discussed checks, with one exception perhaps. self.api is of type SemanticAnalyzerCoreInterface. If it is SementicAnalyzer during runtime, I can use the cur_mod_node attribute to determine how the class has been imported (via MypyFile.imports). Narrowing self.api to SementicAnalyzer via isinstance does not work right away because importing SementicAnalyzer at the module header causes import cycle problems. Is importing SementicAnalyzer locally (within analyze_type_with_type_info) or extending SemanticAnalyzerCoreInterface okay? Or do you see a better way to determine if and how a class has been imported?

[tyralla]

Or do you see a better way to determine if and how a class has been imported?

Or a function, of course.

[tyralla]

As an alternative suggestion, I generalised InstanceDeprecatedVisitor.

The Mypy diff for tanjun is correct. BasicContext is in fact deprecated.

I did not investigate why the deprecation warnings were raised when executing the Python evaluation suite.

Whether we proceed this way or via extending analyze_type_with_type_info, we still need to add more test cases.

[tyralla]

Another thought is that we need to update the TypeInfo branch in snapshot_definition() in astdiff.py and add fine grained test case(s) for deprecating classes.

I did so. There is a problem for:

import b
y: b.D

[tyralla]

I could fix the y: b.D problem and split the fine-grained dependency tests for the cache and the nocache options where necessary.

I will add more tests to check-deprecated tomorrow.

Regarding the Python evaluation suite, I am stuck. Adding type: ignore[deprecated] to these lines helps as expected, but what is the right way to proceed? (@ilevkivskyi and @sobolevn)

[tyralla]

I filed a PR to typeshed, added a few more tests (all passed without further effort), and renamed this PR. So, if the InstanceDeprecatedVisitor approach is okay, this should be ready, in my opinion.

[tyralla]

According to the discussion in #18007, I silenced warn_deprecated for all typeshed stub files so that all tests now pass.

[ilevkivskyi]

TBH I don't like the idea of essentially traversing the whole tree twice, even as a temporary state. The deprecation check for types should be in typeanal.py. If this requires changing semantic analyzer interface, it is fine (although I am not 100% sure why it is needed, but I also don't know all the details of the PEP).

[tyralla]

TBH I don't like the idea of essentially traversing the whole tree twice, even as a temporary state.

To me, it appears cleaner (the separation and leaving everything in checker.py), but I realise the "single purpose traversal" somehow contradicts the usual Mypy design.

If this requires changing semantic analyzer interface, it is fine (although I am not 100% sure why it is needed, but I also don't know all the details of the PEP).

Okay, I will try it this way. (I need to access the import definitions to suppress repeated notes for types that have already been reported as deprecated at a from ... import ... statement.)

[github-actions]

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

Tanjun (https://github.com/FasterSpeeding/Tanjun)
+ tanjun/context/base.py:48: note: class alluka._context.BasicContext is deprecated: Use Context or CachingContext  [deprecated]
+ tanjun/context/autocomplete.py:54: note: class alluka._context.BasicContext is deprecated: Use Context or CachingContext  [deprecated]

core (https://github.com/home-assistant/core)
+ homeassistant/runner.py:84: note: class asyncio.unix_events.AbstractChildWatcher is deprecated: Deprecated as of Python 3.12; will be removed in Python 3.14  [deprecated]

[tyralla]

I moved the deprecation check to analyze_type_with_type_info. I could avoid changing the semantic analyser interface by adding the cur_mod_node attribute to TypeAnalyzer. There is now a little code duplication, but not so much as moving the check and warn methods to a separate module seems appropriate, given the restriction of needing to avoid import cycle errors.

[ilevkivskyi]

Looks great!