Create TypeVarInstance type for legacy typevars
#16538
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
|
Leaving this as draft for now; there's one mdtest regression (an incorrect |
dcreager
force-pushed
the
dcreager/legacy-typevar-instance
branch
2 times, most recently
from
7ce5df6 to
316ab63
Compare
|
|
The |
dcreager
force-pushed
the
dcreager/legacy-typevar-instance
branch
from
d8d5f20 to
f8d0350
Compare
* main: (222 commits) [playground] New default program (#17277) [red-knot] Add `--python-platform` CLI option (#17284) [red-knot] Allow ellipsis default params in stub functions (#17243) [red-knot] Fix stale syntax errors in playground (#17280) Update Rust crate clap to v4.5.35 (#17273) Fix RUF100 to detect unused file-level noqa directives with specific codes (#17042) (#17061) [ci] Fix pattern for code changes (#17275) [`airflow`] Update oudated `AIR301`, `AIR302` rules (#17123) [docs] fix formatting of "See Style Guide" link (#17272) [red-knot] Support stub packages (#17204) ruff_annotate_snippets: address unused code warnings [red-knot] Add a couple more tests for `*` imports (#17270) [red-knot] Add 'Format document' to playground (#17217) Update actions/setup-node action to v4.3.0 (#17259) Update actions/upload-artifact action to v4.6.2 (#17261) Update actions/download-artifact action to v4.2.1 (#17258) Update actions/setup-python action to v5.5.0 (#17260) Update actions/cache action to v4.2.3 (#17256) Update Swatinem/rust-cache action to v2.7.8 (#17255) Update actions/checkout action to v4.2.2 (#17257) ...
* main: (123 commits) [red-knot] Handle explicit class specialization in type expressions (#17434) [red-knot] allow assignment expression in call compare narrowing (#17461) [red-knot] fix building unions with literals and AlwaysTruthy/AlwaysFalsy (#17451) [red-knot] Type narrowing for assertions (take 2) (#17345) [red-knot] class bases are not affected by __future__.annotations (#17456) [red-knot] Add support for overloaded functions (#17366) [`pyupgrade`] Add fix safety section to docs (`UP036`) (#17444) [red-knot] more type-narrowing in match statements (#17302) [red-knot] Add some narrowing for assignment expressions (#17448) [red-knot] Understand `typing.Protocol` and `typing_extensions.Protocol` as equivalent (#17446) Server: Use `min` instead of `max` to limit the number of threads (#17421) [red-knot] Detect version-related syntax errors (#16379) [`pyflakes`] Add fix safety section (`F841`) (#17410) [red-knot] Add `KnownFunction` variants for `is_protocol`, `get_protocol_members` and `runtime_checkable` (#17450) Bump 0.11.6 (#17449) Auto generate `visit_source_order` (#17180) [red-knot] Initial tests for protocols (#17436) [red-knot] Dataclasses: synthesize `__init__` with proper signature (#17428) [red-knot] Dataclasses: support `order=True` (#17406) [red-knot] Super-basic generic inference at call sites (#17301) ...
CodSpeed Performance ReportMerging #16538 will not alter performanceComparing Summary
|
This reverts commit 1073f86.
* main: (27 commits) [red-knot] Add new property tests for subtyping with "bottom" callable (#17635) [red-knot] Create generic context for generic classes lazily (#17617) ruff_db: add tests for annotations with no ranges [`airflow`] Extend `AIR301` rule (#17598) [`airflow`] update existing `AIR302` rules with better suggestions (#17542) red_knot_project: sort diagnostics from checking files [red-knot] fix detecting a metaclass on a not-explicitly-specialized generic base (#17621) [red-knot] fix inheritance-cycle detection for generic classes (#17620) [`pylint`] Detect `global` declarations in module scope (`PLE0118`) (#17411) Add Semantic Error Test for LateFutureImport (#17612) [red-knot] change TypeVarInstance to be interned, not tracked (#17616) [red-knot] Special case `@final`, `@override` (#17608) [red-knot] add TODO comment in specialization code (#17615) [`semantic-syntax-errors`] test for `LoadBeforeGlobalDeclaration` - ruff linter (#17592) [syntax-errors] `nonlocal` declaration at module level (#17559) [`airflow`] Apply auto fix to cases where name has been changed in Airflow 3 (`AIR311`) (#17571) [syntax-errors] Make `async-comprehension-in-sync-comprehension` more specific (#17460) Bump 0.11.7 (#17613) [red-knot] Use iterative approach to collect overloads (#17607) red_knot_python_semantic: avoid Rust's screaming snake case convention in mdtest ...
dcreager
commented
Apr 26, 2025
crates/red_knot_python_semantic/resources/mdtest/type_properties/is_assignable_to.md
Show resolved
Hide resolved
| /// to the target, and so have `None` for this field.) | ||
| #[no_eq] | ||
| #[tracked] | ||
| pub(crate) assigned_to: Option<AstNodeRef<ast::StmtAssign>>, |
There was a problem hiding this comment.
I don't love how this is so specialized to StmtAssign. We're not currently creating a standalone expression for annotated assignments, which means we currently raise a false positive for
T: TypeVar = TypeVar("T")Are there any concerns with adding a standalone expression for annotated assignments too? Then I could make this ast::Stmt (or maybe a separate enum that only allows StmtAssign and StmtAnnAssign)
There was a problem hiding this comment.
I would be happy with disallowing T: TypeVar = TypeVar("T")! I don't think it really makes sense to add the annotation there: it will be ignored by us anyway. It indicates to me that the user is confused about how TypeVars work, and perhaps doesn't realise that a simple T = TypeVar("T") assignment will also be considered a declaration by us. Mypy disallows an annotation in this situation: https://mypy-play.net/?mypy=latest&python=3.12&gist=4dad759b00a84071f89fbe9b5e041dcb
I'm also okay with allowing it, though. No strong opinion, as long as we don't emit a confusing error message if somebody does this 😄
There was a problem hiding this comment.
It would be preferable not to add a standalone expression for every annotated assignment; the more standalone expressions, the more memos, and the slower incremental re-validation gets. If we really need to, we probably can, but I don't think T: TypeVar = TypeVar("T") is a sufficiently motivating use case. As Alex says, I think it would be fine to just error on that; T = TypeVar("T") is effectively a syntactic special form.
There was a problem hiding this comment.
As Alex says, I think it would be fine to just error on that;
...
No strong opinion, as long as we don't emit a confusing error message if somebody does this 😄
The main issue with this is that customizing the error message will require the same extra plumbing as allowing it, since we'd need to detect that the TypeVar call occurs in an annotated assignment. Otherwise we'll have the default error message A legacy `typing.TypeVar` must be immediately assigned to a variable.
There was a problem hiding this comment.
Otherwise we'll have the default error message
A legacy `typing.TypeVar` must be immediately assigned to a variable.
Which, to be clear, I'm okay with, certainly for now
|
Congrats on fixing the performance regression! |
|
There are 20,064 new diagnostics on this PR (an overall increase in diagnostics of 27%). 19,038 of these are of the form "No overload of bound method That still leaves 1,026 new diagnostics on this branch that have various other causes. I understand that this PR increases our coverage a lot, so it's entirely possible that all new diagnostics are unrelated to the changes here. But it might be worth studying this a bit more? (unless you already did, @dcreager) I uploaded an experimental new diff view of the ecosystem changes. I filtered out the 19k Some things that stand out to me:
|
Yep, those are because of the |
* main: (37 commits) [red-knot] Revert blanket `clippy::too_many_arguments` allow (#17688) Add config option to disable `typing_extensions` imports (#17611) ruff_db: render file paths in diagnostics as relative paths if possible Bump mypy_primer pin (#17685) red_knot_python_semantic: improve `not-iterable` diagnostic [red-knot] Allow all callables to be assignable to @Todo-signatures (#17680) [`refurb`] Mark fix as safe for `readlines-in-for` (`FURB129`) (#17644) Collect preview lint behaviors in separate module (#17646) Upgrade Salsa to a more recent commit (#17678) [red-knot] TypedDict: No errors for introspection dunder attributes (#17677) [`flake8-pyi`] Ensure `Literal[None,] | Literal[None,]` is not autofixed to `None | None` (`PYI061`) (#17659) [red-knot] No errors for definitions of `TypedDict`s (#17674) Update actions/download-artifact digest to d3f86a1 (#17664) [red-knot] Use 101 exit code when there's at least one diagnostic with severity 'fatal' (#17640) [`pycodestyle`] Fix duplicated diagnostic in `E712` (#17651) [airflow] fix typos `AIR312` (#17673) [red-knot] Don't ignore hidden files by default (#17655) Update pre-commit hook astral-sh/ruff-pre-commit to v0.11.7 (#17670) Update docker/build-push-action digest to 14487ce (#17665) Update taiki-e/install-action digest to ab3728c (#17666) ...
|
I'm interested in trying to put this PR on top of #17618 to see what effect it has on the diagnostics that this PR adds. I'm going to try it locally and see. This shouldn't block this PR from merging. |
This looks to be coming from here: @overload
def __call__(self, arg: Markable) -> Markable: # type: ignore[overload-overlap]
pass
@overload
def __call__(self, *args: object, **kwargs: object) -> MarkDecorator:
pass
def __call__(self, *args: object, **kwargs: object):where |
Minor additional comment: the bound is |
|
Out of curiosity, I looked into the pytest decorator None of that is to say it needs fixing in this PR, it can be a follow-up. |
It does seem to be reducing the number of overall diagnostics but not by much compared to what already exists. Ref #17690. I haven't looked closely to the diagnostics that it's removing, I can do that a bit later if it'd be useful. |
carljm
approved these changes
Apr 28, 2025
| /// to the target, and so have `None` for this field.) | ||
| #[no_eq] | ||
| #[tracked] | ||
| pub(crate) assigned_to: Option<AstNodeRef<ast::StmtAssign>>, |
There was a problem hiding this comment.
It would be preferable not to add a standalone expression for every annotated assignment; the more standalone expressions, the more memos, and the slower incremental re-validation gets. If we really need to, we probably can, but I don't think T: TypeVar = TypeVar("T") is a sufficiently motivating use case. As Alex says, I think it would be fine to just error on that; T = TypeVar("T") is effectively a syntactic special form.
|
Oh oops, looks like I was looking into the call-non-callable at the same time as others :) |
Isn't the issue here simply that we pay no attention to bounds, so the first overload always matches? |
Most of the changes I'm seeing in #17690 are consistent with what I'm already seeing in #17618, but I've noticed that almost 1/3rd of ┃ Diagnostic ID ┃ Severity ┃ Removed ┃ Added ┃ Net Change ┃
- │ lint:call-non-callable │ error │ 98 │ 497 │ +399 │
+ │ lint:call-non-callable │ error │ 124 │ 158 │ +34 │ |
Ah yes, you're right! I had thought it would be caught by the type-checking code, but if we infer this type mapping and then substitute that for the formal parameter annotation, it will pass. That's worth handling here, I think, and not in a follow-on PR. |
We now check the bounds and constraints when inferring specializations, which removed all of the |
|
I'm going to go ahead and merge so that I can proceed with legacy generic classes. My apologies to our poor ecosystem graphs... 😂 |
|
I'm going to look at those |
carljm
reviewed
Apr 29, 2025
|
|
||
| reveal_type(f(1)) # revealed: Literal[1] | ||
| reveal_type(f(True)) # revealed: Literal[True] | ||
| # error: [invalid-argument-type] "Argument to this function is incorrect: Argument type `Literal["string"]` does not satisfy upper bound of type variable `T`" |
There was a problem hiding this comment.
Would it be worth also having similar tests for legacy typevars with bounds/constraints? Both to show we handle the bounds and constraints correctly, and that the full diagnostic highlights them correctly?
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.
We are currently representing type variables using a
KnownInstancevariant, which wraps aTypeVarInstancethat contains the information about the typevar (name, bounds, constraints, default type). We were previously only constructing that type for PEP 695 typevars. This PR constructs that type for legacy typevars as well.It also detects functions that are generic because they use legacy typevars in their parameter list. With the existing logic for inferring specializations of function calls (#17301), that means that we are correctly detecting that the definition of
reveal_typein the typeshed is generic, and inferring the correct specialization of_Tfor each call site.This does not yet handle legacy generic classes; that will come in a follow-on PR.