[ty] Add a new diagnostic to detect invalid class patterns in match statements#22939
Merged
sharkdp merged 3 commits intoastral-sh:mainfrom Feb 16, 2026
Merged
[ty] Add a new diagnostic to detect invalid class patterns in match statements#22939sharkdp merged 3 commits intoastral-sh:mainfrom
match statements#22939sharkdp merged 3 commits intoastral-sh:mainfrom
Conversation
Typing conformance resultsNo changes detected ✅ |
|
Member
|
Don't worry about the pydantic hits in the primer report — we're a bit non-deterministic right now and those are all our usual flakes! |
Contributor
Author
|
Ah,okay. Thanks! |
abhijeetbodas2001
force-pushed
the
apb-match
branch
3 times, most recently
from
1995bc5 to
185b12d
Compare
crates/ty_python_semantic/resources/mdtest/conditional/match.md
Outdated
Show resolved
Hide resolved
abhijeetbodas2001
requested review from
AlexWaygood,
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
abhijeetbodas2001
changed the title
Contributor
Author
|
The new diagnostic that this PR introduces is emitted on jax code in one file. This is because of how the TiledLayout: Any
if (
hasattr(mgpu.dialect, "TiledLayout") # this is statically inferred False
and (...)
):
TiledLayout = mgpu.dialect.TiledLayout # Never
else:
TiledLayout = TiledLayoutImpl
# later, in the following functions,
def _(foo):
# `TiledLayout` is inferred as `TiledLayoutImpl | Any` in the function scope
match foo:
case TiledLayout: # error! Any not allowed
...
|
Contributor
Author
|
@AlexWaygood this is ready for review now! |
sharkdp
requested changes
Feb 9, 2026
| // TODO: emit diagnostic | ||
| } else if !cls_ty.is_equivalent_to(self.db(), Type::Never) | ||
| && !matches!(cls_ty, Type::Dynamic(_)) | ||
| { |
Contributor
There was a problem hiding this comment.
Is it enough to exclude Never and dynamic types? Maybe there are cases which can not handle above, but which should not raise diagnostics here either?
For example
def f(cls: type[int | str], val):
match val:
case cls():
print(f"Matched as {cls}: {val!r}")
Member
There was a problem hiding this comment.
Under the hood, Python implicitly does a call to isinstance() for class patterns. So we should emulate the runtime semantics and allow any object to be used as a class pattern if we would allow that object to be used as the second argument to isinstance().
Member
There was a problem hiding this comment.
Given that the semantics of isinstance() are really quite complicated (and heavily special-cased by us elsewhere), I would be tempted to do something like this to make sure that we apply consistent handling here to our handling of explicit isinstance() calls elsewhere:
diff --git a/crates/ty_python_semantic/src/types/infer/builder.rs b/crates/ty_python_semantic/src/types/infer/builder.rs
index f4dc33ee83..e7a66c2880 100644
--- a/crates/ty_python_semantic/src/types/infer/builder.rs
+++ b/crates/ty_python_semantic/src/types/infer/builder.rs
@@ -35,9 +35,9 @@ use crate::node_key::NodeKey;
use crate::place::{
ConsideredDefinitions, DefinedPlace, Definedness, LookupError, Place, PlaceAndQualifiers,
TypeOrigin, builtins_module_scope, builtins_symbol, class_body_implicit_symbol,
- explicit_global_symbol, global_symbol, module_type_implicit_global_declaration,
- module_type_implicit_global_symbol, place, place_from_bindings, place_from_declarations,
- typing_extensions_symbol,
+ explicit_global_symbol, global_symbol, known_module_symbol,
+ module_type_implicit_global_declaration, module_type_implicit_global_symbol, place,
+ place_from_bindings, place_from_declarations, typing_extensions_symbol,
};
use crate::semantic_index::ast_ids::node_key::ExpressionNodeKey;
use crate::semantic_index::ast_ids::{HasScopedUseId, ScopedUseId};
@@ -4615,8 +4615,13 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
protocol_class,
);
}
- } else if !cls_ty.is_equivalent_to(self.db(), Type::Never)
- && !matches!(cls_ty, Type::Dynamic(_))
+ } else if known_module_symbol(self.db(), KnownModule::Builtins, "isinstance")
+ .ignore_possibly_undefined()
+ .is_some_and(|isinstance| {
+ isinstance
+ .try_call(self.db(), &CallArguments::positional([Type::any(), cls_ty]))
+ .is_err()
+ })
{
// This also raises a diagnostic for `Any()` calls, since that fails at runtime.
report_called_match_pattern_must_be_a_type(&self.context, &*pattern.cls, cls_ty);
Contributor
Author
There was a problem hiding this comment.
Under the hood, Python implicitly does a call to
isinstance()for class patterns. So we should emulate the runtime semantics and allow any object to be used as a class pattern if we would allow that object to be used as the second argument toisinstance().
There's one more additional check per PEP 634 (name_or_attr being the "class"):
If
name_or_attris not an instance of the builtin type,TypeErroris raised.
So type unions are also not allowed in class pattern (eventhough they are fine for isinstance checks).
For the isinstance check, I'm wondering if it would make more sense to factor out this logic and reuse it from the match validation?
ruff/crates/ty_python_semantic/src/types/function.rs
Lines 2025 to 2084 in a2f546b
Contributor
Author
There was a problem hiding this comment.
The above block does all the isinstance validations and raises diagnostics. If we reuse it, we could also be able to remove the TypedDict and protocol checks and diagnostics which we have re-implemented in validate_class_builder.
Does that seem fine? Or are the separate diagnostics for match intentional (so that we have separate wording)?
Contributor
There was a problem hiding this comment.
I think I'll merge what you have so far (thank you!), since it seems like a good initial step. Integrating isinstance validation and reusing the existing infrastructure sounds great. It would be good to fix the two TODOs that we have (I added more descriptive explanations of the expected outcome). It would be great if you could work on this in a follow-up. Otherwise, just let me know, and I can open an issue for someone else to resolve these in the future.
Contributor
Author
There was a problem hiding this comment.
Makes sense, thanks. I'll open another PR for this.
Contributor
Author
|
Thanks for the reviews. I'll update over the weekend. |
abhijeetbodas2001
force-pushed
the
apb-match
branch
from
f247040 to
ee31fda
Compare
Memory usage reportMemory usage unchanged ✅ |
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-argument-type |
2 | 1 | 27 |
unresolved-attribute |
2 | 0 | 17 |
invalid-match-pattern |
10 | 0 | 0 |
unsupported-operator |
4 | 0 | 2 |
invalid-assignment |
0 | 0 | 1 |
invalid-return-type |
0 | 0 | 1 |
type-assertion-failure |
0 | 1 | 0 |
unused-type-ignore-comment |
1 | 0 | 0 |
| Total | 19 | 2 | 48 |
abhijeetbodas2001
force-pushed
the
apb-match
branch
from
ee31fda to
6d7e2ac
Compare
sharkdp
approved these changes
Feb 16, 2026
| // TODO: emit diagnostic | ||
| } else if !cls_ty.is_equivalent_to(self.db(), Type::Never) | ||
| && !matches!(cls_ty, Type::Dynamic(_)) | ||
| { |
Contributor
There was a problem hiding this comment.
I think I'll merge what you have so far (thank you!), since it seems like a good initial step. Integrating isinstance validation and reusing the existing infrastructure sounds great. It would be good to fix the two TODOs that we have (I added more descriptive explanations of the expected outcome). It would be great if you could work on this in a follow-up. Otherwise, just let me know, and I can open an issue for someone else to resolve these in the future.
AlexWaygood
changed the title
match statements
AlexWaygood
added a commit
that referenced
this pull request
Feb 16, 2026
## Summary Any `type` type can only be inhabited by class objects; any `type | <dynamic type>` union could materialize to a class object; and any `<class object> & <dynamic type>` intersection is a subtype of a class object. So using a value that inhabits any of these types as a class pattern should not cause us to emit the `invalid-match-pattern` diagnostic. This PR fixes several false positives that showed up in the ecosystem report in #22939 ## Test Plan mdtests added
AlexWaygood
reviewed
Feb 16, 2026
Comment on lines
+476
to
+477
| # TODO: this could have the `invalid-match-pattern` error code instead. | ||
| # error: [isinstance-against-typed-dict] "`TypedDict` class `Invalid4` cannot be used in a class pattern" |
Member
There was a problem hiding this comment.
the advantage of a fine-grained error code is that users can double-click on the code straight from their terminal/editor and get taken to very specific rule documentation that describes why the error is being emitted. I'm not so sure that it would be best to consolidate these diagnostics into a single error code.
Contributor
There was a problem hiding this comment.
Users might also find it confusing to see an isinstance diagnostic code here, given that it's hidden in CPython's/ty's implementation.
Member
There was a problem hiding this comment.
that's fair enough. And @abhijeetbodas2001 accurately pointed out that there are lots of types that are accepted in isinstance() checks that are not actually accepted in class patterns. So maybe putting them all under invalid-match-pattern is the more intuitive thing here. No strong opinion.
Contributor
Author
There was a problem hiding this comment.
CPython uses the same message for both the match and isinstance errors. Maybe we could replicate this, as in, use the same message for match and isinstance, but use separate messages for separate causes (TypedDict / protocol / Any).
>>> match 42:
... case C():
... ...
...
Traceback (most recent call last):
File "<python-input-3>", line 2, in <module>
case C():
~^^
# ...
TypeError: TypedDict does not support instance and class checks
>>> isinstance(42, C)
Traceback (most recent call last):
File "<python-input-4>", line 1, in <module>
isinstance(42, C)
~~~~~~~~~~^^^^^^^
# ...
TypeError: TypedDict does not support instance and class checks(To be clear, I mean we stick to invalid-match-pattern for the "allowed by isinstance but not by match set, but have a single diagnostic for each element in the "disallowed by both" set.)
carljm
added a commit
that referenced
this pull request
Feb 16, 2026
* main: (43 commits) [`ruff`] Suppress diagnostic for strings with backslashes in interpolations before Python 3.12 (`RUF027`) (#21069) [flake8-bugbear] Fix B023 false positive for immediately-invoked lambdas (#23294) [ty] Add `Final` mdtests for loops and redeclaration (#23331) [`flake8-pyi`] Also check string annotations (`PYI041`) (#19023) Remove AlexWaygood as a flake8-pyi codeowner (#23347) [ty] Add comments to clarify the purpose of `NominalInstanceType::class_name` and `NominalInstanceType::class_module_name` (#23339) Add attestations for release artifacts and Docker images (#23111) [ty] Fix `assert_type` diagnostic messages (#23342) [ty] Force-update all insta snapshots (#23343) Add Q004 to the list of conflicting rules (#23340) [ty] Fix `invalid-match-pattern` false positives (#23338) [ty] new diagnostic called-match-pattern-must-be-a-type (#22939) [ty] Update flaky projects (#23337) [ty] Increase timeout for ecosystem report to 40 min (#23336) Bump ecosystem-analyzer pin (#23335) [ty] Replace `strsim` with CPython-based Levenshtein implementation (#23291) [ty] Add mdtest for staticmethod assigned in class body (#23330) [ty] fix inferring type variable from string literal argument (#23326) [ty] bytes literal is a sequence of integers (#23329) Update rand and getrandom (#23333) ...
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary
Fixes: astral-sh/ty#2592
Adds a new diagnostic to flag called match patterns where the called object is not a class literal.
Test Plan
New mdtests
Ecosystem changes