[ty] Add support for @warnings.deprecated
#19376
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Gankra
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
Gankra
changed the title
@warnings.deprecated@warnings.deprecated
Gankra
added
ty
Contributor
Author
|
The big thing left to do here is to generalize to "fire on all references". I haven't started on it but I assume it's non-trivial? |
Gankra
commented
Jul 15, 2025
Gankra
commented
Jul 15, 2025
Comment on lines
227
to
236
| @overload | ||
| @deprecated("strings are no longer supported") | ||
| def f(x: str): ... | ||
| @overload | ||
| def f(x: int): ... | ||
| def f(x): | ||
| print(x) | ||
|
|
||
| # TODO: this shouldn't diagnostic | ||
| f(1) # error: [deprecated] "strings are no longer supported" |
Contributor
Author
There was a problem hiding this comment.
I didn't obviously see how to get this precision but I didn't look hard, something to ask @dhruvmanila about.
Member
There was a problem hiding this comment.
I think this might require invoking the call logic i.e., calling the try_call method on the type of f with the argument types. This should give you the Bindings object which would be populated with all the information about the call including the chosen overload which is CallableBinding::matching_overload_index. The problem with this is that the matching_overload_index for now is only populated for step 1 and step 4 of the algorithm.
For step 2, it can easily be done here:
ruff/crates/ty_python_semantic/src/types/call/bind.rs
Lines 1226 to 1229 in f3a2740
Step 3 will have more than one matching overloads so it might require updating the type of matching_overload_index or you could use the existing MatchingOverloadIndex.
Step 5 could also result in multiple matching overloads which can also be represented using MatchingOverloadIndex.
Member
There was a problem hiding this comment.
But, if this PR doesn't result into too many false positives, we could do this in a follow-up PR as well.
Contributor
Author
There was a problem hiding this comment.
I think the false positives will be really annoying here, and deprecated-overloads is pretty narrow, so I think it's best to just disable all support for deprecated overloads until said followup.
Member
There was a problem hiding this comment.
That sounds reasonable. Can you open an issue to keep track of it?
Gankra
commented
Jul 15, 2025
dcreager
reviewed
Jul 15, 2025
| // TODO(Gankra): from Jelle: in the PEP I recommended that if you see from somewhere | ||
| // import deprecated_function and then a bunch of calls to deprecated_function() in | ||
| // the module, you should only issue a diagnostic for the import, not for every call | ||
| if let Some(message) = function_literal.deprecated(self.db()) { |
Member
There was a problem hiding this comment.
I think this is where to get the more precision you mentioned in #19376 (comment). Instead of checking whether function_literal is deprecated (which checks if any overload of the function is deprecated), you only want to check if overload is (that is, the particular overload that was matched for this call).
(My hope is that overload.binding.binding_type is the FunctionLiteral that points right at the particular overload that was matched. If not, you'll have to get the overload index from the matching_overloads_mut iterator (which is already enumerated for you), and use that index to walk through the overloads looking for the right one. I'm about to sign off for the day but I can help you look at this tomorrow)
Contributor
Author
There was a problem hiding this comment.
I believe no matter what you need to iterate a list of overloads, as e.g. int | str might match an int overload and a str overload (but not say, a third bool one).
Member
There was a problem hiding this comment.
Right, I think I spoke poorly. The for loop over matching_overloads_mut handles what you're describing — the loop body will be invoked for each overload that the call actually matches. But for each of those matching overloads, you need to find the OverloadLiteral that describes its signature, so that you can see if it has a deprecated decorator attached. What I was describing was how to go from the Binding (which is what the overload variable is) to that OverloadLiteral. That might require walking through what is effectively a linked list.
sharkdp
reviewed
Jul 16, 2025
sharkdp
reviewed
Jul 16, 2025
sharkdp
reviewed
Jul 16, 2025
Contributor
Author
|
I've pushed up an updated implementation that focuses on places like I still need to do some cleanups to the code though. I also haven't looked at all at the specific clauses about inheritance and overrides. |
Contributor
Author
|
Rebased and addressed all reviews. |
Contributor
|
Contributor
|
Rebased your branch, as I reviewed the changes in #19400 that caused the conflicts here. Hope that's fine |
Contributor
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
deprecated |
240 | 0 | 0 |
call-non-callable |
1 | 0 | 0 |
invalid-argument-type |
1 | 0 | 0 |
| Total | 242 | 0 | 0 |
sharkdp
reviewed
Jul 18, 2025
sharkdp
reviewed
Jul 18, 2025
sharkdp
reviewed
Jul 18, 2025
MichaReiser
approved these changes
Jul 18, 2025
Member
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
This overall looks good to me but I'm curious what the performance numbers will show. You'll have to fix the panic in the benchmark (review what new diagnostics there are) to get those numbers and push the updated benchmark.
The other thing we have to look into is if using StringLiteralType is too strict and how we want to handle very long messages
sharkdp
approved these changes
Jul 18, 2025
Contributor
sharkdp
left a comment
sharkdp
left a comment
There was a problem hiding this comment.
Thank you, this looks good!
Apart from the invalid-argument-type false positive, we should also look into this new diagnostic (also on altair):
altair (https://github.com/vega/altair)
+ error[call-non-callable] altair/vegalite/v6/theme.py:73:5: Object of type `warnings.deprecated` is not callable
Contributor
|
carljm
approved these changes
Jul 18, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Modulo a couple small inline comments, this looks land-ready to me!
Contributor
Author
|
Done. |
UnboundVariable
pushed a commit
to UnboundVariable/ruff
that referenced
this pull request
Jul 19, 2025
* main: [ty] Avoid secondary tree traversal to get call expression for keyword arguments (astral-sh#19429) [ty] Add goto definition to playground (astral-sh#19425) [ty] Add support for `@warnings.deprecated` (astral-sh#19376) [ty] make `del x` force local resolution of `x` in the current scope (astral-sh#19389) # Conflicts: # crates/ty_ide/src/goto.rs
dcreager
added a commit
that referenced
this pull request
Jul 21, 2025
* main: (25 commits) [ty] Sync vendored typeshed stubs (#19461) [ty] Extend tuple `__len__` and `__bool__` special casing to also cover tuple subclasses (#19289) [ty] bump docstring-adder pin (#19458) [ty] Disallow assignment to `Final` class attributes (#19457) Update dependency ruff to v0.12.4 (#19442) Update pre-commit hook astral-sh/ruff-pre-commit to v0.12.4 (#19443) Update rui314/setup-mold digest to 702b190 (#19441) Update taiki-e/install-action action to v2.56.19 (#19448) Update Rust crate strum_macros to v0.27.2 (#19447) Update Rust crate strum to v0.27.2 (#19446) Update Rust crate rand to v0.9.2 (#19444) Update Rust crate serde_json to v1.0.141 (#19445) Fix `unreachable` panic in parser (#19183) [`ruff`] Support byte strings (`RUF055`) (#18926) [ty] Avoid second lookup for `infer_maybe_standalone_expression` (#19439) [ty] Implemented "go to definition" support for import statements (#19428) [ty] Avoid secondary tree traversal to get call expression for keyword arguments (#19429) [ty] Add goto definition to playground (#19425) [ty] Add support for `@warnings.deprecated` (#19376) [ty] make `del x` force local resolution of `x` in the current scope (#19389) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
basic handling
@warnings.deprecatedattributesfunctions
classes
methods
overloads
dunder desugarring (warn on deprecated
__add__if+is invoked)alias supression? (don't warn on uses of variables that deprecated items were assigned to)
import logic
module.mydeprecated)*importFixes astral-sh/ty#153