From f68080b55ebc268c7c7956b9b80b27c6f690e9bf Mon Sep 17 00:00:00 2001 From: Bhuminjay Soni Date: Wed, 3 Dec 2025 12:01:31 +0530 Subject: [PATCH 01/10] [syntax-error] Default type parameter followed by non-default type parameter (#21657) ## Summary This PR implements syntax error where a default type parameter is followed by a non-default type parameter. https://github.com/astral-sh/ruff/issues/17412#issuecomment-3584088217 ## Test Plan I have written inline tests as directed in #17412 --------- Signed-off-by: 11happy Signed-off-by: 11happy --- crates/ruff_linter/src/checkers/ast/mod.rs | 1 + .../err/type_parameter_default_order.py | 3 + .../ruff_python_parser/src/semantic_errors.rs | 57 +++- ...am_param_spec_invalid_default_expr.py.snap | 9 + ...aram_type_var_invalid_default_expr.py.snap | 9 + ...ype_var_tuple_invalid_default_expr.py.snap | 8 + ...yntax@type_parameter_default_order.py.snap | 277 ++++++++++++++++++ 7 files changed, 359 insertions(+), 5 deletions(-) create mode 100644 crates/ruff_python_parser/resources/inline/err/type_parameter_default_order.py create mode 100644 crates/ruff_python_parser/tests/snapshots/invalid_syntax@type_parameter_default_order.py.snap diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index c1d98b6d50..aa9fa839f2 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -747,6 +747,7 @@ impl SemanticSyntaxContext for Checker<'_> { | SemanticSyntaxErrorKind::LoadBeforeNonlocalDeclaration { .. } | SemanticSyntaxErrorKind::NonlocalAndGlobal(_) | SemanticSyntaxErrorKind::AnnotatedGlobal(_) + | SemanticSyntaxErrorKind::TypeParameterDefaultOrder(_) | SemanticSyntaxErrorKind::AnnotatedNonlocal(_) => { self.semantic_errors.borrow_mut().push(error); } diff --git a/crates/ruff_python_parser/resources/inline/err/type_parameter_default_order.py b/crates/ruff_python_parser/resources/inline/err/type_parameter_default_order.py new file mode 100644 index 0000000000..91362a5007 --- /dev/null +++ b/crates/ruff_python_parser/resources/inline/err/type_parameter_default_order.py @@ -0,0 +1,3 @@ +class C[T = int, U]: ... +class C[T1, T2 = int, T3, T4]: ... +type Alias[T = int, U] = ... diff --git a/crates/ruff_python_parser/src/semantic_errors.rs b/crates/ruff_python_parser/src/semantic_errors.rs index cd7335bdbe..2c573271e1 100644 --- a/crates/ruff_python_parser/src/semantic_errors.rs +++ b/crates/ruff_python_parser/src/semantic_errors.rs @@ -144,11 +144,16 @@ impl SemanticSyntaxChecker { } } } - Stmt::ClassDef(ast::StmtClassDef { type_params, .. }) - | Stmt::TypeAlias(ast::StmtTypeAlias { type_params, .. }) => { - if let Some(type_params) = type_params { - Self::duplicate_type_parameter_name(type_params, ctx); - } + Stmt::ClassDef(ast::StmtClassDef { + type_params: Some(type_params), + .. + }) + | Stmt::TypeAlias(ast::StmtTypeAlias { + type_params: Some(type_params), + .. + }) => { + Self::duplicate_type_parameter_name(type_params, ctx); + Self::type_parameter_default_order(type_params, ctx); } Stmt::Assign(ast::StmtAssign { targets, value, .. }) => { if let [Expr::Starred(ast::ExprStarred { range, .. })] = targets.as_slice() { @@ -611,6 +616,39 @@ impl SemanticSyntaxChecker { } } + fn type_parameter_default_order( + type_params: &ast::TypeParams, + ctx: &Ctx, + ) { + let mut seen_default = false; + for type_param in type_params.iter() { + let has_default = match type_param { + ast::TypeParam::TypeVar(ast::TypeParamTypeVar { default, .. }) + | ast::TypeParam::TypeVarTuple(ast::TypeParamTypeVarTuple { default, .. }) + | ast::TypeParam::ParamSpec(ast::TypeParamParamSpec { default, .. }) => { + default.is_some() + } + }; + + if seen_default && !has_default { + // test_err type_parameter_default_order + // class C[T = int, U]: ... + // class C[T1, T2 = int, T3, T4]: ... + // type Alias[T = int, U] = ... + Self::add_error( + ctx, + SemanticSyntaxErrorKind::TypeParameterDefaultOrder( + type_param.name().id.to_string(), + ), + type_param.range(), + ); + } + if has_default { + seen_default = true; + } + } + } + fn duplicate_parameter_name( parameters: &ast::Parameters, ctx: &Ctx, @@ -1066,6 +1104,12 @@ impl Display for SemanticSyntaxError { SemanticSyntaxErrorKind::DuplicateTypeParameter => { f.write_str("duplicate type parameter") } + SemanticSyntaxErrorKind::TypeParameterDefaultOrder(name) => { + write!( + f, + "non default type parameter `{name}` follows default type parameter" + ) + } SemanticSyntaxErrorKind::MultipleCaseAssignment(name) => { write!(f, "multiple assignments to name `{name}` in pattern") } @@ -1572,6 +1616,9 @@ pub enum SemanticSyntaxErrorKind { /// Represents a nonlocal statement for a name that has no binding in an enclosing scope. NonlocalWithoutBinding(String), + + /// Represents a default type parameter followed by a non-default type parameter. + TypeParameterDefaultOrder(String), } #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, get_size2::GetSize)] diff --git a/crates/ruff_python_parser/tests/snapshots/invalid_syntax@type_param_param_spec_invalid_default_expr.py.snap b/crates/ruff_python_parser/tests/snapshots/invalid_syntax@type_param_param_spec_invalid_default_expr.py.snap index 0bcbd3cc52..fd2b2f4714 100644 --- a/crates/ruff_python_parser/tests/snapshots/invalid_syntax@type_param_param_spec_invalid_default_expr.py.snap +++ b/crates/ruff_python_parser/tests/snapshots/invalid_syntax@type_param_param_spec_invalid_default_expr.py.snap @@ -375,3 +375,12 @@ Module( 4 | type X[**P = x := int] = int 5 | type X[**P = *int] = int | + + + | +2 | type X[**P = yield x] = int +3 | type X[**P = yield from x] = int +4 | type X[**P = x := int] = int + | ^^^ Syntax Error: non default type parameter `int` follows default type parameter +5 | type X[**P = *int] = int + | diff --git a/crates/ruff_python_parser/tests/snapshots/invalid_syntax@type_param_type_var_invalid_default_expr.py.snap b/crates/ruff_python_parser/tests/snapshots/invalid_syntax@type_param_type_var_invalid_default_expr.py.snap index c3e38b8e9a..0b4041088c 100644 --- a/crates/ruff_python_parser/tests/snapshots/invalid_syntax@type_param_type_var_invalid_default_expr.py.snap +++ b/crates/ruff_python_parser/tests/snapshots/invalid_syntax@type_param_type_var_invalid_default_expr.py.snap @@ -459,3 +459,12 @@ Module( 5 | type X[T = x := int] = int 6 | type X[T: int = *int] = int | + + + | +3 | type X[T = (yield x)] = int +4 | type X[T = yield from x] = int +5 | type X[T = x := int] = int + | ^^^ Syntax Error: non default type parameter `int` follows default type parameter +6 | type X[T: int = *int] = int + | diff --git a/crates/ruff_python_parser/tests/snapshots/invalid_syntax@type_param_type_var_tuple_invalid_default_expr.py.snap b/crates/ruff_python_parser/tests/snapshots/invalid_syntax@type_param_type_var_tuple_invalid_default_expr.py.snap index 4aff137a73..8cd49704ce 100644 --- a/crates/ruff_python_parser/tests/snapshots/invalid_syntax@type_param_type_var_tuple_invalid_default_expr.py.snap +++ b/crates/ruff_python_parser/tests/snapshots/invalid_syntax@type_param_type_var_tuple_invalid_default_expr.py.snap @@ -384,3 +384,11 @@ Module( | ^^^^^^^^^^^^ Syntax Error: yield expression cannot be used within a TypeVarTuple default 5 | type X[*Ts = x := int] = int | + + + | +3 | type X[*Ts = yield x] = int +4 | type X[*Ts = yield from x] = int +5 | type X[*Ts = x := int] = int + | ^^^ Syntax Error: non default type parameter `int` follows default type parameter + | diff --git a/crates/ruff_python_parser/tests/snapshots/invalid_syntax@type_parameter_default_order.py.snap b/crates/ruff_python_parser/tests/snapshots/invalid_syntax@type_parameter_default_order.py.snap new file mode 100644 index 0000000000..a8280e947a --- /dev/null +++ b/crates/ruff_python_parser/tests/snapshots/invalid_syntax@type_parameter_default_order.py.snap @@ -0,0 +1,277 @@ +--- +source: crates/ruff_python_parser/tests/fixtures.rs +input_file: crates/ruff_python_parser/resources/inline/err/type_parameter_default_order.py +--- +## AST + +``` +Module( + ModModule { + node_index: NodeIndex(None), + range: 0..89, + body: [ + ClassDef( + StmtClassDef { + node_index: NodeIndex(None), + range: 0..24, + decorator_list: [], + name: Identifier { + id: Name("C"), + range: 6..7, + node_index: NodeIndex(None), + }, + type_params: Some( + TypeParams { + range: 7..19, + node_index: NodeIndex(None), + type_params: [ + TypeVar( + TypeParamTypeVar { + node_index: NodeIndex(None), + range: 8..15, + name: Identifier { + id: Name("T"), + range: 8..9, + node_index: NodeIndex(None), + }, + bound: None, + default: Some( + Name( + ExprName { + node_index: NodeIndex(None), + range: 12..15, + id: Name("int"), + ctx: Load, + }, + ), + ), + }, + ), + TypeVar( + TypeParamTypeVar { + node_index: NodeIndex(None), + range: 17..18, + name: Identifier { + id: Name("U"), + range: 17..18, + node_index: NodeIndex(None), + }, + bound: None, + default: None, + }, + ), + ], + }, + ), + arguments: None, + body: [ + Expr( + StmtExpr { + node_index: NodeIndex(None), + range: 21..24, + value: EllipsisLiteral( + ExprEllipsisLiteral { + node_index: NodeIndex(None), + range: 21..24, + }, + ), + }, + ), + ], + }, + ), + ClassDef( + StmtClassDef { + node_index: NodeIndex(None), + range: 25..59, + decorator_list: [], + name: Identifier { + id: Name("C"), + range: 31..32, + node_index: NodeIndex(None), + }, + type_params: Some( + TypeParams { + range: 32..54, + node_index: NodeIndex(None), + type_params: [ + TypeVar( + TypeParamTypeVar { + node_index: NodeIndex(None), + range: 33..35, + name: Identifier { + id: Name("T1"), + range: 33..35, + node_index: NodeIndex(None), + }, + bound: None, + default: None, + }, + ), + TypeVar( + TypeParamTypeVar { + node_index: NodeIndex(None), + range: 37..45, + name: Identifier { + id: Name("T2"), + range: 37..39, + node_index: NodeIndex(None), + }, + bound: None, + default: Some( + Name( + ExprName { + node_index: NodeIndex(None), + range: 42..45, + id: Name("int"), + ctx: Load, + }, + ), + ), + }, + ), + TypeVar( + TypeParamTypeVar { + node_index: NodeIndex(None), + range: 47..49, + name: Identifier { + id: Name("T3"), + range: 47..49, + node_index: NodeIndex(None), + }, + bound: None, + default: None, + }, + ), + TypeVar( + TypeParamTypeVar { + node_index: NodeIndex(None), + range: 51..53, + name: Identifier { + id: Name("T4"), + range: 51..53, + node_index: NodeIndex(None), + }, + bound: None, + default: None, + }, + ), + ], + }, + ), + arguments: None, + body: [ + Expr( + StmtExpr { + node_index: NodeIndex(None), + range: 56..59, + value: EllipsisLiteral( + ExprEllipsisLiteral { + node_index: NodeIndex(None), + range: 56..59, + }, + ), + }, + ), + ], + }, + ), + TypeAlias( + StmtTypeAlias { + node_index: NodeIndex(None), + range: 60..88, + name: Name( + ExprName { + node_index: NodeIndex(None), + range: 65..70, + id: Name("Alias"), + ctx: Store, + }, + ), + type_params: Some( + TypeParams { + range: 70..82, + node_index: NodeIndex(None), + type_params: [ + TypeVar( + TypeParamTypeVar { + node_index: NodeIndex(None), + range: 71..78, + name: Identifier { + id: Name("T"), + range: 71..72, + node_index: NodeIndex(None), + }, + bound: None, + default: Some( + Name( + ExprName { + node_index: NodeIndex(None), + range: 75..78, + id: Name("int"), + ctx: Load, + }, + ), + ), + }, + ), + TypeVar( + TypeParamTypeVar { + node_index: NodeIndex(None), + range: 80..81, + name: Identifier { + id: Name("U"), + range: 80..81, + node_index: NodeIndex(None), + }, + bound: None, + default: None, + }, + ), + ], + }, + ), + value: EllipsisLiteral( + ExprEllipsisLiteral { + node_index: NodeIndex(None), + range: 85..88, + }, + ), + }, + ), + ], + }, +) +``` +## Semantic Syntax Errors + + | +1 | class C[T = int, U]: ... + | ^ Syntax Error: non default type parameter `U` follows default type parameter +2 | class C[T1, T2 = int, T3, T4]: ... +3 | type Alias[T = int, U] = ... + | + + + | +1 | class C[T = int, U]: ... +2 | class C[T1, T2 = int, T3, T4]: ... + | ^^ Syntax Error: non default type parameter `T3` follows default type parameter +3 | type Alias[T = int, U] = ... + | + + + | +1 | class C[T = int, U]: ... +2 | class C[T1, T2 = int, T3, T4]: ... + | ^^ Syntax Error: non default type parameter `T4` follows default type parameter +3 | type Alias[T = int, U] = ... + | + + + | +1 | class C[T = int, U]: ... +2 | class C[T1, T2 = int, T3, T4]: ... +3 | type Alias[T = int, U] = ... + | ^ Syntax Error: non default type parameter `U` follows default type parameter + | From c5b8d551dff5e0ec8db776c6cb5951bbd4a03d57 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 3 Dec 2025 08:05:25 +0000 Subject: [PATCH 02/10] [ty] Suppress false positives when `dataclasses.dataclass(...)(cls)` is called imperatively (#21729) Fixes https://github.com/astral-sh/ty/issues/1705 --- .../mdtest/dataclasses/dataclasses.md | 54 +++++++++++++++++++ crates/ty_python_semantic/src/types.rs | 20 +++++-- 2 files changed, 71 insertions(+), 3 deletions(-) diff --git a/crates/ty_python_semantic/resources/mdtest/dataclasses/dataclasses.md b/crates/ty_python_semantic/resources/mdtest/dataclasses/dataclasses.md index ba5151fa61..fa6f76de75 100644 --- a/crates/ty_python_semantic/resources/mdtest/dataclasses/dataclasses.md +++ b/crates/ty_python_semantic/resources/mdtest/dataclasses/dataclasses.md @@ -1462,3 +1462,57 @@ def test_c(): c = C(1) c.__lt__ = Mock() ``` + +## Imperatively calling `dataclasses.dataclass` + +While we do not currently recognize the special behaviour of `dataclasses.dataclass` if it is called +imperatively, we recognize that it can be called imperatively and do not emit any false-positive +diagnostics on such calls: + +```py +from dataclasses import dataclass +from typing_extensions import TypeVar, dataclass_transform + +U = TypeVar("U") + +@dataclass_transform(kw_only_default=True) +def sequence(cls: type[U]) -> type[U]: + d = dataclass( + repr=False, + eq=False, + match_args=False, + kw_only=True, + )(cls) + reveal_type(d) # revealed: type[U@sequence] & Any + return d + +@dataclass_transform(kw_only_default=True) +def sequence2(cls: type) -> type: + d = dataclass( + repr=False, + eq=False, + match_args=False, + kw_only=True, + )(cls) + reveal_type(d) # revealed: type & Any + return d + +@dataclass_transform(kw_only_default=True) +def sequence3(cls: type[U]) -> type[U]: + # TODO: should reveal `type[U@sequence3]` + return reveal_type(dataclass(cls)) # revealed: Unknown + +@dataclass_transform(kw_only_default=True) +def sequence4(cls: type) -> type: + # TODO: should reveal `type` + return reveal_type(dataclass(cls)) # revealed: Unknown + +class Foo: ... + +ordered_foo = dataclass(order=True)(Foo) +reveal_type(ordered_foo) # revealed: type[Foo] & Any +# TODO: should be `Foo & Any` +reveal_type(ordered_foo()) # revealed: @Todo(Type::Intersection.call) +# TODO: should be `Any` +reveal_type(ordered_foo() < ordered_foo()) # revealed: @Todo(Type::Intersection.call) +``` diff --git a/crates/ty_python_semantic/src/types.rs b/crates/ty_python_semantic/src/types.rs index e64d3802d5..2cafdcfcdd 100644 --- a/crates/ty_python_semantic/src/types.rs +++ b/crates/ty_python_semantic/src/types.rs @@ -6226,11 +6226,25 @@ impl<'db> Type<'db> { ), Type::Intersection(_) => { - Binding::single(self, Signature::todo("Type::Intersection.call()")).into() + Binding::single(self, Signature::todo("Type::Intersection.call")).into() } - // TODO: this is actually callable - Type::DataclassDecorator(_) => CallableBinding::not_callable(self).into(), + Type::DataclassDecorator(_) => { + let typevar = BoundTypeVarInstance::synthetic(db, "T", TypeVarVariance::Invariant); + let typevar_meta = SubclassOfType::from(db, typevar); + let context = GenericContext::from_typevar_instances(db, [typevar]); + let parameters = [Parameter::positional_only(Some(Name::new_static("cls"))) + .with_annotated_type(typevar_meta)]; + // Intersect with `Any` for the return type to reflect the fact that the `dataclass()` + // decorator adds methods to the class + let returns = IntersectionType::from_elements(db, [typevar_meta, Type::any()]); + let signature = Signature::new_generic( + Some(context), + Parameters::new(db, parameters), + Some(returns), + ); + Binding::single(self, signature).into() + } // TODO: some `SpecialForm`s are callable (e.g. TypedDicts) Type::SpecialForm(_) => CallableBinding::not_callable(self).into(), From e6ddeed38667db198424fa276e3efdbd8cea3caa Mon Sep 17 00:00:00 2001 From: David Peter Date: Wed, 3 Dec 2025 09:10:45 +0100 Subject: [PATCH 03/10] [ty] Default-specialization of generic type aliases (#21765) ## Summary Implement default-specialization of generic type aliases (implicit or PEP-613) if they are used in a type expression without an explicit specialization. closes https://github.com/astral-sh/ty/issues/1690 ## Typing conformance ```diff -generics_defaults_specialization.py:26:5: error[type-assertion-failure] Type `SomethingWithNoDefaults[int, str]` does not match asserted type `SomethingWithNoDefaults[int, DefaultStrT]` ``` That's exactly what we want :heavy_check_mark: All other tests in this file pass as well, with the exception of this assertion, which is just wrong (at least according to our interpretation, `type[Bar] != `). I checked that we do correctly default-specialize the type parameter which is not displayed in the diagnostic that we raise. ```py class Bar(SubclassMe[int, DefaultStrT]): ... assert_type(Bar, type[Bar[str]]) # ty: Type `type[Bar[str]]` does not match asserted type `` ``` ## Ecosystem impact Looks like I should have included this last week :sunglasses: ## Test Plan Updated pre-existing tests and add a few new ones. --- .../resources/mdtest/implicit_type_aliases.md | 57 ++++++++----------- crates/ty_python_semantic/src/types.rs | 10 ++++ .../infer/builder/annotation_expression.rs | 23 ++++---- .../types/infer/builder/type_expression.rs | 2 + 4 files changed, 49 insertions(+), 43 deletions(-) diff --git a/crates/ty_python_semantic/resources/mdtest/implicit_type_aliases.md b/crates/ty_python_semantic/resources/mdtest/implicit_type_aliases.md index 8207d55bcc..ed73c9323b 100644 --- a/crates/ty_python_semantic/resources/mdtest/implicit_type_aliases.md +++ b/crates/ty_python_semantic/resources/mdtest/implicit_type_aliases.md @@ -190,14 +190,10 @@ def _( reveal_type(type_of_str_or_int) # revealed: type[str] | int reveal_type(int_or_callable) # revealed: int | ((str, /) -> bytes) reveal_type(callable_or_int) # revealed: ((str, /) -> bytes) | int - # TODO should be Unknown | int - reveal_type(type_var_or_int) # revealed: T@TypeVarOrInt | int - # TODO should be int | Unknown - reveal_type(int_or_type_var) # revealed: int | T@IntOrTypeVar - # TODO should be Unknown | None - reveal_type(type_var_or_none) # revealed: T@TypeVarOrNone | None - # TODO should be None | Unknown - reveal_type(none_or_type_var) # revealed: None | T@NoneOrTypeVar + reveal_type(type_var_or_int) # revealed: Unknown | int + reveal_type(int_or_type_var) # revealed: int | Unknown + reveal_type(type_var_or_none) # revealed: Unknown | None + reveal_type(none_or_type_var) # revealed: None | Unknown ``` If a type is unioned with itself in a value expression, the result is just that type. No @@ -529,28 +525,18 @@ def _( annotated_unknown: AnnotatedType, optional_unknown: MyOptional, ): - # TODO: This should be `list[Unknown]` - reveal_type(list_unknown) # revealed: list[T@MyList] - # TODO: This should be `dict[Unknown, Unknown]` - reveal_type(dict_unknown) # revealed: dict[T@MyDict, U@MyDict] - # TODO: Should be `type[Unknown]` - reveal_type(subclass_of_unknown) # revealed: type[T@MyType] - # TODO: Should be `tuple[int, Unknown]` - reveal_type(int_and_unknown) # revealed: tuple[int, T@IntAndType] - # TODO: Should be `tuple[Unknown, Unknown]` - reveal_type(pair_of_unknown) # revealed: tuple[T@Pair, T@Pair] - # TODO: Should be `tuple[Unknown, Unknown]` - reveal_type(unknown_and_unknown) # revealed: tuple[T@Sum, U@Sum] - # TODO: Should be `list[Unknown] | tuple[Unknown, ...]` - reveal_type(list_or_tuple) # revealed: list[T@ListOrTuple] | tuple[T@ListOrTuple, ...] - # TODO: Should be `list[Unknown] | tuple[Unknown, ...]` - reveal_type(list_or_tuple_legacy) # revealed: list[T@ListOrTupleLegacy] | tuple[T@ListOrTupleLegacy, ...] - # TODO: Should be `(...) -> Unknown` + reveal_type(list_unknown) # revealed: list[Unknown] + reveal_type(dict_unknown) # revealed: dict[Unknown, Unknown] + reveal_type(subclass_of_unknown) # revealed: type[Unknown] + reveal_type(int_and_unknown) # revealed: tuple[int, Unknown] + reveal_type(pair_of_unknown) # revealed: tuple[Unknown, Unknown] + reveal_type(unknown_and_unknown) # revealed: tuple[Unknown, Unknown] + reveal_type(list_or_tuple) # revealed: list[Unknown] | tuple[Unknown, ...] + reveal_type(list_or_tuple_legacy) # revealed: list[Unknown] | tuple[Unknown, ...] + # TODO: should be (...) -> Unknown reveal_type(my_callable) # revealed: @Todo(Callable[..] specialized with ParamSpec) - # TODO: Should be `Unknown` - reveal_type(annotated_unknown) # revealed: T@AnnotatedType - # TODO: Should be `Unknown | None` - reveal_type(optional_unknown) # revealed: T@MyOptional | None + reveal_type(annotated_unknown) # revealed: Unknown + reveal_type(optional_unknown) # revealed: Unknown | None ``` For a type variable with a default, we use the default type: @@ -563,10 +549,13 @@ MyListWithDefault = list[T_default] def _( list_of_str: MyListWithDefault[str], list_of_int: MyListWithDefault, + list_of_str_or_none: MyListWithDefault[str] | None, + list_of_int_or_none: MyListWithDefault | None, ): reveal_type(list_of_str) # revealed: list[str] - # TODO: this should be `list[int]` - reveal_type(list_of_int) # revealed: list[T_default@MyListWithDefault] + reveal_type(list_of_int) # revealed: list[int] + reveal_type(list_of_str_or_none) # revealed: list[str] | None + reveal_type(list_of_int_or_none) # revealed: list[int] | None ``` (Generic) implicit type aliases can be used as base classes: @@ -601,7 +590,7 @@ Generic implicit type aliases can be imported from other modules and specialized ```py from typing_extensions import TypeVar -T = TypeVar("T") +T = TypeVar("T", default=str) MyList = list[T] ``` @@ -615,9 +604,13 @@ import my_types as mt def _( list_of_ints1: MyList[int], list_of_ints2: mt.MyList[int], + list_of_str: mt.MyList, + list_of_str_or_none: mt.MyList | None, ): reveal_type(list_of_ints1) # revealed: list[int] reveal_type(list_of_ints2) # revealed: list[int] + reveal_type(list_of_str) # revealed: list[str] + reveal_type(list_of_str_or_none) # revealed: list[str] | None ``` ### In stringified annotations diff --git a/crates/ty_python_semantic/src/types.rs b/crates/ty_python_semantic/src/types.rs index 2cafdcfcdd..f957c2193c 100644 --- a/crates/ty_python_semantic/src/types.rs +++ b/crates/ty_python_semantic/src/types.rs @@ -8263,6 +8263,16 @@ impl<'db> Type<'db> { _ => None, } } + + /// Default-specialize all legacy typevars in this type. + /// + /// This is used when an implicit type alias is referenced without explicitly specializing it. + pub(crate) fn default_specialize(self, db: &'db dyn Db) -> Type<'db> { + let mut variables = FxOrderSet::default(); + self.find_legacy_typevars(db, None, &mut variables); + let generic_context = GenericContext::from_typevar_instances(db, variables); + self.apply_specialization(db, generic_context.default_specialization(db, None)) + } } impl<'db> From<&Type<'db>> for Type<'db> { diff --git a/crates/ty_python_semantic/src/types/infer/builder/annotation_expression.rs b/crates/ty_python_semantic/src/types/infer/builder/annotation_expression.rs index 8c211735cd..e143b5c7fa 100644 --- a/crates/ty_python_semantic/src/types/infer/builder/annotation_expression.rs +++ b/crates/ty_python_semantic/src/types/infer/builder/annotation_expression.rs @@ -144,18 +144,19 @@ impl<'db> TypeInferenceBuilder<'db, '_> { ) } _ => TypeAndQualifiers::declared( - ty.in_type_expression( - builder.db(), - builder.scope(), - builder.typevar_binding_context, - ) - .unwrap_or_else(|error| { - error.into_fallback_type( - &builder.context, - annotation, - builder.is_reachable(annotation), + ty.default_specialize(builder.db()) + .in_type_expression( + builder.db(), + builder.scope(), + builder.typevar_binding_context, ) - }), + .unwrap_or_else(|error| { + error.into_fallback_type( + &builder.context, + annotation, + builder.is_reachable(annotation), + ) + }), ), } } diff --git a/crates/ty_python_semantic/src/types/infer/builder/type_expression.rs b/crates/ty_python_semantic/src/types/infer/builder/type_expression.rs index 0e3a326139..edee2bb5da 100644 --- a/crates/ty_python_semantic/src/types/infer/builder/type_expression.rs +++ b/crates/ty_python_semantic/src/types/infer/builder/type_expression.rs @@ -91,6 +91,7 @@ impl<'db> TypeInferenceBuilder<'db, '_> { ast::Expr::Name(name) => match name.ctx { ast::ExprContext::Load => self .infer_name_expression(name) + .default_specialize(self.db()) .in_type_expression(self.db(), self.scope(), self.typevar_binding_context) .unwrap_or_else(|error| { error.into_fallback_type( @@ -108,6 +109,7 @@ impl<'db> TypeInferenceBuilder<'db, '_> { ast::Expr::Attribute(attribute_expression) => match attribute_expression.ctx { ast::ExprContext::Load => self .infer_attribute_expression(attribute_expression) + .default_specialize(self.db()) .in_type_expression(self.db(), self.scope(), self.typevar_binding_context) .unwrap_or_else(|error| { error.into_fallback_type( From f4e4229683936a486f689aebb9d7d06b3985952d Mon Sep 17 00:00:00 2001 From: Denys Zhak Date: Wed, 3 Dec 2025 09:15:17 +0100 Subject: [PATCH 04/10] Add token based `parenthesized_ranges` implementation (#21738) Co-authored-by: Micha Reiser --- crates/ruff_python_ast/src/parenthesize.rs | 4 + crates/ruff_python_ast/src/token.rs | 2 + .../ruff_python_ast/src/token/parentheses.rs | 58 +++++ .../tests/parentheses.rs | 199 ++++++++++++++++++ crates/ty/docs/rules.md | 148 ++++++------- .../src/types/diagnostic.rs | 8 +- 6 files changed, 339 insertions(+), 80 deletions(-) create mode 100644 crates/ruff_python_ast/src/token/parentheses.rs create mode 100644 crates/ruff_python_ast_integration_tests/tests/parentheses.rs diff --git a/crates/ruff_python_ast/src/parenthesize.rs b/crates/ruff_python_ast/src/parenthesize.rs index a7fb1224ce..786ca0572c 100644 --- a/crates/ruff_python_ast/src/parenthesize.rs +++ b/crates/ruff_python_ast/src/parenthesize.rs @@ -11,6 +11,8 @@ use crate::ExprRef; /// Note that without a parent the range can be inaccurate, e.g. `f(a)` we falsely return a set of /// parentheses around `a` even if the parentheses actually belong to `f`. That is why you should /// generally prefer [`parenthesized_range`]. +/// +/// Prefer [`crate::token::parentheses_iterator`] if you have access to [`crate::token::Tokens`]. pub fn parentheses_iterator<'a>( expr: ExprRef<'a>, parent: Option, @@ -57,6 +59,8 @@ pub fn parentheses_iterator<'a>( /// Returns the [`TextRange`] of a given expression including parentheses, if the expression is /// parenthesized; or `None`, if the expression is not parenthesized. +/// +/// Prefer [`crate::token::parenthesized_range`] if you have access to [`crate::token::Tokens`]. pub fn parenthesized_range( expr: ExprRef, parent: AnyNodeRef, diff --git a/crates/ruff_python_ast/src/token.rs b/crates/ruff_python_ast/src/token.rs index fc1b62a366..4b9d98ec5c 100644 --- a/crates/ruff_python_ast/src/token.rs +++ b/crates/ruff_python_ast/src/token.rs @@ -16,8 +16,10 @@ use crate::str_prefix::{ use crate::{AnyStringFlags, BoolOp, Operator, StringFlags, UnaryOp}; use ruff_text_size::{Ranged, TextRange}; +mod parentheses; mod tokens; +pub use parentheses::{parentheses_iterator, parenthesized_range}; pub use tokens::{TokenAt, TokenIterWithContext, Tokens}; #[derive(Clone, Copy, PartialEq, Eq)] diff --git a/crates/ruff_python_ast/src/token/parentheses.rs b/crates/ruff_python_ast/src/token/parentheses.rs new file mode 100644 index 0000000000..c1d6f40650 --- /dev/null +++ b/crates/ruff_python_ast/src/token/parentheses.rs @@ -0,0 +1,58 @@ +use ruff_text_size::{Ranged, TextLen, TextRange}; + +use super::{TokenKind, Tokens}; +use crate::{AnyNodeRef, ExprRef}; + +/// Returns an iterator over the ranges of the optional parentheses surrounding an expression. +/// +/// E.g. for `((f()))` with `f()` as expression, the iterator returns the ranges (1, 6) and (0, 7). +/// +/// Note that without a parent the range can be inaccurate, e.g. `f(a)` we falsely return a set of +/// parentheses around `a` even if the parentheses actually belong to `f`. That is why you should +/// generally prefer [`parenthesized_range`]. +pub fn parentheses_iterator<'a>( + expr: ExprRef<'a>, + parent: Option, + tokens: &'a Tokens, +) -> impl Iterator + 'a { + let after_tokens = if let Some(parent) = parent { + // If the parent is a node that brings its own parentheses, exclude the closing parenthesis + // from our search range. Otherwise, we risk matching on calls, like `func(x)`, for which + // the open and close parentheses are part of the `Arguments` node. + let exclusive_parent_end = if parent.is_arguments() { + parent.end() - ")".text_len() + } else { + parent.end() + }; + + tokens.in_range(TextRange::new(expr.end(), exclusive_parent_end)) + } else { + tokens.after(expr.end()) + }; + + let right_parens = after_tokens + .iter() + .filter(|token| !token.kind().is_trivia()) + .take_while(move |token| token.kind() == TokenKind::Rpar); + + let left_parens = tokens + .before(expr.start()) + .iter() + .rev() + .filter(|token| !token.kind().is_trivia()) + .take_while(|token| token.kind() == TokenKind::Lpar); + + right_parens + .zip(left_parens) + .map(|(right, left)| TextRange::new(left.start(), right.end())) +} + +/// Returns the [`TextRange`] of a given expression including parentheses, if the expression is +/// parenthesized; or `None`, if the expression is not parenthesized. +pub fn parenthesized_range( + expr: ExprRef, + parent: AnyNodeRef, + tokens: &Tokens, +) -> Option { + parentheses_iterator(expr, Some(parent), tokens).last() +} diff --git a/crates/ruff_python_ast_integration_tests/tests/parentheses.rs b/crates/ruff_python_ast_integration_tests/tests/parentheses.rs new file mode 100644 index 0000000000..479c456c39 --- /dev/null +++ b/crates/ruff_python_ast_integration_tests/tests/parentheses.rs @@ -0,0 +1,199 @@ +//! Tests for [`ruff_python_ast::tokens::parentheses_iterator`] and +//! [`ruff_python_ast::tokens::parenthesized_range`]. + +use ruff_python_ast::{ + self as ast, Expr, + token::{parentheses_iterator, parenthesized_range}, +}; +use ruff_python_parser::parse_module; + +#[test] +fn test_no_parentheses() { + let source = "x = 2 + 2"; + let parsed = parse_module(source).expect("should parse valid python"); + let tokens = parsed.tokens(); + let module = parsed.syntax(); + + let stmt = module.body.first().expect("module should have a statement"); + let ast::Stmt::Assign(assign) = stmt else { + panic!("expected `Assign` statement, got {stmt:?}"); + }; + + let result = parenthesized_range(assign.value.as_ref().into(), stmt.into(), tokens); + assert_eq!(result, None); +} + +#[test] +fn test_single_parentheses() { + let source = "x = (2 + 2)"; + let parsed = parse_module(source).expect("should parse valid python"); + let tokens = parsed.tokens(); + let module = parsed.syntax(); + + let stmt = module.body.first().expect("module should have a statement"); + let ast::Stmt::Assign(assign) = stmt else { + panic!("expected `Assign` statement, got {stmt:?}"); + }; + + let result = parenthesized_range(assign.value.as_ref().into(), stmt.into(), tokens); + let range = result.expect("should find parentheses"); + assert_eq!(&source[range], "(2 + 2)"); +} + +#[test] +fn test_double_parentheses() { + let source = "x = ((2 + 2))"; + let parsed = parse_module(source).expect("should parse valid python"); + let tokens = parsed.tokens(); + let module = parsed.syntax(); + + let stmt = module.body.first().expect("module should have a statement"); + let ast::Stmt::Assign(assign) = stmt else { + panic!("expected `Assign` statement, got {stmt:?}"); + }; + + let result = parenthesized_range(assign.value.as_ref().into(), stmt.into(), tokens); + let range = result.expect("should find parentheses"); + assert_eq!(&source[range], "((2 + 2))"); +} + +#[test] +fn test_parentheses_with_whitespace() { + let source = "x = ( 2 + 2 )"; + let parsed = parse_module(source).expect("should parse valid python"); + let tokens = parsed.tokens(); + let module = parsed.syntax(); + + let stmt = module.body.first().expect("module should have a statement"); + let ast::Stmt::Assign(assign) = stmt else { + panic!("expected `Assign` statement, got {stmt:?}"); + }; + + let result = parenthesized_range(assign.value.as_ref().into(), stmt.into(), tokens); + let range = result.expect("should find parentheses"); + assert_eq!(&source[range], "( 2 + 2 )"); +} + +#[test] +fn test_parentheses_with_comments() { + let source = "x = ( # comment\n 2 + 2\n)"; + let parsed = parse_module(source).expect("should parse valid python"); + let tokens = parsed.tokens(); + let module = parsed.syntax(); + + let stmt = module.body.first().expect("module should have a statement"); + let ast::Stmt::Assign(assign) = stmt else { + panic!("expected `Assign` statement, got {stmt:?}"); + }; + + let result = parenthesized_range(assign.value.as_ref().into(), stmt.into(), tokens); + let range = result.expect("should find parentheses"); + assert_eq!(&source[range], "( # comment\n 2 + 2\n)"); +} + +#[test] +fn test_parenthesized_range_multiple() { + let source = "x = (((2 + 2)))"; + let parsed = parse_module(source).expect("should parse valid python"); + let tokens = parsed.tokens(); + let module = parsed.syntax(); + + let stmt = module.body.first().expect("module should have a statement"); + let ast::Stmt::Assign(assign) = stmt else { + panic!("expected `Assign` statement, got {stmt:?}"); + }; + + let result = parenthesized_range(assign.value.as_ref().into(), stmt.into(), tokens); + let range = result.expect("should find parentheses"); + assert_eq!(&source[range], "(((2 + 2)))"); +} + +#[test] +fn test_parentheses_iterator_multiple() { + let source = "x = (((2 + 2)))"; + let parsed = parse_module(source).expect("should parse valid python"); + let tokens = parsed.tokens(); + let module = parsed.syntax(); + + let stmt = module.body.first().expect("module should have a statement"); + let ast::Stmt::Assign(assign) = stmt else { + panic!("expected `Assign` statement, got {stmt:?}"); + }; + + let ranges: Vec<_> = + parentheses_iterator(assign.value.as_ref().into(), Some(stmt.into()), tokens).collect(); + assert_eq!(ranges.len(), 3); + assert_eq!(&source[ranges[0]], "(2 + 2)"); + assert_eq!(&source[ranges[1]], "((2 + 2))"); + assert_eq!(&source[ranges[2]], "(((2 + 2)))"); +} + +#[test] +fn test_call_arguments_not_counted() { + let source = "f(x)"; + let parsed = parse_module(source).expect("should parse valid python"); + let tokens = parsed.tokens(); + let module = parsed.syntax(); + + let stmt = module.body.first().expect("module should have a statement"); + let ast::Stmt::Expr(expr_stmt) = stmt else { + panic!("expected `Expr` statement, got {stmt:?}"); + }; + + let Expr::Call(call) = expr_stmt.value.as_ref() else { + panic!("expected Call expression, got {:?}", expr_stmt.value); + }; + + let arg = call + .arguments + .args + .first() + .expect("call should have an argument"); + let result = parenthesized_range(arg.into(), (&call.arguments).into(), tokens); + // The parentheses belong to the call, not the argument + assert_eq!(result, None); +} + +#[test] +fn test_call_with_parenthesized_argument() { + let source = "f((x))"; + let parsed = parse_module(source).expect("should parse valid python"); + let tokens = parsed.tokens(); + let module = parsed.syntax(); + + let stmt = module.body.first().expect("module should have a statement"); + let ast::Stmt::Expr(expr_stmt) = stmt else { + panic!("expected Expr statement, got {stmt:?}"); + }; + + let Expr::Call(call) = expr_stmt.value.as_ref() else { + panic!("expected `Call` expression, got {:?}", expr_stmt.value); + }; + + let arg = call + .arguments + .args + .first() + .expect("call should have an argument"); + let result = parenthesized_range(arg.into(), (&call.arguments).into(), tokens); + + let range = result.expect("should find parentheses around argument"); + assert_eq!(&source[range], "(x)"); +} + +#[test] +fn test_multiline_with_parentheses() { + let source = "x = (\n 2 + 2 + 2\n)"; + let parsed = parse_module(source).expect("should parse valid python"); + let tokens = parsed.tokens(); + let module = parsed.syntax(); + + let stmt = module.body.first().expect("module should have a statement"); + let ast::Stmt::Assign(assign) = stmt else { + panic!("expected `Assign` statement, got {stmt:?}"); + }; + + let result = parenthesized_range(assign.value.as_ref().into(), stmt.into(), tokens); + let range = result.expect("should find parentheses"); + assert_eq!(&source[range], "(\n 2 + 2 + 2\n)"); +} diff --git a/crates/ty/docs/rules.md b/crates/ty/docs/rules.md index 12eb74d15a..5ac36c4fb9 100644 --- a/crates/ty/docs/rules.md +++ b/crates/ty/docs/rules.md @@ -39,7 +39,7 @@ def test(): -> "int": Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -63,7 +63,7 @@ Calling a non-callable object will raise a `TypeError` at runtime. Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -95,7 +95,7 @@ f(int) # error Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -126,7 +126,7 @@ a = 1 Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -158,7 +158,7 @@ class C(A, B): ... Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -190,7 +190,7 @@ class B(A): ... Default level: error · Preview (since 1.0.0) · Related issues · -View source +View source @@ -218,7 +218,7 @@ type B = A Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -245,7 +245,7 @@ class B(A, A): ... Default level: error · Added in 0.0.1-alpha.12 · Related issues · -View source +View source @@ -357,7 +357,7 @@ def test(): -> "Literal[5]": Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -387,7 +387,7 @@ class C(A, B): ... Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -413,7 +413,7 @@ t[3] # IndexError: tuple index out of range Default level: error · Added in 0.0.1-alpha.12 · Related issues · -View source +View source @@ -502,7 +502,7 @@ an atypical memory layout. Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -529,7 +529,7 @@ func("foo") # error: [invalid-argument-type] Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -557,7 +557,7 @@ a: int = '' Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -591,7 +591,7 @@ C.instance_var = 3 # error: Cannot assign to instance variable Default level: error · Added in 0.0.1-alpha.19 · Related issues · -View source +View source @@ -627,7 +627,7 @@ asyncio.run(main()) Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -651,7 +651,7 @@ class A(42): ... # error: [invalid-base] Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -678,7 +678,7 @@ with 1: Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -707,7 +707,7 @@ a: str Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -751,7 +751,7 @@ except ZeroDivisionError: Default level: error · Added in 0.0.1-alpha.28 · Related issues · -View source +View source @@ -793,7 +793,7 @@ class D(A): Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -826,7 +826,7 @@ class C[U](Generic[T]): ... Default level: error · Added in 0.0.1-alpha.17 · Related issues · -View source +View source @@ -865,7 +865,7 @@ carol = Person(name="Carol", age=25) # typo! Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -900,7 +900,7 @@ def f(t: TypeVar("U")): ... Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -934,7 +934,7 @@ class B(metaclass=f): ... Default level: error · Added in 0.0.1-alpha.20 · Related issues · -View source +View source @@ -1041,7 +1041,7 @@ Correct use of `@override` is enforced by ty's `invalid-explicit-override` rule. Default level: error · Added in 0.0.1-alpha.19 · Related issues · -View source +View source @@ -1095,7 +1095,7 @@ AttributeError: Cannot overwrite NamedTuple attribute _asdict Default level: error · Preview (since 1.0.0) · Related issues · -View source +View source @@ -1125,7 +1125,7 @@ Baz = NewType("Baz", int | str) # error: invalid base for `typing.NewType` Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1175,7 +1175,7 @@ def foo(x: int) -> int: ... Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1201,7 +1201,7 @@ def f(a: int = ''): ... Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1232,7 +1232,7 @@ P2 = ParamSpec("S2") # error: ParamSpec name must match the variable it's assig Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1266,7 +1266,7 @@ TypeError: Protocols can only inherit from other protocols, got Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1315,7 +1315,7 @@ def g(): Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1340,7 +1340,7 @@ def func() -> int: Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1398,7 +1398,7 @@ TODO #14889 Default level: error · Added in 0.0.1-alpha.6 · Related issues · -View source +View source @@ -1425,7 +1425,7 @@ NewAlias = TypeAliasType(get_name(), int) # error: TypeAliasType name mus Default level: error · Added in 0.0.1-alpha.29 · Related issues · -View source +View source @@ -1472,7 +1472,7 @@ Bar[int] # error: too few arguments Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1502,7 +1502,7 @@ TYPE_CHECKING = '' Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1532,7 +1532,7 @@ b: Annotated[int] # `Annotated` expects at least two arguments Default level: error · Added in 0.0.1-alpha.11 · Related issues · -View source +View source @@ -1566,7 +1566,7 @@ f(10) # Error Default level: error · Added in 0.0.1-alpha.11 · Related issues · -View source +View source @@ -1600,7 +1600,7 @@ class C: Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1635,7 +1635,7 @@ T = TypeVar('T', bound=str) # valid bound TypeVar Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1660,7 +1660,7 @@ func() # TypeError: func() missing 1 required positional argument: 'x' Default level: error · Added in 0.0.1-alpha.20 · Related issues · -View source +View source @@ -1693,7 +1693,7 @@ alice["age"] # KeyError Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1722,7 +1722,7 @@ func("string") # error: [no-matching-overload] Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1746,7 +1746,7 @@ Subscripting an object that does not support it will raise a `TypeError` at runt Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1772,7 +1772,7 @@ for i in 34: # TypeError: 'int' object is not iterable Default level: error · Added in 0.0.1-alpha.29 · Related issues · -View source +View source @@ -1805,7 +1805,7 @@ class B(A): Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1832,7 +1832,7 @@ f(1, x=2) # Error raised here Default level: error · Added in 0.0.1-alpha.22 · Related issues · -View source +View source @@ -1890,7 +1890,7 @@ def test(): -> "int": Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1920,7 +1920,7 @@ static_assert(int(2.0 * 3.0) == 6) # error: does not have a statically known tr Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1949,7 +1949,7 @@ class B(A): ... # Error raised here Default level: error · Preview (since 0.0.1-alpha.30) · Related issues · -View source +View source @@ -1983,7 +1983,7 @@ class F(NamedTuple): Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2010,7 +2010,7 @@ f("foo") # Error raised here Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2038,7 +2038,7 @@ def _(x: int): Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2084,7 +2084,7 @@ class A: Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2111,7 +2111,7 @@ f(x=1, y=2) # Error raised here Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2139,7 +2139,7 @@ A().foo # AttributeError: 'A' object has no attribute 'foo' Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2164,7 +2164,7 @@ import foo # ModuleNotFoundError: No module named 'foo' Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2189,7 +2189,7 @@ print(x) # NameError: name 'x' is not defined Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2226,7 +2226,7 @@ b1 < b2 < b1 # exception raised here Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2254,7 +2254,7 @@ A() + A() # TypeError: unsupported operand type(s) for +: 'A' and 'A' Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2279,7 +2279,7 @@ l[1:10:0] # ValueError: slice step cannot be zero Default level: warn · Added in 0.0.1-alpha.20 · Related issues · -View source +View source @@ -2320,7 +2320,7 @@ class SubProto(BaseProto, Protocol): Default level: warn · Added in 0.0.1-alpha.16 · Related issues · -View source +View source @@ -2408,7 +2408,7 @@ a = 20 / 0 # type: ignore Default level: warn · Added in 0.0.1-alpha.22 · Related issues · -View source +View source @@ -2436,7 +2436,7 @@ A.c # AttributeError: type object 'A' has no attribute 'c' Default level: warn · Added in 0.0.1-alpha.22 · Related issues · -View source +View source @@ -2468,7 +2468,7 @@ A()[0] # TypeError: 'A' object is not subscriptable Default level: warn · Added in 0.0.1-alpha.22 · Related issues · -View source +View source @@ -2500,7 +2500,7 @@ from module import a # ImportError: cannot import name 'a' from 'module' Default level: warn · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2527,7 +2527,7 @@ cast(int, f()) # Redundant Default level: warn · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2551,7 +2551,7 @@ reveal_type(1) # NameError: name 'reveal_type' is not defined Default level: warn · Added in 0.0.1-alpha.15 · Related issues · -View source +View source @@ -2609,7 +2609,7 @@ def g(): Default level: warn · Added in 0.0.1-alpha.7 · Related issues · -View source +View source @@ -2648,7 +2648,7 @@ class D(C): ... # error: [unsupported-base] Default level: warn · Added in 0.0.1-alpha.22 · Related issues · -View source +View source @@ -2711,7 +2711,7 @@ def foo(x: int | str) -> int | str: Default level: ignore · Preview (since 0.0.1-alpha.1) · Related issues · -View source +View source @@ -2735,7 +2735,7 @@ Dividing by zero raises a `ZeroDivisionError` at runtime. Default level: ignore · Added in 0.0.1-alpha.1 · Related issues · -View source +View source diff --git a/crates/ty_python_semantic/src/types/diagnostic.rs b/crates/ty_python_semantic/src/types/diagnostic.rs index 8b20466b51..d72cbf8dbc 100644 --- a/crates/ty_python_semantic/src/types/diagnostic.rs +++ b/crates/ty_python_semantic/src/types/diagnostic.rs @@ -37,13 +37,11 @@ use itertools::Itertools; use ruff_db::{ diagnostic::{Annotation, Diagnostic, Span, SubDiagnostic, SubDiagnosticSeverity}, parsed::parsed_module, - source::source_text, }; use ruff_diagnostics::{Edit, Fix}; use ruff_python_ast::name::Name; -use ruff_python_ast::parenthesize::parentheses_iterator; +use ruff_python_ast::token::parentheses_iterator; use ruff_python_ast::{self as ast, AnyNodeRef, StringFlags}; -use ruff_python_trivia::CommentRanges; use ruff_text_size::{Ranged, TextRange}; use rustc_hash::FxHashSet; use std::fmt::{self, Formatter}; @@ -2423,9 +2421,7 @@ pub(super) fn report_invalid_assignment<'db>( // ) # ty: ignore <- or here // ``` - let comment_ranges = CommentRanges::from(context.module().tokens()); - let source = source_text(context.db(), context.file()); - parentheses_iterator(value_node.into(), None, &comment_ranges, &source) + parentheses_iterator(value_node.into(), None, context.module().tokens()) .last() .unwrap_or(value_node.range()) } else { From 21e5a57296696a97c630150c13ab22f167d9558e Mon Sep 17 00:00:00 2001 From: David Peter Date: Wed, 3 Dec 2025 10:00:02 +0100 Subject: [PATCH 05/10] [ty] Support typevar-specialized dynamic types in generic type aliases (#21730) ## Summary For a type alias like the one below, where `UnknownClass` is something with a dynamic type, we previously lost track of the fact that this dynamic type was explicitly specialized *with a type variable*. If that alias is then later explicitly specialized itself (`MyAlias[int]`), we would miscount the number of legacy type variables and emit a `invalid-type-arguments` diagnostic ([playground](https://play.ty.dev/886ae6cc-86c3-4304-a365-510d29211f85)). ```py T = TypeVar("T") MyAlias: TypeAlias = UnknownClass[T] | None ``` The solution implemented here is not pretty, but we can hopefully get rid of it via https://github.com/astral-sh/ty/issues/1711. Also, once we properly support `ParamSpec` and `Concatenate`, we should be able to remove some of this code. This addresses many of the `invalid-type-arguments` false-positives in https://github.com/astral-sh/ty/issues/1685. With this change, there are still some diagnostics of this type left. Instead of implementing even more (rather sophisticated) workarounds for these cases as well, it might be much easier to wait for full `ParamSpec`/`Concatenate` support and then try again. A disadvantage of this implementation is that we lose track of some `@Todo` types and replace them with `Unknown`. We could spend more effort and try to preserve them, but I'm unsure if this is the best use of our time right now. ## Test Plan New Markdown tests. --- .../resources/mdtest/pep613_type_aliases.md | 95 +++++++++++++ .../resources/mdtest/pep695_type_aliases.md | 3 +- crates/ty_python_semantic/src/types.rs | 77 ++++++++--- .../src/types/bound_super.rs | 2 +- .../src/types/class_base.rs | 4 +- .../src/types/infer/builder.rs | 126 ++++++++++++++---- .../types/infer/builder/type_expression.rs | 16 ++- .../src/types/subclass_of.rs | 8 +- .../src/types/type_ordering.rs | 3 + 9 files changed, 285 insertions(+), 49 deletions(-) diff --git a/crates/ty_python_semantic/resources/mdtest/pep613_type_aliases.md b/crates/ty_python_semantic/resources/mdtest/pep613_type_aliases.md index c3e63c7b8e..c41b88b3b1 100644 --- a/crates/ty_python_semantic/resources/mdtest/pep613_type_aliases.md +++ b/crates/ty_python_semantic/resources/mdtest/pep613_type_aliases.md @@ -149,6 +149,101 @@ def _(x: MyAlias): reveal_type(x) # revealed: int | list[str] | set[str] ``` +## Typevar-specialized dynamic types + +We still recognize type aliases as being generic if a symbol of a dynamic type is explicitly +specialized with a type variable: + +```py +from typing import TypeVar, TypeAlias + +from unknown_module import UnknownClass # type: ignore + +T = TypeVar("T") + +MyAlias1: TypeAlias = UnknownClass[T] | None + +def _(a: MyAlias1[int]): + reveal_type(a) # revealed: Unknown | None +``` + +This also works with multiple type arguments: + +```py +U = TypeVar("U") +V = TypeVar("V") + +MyAlias2: TypeAlias = UnknownClass[T, U, V] | int + +def _(a: MyAlias2[int, str, bytes]): + reveal_type(a) # revealed: Unknown | int +``` + +If we specialize with fewer or more type arguments than expected, we emit an error: + +```py +def _( + # error: [invalid-type-arguments] "No type argument provided for required type variable `V`" + too_few: MyAlias2[int, str], + # error: [invalid-type-arguments] "Too many type arguments: expected 3, got 4" + too_many: MyAlias2[int, str, bytes, float], +): ... +``` + +We can also reference these type aliases from other type aliases: + +```py +MyAlias3: TypeAlias = MyAlias1[str] | MyAlias2[int, str, bytes] + +def _(c: MyAlias3): + reveal_type(c) # revealed: Unknown | None | int +``` + +Here, we test some other cases that might involve `@Todo` types, which also need special handling: + +```py +from typing_extensions import Callable, Concatenate, TypeAliasType + +MyAlias4: TypeAlias = Callable[Concatenate[dict[str, T], ...], list[U]] + +def _(c: MyAlias4[int, str]): + # TODO: should be (int, / ...) -> str + reveal_type(c) # revealed: Unknown + +T = TypeVar("T") + +MyList = TypeAliasType("MyList", list[T], type_params=(T,)) + +MyAlias5 = Callable[[MyList[T]], int] + +def _(c: MyAlias5[int]): + # TODO: should be (list[int], /) -> int + reveal_type(c) # revealed: (Unknown, /) -> int + +K = TypeVar("K") +V = TypeVar("V") + +MyDict = TypeAliasType("MyDict", dict[K, V], type_params=(K, V)) + +MyAlias6 = Callable[[MyDict[K, V]], int] + +def _(c: MyAlias6[str, bytes]): + # TODO: should be (dict[str, bytes], /) -> int + reveal_type(c) # revealed: (Unknown, /) -> int + +ListOrDict: TypeAlias = MyList[T] | dict[str, T] + +def _(x: ListOrDict[int]): + # TODO: should be list[int] | dict[str, int] + reveal_type(x) # revealed: Unknown | dict[str, int] + +MyAlias7: TypeAlias = Callable[Concatenate[T, ...], None] + +def _(c: MyAlias7[int]): + # TODO: should be (int, / ...) -> None + reveal_type(c) # revealed: Unknown +``` + ## Imported `alias.py`: diff --git a/crates/ty_python_semantic/resources/mdtest/pep695_type_aliases.md b/crates/ty_python_semantic/resources/mdtest/pep695_type_aliases.md index d4e4fafc73..799d1fc36f 100644 --- a/crates/ty_python_semantic/resources/mdtest/pep695_type_aliases.md +++ b/crates/ty_python_semantic/resources/mdtest/pep695_type_aliases.md @@ -223,7 +223,8 @@ T = TypeVar("T") IntAndT = TypeAliasType("IntAndT", tuple[int, T], type_params=(T,)) def f(x: IntAndT[str]) -> None: - reveal_type(x) # revealed: @Todo(Generic manual PEP-695 type alias) + # TODO: This should be `tuple[int, str]` + reveal_type(x) # revealed: Unknown ``` ### Error cases diff --git a/crates/ty_python_semantic/src/types.rs b/crates/ty_python_semantic/src/types.rs index f957c2193c..e3b0aa7717 100644 --- a/crates/ty_python_semantic/src/types.rs +++ b/crates/ty_python_semantic/src/types.rs @@ -763,7 +763,7 @@ impl<'db> DataclassParams<'db> { #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, salsa::Update, get_size2::GetSize)] pub enum Type<'db> { /// The dynamic type: a statically unknown set of values - Dynamic(DynamicType), + Dynamic(DynamicType<'db>), /// The empty set of values Never, /// A specific function object @@ -889,7 +889,10 @@ impl<'db> Type<'db> { } pub const fn is_unknown(&self) -> bool { - matches!(self, Type::Dynamic(DynamicType::Unknown)) + matches!( + self, + Type::Dynamic(DynamicType::Unknown | DynamicType::UnknownGeneric(_)) + ) } pub(crate) const fn is_never(&self) -> bool { @@ -959,7 +962,10 @@ impl<'db> Type<'db> { pub(crate) fn is_todo(&self) -> bool { self.as_dynamic().is_some_and(|dynamic| match dynamic { - DynamicType::Any | DynamicType::Unknown | DynamicType::Divergent(_) => false, + DynamicType::Any + | DynamicType::Unknown + | DynamicType::UnknownGeneric(_) + | DynamicType::Divergent(_) => false, DynamicType::Todo(_) | DynamicType::TodoStarredExpression | DynamicType::TodoUnpack => { true } @@ -1146,7 +1152,7 @@ impl<'db> Type<'db> { } } - pub(crate) const fn as_dynamic(self) -> Option { + pub(crate) const fn as_dynamic(self) -> Option> { match self { Type::Dynamic(dynamic_type) => Some(dynamic_type), _ => None, @@ -1160,7 +1166,7 @@ impl<'db> Type<'db> { } } - pub(crate) const fn expect_dynamic(self) -> DynamicType { + pub(crate) const fn expect_dynamic(self) -> DynamicType<'db> { self.as_dynamic().expect("Expected a Type::Dynamic variant") } @@ -7851,14 +7857,18 @@ impl<'db> Type<'db> { typevars: &mut FxOrderSet>, visitor: &FindLegacyTypeVarsVisitor<'db>, ) { + let is_matching_typevar = |bound_typevar: &BoundTypeVarInstance<'db>| { + matches!( + bound_typevar.typevar(db).kind(db), + TypeVarKind::Legacy | TypeVarKind::TypingSelf | TypeVarKind::ParamSpec + ) && binding_context.is_none_or(|binding_context| { + bound_typevar.binding_context(db) == BindingContext::Definition(binding_context) + }) + }; + match self { Type::TypeVar(bound_typevar) => { - if matches!( - bound_typevar.typevar(db).kind(db), - TypeVarKind::Legacy | TypeVarKind::TypingSelf | TypeVarKind::ParamSpec - ) && binding_context.is_none_or(|binding_context| { - bound_typevar.binding_context(db) == BindingContext::Definition(binding_context) - }) { + if is_matching_typevar(&bound_typevar) { typevars.insert(bound_typevar); } } @@ -7998,6 +8008,14 @@ impl<'db> Type<'db> { } }, + Type::Dynamic(DynamicType::UnknownGeneric(generic_context)) => { + for variable in generic_context.variables(db) { + if is_matching_typevar(&variable) { + typevars.insert(variable); + } + } + } + Type::Dynamic(_) | Type::Never | Type::AlwaysTruthy @@ -8029,6 +8047,26 @@ impl<'db> Type<'db> { } } + /// Bind all unbound legacy type variables to the given context and then + /// add all legacy typevars to the provided set. + pub(crate) fn bind_and_find_all_legacy_typevars( + self, + db: &'db dyn Db, + binding_context: Option>, + variables: &mut FxOrderSet>, + ) { + self.apply_type_mapping( + db, + &TypeMapping::BindLegacyTypevars( + binding_context + .map(BindingContext::Definition) + .unwrap_or(BindingContext::Synthetic), + ), + TypeContext::default(), + ) + .find_legacy_typevars(db, None, variables); + } + /// Replace default types in parameters of callables with `Unknown`. pub(crate) fn replace_parameter_defaults(self, db: &'db dyn Db) -> Type<'db> { self.apply_type_mapping( @@ -8177,7 +8215,7 @@ impl<'db> Type<'db> { Self::SpecialForm(special_form) => special_form.definition(db), Self::Never => Type::SpecialForm(SpecialFormType::Never).definition(db), Self::Dynamic(DynamicType::Any) => Type::SpecialForm(SpecialFormType::Any).definition(db), - Self::Dynamic(DynamicType::Unknown) => Type::SpecialForm(SpecialFormType::Unknown).definition(db), + Self::Dynamic(DynamicType::Unknown | DynamicType::UnknownGeneric(_)) => Type::SpecialForm(SpecialFormType::Unknown).definition(db), Self::AlwaysTruthy => Type::SpecialForm(SpecialFormType::AlwaysTruthy).definition(db), Self::AlwaysFalsy => Type::SpecialForm(SpecialFormType::AlwaysFalsy).definition(db), @@ -8839,11 +8877,18 @@ pub struct DivergentType { impl get_size2::GetSize for DivergentType {} #[derive(Copy, Clone, Debug, Eq, Hash, PartialEq, salsa::Update, get_size2::GetSize)] -pub enum DynamicType { +pub enum DynamicType<'db> { /// An explicitly annotated `typing.Any` Any, /// An unannotated value, or a dynamic type resulting from an error Unknown, + /// Similar to `Unknown`, this represents a dynamic type that has been explicitly specialized + /// with legacy typevars, e.g. `UnknownClass[T]`, where `T` is a legacy typevar. We keep track + /// of the type variables in the generic context in case this type is later specialized again. + /// + /// TODO: Once we implement , this variant might + /// not be needed anymore. + UnknownGeneric(GenericContext<'db>), /// Temporary type for symbols that can't be inferred yet because of missing implementations. /// /// This variant should eventually be removed once ty is spec-compliant. @@ -8862,7 +8907,7 @@ pub enum DynamicType { Divergent(DivergentType), } -impl DynamicType { +impl DynamicType<'_> { fn normalized(self) -> Self { if matches!(self, Self::Divergent(_)) { self @@ -8880,11 +8925,11 @@ impl DynamicType { } } -impl std::fmt::Display for DynamicType { +impl std::fmt::Display for DynamicType<'_> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { DynamicType::Any => f.write_str("Any"), - DynamicType::Unknown => f.write_str("Unknown"), + DynamicType::Unknown | DynamicType::UnknownGeneric(_) => f.write_str("Unknown"), // `DynamicType::Todo`'s display should be explicit that is not a valid display of // any other type DynamicType::Todo(todo) => write!(f, "@Todo{todo}"), diff --git a/crates/ty_python_semantic/src/types/bound_super.rs b/crates/ty_python_semantic/src/types/bound_super.rs index 04cd24e40e..c67aacb323 100644 --- a/crates/ty_python_semantic/src/types/bound_super.rs +++ b/crates/ty_python_semantic/src/types/bound_super.rs @@ -172,7 +172,7 @@ impl<'db> BoundSuperError<'db> { #[derive(Debug, Copy, Clone, Hash, PartialEq, Eq, get_size2::GetSize)] pub enum SuperOwnerKind<'db> { - Dynamic(DynamicType), + Dynamic(DynamicType<'db>), Class(ClassType<'db>), Instance(NominalInstanceType<'db>), } diff --git a/crates/ty_python_semantic/src/types/class_base.rs b/crates/ty_python_semantic/src/types/class_base.rs index c29f72d4f9..c5ce573d3b 100644 --- a/crates/ty_python_semantic/src/types/class_base.rs +++ b/crates/ty_python_semantic/src/types/class_base.rs @@ -18,7 +18,7 @@ use crate::types::{ /// automatically construct the default specialization for that class. #[derive(Debug, Copy, Clone, Hash, PartialEq, Eq, salsa::Update, get_size2::GetSize)] pub enum ClassBase<'db> { - Dynamic(DynamicType), + Dynamic(DynamicType<'db>), Class(ClassType<'db>), /// Although `Protocol` is not a class in typeshed's stubs, it is at runtime, /// and can appear in the MRO of a class. @@ -62,7 +62,7 @@ impl<'db> ClassBase<'db> { match self { ClassBase::Class(class) => class.name(db), ClassBase::Dynamic(DynamicType::Any) => "Any", - ClassBase::Dynamic(DynamicType::Unknown) => "Unknown", + ClassBase::Dynamic(DynamicType::Unknown | DynamicType::UnknownGeneric(_)) => "Unknown", ClassBase::Dynamic( DynamicType::Todo(_) | DynamicType::TodoUnpack | DynamicType::TodoStarredExpression, ) => "@Todo", diff --git a/crates/ty_python_semantic/src/types/infer/builder.rs b/crates/ty_python_semantic/src/types/infer/builder.rs index d3a65e88b2..6a1003acb6 100644 --- a/crates/ty_python_semantic/src/types/infer/builder.rs +++ b/crates/ty_python_semantic/src/types/infer/builder.rs @@ -9571,6 +9571,9 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { (unknown @ Type::Dynamic(DynamicType::Unknown), _, _) | (_, unknown @ Type::Dynamic(DynamicType::Unknown), _) => Some(unknown), + (unknown @ Type::Dynamic(DynamicType::UnknownGeneric(_)), _, _) + | (_, unknown @ Type::Dynamic(DynamicType::UnknownGeneric(_)), _) => Some(unknown), + ( todo @ Type::Dynamic( DynamicType::Todo(_) @@ -10969,6 +10972,19 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { ); } } + Type::KnownInstance(KnownInstanceType::TypeAliasType(TypeAliasType::ManualPEP695( + _, + ))) => { + let slice_ty = self.infer_expression(slice, TypeContext::default()); + let mut variables = FxOrderSet::default(); + slice_ty.bind_and_find_all_legacy_typevars( + self.db(), + self.typevar_binding_context, + &mut variables, + ); + let generic_context = GenericContext::from_typevar_instances(self.db(), variables); + return Type::Dynamic(DynamicType::UnknownGeneric(generic_context)); + } Type::KnownInstance(KnownInstanceType::TypeAliasType(type_alias)) => { if let Some(generic_context) = type_alias.generic_context(self.db()) { return self.infer_explicit_type_alias_type_specialization( @@ -11107,33 +11123,74 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { )); } Type::SpecialForm(SpecialFormType::Callable) => { - // TODO: Remove this once we support ParamSpec properly. This is necessary to avoid - // a lot of false positives downstream, because we can't represent the specialized - // `Callable[P, _]` type yet. - if let Some(first_arg) = subscript - .slice - .as_ref() - .as_tuple_expr() - .and_then(|args| args.elts.first()) - && first_arg.is_name_expr() - { - let first_arg_ty = self.infer_expression(first_arg, TypeContext::default()); + let arguments = if let ast::Expr::Tuple(tuple) = &*subscript.slice { + &*tuple.elts + } else { + std::slice::from_ref(&*subscript.slice) + }; - if let Type::KnownInstance(KnownInstanceType::TypeVar(typevar)) = first_arg_ty - && typevar.kind(self.db()).is_paramspec() - { - return todo_type!("Callable[..] specialized with ParamSpec"); - } + // TODO: Remove this once we support ParamSpec and Concatenate properly. This is necessary + // to avoid a lot of false positives downstream, because we can't represent the typevar- + // specialized `Callable` types yet. + let num_arguments = arguments.len(); + if num_arguments == 2 { + let first_arg = &arguments[0]; + let second_arg = &arguments[1]; - if let Some(builder) = self.context.report_lint(&INVALID_TYPE_FORM, subscript) { - builder.into_diagnostic(format_args!( - "The first argument to `Callable` must be either a list of types, \ - ParamSpec, Concatenate, or `...`", + if first_arg.is_name_expr() { + let first_arg_ty = self.infer_expression(first_arg, TypeContext::default()); + if let Type::KnownInstance(KnownInstanceType::TypeVar(typevar)) = + first_arg_ty + && typevar.kind(self.db()).is_paramspec() + { + return todo_type!("Callable[..] specialized with ParamSpec"); + } + + if let Some(builder) = + self.context.report_lint(&INVALID_TYPE_FORM, subscript) + { + builder.into_diagnostic(format_args!( + "The first argument to `Callable` must be either a list of types, \ + ParamSpec, Concatenate, or `...`", + )); + } + return Type::KnownInstance(KnownInstanceType::Callable( + CallableType::unknown(self.db()), + )); + } else if first_arg.is_subscript_expr() { + let first_arg_ty = self.infer_expression(first_arg, TypeContext::default()); + if let Type::Dynamic(DynamicType::UnknownGeneric(generic_context)) = + first_arg_ty + { + let mut variables = generic_context + .variables(self.db()) + .collect::>(); + + let return_ty = + self.infer_expression(second_arg, TypeContext::default()); + return_ty.bind_and_find_all_legacy_typevars( + self.db(), + self.typevar_binding_context, + &mut variables, + ); + + let generic_context = + GenericContext::from_typevar_instances(self.db(), variables); + return Type::Dynamic(DynamicType::UnknownGeneric(generic_context)); + } + + if let Some(builder) = + self.context.report_lint(&INVALID_TYPE_FORM, subscript) + { + builder.into_diagnostic(format_args!( + "The first argument to `Callable` must be either a list of types, \ + ParamSpec, Concatenate, or `...`", + )); + } + return Type::KnownInstance(KnownInstanceType::Callable( + CallableType::unknown(self.db()), )); } - return Type::KnownInstance(KnownInstanceType::Callable( - CallableType::unknown(self.db()), - )); } let callable = self @@ -11240,6 +11297,17 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { ) => { return self.infer_explicit_type_alias_specialization(subscript, value_ty, false); } + Type::Dynamic(DynamicType::Unknown) => { + let slice_ty = self.infer_expression(slice, TypeContext::default()); + let mut variables = FxOrderSet::default(); + slice_ty.bind_and_find_all_legacy_typevars( + self.db(), + self.typevar_binding_context, + &mut variables, + ); + let generic_context = GenericContext::from_typevar_instances(self.db(), variables); + return Type::Dynamic(DynamicType::UnknownGeneric(generic_context)); + } _ => {} } @@ -11696,6 +11764,18 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { Some(Type::Dynamic(DynamicType::TodoUnpack)) } + (Type::SpecialForm(SpecialFormType::Concatenate), _) => { + // TODO: Add proper support for `Concatenate` + let mut variables = FxOrderSet::default(); + slice_ty.bind_and_find_all_legacy_typevars( + db, + self.typevar_binding_context, + &mut variables, + ); + let generic_context = GenericContext::from_typevar_instances(self.db(), variables); + Some(Type::Dynamic(DynamicType::UnknownGeneric(generic_context))) + } + (Type::SpecialForm(special_form), _) if special_form.class().is_special_form() => { Some(todo_type!("Inference of subscript on special form")) } diff --git a/crates/ty_python_semantic/src/types/infer/builder/type_expression.rs b/crates/ty_python_semantic/src/types/infer/builder/type_expression.rs index edee2bb5da..56b0db5a09 100644 --- a/crates/ty_python_semantic/src/types/infer/builder/type_expression.rs +++ b/crates/ty_python_semantic/src/types/infer/builder/type_expression.rs @@ -946,8 +946,17 @@ impl<'db> TypeInferenceBuilder<'db, '_> { } } KnownInstanceType::TypeAliasType(TypeAliasType::ManualPEP695(_)) => { - self.infer_type_expression(slice); - todo_type!("Generic manual PEP-695 type alias") + // TODO: support generic "manual" PEP 695 type aliases + let slice_ty = self.infer_expression(slice, TypeContext::default()); + let mut variables = FxOrderSet::default(); + slice_ty.bind_and_find_all_legacy_typevars( + self.db(), + self.typevar_binding_context, + &mut variables, + ); + let generic_context = + GenericContext::from_typevar_instances(self.db(), variables); + Type::Dynamic(DynamicType::UnknownGeneric(generic_context)) } KnownInstanceType::LiteralStringAlias(_) => { self.infer_type_expression(slice); @@ -984,6 +993,9 @@ impl<'db> TypeInferenceBuilder<'db, '_> { Type::unknown() } }, + Type::Dynamic(DynamicType::UnknownGeneric(_)) => { + self.infer_explicit_type_alias_specialization(subscript, value_ty, true) + } Type::Dynamic(_) => { // Infer slice as a value expression to avoid false-positive // `invalid-type-form` diagnostics, when we have e.g. diff --git a/crates/ty_python_semantic/src/types/subclass_of.rs b/crates/ty_python_semantic/src/types/subclass_of.rs index 0e3deed233..d906a472c3 100644 --- a/crates/ty_python_semantic/src/types/subclass_of.rs +++ b/crates/ty_python_semantic/src/types/subclass_of.rs @@ -322,7 +322,7 @@ impl<'db> VarianceInferable<'db> for SubclassOfType<'db> { #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, salsa::Update, get_size2::GetSize)] pub(crate) enum SubclassOfInner<'db> { Class(ClassType<'db>), - Dynamic(DynamicType), + Dynamic(DynamicType<'db>), TypeVar(BoundTypeVarInstance<'db>), } @@ -362,7 +362,7 @@ impl<'db> SubclassOfInner<'db> { } } - pub(crate) const fn into_dynamic(self) -> Option { + pub(crate) const fn into_dynamic(self) -> Option> { match self { Self::Class(_) | Self::TypeVar(_) => None, Self::Dynamic(dynamic) => Some(dynamic), @@ -465,8 +465,8 @@ impl<'db> From> for SubclassOfInner<'db> { } } -impl From for SubclassOfInner<'_> { - fn from(value: DynamicType) -> Self { +impl<'db> From> for SubclassOfInner<'db> { + fn from(value: DynamicType<'db>) -> Self { SubclassOfInner::Dynamic(value) } } diff --git a/crates/ty_python_semantic/src/types/type_ordering.rs b/crates/ty_python_semantic/src/types/type_ordering.rs index f57855fcb4..c47720011c 100644 --- a/crates/ty_python_semantic/src/types/type_ordering.rs +++ b/crates/ty_python_semantic/src/types/type_ordering.rs @@ -265,6 +265,9 @@ fn dynamic_elements_ordering(left: DynamicType, right: DynamicType) -> Ordering (DynamicType::Unknown, _) => Ordering::Less, (_, DynamicType::Unknown) => Ordering::Greater, + (DynamicType::UnknownGeneric(_), _) => Ordering::Less, + (_, DynamicType::UnknownGeneric(_)) => Ordering::Greater, + #[cfg(debug_assertions)] (DynamicType::Todo(TodoType(left)), DynamicType::Todo(TodoType(right))) => left.cmp(right), From 92c5f62ec01b83396c62e4e2dc33223a7a8d7373 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Wed, 3 Dec 2025 12:16:18 +0100 Subject: [PATCH 06/10] [ty] Enable LRU collection for parsed module (#21749) --- crates/ruff_db/src/parsed.rs | 17 ++++---- crates/ruff_memory_usage/src/lib.rs | 43 ++++++++++++++++--- crates/ty_ide/src/symbols.rs | 10 +++++ crates/ty_project/src/db.rs | 6 ++- crates/ty_python_semantic/src/ast_node_ref.rs | 9 ++-- .../src/module_resolver/module.rs | 2 +- 6 files changed, 65 insertions(+), 22 deletions(-) diff --git a/crates/ruff_db/src/parsed.rs b/crates/ruff_db/src/parsed.rs index ad241b3ded..9d9604aea0 100644 --- a/crates/ruff_db/src/parsed.rs +++ b/crates/ruff_db/src/parsed.rs @@ -21,7 +21,11 @@ use crate::source::source_text; /// reflected in the changed AST offsets. /// The other reason is that Ruff's AST doesn't implement `Eq` which Salsa requires /// for determining if a query result is unchanged. -#[salsa::tracked(returns(ref), no_eq, heap_size=ruff_memory_usage::heap_size)] +/// +/// The LRU capacity of 200 was picked without any empirical evidence that it's optimal, +/// instead it's a wild guess that it should be unlikely that incremental changes involve +/// more than 200 modules. Parsed ASTs within the same revision are never evicted by Salsa. +#[salsa::tracked(returns(ref), no_eq, heap_size=ruff_memory_usage::heap_size, lru=200)] pub fn parsed_module(db: &dyn Db, file: File) -> ParsedModule { let _span = tracing::trace_span!("parsed_module", ?file).entered(); @@ -92,14 +96,9 @@ impl ParsedModule { self.inner.store(None); } - /// Returns the pointer address of this [`ParsedModule`]. - /// - /// The pointer uniquely identifies the module within the current Salsa revision, - /// regardless of whether particular [`ParsedModuleRef`] instances are garbage collected. - pub fn addr(&self) -> usize { - // Note that the outer `Arc` in `inner` is stable across garbage collection, while the inner - // `Arc` within the `ArcSwap` may change. - Arc::as_ptr(&self.inner).addr() + /// Returns the file to which this module belongs. + pub fn file(&self) -> File { + self.file } } diff --git a/crates/ruff_memory_usage/src/lib.rs b/crates/ruff_memory_usage/src/lib.rs index e75c75808e..81444d5197 100644 --- a/crates/ruff_memory_usage/src/lib.rs +++ b/crates/ruff_memory_usage/src/lib.rs @@ -1,17 +1,46 @@ -use std::sync::{LazyLock, Mutex}; +use std::cell::RefCell; use get_size2::{GetSize, StandardTracker}; use ordermap::{OrderMap, OrderSet}; +thread_local! { + pub static TRACKER: RefCell>= const { RefCell::new(None) }; +} + +struct TrackerGuard(Option); + +impl Drop for TrackerGuard { + fn drop(&mut self) { + TRACKER.set(self.0.take()); + } +} + +pub fn attach_tracker(tracker: StandardTracker, f: impl FnOnce() -> R) -> R { + let prev = TRACKER.replace(Some(tracker)); + let _guard = TrackerGuard(prev); + f() +} + +fn with_tracker(f: F) -> R +where + F: FnOnce(Option<&mut StandardTracker>) -> R, +{ + TRACKER.with(|tracker| { + let mut tracker = tracker.borrow_mut(); + f(tracker.as_mut()) + }) +} + /// Returns the memory usage of the provided object, using a global tracker to avoid /// double-counting shared objects. pub fn heap_size(value: &T) -> usize { - static TRACKER: LazyLock> = - LazyLock::new(|| Mutex::new(StandardTracker::new())); - - value - .get_heap_size_with_tracker(&mut *TRACKER.lock().unwrap()) - .0 + with_tracker(|tracker| { + if let Some(tracker) = tracker { + value.get_heap_size_with_tracker(tracker).0 + } else { + value.get_heap_size() + } + }) } /// An implementation of [`GetSize::get_heap_size`] for [`OrderSet`]. diff --git a/crates/ty_ide/src/symbols.rs b/crates/ty_ide/src/symbols.rs index 01b177619e..a2278f40e6 100644 --- a/crates/ty_ide/src/symbols.rs +++ b/crates/ty_ide/src/symbols.rs @@ -379,6 +379,16 @@ pub(crate) fn symbols_for_file_global_only(db: &dyn Db, file: File) -> FlatSymbo global_only: true, }; visitor.visit_body(&module.syntax().body); + + if file + .path(db) + .as_system_path() + .is_none_or(|path| !db.project().is_file_included(db, path)) + { + // Eagerly clear ASTs of third party files. + parsed.clear(); + } + FlatSymbols { symbols: visitor.symbols, } diff --git a/crates/ty_project/src/db.rs b/crates/ty_project/src/db.rs index 8f6ec20c95..9c8eab8204 100644 --- a/crates/ty_project/src/db.rs +++ b/crates/ty_project/src/db.rs @@ -7,6 +7,7 @@ pub use self::changes::ChangeResult; use crate::CollectReporter; use crate::metadata::settings::file_settings; use crate::{ProgressReporter, Project, ProjectMetadata}; +use get_size2::StandardTracker; use ruff_db::Db as SourceDb; use ruff_db::diagnostic::Diagnostic; use ruff_db::files::{File, Files}; @@ -129,7 +130,10 @@ impl ProjectDatabase { /// Returns a [`SalsaMemoryDump`] that can be use to dump Salsa memory usage information /// to the CLI after a typechecker run. pub fn salsa_memory_dump(&self) -> SalsaMemoryDump { - let memory_usage = ::memory_usage(self); + let memory_usage = ruff_memory_usage::attach_tracker(StandardTracker::new(), || { + ::memory_usage(self) + }); + let mut ingredients = memory_usage .structs .into_iter() diff --git a/crates/ty_python_semantic/src/ast_node_ref.rs b/crates/ty_python_semantic/src/ast_node_ref.rs index 14916ec807..a3d1fae49a 100644 --- a/crates/ty_python_semantic/src/ast_node_ref.rs +++ b/crates/ty_python_semantic/src/ast_node_ref.rs @@ -1,6 +1,8 @@ use std::fmt::Debug; use std::marker::PhantomData; +#[cfg(debug_assertions)] +use ruff_db::files::File; use ruff_db::parsed::ParsedModuleRef; use ruff_python_ast::{AnyNodeRef, NodeIndex}; use ruff_python_ast::{AnyRootNodeRef, HasNodeIndex}; @@ -44,7 +46,7 @@ pub struct AstNodeRef { // cannot implement `Eq`, as indices are only unique within a given instance of the // AST. #[cfg(debug_assertions)] - module_addr: usize, + file: File, _node: PhantomData, } @@ -72,7 +74,7 @@ where Self { index, #[cfg(debug_assertions)] - module_addr: module_ref.module().addr(), + file: module_ref.module().file(), #[cfg(debug_assertions)] kind: AnyNodeRef::from(node).kind(), #[cfg(debug_assertions)] @@ -88,8 +90,7 @@ where #[track_caller] pub fn node<'ast>(&self, module_ref: &'ast ParsedModuleRef) -> &'ast T { #[cfg(debug_assertions)] - assert_eq!(module_ref.module().addr(), self.module_addr); - + assert_eq!(module_ref.module().file(), self.file); // The user guarantees that the module is from the same file and Salsa // revision, so the file contents cannot have changed. module_ref diff --git a/crates/ty_python_semantic/src/module_resolver/module.rs b/crates/ty_python_semantic/src/module_resolver/module.rs index 6927c3b89f..118c2aff45 100644 --- a/crates/ty_python_semantic/src/module_resolver/module.rs +++ b/crates/ty_python_semantic/src/module_resolver/module.rs @@ -120,7 +120,7 @@ impl std::fmt::Debug for Module<'_> { } #[allow(clippy::ref_option)] -#[salsa::tracked(returns(ref))] +#[salsa::tracked(returns(ref), heap_size=ruff_memory_usage::heap_size)] fn all_submodule_names_for_package<'db>( db: &'db dyn Db, module: Module<'db>, From 5756b3809cbd1e5b0372807f95548a7f28a87b52 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 3 Dec 2025 11:27:47 +0000 Subject: [PATCH 07/10] [ty] Extend `invalid-explicit-override` to also cover properties decorated with `@override` that do not override anything (#21756) --- .../resources/mdtest/override.md | 52 +- ...override`_-_Basics_(b7c220f8171f11f0).snap | 485 +++++++++++------- .../ty_python_semantic/src/types/overrides.rs | 130 +++-- 3 files changed, 415 insertions(+), 252 deletions(-) diff --git a/crates/ty_python_semantic/resources/mdtest/override.md b/crates/ty_python_semantic/resources/mdtest/override.md index 386db9b340..7f726d4289 100644 --- a/crates/ty_python_semantic/resources/mdtest/override.md +++ b/crates/ty_python_semantic/resources/mdtest/override.md @@ -19,54 +19,74 @@ class A: class Parent: def foo(self): ... + @property def my_property1(self) -> int: ... + @property def my_property2(self) -> int: ... + baz = None + @classmethod def class_method1(cls) -> int: ... + @staticmethod def static_method1() -> int: ... + @classmethod def class_method2(cls) -> int: ... + @staticmethod def static_method2() -> int: ... + @lossy_decorator def decorated_1(self): ... + @lossy_decorator def decorated_2(self): ... + @lossy_decorator def decorated_3(self): ... class Child(Parent): @override def foo(self): ... # fine: overrides `Parent.foo` + @property @override def my_property1(self) -> int: ... # fine: overrides `Parent.my_property1` + @override @property def my_property2(self) -> int: ... # fine: overrides `Parent.my_property2` + @override def baz(self): ... # fine: overrides `Parent.baz` + @classmethod @override def class_method1(cls) -> int: ... # fine: overrides `Parent.class_method1` + @staticmethod @override def static_method1() -> int: ... # fine: overrides `Parent.static_method1` + @override @classmethod def class_method2(cls) -> int: ... # fine: overrides `Parent.class_method2` + @override @staticmethod def static_method2() -> int: ... # fine: overrides `Parent.static_method2` + @override def decorated_1(self): ... # fine: overrides `Parent.decorated_1` + @override @lossy_decorator def decorated_2(self): ... # fine: overrides `Parent.decorated_2` + @lossy_decorator @override def decorated_3(self): ... # fine: overrides `Parent.decorated_3` @@ -76,28 +96,37 @@ class OtherChild(Parent): ... class Grandchild(OtherChild): @override def foo(self): ... # fine: overrides `Parent.foo` + @override @property - def bar(self) -> int: ... # fine: overrides `Parent.bar` + def my_property1(self) -> int: ... # fine: overrides `Parent.my_property1` + @override def baz(self): ... # fine: overrides `Parent.baz` + @classmethod @override def class_method1(cls) -> int: ... # fine: overrides `Parent.class_method1` + @staticmethod @override def static_method1() -> int: ... # fine: overrides `Parent.static_method1` + @override @classmethod def class_method2(cls) -> int: ... # fine: overrides `Parent.class_method2` + @override @staticmethod def static_method2() -> int: ... # fine: overrides `Parent.static_method2` + @override def decorated_1(self): ... # fine: overrides `Parent.decorated_1` + @override @lossy_decorator def decorated_2(self): ... # fine: overrides `Parent.decorated_2` + @lossy_decorator @override def decorated_3(self): ... # fine: overrides `Parent.decorated_3` @@ -105,27 +134,41 @@ class Grandchild(OtherChild): class Invalid: @override def ___reprrr__(self): ... # error: [invalid-explicit-override] + @override @classmethod def foo(self): ... # error: [invalid-explicit-override] + @classmethod @override def bar(self): ... # error: [invalid-explicit-override] + @staticmethod @override def baz(): ... # error: [invalid-explicit-override] + @override @staticmethod def eggs(): ... # error: [invalid-explicit-override] + @property @override - def bad_property1(self) -> int: ... # TODO: should emit `invalid-explicit-override` here + def bad_property1(self) -> int: ... # error: [invalid-explicit-override] + @override @property - def bad_property2(self) -> int: ... # TODO: should emit `invalid-explicit-override` here + def bad_property2(self) -> int: ... # error: [invalid-explicit-override] + + @property + @override + def bad_settable_property(self) -> int: ... # error: [invalid-explicit-override] + @bad_settable_property.setter + def bad_settable_property(self, x: int) -> None: ... + @lossy_decorator @override def lossy(self): ... # TODO: should emit `invalid-explicit-override` here + @override @lossy_decorator def lossy2(self): ... # TODO: should emit `invalid-explicit-override` here @@ -136,11 +179,14 @@ class LiskovViolatingButNotOverrideViolating(Parent): @override @property def foo(self) -> int: ... + @override def my_property1(self) -> int: ... + @staticmethod @override def class_method1() -> int: ... + @classmethod @override def static_method1(cls) -> int: ... diff --git a/crates/ty_python_semantic/resources/mdtest/snapshots/override.md_-_`typing.override`_-_Basics_(b7c220f8171f11f0).snap b/crates/ty_python_semantic/resources/mdtest/snapshots/override.md_-_`typing.override`_-_Basics_(b7c220f8171f11f0).snap index 6e0cf0c01e..6c6c55d590 100644 --- a/crates/ty_python_semantic/resources/mdtest/snapshots/override.md_-_`typing.override`_-_Basics_(b7c220f8171f11f0).snap +++ b/crates/ty_python_semantic/resources/mdtest/snapshots/override.md_-_`typing.override`_-_Basics_(b7c220f8171f11f0).snap @@ -22,175 +22,221 @@ mdtest path: crates/ty_python_semantic/resources/mdtest/override.md 8 | 9 | class Parent: 10 | def foo(self): ... - 11 | @property - 12 | def my_property1(self) -> int: ... - 13 | @property - 14 | def my_property2(self) -> int: ... - 15 | baz = None - 16 | @classmethod - 17 | def class_method1(cls) -> int: ... - 18 | @staticmethod - 19 | def static_method1() -> int: ... + 11 | + 12 | @property + 13 | def my_property1(self) -> int: ... + 14 | + 15 | @property + 16 | def my_property2(self) -> int: ... + 17 | + 18 | baz = None + 19 | 20 | @classmethod - 21 | def class_method2(cls) -> int: ... - 22 | @staticmethod - 23 | def static_method2() -> int: ... - 24 | @lossy_decorator - 25 | def decorated_1(self): ... - 26 | @lossy_decorator - 27 | def decorated_2(self): ... - 28 | @lossy_decorator - 29 | def decorated_3(self): ... - 30 | - 31 | class Child(Parent): - 32 | @override - 33 | def foo(self): ... # fine: overrides `Parent.foo` - 34 | @property - 35 | @override - 36 | def my_property1(self) -> int: ... # fine: overrides `Parent.my_property1` - 37 | @override - 38 | @property - 39 | def my_property2(self) -> int: ... # fine: overrides `Parent.my_property2` - 40 | @override - 41 | def baz(self): ... # fine: overrides `Parent.baz` - 42 | @classmethod - 43 | @override - 44 | def class_method1(cls) -> int: ... # fine: overrides `Parent.class_method1` - 45 | @staticmethod + 21 | def class_method1(cls) -> int: ... + 22 | + 23 | @staticmethod + 24 | def static_method1() -> int: ... + 25 | + 26 | @classmethod + 27 | def class_method2(cls) -> int: ... + 28 | + 29 | @staticmethod + 30 | def static_method2() -> int: ... + 31 | + 32 | @lossy_decorator + 33 | def decorated_1(self): ... + 34 | + 35 | @lossy_decorator + 36 | def decorated_2(self): ... + 37 | + 38 | @lossy_decorator + 39 | def decorated_3(self): ... + 40 | + 41 | class Child(Parent): + 42 | @override + 43 | def foo(self): ... # fine: overrides `Parent.foo` + 44 | + 45 | @property 46 | @override - 47 | def static_method1() -> int: ... # fine: overrides `Parent.static_method1` - 48 | @override - 49 | @classmethod - 50 | def class_method2(cls) -> int: ... # fine: overrides `Parent.class_method2` - 51 | @override - 52 | @staticmethod - 53 | def static_method2() -> int: ... # fine: overrides `Parent.static_method2` - 54 | @override - 55 | def decorated_1(self): ... # fine: overrides `Parent.decorated_1` - 56 | @override - 57 | @lossy_decorator - 58 | def decorated_2(self): ... # fine: overrides `Parent.decorated_2` - 59 | @lossy_decorator - 60 | @override - 61 | def decorated_3(self): ... # fine: overrides `Parent.decorated_3` - 62 | - 63 | class OtherChild(Parent): ... - 64 | - 65 | class Grandchild(OtherChild): - 66 | @override - 67 | def foo(self): ... # fine: overrides `Parent.foo` + 47 | def my_property1(self) -> int: ... # fine: overrides `Parent.my_property1` + 48 | + 49 | @override + 50 | @property + 51 | def my_property2(self) -> int: ... # fine: overrides `Parent.my_property2` + 52 | + 53 | @override + 54 | def baz(self): ... # fine: overrides `Parent.baz` + 55 | + 56 | @classmethod + 57 | @override + 58 | def class_method1(cls) -> int: ... # fine: overrides `Parent.class_method1` + 59 | + 60 | @staticmethod + 61 | @override + 62 | def static_method1() -> int: ... # fine: overrides `Parent.static_method1` + 63 | + 64 | @override + 65 | @classmethod + 66 | def class_method2(cls) -> int: ... # fine: overrides `Parent.class_method2` + 67 | 68 | @override - 69 | @property - 70 | def bar(self) -> int: ... # fine: overrides `Parent.bar` - 71 | @override - 72 | def baz(self): ... # fine: overrides `Parent.baz` - 73 | @classmethod - 74 | @override - 75 | def class_method1(cls) -> int: ... # fine: overrides `Parent.class_method1` - 76 | @staticmethod - 77 | @override - 78 | def static_method1() -> int: ... # fine: overrides `Parent.static_method1` - 79 | @override - 80 | @classmethod - 81 | def class_method2(cls) -> int: ... # fine: overrides `Parent.class_method2` - 82 | @override - 83 | @staticmethod - 84 | def static_method2() -> int: ... # fine: overrides `Parent.static_method2` - 85 | @override - 86 | def decorated_1(self): ... # fine: overrides `Parent.decorated_1` - 87 | @override - 88 | @lossy_decorator - 89 | def decorated_2(self): ... # fine: overrides `Parent.decorated_2` - 90 | @lossy_decorator - 91 | @override - 92 | def decorated_3(self): ... # fine: overrides `Parent.decorated_3` - 93 | - 94 | class Invalid: - 95 | @override - 96 | def ___reprrr__(self): ... # error: [invalid-explicit-override] + 69 | @staticmethod + 70 | def static_method2() -> int: ... # fine: overrides `Parent.static_method2` + 71 | + 72 | @override + 73 | def decorated_1(self): ... # fine: overrides `Parent.decorated_1` + 74 | + 75 | @override + 76 | @lossy_decorator + 77 | def decorated_2(self): ... # fine: overrides `Parent.decorated_2` + 78 | + 79 | @lossy_decorator + 80 | @override + 81 | def decorated_3(self): ... # fine: overrides `Parent.decorated_3` + 82 | + 83 | class OtherChild(Parent): ... + 84 | + 85 | class Grandchild(OtherChild): + 86 | @override + 87 | def foo(self): ... # fine: overrides `Parent.foo` + 88 | + 89 | @override + 90 | @property + 91 | def my_property1(self) -> int: ... # fine: overrides `Parent.my_property1` + 92 | + 93 | @override + 94 | def baz(self): ... # fine: overrides `Parent.baz` + 95 | + 96 | @classmethod 97 | @override - 98 | @classmethod - 99 | def foo(self): ... # error: [invalid-explicit-override] -100 | @classmethod + 98 | def class_method1(cls) -> int: ... # fine: overrides `Parent.class_method1` + 99 | +100 | @staticmethod 101 | @override -102 | def bar(self): ... # error: [invalid-explicit-override] -103 | @staticmethod +102 | def static_method1() -> int: ... # fine: overrides `Parent.static_method1` +103 | 104 | @override -105 | def baz(): ... # error: [invalid-explicit-override] -106 | @override -107 | @staticmethod -108 | def eggs(): ... # error: [invalid-explicit-override] -109 | @property -110 | @override -111 | def bad_property1(self) -> int: ... # TODO: should emit `invalid-explicit-override` here +105 | @classmethod +106 | def class_method2(cls) -> int: ... # fine: overrides `Parent.class_method2` +107 | +108 | @override +109 | @staticmethod +110 | def static_method2() -> int: ... # fine: overrides `Parent.static_method2` +111 | 112 | @override -113 | @property -114 | def bad_property2(self) -> int: ... # TODO: should emit `invalid-explicit-override` here -115 | @lossy_decorator -116 | @override -117 | def lossy(self): ... # TODO: should emit `invalid-explicit-override` here -118 | @override +113 | def decorated_1(self): ... # fine: overrides `Parent.decorated_1` +114 | +115 | @override +116 | @lossy_decorator +117 | def decorated_2(self): ... # fine: overrides `Parent.decorated_2` +118 | 119 | @lossy_decorator -120 | def lossy2(self): ... # TODO: should emit `invalid-explicit-override` here -121 | -122 | # TODO: all overrides in this class should cause us to emit *Liskov* violations, -123 | # but not `@override` violations -124 | class LiskovViolatingButNotOverrideViolating(Parent): -125 | @override -126 | @property -127 | def foo(self) -> int: ... -128 | @override -129 | def my_property1(self) -> int: ... -130 | @staticmethod -131 | @override -132 | def class_method1() -> int: ... -133 | @classmethod -134 | @override -135 | def static_method1(cls) -> int: ... -136 | -137 | # Diagnostic edge case: `override` is very far away from the method definition in the source code: +120 | @override +121 | def decorated_3(self): ... # fine: overrides `Parent.decorated_3` +122 | +123 | class Invalid: +124 | @override +125 | def ___reprrr__(self): ... # error: [invalid-explicit-override] +126 | +127 | @override +128 | @classmethod +129 | def foo(self): ... # error: [invalid-explicit-override] +130 | +131 | @classmethod +132 | @override +133 | def bar(self): ... # error: [invalid-explicit-override] +134 | +135 | @staticmethod +136 | @override +137 | def baz(): ... # error: [invalid-explicit-override] 138 | -139 | T = TypeVar("T") -140 | -141 | def identity(x: T) -> T: ... +139 | @override +140 | @staticmethod +141 | def eggs(): ... # error: [invalid-explicit-override] 142 | -143 | class Foo: +143 | @property 144 | @override -145 | @identity -146 | @identity -147 | @identity -148 | @identity -149 | @identity -150 | @identity -151 | @identity -152 | @identity -153 | @identity -154 | @identity -155 | @identity -156 | @identity -157 | @identity -158 | @identity -159 | @identity -160 | @identity -161 | @identity -162 | @identity -163 | def bar(self): ... # error: [invalid-explicit-override] +145 | def bad_property1(self) -> int: ... # error: [invalid-explicit-override] +146 | +147 | @override +148 | @property +149 | def bad_property2(self) -> int: ... # error: [invalid-explicit-override] +150 | +151 | @property +152 | @override +153 | def bad_settable_property(self) -> int: ... # error: [invalid-explicit-override] +154 | @bad_settable_property.setter +155 | def bad_settable_property(self, x: int) -> None: ... +156 | +157 | @lossy_decorator +158 | @override +159 | def lossy(self): ... # TODO: should emit `invalid-explicit-override` here +160 | +161 | @override +162 | @lossy_decorator +163 | def lossy2(self): ... # TODO: should emit `invalid-explicit-override` here +164 | +165 | # TODO: all overrides in this class should cause us to emit *Liskov* violations, +166 | # but not `@override` violations +167 | class LiskovViolatingButNotOverrideViolating(Parent): +168 | @override +169 | @property +170 | def foo(self) -> int: ... +171 | +172 | @override +173 | def my_property1(self) -> int: ... +174 | +175 | @staticmethod +176 | @override +177 | def class_method1() -> int: ... +178 | +179 | @classmethod +180 | @override +181 | def static_method1(cls) -> int: ... +182 | +183 | # Diagnostic edge case: `override` is very far away from the method definition in the source code: +184 | +185 | T = TypeVar("T") +186 | +187 | def identity(x: T) -> T: ... +188 | +189 | class Foo: +190 | @override +191 | @identity +192 | @identity +193 | @identity +194 | @identity +195 | @identity +196 | @identity +197 | @identity +198 | @identity +199 | @identity +200 | @identity +201 | @identity +202 | @identity +203 | @identity +204 | @identity +205 | @identity +206 | @identity +207 | @identity +208 | @identity +209 | def bar(self): ... # error: [invalid-explicit-override] ``` # Diagnostics ``` error[invalid-explicit-override]: Method `___reprrr__` is decorated with `@override` but does not override anything - --> src/mdtest_snippet.pyi:95:5 - | -94 | class Invalid: -95 | @override - | --------- -96 | def ___reprrr__(self): ... # error: [invalid-explicit-override] - | ^^^^^^^^^^^ -97 | @override -98 | @classmethod - | + --> src/mdtest_snippet.pyi:124:5 + | +123 | class Invalid: +124 | @override + | --------- +125 | def ___reprrr__(self): ... # error: [invalid-explicit-override] + | ^^^^^^^^^^^ +126 | +127 | @override + | info: No `___reprrr__` definitions were found on any superclasses of `Invalid` info: rule `invalid-explicit-override` is enabled by default @@ -198,17 +244,17 @@ info: rule `invalid-explicit-override` is enabled by default ``` error[invalid-explicit-override]: Method `foo` is decorated with `@override` but does not override anything - --> src/mdtest_snippet.pyi:97:5 + --> src/mdtest_snippet.pyi:127:5 | - 95 | @override - 96 | def ___reprrr__(self): ... # error: [invalid-explicit-override] - 97 | @override +125 | def ___reprrr__(self): ... # error: [invalid-explicit-override] +126 | +127 | @override | --------- - 98 | @classmethod - 99 | def foo(self): ... # error: [invalid-explicit-override] +128 | @classmethod +129 | def foo(self): ... # error: [invalid-explicit-override] | ^^^ -100 | @classmethod -101 | @override +130 | +131 | @classmethod | info: No `foo` definitions were found on any superclasses of `Invalid` info: rule `invalid-explicit-override` is enabled by default @@ -217,16 +263,15 @@ info: rule `invalid-explicit-override` is enabled by default ``` error[invalid-explicit-override]: Method `bar` is decorated with `@override` but does not override anything - --> src/mdtest_snippet.pyi:101:5 + --> src/mdtest_snippet.pyi:132:5 | - 99 | def foo(self): ... # error: [invalid-explicit-override] -100 | @classmethod -101 | @override +131 | @classmethod +132 | @override | --------- -102 | def bar(self): ... # error: [invalid-explicit-override] +133 | def bar(self): ... # error: [invalid-explicit-override] | ^^^ -103 | @staticmethod -104 | @override +134 | +135 | @staticmethod | info: No `bar` definitions were found on any superclasses of `Invalid` info: rule `invalid-explicit-override` is enabled by default @@ -235,16 +280,15 @@ info: rule `invalid-explicit-override` is enabled by default ``` error[invalid-explicit-override]: Method `baz` is decorated with `@override` but does not override anything - --> src/mdtest_snippet.pyi:104:5 + --> src/mdtest_snippet.pyi:136:5 | -102 | def bar(self): ... # error: [invalid-explicit-override] -103 | @staticmethod -104 | @override +135 | @staticmethod +136 | @override | --------- -105 | def baz(): ... # error: [invalid-explicit-override] +137 | def baz(): ... # error: [invalid-explicit-override] | ^^^ -106 | @override -107 | @staticmethod +138 | +139 | @override | info: No `baz` definitions were found on any superclasses of `Invalid` info: rule `invalid-explicit-override` is enabled by default @@ -253,17 +297,17 @@ info: rule `invalid-explicit-override` is enabled by default ``` error[invalid-explicit-override]: Method `eggs` is decorated with `@override` but does not override anything - --> src/mdtest_snippet.pyi:106:5 + --> src/mdtest_snippet.pyi:139:5 | -104 | @override -105 | def baz(): ... # error: [invalid-explicit-override] -106 | @override +137 | def baz(): ... # error: [invalid-explicit-override] +138 | +139 | @override | --------- -107 | @staticmethod -108 | def eggs(): ... # error: [invalid-explicit-override] +140 | @staticmethod +141 | def eggs(): ... # error: [invalid-explicit-override] | ^^^^ -109 | @property -110 | @override +142 | +143 | @property | info: No `eggs` definitions were found on any superclasses of `Invalid` info: rule `invalid-explicit-override` is enabled by default @@ -271,21 +315,74 @@ info: rule `invalid-explicit-override` is enabled by default ``` ``` -error[invalid-explicit-override]: Method `bar` is decorated with `@override` but does not override anything - --> src/mdtest_snippet.pyi:163:9 +error[invalid-explicit-override]: Method `bad_property1` is decorated with `@override` but does not override anything + --> src/mdtest_snippet.pyi:144:5 | -161 | @identity -162 | @identity -163 | def bar(self): ... # error: [invalid-explicit-override] - | ^^^ - | - ::: src/mdtest_snippet.pyi:144:5 - | -143 | class Foo: +143 | @property 144 | @override | --------- -145 | @identity -146 | @identity +145 | def bad_property1(self) -> int: ... # error: [invalid-explicit-override] + | ^^^^^^^^^^^^^ +146 | +147 | @override + | +info: No `bad_property1` definitions were found on any superclasses of `Invalid` +info: rule `invalid-explicit-override` is enabled by default + +``` + +``` +error[invalid-explicit-override]: Method `bad_property2` is decorated with `@override` but does not override anything + --> src/mdtest_snippet.pyi:147:5 + | +145 | def bad_property1(self) -> int: ... # error: [invalid-explicit-override] +146 | +147 | @override + | --------- +148 | @property +149 | def bad_property2(self) -> int: ... # error: [invalid-explicit-override] + | ^^^^^^^^^^^^^ +150 | +151 | @property + | +info: No `bad_property2` definitions were found on any superclasses of `Invalid` +info: rule `invalid-explicit-override` is enabled by default + +``` + +``` +error[invalid-explicit-override]: Method `bad_settable_property` is decorated with `@override` but does not override anything + --> src/mdtest_snippet.pyi:152:5 + | +151 | @property +152 | @override + | --------- +153 | def bad_settable_property(self) -> int: ... # error: [invalid-explicit-override] + | ^^^^^^^^^^^^^^^^^^^^^ +154 | @bad_settable_property.setter +155 | def bad_settable_property(self, x: int) -> None: ... + | +info: No `bad_settable_property` definitions were found on any superclasses of `Invalid` +info: rule `invalid-explicit-override` is enabled by default + +``` + +``` +error[invalid-explicit-override]: Method `bar` is decorated with `@override` but does not override anything + --> src/mdtest_snippet.pyi:209:9 + | +207 | @identity +208 | @identity +209 | def bar(self): ... # error: [invalid-explicit-override] + | ^^^ + | + ::: src/mdtest_snippet.pyi:190:5 + | +189 | class Foo: +190 | @override + | --------- +191 | @identity +192 | @identity | info: No `bar` definitions were found on any superclasses of `Foo` info: rule `invalid-explicit-override` is enabled by default diff --git a/crates/ty_python_semantic/src/types/overrides.rs b/crates/ty_python_semantic/src/types/overrides.rs index 683f5549d1..97f89aa8e5 100644 --- a/crates/ty_python_semantic/src/types/overrides.rs +++ b/crates/ty_python_semantic/src/types/overrides.rs @@ -25,7 +25,7 @@ use crate::{ report_overridden_final_method, }, function::{FunctionDecorators, FunctionType, KnownFunction}, - list_members::{MemberWithDefinition, all_members_of_scope}, + list_members::{Member, MemberWithDefinition, all_members_of_scope}, }, }; @@ -101,33 +101,6 @@ fn check_class_declaration<'db>( .any(|definition| definition.kind(db).is_function_def()) } - fn extract_underlying_functions<'db>( - db: &'db dyn Db, - ty: Type<'db>, - ) -> Option; 1]>> { - match ty { - Type::FunctionLiteral(function) => Some(smallvec::smallvec_inline![function]), - Type::BoundMethod(method) => Some(smallvec::smallvec_inline![method.function(db)]), - Type::PropertyInstance(property) => { - extract_underlying_functions(db, property.getter(db)?) - } - Type::Union(union) => { - let mut functions = smallvec::smallvec![]; - for member in union.elements(db) { - if let Some(mut member_functions) = extract_underlying_functions(db, *member) { - functions.append(&mut member_functions); - } - } - if functions.is_empty() { - None - } else { - Some(functions) - } - } - _ => None, - } - } - let db = context.db(); let MemberWithDefinition { member, definition } = member; @@ -153,6 +126,8 @@ fn check_class_declaration<'db>( if class_kind == Some(CodeGeneratorKind::NamedTuple) && configuration.check_prohibited_named_tuple_attrs() && PROHIBITED_NAMEDTUPLE_ATTRS.contains(&member.name.as_str()) + // accessing `.kind()` here is fine as `definition` + // will always be a definition in the file currently being checked && !matches!(definition.kind(db), DefinitionKind::AnnotatedAssignment(_)) && let Some(builder) = context.report_lint( &INVALID_NAMED_TUPLE, @@ -331,35 +306,11 @@ fn check_class_declaration<'db>( if !subclass_overrides_superclass_declaration && !has_dynamic_superclass + // accessing `.kind()` here is fine as `definition` + // will always be a definition in the file currently being checked && definition.kind(db).is_function_def() - && let Type::FunctionLiteral(function) = member.ty - && function.has_known_decorator(db, FunctionDecorators::OVERRIDE) { - let function_literal = if context.in_stub() { - function.first_overload_or_implementation(db) - } else { - function.literal(db).last_definition(db) - }; - - if let Some(builder) = context.report_lint( - &INVALID_EXPLICIT_OVERRIDE, - function_literal.focus_range(db, context.module()), - ) { - let mut diagnostic = builder.into_diagnostic(format_args!( - "Method `{}` is decorated with `@override` but does not override anything", - member.name - )); - if let Some(decorator_span) = - function_literal.find_known_decorator_span(db, KnownFunction::Override) - { - diagnostic.annotate(Annotation::secondary(decorator_span)); - } - diagnostic.info(format_args!( - "No `{member}` definitions were found on any superclasses of `{class}`", - member = &member.name, - class = class.name(db) - )); - } + check_explicit_overrides(context, member, class); } if let Some((superclass, superclass_method)) = overridden_final_method { @@ -434,3 +385,72 @@ impl OverrideRulesConfig { self.contains(OverrideRulesConfig::PROHIBITED_NAMED_TUPLE_ATTR) } } + +fn check_explicit_overrides<'db>( + context: &InferContext<'db, '_>, + member: &Member<'db>, + class: ClassType<'db>, +) { + let db = context.db(); + let underlying_functions = extract_underlying_functions(db, member.ty); + let Some(functions) = underlying_functions else { + return; + }; + if !functions + .iter() + .any(|function| function.has_known_decorator(db, FunctionDecorators::OVERRIDE)) + { + return; + } + let function_literal = if context.in_stub() { + functions[0].first_overload_or_implementation(db) + } else { + functions[0].literal(db).last_definition(db) + }; + + let Some(builder) = context.report_lint( + &INVALID_EXPLICIT_OVERRIDE, + function_literal.focus_range(db, context.module()), + ) else { + return; + }; + let mut diagnostic = builder.into_diagnostic(format_args!( + "Method `{}` is decorated with `@override` but does not override anything", + member.name + )); + if let Some(decorator_span) = + function_literal.find_known_decorator_span(db, KnownFunction::Override) + { + diagnostic.annotate(Annotation::secondary(decorator_span)); + } + diagnostic.info(format_args!( + "No `{member}` definitions were found on any superclasses of `{class}`", + member = &member.name, + class = class.name(db) + )); +} + +fn extract_underlying_functions<'db>( + db: &'db dyn Db, + ty: Type<'db>, +) -> Option; 1]>> { + match ty { + Type::FunctionLiteral(function) => Some(smallvec::smallvec_inline![function]), + Type::BoundMethod(method) => Some(smallvec::smallvec_inline![method.function(db)]), + Type::PropertyInstance(property) => extract_underlying_functions(db, property.getter(db)?), + Type::Union(union) => { + let mut functions = smallvec::smallvec![]; + for member in union.elements(db) { + if let Some(mut member_functions) = extract_underlying_functions(db, *member) { + functions.append(&mut member_functions); + } + } + if functions.is_empty() { + None + } else { + Some(functions) + } + } + _ => None, + } +} From cd079bd92e2e8d70e0e37fea6d0220cfd0548f15 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 3 Dec 2025 12:51:36 +0000 Subject: [PATCH 08/10] [ty] Improve `@override`, `@final` and Liskov checks in cases where there are multiple reachable definitions (#21767) --- crates/ty_ide/src/importer.rs | 12 +- .../resources/mdtest/final.md | 105 ++++++++- .../resources/mdtest/liskov.md | 14 ++ .../resources/mdtest/named_tuple.md | 21 ++ .../resources/mdtest/override.md | 205 ++++++++++++++++++ ..._possibly-undefined…_(fc7b496fd1986deb).snap | 53 ++++- ...verloads_in_statica…_(29a698d9deaf7318).snap | 94 ++++++++ ..._case___multiple_…_(f30babd05c89dce9).snap | 74 +++++++ crates/ty_python_semantic/src/place.rs | 48 ++-- .../ty_python_semantic/src/semantic_model.rs | 6 +- crates/ty_python_semantic/src/types/class.rs | 18 +- crates/ty_python_semantic/src/types/enums.rs | 4 +- .../ty_python_semantic/src/types/function.rs | 2 +- .../src/types/infer/builder.rs | 29 +-- .../src/types/list_members.rs | 74 +++---- crates/ty_python_semantic/src/types/member.rs | 2 +- .../ty_python_semantic/src/types/overrides.rs | 51 +++-- .../src/types/protocol_class.rs | 5 +- .../src/types/typed_dict.rs | 2 +- 19 files changed, 688 insertions(+), 131 deletions(-) create mode 100644 crates/ty_python_semantic/resources/mdtest/snapshots/final.md_-_Tests_for_the_`@typi…_-_Overloads_in_statica…_(29a698d9deaf7318).snap create mode 100644 crates/ty_python_semantic/resources/mdtest/snapshots/named_tuple.md_-_`NamedTuple`_-_Edge_case___multiple_…_(f30babd05c89dce9).snap diff --git a/crates/ty_ide/src/importer.rs b/crates/ty_ide/src/importer.rs index bf75157147..5ff46a1ae1 100644 --- a/crates/ty_ide/src/importer.rs +++ b/crates/ty_ide/src/importer.rs @@ -327,9 +327,7 @@ impl<'ast> MembersInScope<'ast> { .members_in_scope_at(node) .into_iter() .map(|(name, memberdef)| { - let Some(def) = memberdef.definition else { - return (name, MemberInScope::other(memberdef.ty)); - }; + let def = memberdef.first_reachable_definition; let kind = match *def.kind(db) { DefinitionKind::Import(ref kind) => { MemberImportKind::Imported(AstImportKind::Import(kind.import(parsed))) @@ -1891,13 +1889,13 @@ else: "#); assert_snapshot!( test.import_from("foo", "MAGIC"), @r#" - import foo + from foo import MAGIC if os.getenv("WHATEVER"): from foo import MAGIC else: from bar import MAGIC - (foo.MAGIC) + (MAGIC) "#); } @@ -2108,13 +2106,13 @@ except ImportError: "); assert_snapshot!( test.import_from("foo", "MAGIC"), @r" - import foo + from foo import MAGIC try: from foo import MAGIC except ImportError: from bar import MAGIC - (foo.MAGIC) + (MAGIC) "); } diff --git a/crates/ty_python_semantic/resources/mdtest/final.md b/crates/ty_python_semantic/resources/mdtest/final.md index 91c6f9273f..fc5233e246 100644 --- a/crates/ty_python_semantic/resources/mdtest/final.md +++ b/crates/ty_python_semantic/resources/mdtest/final.md @@ -488,11 +488,110 @@ class C(A): pass if coinflip(): - def method2(self) -> None: ... # TODO: should emit [override-of-final-method] + def method2(self) -> None: ... # error: [override-of-final-method] else: - def method2(self) -> None: ... # TODO: should emit [override-of-final-method] + def method2(self) -> None: ... if coinflip(): def method3(self) -> None: ... # error: [override-of-final-method] - def method4(self) -> None: ... # error: [override-of-final-method] + + # TODO: we should emit Liskov violations here too: + if coinflip(): + method4 = 42 # error: [override-of-final-method] + else: + method4 = 56 +``` + +## Definitions in statically known branches + +```toml +[environment] +python-version = "3.10" +``` + +```py +import sys +from typing_extensions import final + +class Parent: + if sys.version_info >= (3, 10): + @final + def foo(self) -> None: ... + @final + def foooo(self) -> None: ... + @final + def baaaaar(self) -> None: ... + else: + @final + def bar(self) -> None: ... + @final + def baz(self) -> None: ... + @final + def spam(self) -> None: ... + +class Child(Parent): + def foo(self) -> None: ... # error: [override-of-final-method] + + # The declaration on `Parent` is not reachable, + # so this is fine + def bar(self) -> None: ... + + if sys.version_info >= (3, 10): + def foooo(self) -> None: ... # error: [override-of-final-method] + def baz(self) -> None: ... + else: + # Fine because this doesn't override any reachable definitions + def foooo(self) -> None: ... + # There are `@final` definitions being overridden here, + # but the definitions that override them are unreachable + def spam(self) -> None: ... + def baaaaar(self) -> None: ... +``` + +## Overloads in statically-known branches in stub files + + + +```toml +[environment] +python-version = "3.10" +``` + +```pyi +import sys +from typing_extensions import overload, final + +class Foo: + if sys.version_info >= (3, 10): + @overload + @final + def method(self, x: int) -> int: ... + else: + @overload + def method(self, x: int) -> int: ... + @overload + def method(self, x: str) -> str: ... + + if sys.version_info >= (3, 10): + @overload + def method2(self, x: int) -> int: ... + else: + @overload + @final + def method2(self, x: int) -> int: ... + @overload + def method2(self, x: str) -> str: ... + +class Bar(Foo): + @overload + def method(self, x: int) -> int: ... + @overload + def method(self, x: str) -> str: ... # error: [override-of-final-method] + + # This is fine: the only overload that is marked `@final` + # is in a statically-unreachable branch + @overload + def method2(self, x: int) -> int: ... + @overload + def method2(self, x: str) -> str: ... ``` diff --git a/crates/ty_python_semantic/resources/mdtest/liskov.md b/crates/ty_python_semantic/resources/mdtest/liskov.md index 24a1c3e2c2..ad16b99f08 100644 --- a/crates/ty_python_semantic/resources/mdtest/liskov.md +++ b/crates/ty_python_semantic/resources/mdtest/liskov.md @@ -583,3 +583,17 @@ class GoodChild2(Parent): @staticmethod def static_method(x: object) -> bool: ... ``` + +## Definitely bound members with no reachable definitions(!) + +We don't emit a Liskov-violation diagnostic here, but if you're writing code like this, you probably +have bigger problems: + +```py +from __future__ import annotations + +class MaybeEqWhile: + while ...: + def __eq__(self, other: MaybeEqWhile) -> bool: + return True +``` diff --git a/crates/ty_python_semantic/resources/mdtest/named_tuple.md b/crates/ty_python_semantic/resources/mdtest/named_tuple.md index 3d1a0ec544..ab1dafa187 100644 --- a/crates/ty_python_semantic/resources/mdtest/named_tuple.md +++ b/crates/ty_python_semantic/resources/mdtest/named_tuple.md @@ -610,3 +610,24 @@ class Child(Base): # This is fine - Child is not directly a NamedTuple _asdict = 42 ``` + +## Edge case: multiple reachable definitions with distinct issues + + + +```py +from typing import NamedTuple + +def coinflip() -> bool: + return True + +class Foo(NamedTuple): + if coinflip(): + _asdict: bool # error: [invalid-named-tuple] "NamedTuple field `_asdict` cannot start with an underscore" + else: + # TODO: there should only be one diagnostic here... + # + # error: [invalid-named-tuple] "Cannot overwrite NamedTuple attribute `_asdict`" + # error: [invalid-named-tuple] "Cannot overwrite NamedTuple attribute `_asdict`" + _asdict = True +``` diff --git a/crates/ty_python_semantic/resources/mdtest/override.md b/crates/ty_python_semantic/resources/mdtest/override.md index 7f726d4289..9a963c07d7 100644 --- a/crates/ty_python_semantic/resources/mdtest/override.md +++ b/crates/ty_python_semantic/resources/mdtest/override.md @@ -220,6 +220,178 @@ class Foo: def bar(self): ... # error: [invalid-explicit-override] ``` +## Possibly-unbound definitions + +```py +from typing_extensions import override + +def coinflip() -> bool: + return False + +class Parent: + if coinflip(): + def method1(self) -> None: ... + def method2(self) -> None: ... + + if coinflip(): + def method3(self) -> None: ... + def method4(self) -> None: ... + else: + def method3(self) -> None: ... + def method4(self) -> None: ... + + def method5(self) -> None: ... + def method6(self) -> None: ... + +class Child(Parent): + @override + def method1(self) -> None: ... + @override + def method2(self) -> None: ... + + if coinflip(): + @override + def method3(self) -> None: ... + + if coinflip(): + @override + def method4(self) -> None: ... + else: + @override + def method4(self) -> None: ... + + if coinflip(): + @override + def method5(self) -> None: ... + + if coinflip(): + @override + def method6(self) -> None: ... + else: + @override + def method6(self) -> None: ... + + if coinflip(): + @override + def method7(self) -> None: ... # error: [invalid-explicit-override] + + if coinflip(): + @override + def method8(self) -> None: ... # error: [invalid-explicit-override] + else: + @override + def method8(self) -> None: ... +``` + +## Multiple reachable definitions, only one of which is decorated with `@override` + +The diagnostic should point to the first definition decorated with `@override`, which may not +necessarily be the first definition of the symbol overall: + +`runtime.py`: + +```py +from typing_extensions import override, overload + +def coinflip() -> bool: + return True + +class Foo: + if coinflip(): + def method(self, x): ... + elif coinflip(): + @overload + def method(self, x: str) -> str: ... + @overload + def method(self, x: int) -> int: ... + @override + def method(self, x: str | int) -> str | int: # error: [invalid-explicit-override] + return x + elif coinflip(): + @override + def method(self, x): ... +``` + +stub.pyi\`: + +```pyi +from typing_extensions import override, overload + +def coinflip() -> bool: + return True + +class Foo: + if coinflip(): + def method(self, x): ... + elif coinflip(): + @overload + @override + def method(self, x: str) -> str: ... # error: [invalid-explicit-override] + @overload + def method(self, x: int) -> int: ... + + if coinflip(): + def method2(self, x): ... + elif coinflip(): + @overload + @override + def method2(self, x: str) -> str: ... + @overload + def method2(self, x: int) -> int: ... + else: + # TODO: not sure why this is being emitted on this line rather than on + # the first overload in the `elif` block? Ideally it would be emitted + # on the first reachable definition, but perhaps this is due to the way + # name lookups are deferred in stub files...? -- AW + @override + def method2(self, x): ... # error: [invalid-explicit-override] +``` + +## Definitions in statically known branches + +```toml +[environment] +python-version = "3.10" +``` + +```py +import sys +from typing_extensions import override, overload + +class Parent: + if sys.version_info >= (3, 10): + def foo(self) -> None: ... + def foooo(self) -> None: ... + else: + def bar(self) -> None: ... + def baz(self) -> None: ... + def spam(self) -> None: ... + +class Child(Parent): + @override + def foo(self) -> None: ... + + # The declaration on `Parent` is not reachable, + # so this is an error + @override + def bar(self) -> None: ... # error: [invalid-explicit-override] + + if sys.version_info >= (3, 10): + @override + def foooo(self) -> None: ... + @override + def baz(self) -> None: ... # error: [invalid-explicit-override] + else: + # This doesn't override any reachable definitions, + # but the subclass definition also isn't a reachable definition + # from the end of the scope with the given configuration, + # so it's not flagged + @override + def foooo(self) -> None: ... + @override + def spam(self) -> None: ... +``` + ## Overloads The typing spec states that for an overloaded method, `@override` should only be applied to the @@ -293,6 +465,39 @@ class Spam: def baz(self, x: int) -> int: ... ``` +## Overloads in statically-known branches in stub files + +```toml +[environment] +python-version = "3.10" +``` + +```pyi +import sys +from typing_extensions import overload, override + +class Foo: + if sys.version_info >= (3, 10): + @overload + @override + def method(self, x: int) -> int: ... # error: [invalid-explicit-override] + else: + @overload + def method(self, x: int) -> int: ... + @overload + def method(self, x: str) -> str: ... + + if sys.version_info >= (3, 10): + @overload + def method2(self, x: int) -> int: ... + else: + @overload + @override + def method2(self, x: int) -> int: ... + @overload + def method2(self, x: str) -> str: ... +``` + ## Classes inheriting from `Any` ```py diff --git a/crates/ty_python_semantic/resources/mdtest/snapshots/final.md_-_Tests_for_the_`@typi…_-_A_possibly-undefined…_(fc7b496fd1986deb).snap b/crates/ty_python_semantic/resources/mdtest/snapshots/final.md_-_Tests_for_the_`@typi…_-_A_possibly-undefined…_(fc7b496fd1986deb).snap index a137c44c51..68bd72f865 100644 --- a/crates/ty_python_semantic/resources/mdtest/snapshots/final.md_-_Tests_for_the_`@typi…_-_A_possibly-undefined…_(fc7b496fd1986deb).snap +++ b/crates/ty_python_semantic/resources/mdtest/snapshots/final.md_-_Tests_for_the_`@typi…_-_A_possibly-undefined…_(fc7b496fd1986deb).snap @@ -65,13 +65,18 @@ mdtest path: crates/ty_python_semantic/resources/mdtest/final.md 51 | pass 52 | 53 | if coinflip(): -54 | def method2(self) -> None: ... # TODO: should emit [override-of-final-method] +54 | def method2(self) -> None: ... # error: [override-of-final-method] 55 | else: -56 | def method2(self) -> None: ... # TODO: should emit [override-of-final-method] +56 | def method2(self) -> None: ... 57 | 58 | if coinflip(): 59 | def method3(self) -> None: ... # error: [override-of-final-method] -60 | def method4(self) -> None: ... # error: [override-of-final-method] +60 | +61 | # TODO: we should emit Liskov violations here too: +62 | if coinflip(): +63 | method4 = 42 # error: [override-of-final-method] +64 | else: +65 | method4 = 56 ``` # Diagnostics @@ -240,6 +245,33 @@ info: rule `override-of-final-method` is enabled by default ``` +``` +error[override-of-final-method]: Cannot override `A.method2` + --> src/mdtest_snippet.py:54:13 + | +53 | if coinflip(): +54 | def method2(self) -> None: ... # error: [override-of-final-method] + | ^^^^^^^ Overrides a definition from superclass `A` +55 | else: +56 | def method2(self) -> None: ... + | +info: `A.method2` is decorated with `@final`, forbidding overrides + --> src/mdtest_snippet.py:16:9 + | +14 | def method2(self) -> None: ... +15 | else: +16 | @final + | ------ +17 | def method2(self) -> None: ... + | ------- `A.method2` defined here +18 | +19 | if coinflip(): + | +help: Remove the override of `method2` +info: rule `override-of-final-method` is enabled by default + +``` + ``` error[override-of-final-method]: Cannot override `A.method3` --> src/mdtest_snippet.py:59:13 @@ -247,7 +279,8 @@ error[override-of-final-method]: Cannot override `A.method3` 58 | if coinflip(): 59 | def method3(self) -> None: ... # error: [override-of-final-method] | ^^^^^^^ Overrides a definition from superclass `A` -60 | def method4(self) -> None: ... # error: [override-of-final-method] +60 | +61 | # TODO: we should emit Liskov violations here too: | info: `A.method3` is decorated with `@final`, forbidding overrides --> src/mdtest_snippet.py:20:9 @@ -267,12 +300,14 @@ info: rule `override-of-final-method` is enabled by default ``` error[override-of-final-method]: Cannot override `A.method4` - --> src/mdtest_snippet.py:60:13 + --> src/mdtest_snippet.py:63:9 | -58 | if coinflip(): -59 | def method3(self) -> None: ... # error: [override-of-final-method] -60 | def method4(self) -> None: ... # error: [override-of-final-method] - | ^^^^^^^ Overrides a definition from superclass `A` +61 | # TODO: we should emit Liskov violations here too: +62 | if coinflip(): +63 | method4 = 42 # error: [override-of-final-method] + | ^^^^^^^ Overrides a definition from superclass `A` +64 | else: +65 | method4 = 56 | info: `A.method4` is decorated with `@final`, forbidding overrides --> src/mdtest_snippet.py:29:9 diff --git a/crates/ty_python_semantic/resources/mdtest/snapshots/final.md_-_Tests_for_the_`@typi…_-_Overloads_in_statica…_(29a698d9deaf7318).snap b/crates/ty_python_semantic/resources/mdtest/snapshots/final.md_-_Tests_for_the_`@typi…_-_Overloads_in_statica…_(29a698d9deaf7318).snap new file mode 100644 index 0000000000..fabea719d2 --- /dev/null +++ b/crates/ty_python_semantic/resources/mdtest/snapshots/final.md_-_Tests_for_the_`@typi…_-_Overloads_in_statica…_(29a698d9deaf7318).snap @@ -0,0 +1,94 @@ +--- +source: crates/ty_test/src/lib.rs +expression: snapshot +--- +--- +mdtest name: final.md - Tests for the `@typing(_extensions).final` decorator - Overloads in statically-known branches in stub files +mdtest path: crates/ty_python_semantic/resources/mdtest/final.md +--- + +# Python source files + +## mdtest_snippet.pyi + +``` + 1 | import sys + 2 | from typing_extensions import overload, final + 3 | + 4 | class Foo: + 5 | if sys.version_info >= (3, 10): + 6 | @overload + 7 | @final + 8 | def method(self, x: int) -> int: ... + 9 | else: +10 | @overload +11 | def method(self, x: int) -> int: ... +12 | @overload +13 | def method(self, x: str) -> str: ... +14 | +15 | if sys.version_info >= (3, 10): +16 | @overload +17 | def method2(self, x: int) -> int: ... +18 | else: +19 | @overload +20 | @final +21 | def method2(self, x: int) -> int: ... +22 | @overload +23 | def method2(self, x: str) -> str: ... +24 | +25 | class Bar(Foo): +26 | @overload +27 | def method(self, x: int) -> int: ... +28 | @overload +29 | def method(self, x: str) -> str: ... # error: [override-of-final-method] +30 | +31 | # This is fine: the only overload that is marked `@final` +32 | # is in a statically-unreachable branch +33 | @overload +34 | def method2(self, x: int) -> int: ... +35 | @overload +36 | def method2(self, x: str) -> str: ... +``` + +# Diagnostics + +``` +error[override-of-final-method]: Cannot override `Foo.method` + --> src/mdtest_snippet.pyi:29:9 + | +27 | def method(self, x: int) -> int: ... +28 | @overload +29 | def method(self, x: str) -> str: ... # error: [override-of-final-method] + | ^^^^^^ Overrides a definition from superclass `Foo` +30 | +31 | # This is fine: the only overload that is marked `@final` + | +info: `Foo.method` is decorated with `@final`, forbidding overrides + --> src/mdtest_snippet.pyi:7:9 + | + 5 | if sys.version_info >= (3, 10): + 6 | @overload + 7 | @final + | ------ + 8 | def method(self, x: int) -> int: ... + | ------ `Foo.method` defined here + 9 | else: +10 | @overload + | +help: Remove all overloads for `method` +info: rule `override-of-final-method` is enabled by default +23 | def method2(self, x: str) -> str: ... +24 | +25 | class Bar(Foo): + - @overload + - def method(self, x: int) -> int: ... + - @overload + - def method(self, x: str) -> str: ... # error: [override-of-final-method] +26 + +27 + # error: [override-of-final-method] +28 | +29 | # This is fine: the only overload that is marked `@final` +30 | # is in a statically-unreachable branch +note: This is an unsafe fix and may change runtime behavior + +``` diff --git a/crates/ty_python_semantic/resources/mdtest/snapshots/named_tuple.md_-_`NamedTuple`_-_Edge_case___multiple_…_(f30babd05c89dce9).snap b/crates/ty_python_semantic/resources/mdtest/snapshots/named_tuple.md_-_`NamedTuple`_-_Edge_case___multiple_…_(f30babd05c89dce9).snap new file mode 100644 index 0000000000..c7cd93b886 --- /dev/null +++ b/crates/ty_python_semantic/resources/mdtest/snapshots/named_tuple.md_-_`NamedTuple`_-_Edge_case___multiple_…_(f30babd05c89dce9).snap @@ -0,0 +1,74 @@ +--- +source: crates/ty_test/src/lib.rs +expression: snapshot +--- +--- +mdtest name: named_tuple.md - `NamedTuple` - Edge case: multiple reachable definitions with distinct issues +mdtest path: crates/ty_python_semantic/resources/mdtest/named_tuple.md +--- + +# Python source files + +## mdtest_snippet.py + +``` + 1 | from typing import NamedTuple + 2 | + 3 | def coinflip() -> bool: + 4 | return True + 5 | + 6 | class Foo(NamedTuple): + 7 | if coinflip(): + 8 | _asdict: bool # error: [invalid-named-tuple] "NamedTuple field `_asdict` cannot start with an underscore" + 9 | else: +10 | # TODO: there should only be one diagnostic here... +11 | # +12 | # error: [invalid-named-tuple] "Cannot overwrite NamedTuple attribute `_asdict`" +13 | # error: [invalid-named-tuple] "Cannot overwrite NamedTuple attribute `_asdict`" +14 | _asdict = True +``` + +# Diagnostics + +``` +error[invalid-named-tuple]: NamedTuple field name cannot start with an underscore + --> src/mdtest_snippet.py:8:9 + | + 6 | class Foo(NamedTuple): + 7 | if coinflip(): + 8 | _asdict: bool # error: [invalid-named-tuple] "NamedTuple field `_asdict` cannot start with an underscore" + | ^^^^^^^^^^^^^ Class definition will raise `TypeError` at runtime due to this field + 9 | else: +10 | # TODO: there should only be one diagnostic here... + | +info: rule `invalid-named-tuple` is enabled by default + +``` + +``` +error[invalid-named-tuple]: Cannot overwrite NamedTuple attribute `_asdict` + --> src/mdtest_snippet.py:14:9 + | +12 | # error: [invalid-named-tuple] "Cannot overwrite NamedTuple attribute `_asdict`" +13 | # error: [invalid-named-tuple] "Cannot overwrite NamedTuple attribute `_asdict`" +14 | _asdict = True + | ^^^^^^^ + | +info: This will cause the class creation to fail at runtime +info: rule `invalid-named-tuple` is enabled by default + +``` + +``` +error[invalid-named-tuple]: Cannot overwrite NamedTuple attribute `_asdict` + --> src/mdtest_snippet.py:14:9 + | +12 | # error: [invalid-named-tuple] "Cannot overwrite NamedTuple attribute `_asdict`" +13 | # error: [invalid-named-tuple] "Cannot overwrite NamedTuple attribute `_asdict`" +14 | _asdict = True + | ^^^^^^^ + | +info: This will cause the class creation to fail at runtime +info: rule `invalid-named-tuple` is enabled by default + +``` diff --git a/crates/ty_python_semantic/src/place.rs b/crates/ty_python_semantic/src/place.rs index 6e46819b14..3cb66efe33 100644 --- a/crates/ty_python_semantic/src/place.rs +++ b/crates/ty_python_semantic/src/place.rs @@ -459,7 +459,7 @@ fn core_module_scope(db: &dyn Db, core_module: KnownModule) -> Option( db: &'db dyn Db, bindings_with_constraints: BindingWithConstraintsIterator<'_, 'db>, -) -> Place<'db> { +) -> PlaceWithDefinition<'db> { place_from_bindings_impl(db, bindings_with_constraints, RequiresExplicitReExport::No) } @@ -487,20 +487,21 @@ type DeclaredTypeAndConflictingTypes<'db> = ( pub(crate) struct PlaceFromDeclarationsResult<'db> { place_and_quals: PlaceAndQualifiers<'db>, conflicting_types: Option>>>, - /// Contains `Some(declaration)` if the declared type originates from exactly one declaration. + /// Contains the first reachable declaration for this place, if any. /// This field is used for backreferences in diagnostics. - pub(crate) single_declaration: Option>, + pub(crate) first_declaration: Option>, } impl<'db> PlaceFromDeclarationsResult<'db> { fn conflict( place_and_quals: PlaceAndQualifiers<'db>, conflicting_types: Box>>, + first_declaration: Option>, ) -> Self { PlaceFromDeclarationsResult { place_and_quals, conflicting_types: Some(conflicting_types), - single_declaration: None, + first_declaration, } } @@ -798,6 +799,7 @@ pub(crate) fn place_by_id<'db>( if let Some(qualifiers) = declared.is_bare_final() { let bindings = all_considered_bindings(); return place_from_bindings_impl(db, bindings, requires_explicit_reexport) + .place .with_qualifiers(qualifiers); } @@ -809,7 +811,7 @@ pub(crate) fn place_by_id<'db>( qualifiers, } if qualifiers.contains(TypeQualifiers::CLASS_VAR) => { let bindings = all_considered_bindings(); - match place_from_bindings_impl(db, bindings, requires_explicit_reexport) { + match place_from_bindings_impl(db, bindings, requires_explicit_reexport).place { Place::Defined(inferred, origin, boundness) => Place::Defined( UnionType::from_elements(db, [Type::unknown(), inferred]), origin, @@ -835,7 +837,7 @@ pub(crate) fn place_by_id<'db>( let boundness_analysis = bindings.boundness_analysis; let inferred = place_from_bindings_impl(db, bindings, requires_explicit_reexport); - let place = match inferred { + let place = match inferred.place { // Place is possibly undeclared and definitely unbound Place::Undefined => { // TODO: We probably don't want to report `AlwaysDefined` here. This requires a bit of @@ -864,7 +866,8 @@ pub(crate) fn place_by_id<'db>( } => { let bindings = all_considered_bindings(); let boundness_analysis = bindings.boundness_analysis; - let mut inferred = place_from_bindings_impl(db, bindings, requires_explicit_reexport); + let mut inferred = + place_from_bindings_impl(db, bindings, requires_explicit_reexport).place; if boundness_analysis == BoundnessAnalysis::AssumeBound { if let Place::Defined(ty, origin, Definedness::PossiblyUndefined) = inferred { @@ -1010,7 +1013,7 @@ fn place_from_bindings_impl<'db>( db: &'db dyn Db, bindings_with_constraints: BindingWithConstraintsIterator<'_, 'db>, requires_explicit_reexport: RequiresExplicitReExport, -) -> Place<'db> { +) -> PlaceWithDefinition<'db> { let predicates = bindings_with_constraints.predicates; let reachability_constraints = bindings_with_constraints.reachability_constraints; let boundness_analysis = bindings_with_constraints.boundness_analysis; @@ -1039,6 +1042,8 @@ fn place_from_bindings_impl<'db>( }) }; + let mut first_definition = None; + let mut types = bindings_with_constraints.filter_map( |BindingWithConstraints { binding, @@ -1119,12 +1124,13 @@ fn place_from_bindings_impl<'db>( return None; } + first_definition.get_or_insert(binding); let binding_ty = binding_type(db, binding); Some(narrowing_constraint.narrow(db, binding_ty, binding.place(db))) }, ); - if let Some(first) = types.next() { + let place = if let Some(first) = types.next() { let ty = if let Some(second) = types.next() { let mut builder = PublicTypeBuilder::new(db); builder.add(first); @@ -1161,9 +1167,19 @@ fn place_from_bindings_impl<'db>( } } else { Place::Undefined + }; + + PlaceWithDefinition { + place, + first_definition, } } +pub(super) struct PlaceWithDefinition<'db> { + pub(super) place: Place<'db>, + pub(super) first_definition: Option>, +} + /// Accumulates types from multiple bindings or declarations, and eventually builds a /// union type from them. /// @@ -1294,7 +1310,6 @@ fn place_from_declarations_impl<'db>( let boundness_analysis = declarations.boundness_analysis; let mut declarations = declarations.peekable(); let mut first_declaration = None; - let mut exactly_one_declaration = false; let is_non_exported = |declaration: Definition<'db>| { requires_explicit_reexport.is_yes() && !is_reexported(db, declaration) @@ -1325,12 +1340,7 @@ fn place_from_declarations_impl<'db>( return None; } - if first_declaration.is_none() { - first_declaration = Some(declaration); - exactly_one_declaration = true; - } else { - exactly_one_declaration = false; - } + first_declaration.get_or_insert(declaration); let static_reachability = reachability_constraints.evaluate(db, predicates, reachability_constraint); @@ -1387,19 +1397,19 @@ fn place_from_declarations_impl<'db>( .with_qualifiers(declared.qualifiers()); if let Some(conflicting) = conflicting { - PlaceFromDeclarationsResult::conflict(place_and_quals, conflicting) + PlaceFromDeclarationsResult::conflict(place_and_quals, conflicting, first_declaration) } else { PlaceFromDeclarationsResult { place_and_quals, conflicting_types: None, - single_declaration: first_declaration.filter(|_| exactly_one_declaration), + first_declaration, } } } else { PlaceFromDeclarationsResult { place_and_quals: Place::Undefined.into(), conflicting_types: None, - single_declaration: None, + first_declaration: None, } } } diff --git a/crates/ty_python_semantic/src/semantic_model.rs b/crates/ty_python_semantic/src/semantic_model.rs index f9ac728c40..2057db47ab 100644 --- a/crates/ty_python_semantic/src/semantic_model.rs +++ b/crates/ty_python_semantic/src/semantic_model.rs @@ -82,7 +82,7 @@ impl<'db> SemanticModel<'db> { memberdef.member.name, MemberDefinition { ty: memberdef.member.ty, - definition: memberdef.definition, + first_reachable_definition: memberdef.first_reachable_definition, }, ); } @@ -328,11 +328,11 @@ impl<'db> SemanticModel<'db> { } } -/// The type and definition (if available) of a symbol. +/// The type and definition of a symbol. #[derive(Clone, Debug)] pub struct MemberDefinition<'db> { pub ty: Type<'db>, - pub definition: Option>, + pub first_reachable_definition: Definition<'db>, } /// A classification of symbol names. diff --git a/crates/ty_python_semantic/src/types/class.rs b/crates/ty_python_semantic/src/types/class.rs index 1285fb0093..0abb33c54f 100644 --- a/crates/ty_python_semantic/src/types/class.rs +++ b/crates/ty_python_semantic/src/types/class.rs @@ -1375,9 +1375,9 @@ pub(crate) struct Field<'db> { pub(crate) declared_ty: Type<'db>, /// Kind-specific metadata for this field pub(crate) kind: FieldKind<'db>, - /// The original declaration of this field, if there is exactly one. + /// The first declaration of this field. /// This field is used for backreferences in diagnostics. - pub(crate) single_declaration: Option>, + pub(crate) first_declaration: Option>, } impl Field<'_> { @@ -3039,7 +3039,7 @@ impl<'db> ClassLiteral<'db> { let symbol = table.symbol(symbol_id); let result = place_from_declarations(db, declarations.clone()); - let single_declaration = result.single_declaration; + let first_declaration = result.first_declaration; let attr = result.ignore_conflicting_declarations(); if attr.is_class_var() { continue; @@ -3047,7 +3047,9 @@ impl<'db> ClassLiteral<'db> { if let Some(attr_ty) = attr.place.ignore_possibly_undefined() { let bindings = use_def.end_of_scope_symbol_bindings(symbol_id); - let mut default_ty = place_from_bindings(db, bindings).ignore_possibly_undefined(); + let mut default_ty = place_from_bindings(db, bindings) + .place + .ignore_possibly_undefined(); default_ty = default_ty.map(|ty| ty.apply_optional_specialization(db, specialization)); @@ -3105,7 +3107,7 @@ impl<'db> ClassLiteral<'db> { let mut field = Field { declared_ty: attr_ty.apply_optional_specialization(db, specialization), kind, - single_declaration, + first_declaration, }; // Check if this is a KW_ONLY sentinel and mark subsequent fields as keyword-only @@ -3588,7 +3590,7 @@ impl<'db> ClassLiteral<'db> { // The attribute is declared in the class body. let bindings = use_def.end_of_scope_symbol_bindings(symbol_id); - let inferred = place_from_bindings(db, bindings); + let inferred = place_from_bindings(db, bindings).place; let has_binding = !inferred.is_undefined(); if has_binding { @@ -3831,7 +3833,9 @@ impl<'db> VarianceInferable<'db> for ClassLiteral<'db> { (symbol_id, place_and_qual) }) .chain(use_def_map.all_end_of_scope_symbol_bindings().map( - |(symbol_id, bindings)| (symbol_id, place_from_bindings(db, bindings).into()), + |(symbol_id, bindings)| { + (symbol_id, place_from_bindings(db, bindings).place.into()) + }, )) .filter_map(|(symbol_id, place_and_qual)| { if let Some(name) = table.place(symbol_id).as_symbol().map(Symbol::name) { diff --git a/crates/ty_python_semantic/src/types/enums.rs b/crates/ty_python_semantic/src/types/enums.rs index dbde339221..9a91d1da3d 100644 --- a/crates/ty_python_semantic/src/types/enums.rs +++ b/crates/ty_python_semantic/src/types/enums.rs @@ -77,7 +77,7 @@ pub(crate) fn enum_metadata<'db>( let ignored_names: Option> = if let Some(ignore) = table.symbol_id("_ignore_") { let ignore_bindings = use_def_map.all_reachable_symbol_bindings(ignore); - let ignore_place = place_from_bindings(db, ignore_bindings); + let ignore_place = place_from_bindings(db, ignore_bindings).place; match ignore_place { Place::Defined(Type::StringLiteral(ignored_names), _, _) => { @@ -111,7 +111,7 @@ pub(crate) fn enum_metadata<'db>( return None; } - let inferred = place_from_bindings(db, bindings); + let inferred = place_from_bindings(db, bindings).place; let value_ty = match inferred { Place::Undefined => { diff --git a/crates/ty_python_semantic/src/types/function.rs b/crates/ty_python_semantic/src/types/function.rs index 339689169c..4437e09698 100644 --- a/crates/ty_python_semantic/src/types/function.rs +++ b/crates/ty_python_semantic/src/types/function.rs @@ -373,7 +373,7 @@ impl<'db> OverloadLiteral<'db> { .scoped_use_id(db, scope); let Place::Defined(Type::FunctionLiteral(previous_type), _, Definedness::AlwaysDefined) = - place_from_bindings(db, use_def.bindings_at_use(use_id)) + place_from_bindings(db, use_def.bindings_at_use(use_id)).place else { return None; }; diff --git a/crates/ty_python_semantic/src/types/infer/builder.rs b/crates/ty_python_semantic/src/types/infer/builder.rs index 6a1003acb6..4d05719ebf 100644 --- a/crates/ty_python_semantic/src/types/infer/builder.rs +++ b/crates/ty_python_semantic/src/types/infer/builder.rs @@ -634,7 +634,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { &self.context, class, &field_name, - field.single_declaration, + field.first_declaration, ); } @@ -645,13 +645,13 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { } ) { field_with_default_encountered = - Some((field_name, field.single_declaration)); + Some((field_name, field.first_declaration)); } else if let Some(field_with_default) = field_with_default_encountered.as_ref() { report_namedtuple_field_without_default_after_field_with_default( &self.context, class, - (&field_name, field.single_declaration), + (&field_name, field.first_declaration), field_with_default, ); } @@ -1034,6 +1034,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { self.db(), use_def.end_of_scope_symbol_bindings(place.as_symbol().unwrap()), ) + .place { if function.file(self.db()) != self.file() { // If the function is not in this file, we don't need to check it. @@ -1727,6 +1728,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { let prior_bindings = use_def.bindings_at_definition(declaration); // unbound_ty is Never because for this check we don't care about unbound let inferred_ty = place_from_bindings(self.db(), prior_bindings) + .place .with_qualifiers(TypeQualifiers::empty()) .or_fall_back_to(self.db(), || { // Fallback to bindings declared on `types.ModuleType` if it's a global symbol @@ -8673,7 +8675,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { // If we're inferring types of deferred expressions, look them up from end-of-scope. if self.is_deferred() { let place = if let Some(place_id) = place_table.place_id(expr) { - place_from_bindings(db, use_def.all_reachable_bindings(place_id)) + place_from_bindings(db, use_def.all_reachable_bindings(place_id)).place } else { assert!( self.deferred_state.in_string_annotation(), @@ -8691,7 +8693,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { } let use_id = expr_ref.scoped_use_id(db, scope); - let place = place_from_bindings(db, use_def.bindings_at_use(use_id)); + let place = place_from_bindings(db, use_def.bindings_at_use(use_id)).place; (place, Some(use_id)) } @@ -8832,7 +8834,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { } } EnclosingSnapshotResult::FoundBindings(bindings) => { - let place = place_from_bindings(db, bindings).map_type(|ty| { + let place = place_from_bindings(db, bindings).place.map_type(|ty| { self.narrow_place_with_applicable_constraints( place_expr, ty, @@ -8952,13 +8954,14 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { return Place::Undefined.into(); } EnclosingSnapshotResult::FoundBindings(bindings) => { - let place = place_from_bindings(db, bindings).map_type(|ty| { - self.narrow_place_with_applicable_constraints( - place_expr, - ty, - &constraint_keys, - ) - }); + let place = + place_from_bindings(db, bindings).place.map_type(|ty| { + self.narrow_place_with_applicable_constraints( + place_expr, + ty, + &constraint_keys, + ) + }); constraint_keys.push(( FileScopeId::global(), ConstraintKey::NestedScope(file_scope_id), diff --git a/crates/ty_python_semantic/src/types/list_members.rs b/crates/ty_python_semantic/src/types/list_members.rs index 66b92d69bc..eb47745b74 100644 --- a/crates/ty_python_semantic/src/types/list_members.rs +++ b/crates/ty_python_semantic/src/types/list_members.rs @@ -12,7 +12,9 @@ use rustc_hash::FxHashSet; use crate::{ Db, NameKind, - place::{Place, imported_symbol, place_from_bindings, place_from_declarations}, + place::{ + Place, PlaceWithDefinition, imported_symbol, place_from_bindings, place_from_declarations, + }, semantic_index::{ attribute_scopes, definition::Definition, global_scope, place_table, scope::ScopeId, semantic_index, use_def_map, @@ -35,48 +37,40 @@ pub(crate) fn all_members_of_scope<'db>( .all_end_of_scope_symbol_declarations() .filter_map(move |(symbol_id, declarations)| { let place_result = place_from_declarations(db, declarations); - let definition = place_result.single_declaration; - place_result + let first_reachable_definition = place_result.first_declaration?; + let ty = place_result .ignore_conflicting_declarations() .place - .ignore_possibly_undefined() - .map(|ty| { - let symbol = table.symbol(symbol_id); - let member = Member { - name: symbol.name().clone(), - ty, - }; - MemberWithDefinition { member, definition } - }) + .ignore_possibly_undefined()?; + let symbol = table.symbol(symbol_id); + let member = Member { + name: symbol.name().clone(), + ty, + }; + Some(MemberWithDefinition { + member, + first_reachable_definition, + }) }) .chain(use_def_map.all_end_of_scope_symbol_bindings().filter_map( move |(symbol_id, bindings)| { - // It's not clear to AG how to using a bindings - // iterator here to get the correct definition for - // this binding. Below, we look through all bindings - // with a definition and only take one if there is - // exactly one. I don't think this can be wrong, but - // it's probably omitting definitions in some cases. - let mut definition = None; - for binding in bindings.clone() { - if let Some(def) = binding.binding.definition() { - if definition.is_some() { - definition = None; - break; - } - definition = Some(def); - } - } - place_from_bindings(db, bindings) - .ignore_possibly_undefined() - .map(|ty| { - let symbol = table.symbol(symbol_id); - let member = Member { - name: symbol.name().clone(), - ty, - }; - MemberWithDefinition { member, definition } - }) + let PlaceWithDefinition { + place, + first_definition, + } = place_from_bindings(db, bindings); + + let first_reachable_definition = first_definition?; + let ty = place.ignore_possibly_undefined()?; + + let symbol = table.symbol(symbol_id); + let member = Member { + name: symbol.name().clone(), + ty, + }; + Some(MemberWithDefinition { + member, + first_reachable_definition, + }) }, )) } @@ -457,11 +451,11 @@ impl<'db> AllMembers<'db> { } } -/// A member of a type or scope, with an optional definition. +/// A member of a type or scope, with the first reachable definition of that member. #[derive(Clone, Debug, Eq, PartialEq, Hash)] pub struct MemberWithDefinition<'db> { pub member: Member<'db>, - pub definition: Option>, + pub first_reachable_definition: Definition<'db>, } /// A member of a type or scope. diff --git a/crates/ty_python_semantic/src/types/member.rs b/crates/ty_python_semantic/src/types/member.rs index 11a8648f50..dfb6cd47fa 100644 --- a/crates/ty_python_semantic/src/types/member.rs +++ b/crates/ty_python_semantic/src/types/member.rs @@ -75,7 +75,7 @@ pub(super) fn class_member<'db>(db: &'db dyn Db, scope: ScopeId<'db>, name: &str // Otherwise, we need to check if the symbol has bindings let use_def = use_def_map(db, scope); let bindings = use_def.end_of_scope_symbol_bindings(symbol_id); - let inferred = place_from_bindings(db, bindings); + let inferred = place_from_bindings(db, bindings).place; // TODO: we should not need to calculate inferred type second time. This is a temporary // solution until the notion of Boundness and Declaredness is split. See #16036, #16264 diff --git a/crates/ty_python_semantic/src/types/overrides.rs b/crates/ty_python_semantic/src/types/overrides.rs index 97f89aa8e5..5d6328a606 100644 --- a/crates/ty_python_semantic/src/types/overrides.rs +++ b/crates/ty_python_semantic/src/types/overrides.rs @@ -12,8 +12,8 @@ use crate::{ lint::LintId, place::Place, semantic_index::{ - definition::DefinitionKind, place_table, scope::ScopeId, symbol::ScopedSymbolId, - use_def_map, + definition::DefinitionKind, place::ScopedPlaceId, place_table, scope::ScopeId, + symbol::ScopedSymbolId, use_def_map, }, types::{ ClassBase, ClassLiteral, ClassType, KnownClass, Type, @@ -53,10 +53,11 @@ pub(super) fn check_class<'db>(context: &InferContext<'db, '_>, class: ClassLite } let class_specialized = class.identity_specialization(db); - let own_class_members: FxHashSet<_> = all_members_of_scope(db, class.body_scope(db)).collect(); + let scope = class.body_scope(db); + let own_class_members: FxHashSet<_> = all_members_of_scope(db, scope).collect(); for member in own_class_members { - check_class_declaration(context, configuration, class_specialized, &member); + check_class_declaration(context, configuration, class_specialized, scope, &member); } } @@ -64,6 +65,7 @@ fn check_class_declaration<'db>( context: &InferContext<'db, '_>, configuration: OverrideRulesConfig, class: ClassType<'db>, + class_scope: ScopeId<'db>, member: &MemberWithDefinition<'db>, ) { /// Salsa-tracked query to check whether any of the definitions of a symbol @@ -103,11 +105,10 @@ fn check_class_declaration<'db>( let db = context.db(); - let MemberWithDefinition { member, definition } = member; - - let Some(definition) = definition else { - return; - }; + let MemberWithDefinition { + member, + first_reachable_definition, + } = member; let Place::Defined(type_on_subclass_instance, _, _) = Type::instance(db, class).member(db, &member.name).place @@ -126,12 +127,14 @@ fn check_class_declaration<'db>( if class_kind == Some(CodeGeneratorKind::NamedTuple) && configuration.check_prohibited_named_tuple_attrs() && PROHIBITED_NAMEDTUPLE_ATTRS.contains(&member.name.as_str()) - // accessing `.kind()` here is fine as `definition` - // will always be a definition in the file currently being checked - && !matches!(definition.kind(db), DefinitionKind::AnnotatedAssignment(_)) + && let Some(symbol_id) = place_table(db, class_scope).symbol_id(&member.name) + && let Some(bad_definition) = use_def_map(db, class_scope) + .all_reachable_bindings(ScopedPlaceId::Symbol(symbol_id)) + .filter_map(|binding| binding.binding.definition()) + .find(|def| !matches!(def.kind(db), DefinitionKind::AnnotatedAssignment(_))) && let Some(builder) = context.report_lint( &INVALID_NAMED_TUPLE, - definition.focus_range(db, context.module()), + bad_definition.focus_range(db, context.module()), ) { let mut diagnostic = builder.into_diagnostic(format_args!( @@ -187,8 +190,6 @@ fn check_class_declaration<'db>( .unwrap_or_default(); } - subclass_overrides_superclass_declaration = true; - let Place::Defined(superclass_type, _, _) = Type::instance(db, superclass) .member(db, &member.name) .place @@ -197,6 +198,8 @@ fn check_class_declaration<'db>( break; }; + subclass_overrides_superclass_declaration = true; + if configuration.check_final_method_overridden() { overridden_final_method = overridden_final_method.or_else(|| { let superclass_symbol_id = superclass_symbol_id?; @@ -272,7 +275,7 @@ fn check_class_declaration<'db>( context, &member.name, class, - *definition, + *first_reachable_definition, subclass_function, superclass, superclass_type, @@ -308,7 +311,7 @@ fn check_class_declaration<'db>( && !has_dynamic_superclass // accessing `.kind()` here is fine as `definition` // will always be a definition in the file currently being checked - && definition.kind(db).is_function_def() + && first_reachable_definition.kind(db).is_function_def() { check_explicit_overrides(context, member, class); } @@ -317,7 +320,7 @@ fn check_class_declaration<'db>( report_overridden_final_method( context, &member.name, - *definition, + *first_reachable_definition, member.ty, superclass, class, @@ -396,16 +399,16 @@ fn check_explicit_overrides<'db>( let Some(functions) = underlying_functions else { return; }; - if !functions + let Some(decorated_function) = functions .iter() - .any(|function| function.has_known_decorator(db, FunctionDecorators::OVERRIDE)) - { + .find(|function| function.has_known_decorator(db, FunctionDecorators::OVERRIDE)) + else { return; - } + }; let function_literal = if context.in_stub() { - functions[0].first_overload_or_implementation(db) + decorated_function.first_overload_or_implementation(db) } else { - functions[0].literal(db).last_definition(db) + decorated_function.literal(db).last_definition(db) }; let Some(builder) = context.report_lint( diff --git a/crates/ty_python_semantic/src/types/protocol_class.rs b/crates/ty_python_semantic/src/types/protocol_class.rs index 862349ea40..80ce2af47c 100644 --- a/crates/ty_python_semantic/src/types/protocol_class.rs +++ b/crates/ty_python_semantic/src/types/protocol_class.rs @@ -896,7 +896,10 @@ fn cached_protocol_interface<'db>( // type narrowing that uses `isinstance()` or `issubclass()` with // runtime-checkable protocols. for (symbol_id, bindings) in use_def_map.all_end_of_scope_symbol_bindings() { - let Some(ty) = place_from_bindings(db, bindings).ignore_possibly_undefined() else { + let Some(ty) = place_from_bindings(db, bindings) + .place + .ignore_possibly_undefined() + else { continue; }; direct_members.insert( diff --git a/crates/ty_python_semantic/src/types/typed_dict.rs b/crates/ty_python_semantic/src/types/typed_dict.rs index 7a22f62507..14ee100d2d 100644 --- a/crates/ty_python_semantic/src/types/typed_dict.rs +++ b/crates/ty_python_semantic/src/types/typed_dict.rs @@ -365,7 +365,7 @@ pub(super) fn validate_typed_dict_key_assignment<'db, 'ast>( }; let add_item_definition_subdiagnostic = |diagnostic: &mut Diagnostic, message| { - if let Some(declaration) = item.single_declaration { + if let Some(declaration) = item.first_declaration { let file = declaration.file(db); let module = parsed_module(db, file).load(db); From 45842cc0341ca7c661ef17fcda3161a4b67fbcab Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Wed, 3 Dec 2025 10:19:39 -0500 Subject: [PATCH 09/10] [ty] Fix non-determinism in `ConstraintSet.specialize_constrained` (#21744) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes a non-determinism that we were seeing in the constraint set tests in https://github.com/astral-sh/ruff/pull/21715. In this test, we create the following constraint set, and then try to create a specialization from it: ``` (T@constrained_by_gradual_list = list[Base]) ∨ (Bottom[list[Any]] ≤ T@constrained_by_gradual_list ≤ Top[list[Any]]) ``` That is, `T` is either specifically `list[Base]`, or it's any `list`. Our current heuristics say that, absent other restrictions, we should specialize `T` to the more specific type (`list[Base]`). In the correct test output, we end up creating a BDD that looks like this: ``` (T@constrained_by_gradual_list = list[Base]) ┡━₁ always └─₀ (Bottom[list[Any]] ≤ T@constrained_by_gradual_list ≤ Top[list[Any]]) ┡━₁ always └─₀ never ``` In the incorrect output, the BDD looks like this: ``` (Bottom[list[Any]] ≤ T@constrained_by_gradual_list ≤ Top[list[Any]]) ┡━₁ always └─₀ never ``` The difference is the ordering of the two individual constraints. Both constraints appear in the first BDD, but the second BDD only contains `T is any list`. If we were to force the second BDD to contain both constraints, it would look like this: ``` (Bottom[list[Any]] ≤ T@constrained_by_gradual_list ≤ Top[list[Any]]) ┡━₁ always └─₀ (T@constrained_by_gradual_list = list[Base]) ┡━₁ always └─₀ never ``` This is the standard shape for an OR of two constraints. However! Those two constraints are not independent of each other! If `T` is specifically `list[Base]`, then it's definitely also "any `list`". From that, we can infer the contrapositive: that if `T` is not any list, then it cannot be `list[Base]` specifically. When we encounter impossible situations like that, we prune that path in the BDD, and treat it as `false`. That rewrites the second BDD to the following: ``` (Bottom[list[Any]] ≤ T@constrained_by_gradual_list ≤ Top[list[Any]]) ┡━₁ always └─₀ (T@constrained_by_gradual_list = list[Base]) ┡━₁ never <-- IMPOSSIBLE, rewritten to never └─₀ never ``` We then would see that that BDD node is redundant, since both of its outgoing edges point at the `never` node. Our BDDs are _reduced_, which means we have to remove that redundant node, resulting in the BDD we saw above: ``` (Bottom[list[Any]] ≤ T@constrained_by_gradual_list ≤ Top[list[Any]]) ┡━₁ always └─₀ never <-- redundant node removed ``` The end result is that we were "forgetting" about the `T = list[Base]` constraint, but only for some BDD variable orderings. To fix this, I'm leaning in to the fact that our BDDs really do need to "remember" all of the constraints that they were created with. Some combinations might not be possible, but we now have the sequent map, which is quite good at detecting and pruning those. So now our BDDs are _quasi-reduced_, which just means that redundant nodes are allowed. (At first I was worried that allowing redundant nodes would be an unsound "fix the glitch". But it turns out they're real! [This](https://ieeexplore.ieee.org/abstract/document/130209) is the paper that introduces them, though it's very difficult to read. Knuth mentions them in §7.1.4 of [TAOCP](https://course.khoury.northeastern.edu/csu690/ssl/bdd-knuth.pdf), and [this paper](https://par.nsf.gov/servlets/purl/10128966) has a nice short summary of them in §2.) While we're here, I've added a bunch of `debug` and `trace` level log messages to the constraint set implementation. I was getting tired of having to add these by hands over and over. To enable them, just set `TY_LOG` in your environment, e.g. ```sh env TY_LOG=ty_python_semantic::types::constraints::SequentMap=trace ty check ... ``` [Note, this has an `internal` label because are still not using `specialize_constrained` in anything user-facing yet.] --- .../mdtest/generics/specialize_constrained.md | 29 +- .../src/types/constraints.rs | 324 +++++++++++++++--- 2 files changed, 300 insertions(+), 53 deletions(-) diff --git a/crates/ty_python_semantic/resources/mdtest/generics/specialize_constrained.md b/crates/ty_python_semantic/resources/mdtest/generics/specialize_constrained.md index bd4ee67aaf..4e04b91944 100644 --- a/crates/ty_python_semantic/resources/mdtest/generics/specialize_constrained.md +++ b/crates/ty_python_semantic/resources/mdtest/generics/specialize_constrained.md @@ -41,7 +41,8 @@ def unbounded[T](): # revealed: None reveal_type(generic_context(unbounded).specialize_constrained(ConstraintSet.range(bool, T, bool) & ConstraintSet.range(Never, T, str))) - # revealed: ty_extensions.Specialization[T@unbounded = int] + # TODO: revealed: ty_extensions.Specialization[T@unbounded = int] + # revealed: ty_extensions.Specialization[T@unbounded = bool] reveal_type(generic_context(unbounded).specialize_constrained(ConstraintSet.range(Never, T, int) | ConstraintSet.range(Never, T, bool))) # revealed: ty_extensions.Specialization[T@unbounded = Never] reveal_type(generic_context(unbounded).specialize_constrained(ConstraintSet.range(Never, T, int) | ConstraintSet.range(Never, T, str))) @@ -175,7 +176,7 @@ def constrained_by_gradual[T: (Base, Any)](): # revealed: ty_extensions.Specialization[T@constrained_by_gradual = Base] reveal_type(generic_context(constrained_by_gradual).specialize_constrained(ConstraintSet.always())) # TODO: revealed: ty_extensions.Specialization[T@constrained_by_gradual = Any] - # revealed: ty_extensions.Specialization[T@constrained_by_gradual = object] + # revealed: ty_extensions.Specialization[T@constrained_by_gradual = Base] reveal_type(generic_context(constrained_by_gradual).specialize_constrained(ConstraintSet.range(Never, T, object))) # revealed: None reveal_type(generic_context(constrained_by_gradual).specialize_constrained(ConstraintSet.never())) @@ -251,6 +252,30 @@ def constrained_by_gradual_list[T: (list[Base], list[Any])](): # revealed: ty_extensions.Specialization[T@constrained_by_gradual_list = list[Sub]] reveal_type(generic_context(constrained_by_gradual_list).specialize_constrained(ConstraintSet.range(list[Sub], T, list[Sub]))) +# Same tests as above, but with the typevar constraints in a different order, to make sure the +# results do not depend on our BDD variable ordering. +def constrained_by_gradual_list_reverse[T: (list[Any], list[Base])](): + # revealed: ty_extensions.Specialization[T@constrained_by_gradual_list_reverse = list[Base]] + reveal_type(generic_context(constrained_by_gradual_list_reverse).specialize_constrained(ConstraintSet.always())) + # revealed: None + reveal_type(generic_context(constrained_by_gradual_list_reverse).specialize_constrained(ConstraintSet.never())) + + # revealed: ty_extensions.Specialization[T@constrained_by_gradual_list_reverse = list[Base]] + reveal_type(generic_context(constrained_by_gradual_list_reverse).specialize_constrained(ConstraintSet.range(Never, T, list[Base]))) + # TODO: revealed: ty_extensions.Specialization[T@constrained_by_gradual = list[Any]] + # revealed: ty_extensions.Specialization[T@constrained_by_gradual_list_reverse = list[Unrelated]] + reveal_type(generic_context(constrained_by_gradual_list_reverse).specialize_constrained(ConstraintSet.range(Never, T, list[Unrelated]))) + + # TODO: revealed: ty_extensions.Specialization[T@constrained_by_gradual = list[Any]] + # revealed: ty_extensions.Specialization[T@constrained_by_gradual_list_reverse = list[Super]] + reveal_type(generic_context(constrained_by_gradual_list_reverse).specialize_constrained(ConstraintSet.range(Never, T, list[Super]))) + # TODO: revealed: ty_extensions.Specialization[T@constrained_by_gradual = list[Any]] + # revealed: ty_extensions.Specialization[T@constrained_by_gradual_list_reverse = list[Super]] + reveal_type(generic_context(constrained_by_gradual_list_reverse).specialize_constrained(ConstraintSet.range(list[Super], T, list[Super]))) + # TODO: revealed: ty_extensions.Specialization[T@constrained_by_gradual = list[Any]] + # revealed: ty_extensions.Specialization[T@constrained_by_gradual_list_reverse = list[Sub]] + reveal_type(generic_context(constrained_by_gradual_list_reverse).specialize_constrained(ConstraintSet.range(list[Sub], T, list[Sub]))) + def constrained_by_two_gradual_lists[T: (list[Any], list[Any])](): # TODO: revealed: ty_extensions.Specialization[T@constrained_by_gradual = list[Any]] # revealed: ty_extensions.Specialization[T@constrained_by_two_gradual_lists = Top[list[Any]]] diff --git a/crates/ty_python_semantic/src/types/constraints.rs b/crates/ty_python_semantic/src/types/constraints.rs index 91a1770141..4ec970403b 100644 --- a/crates/ty_python_semantic/src/types/constraints.rs +++ b/crates/ty_python_semantic/src/types/constraints.rs @@ -51,6 +51,19 @@ //! the constraint says that the typevar must specialize to that _exact_ type, not to a subtype or //! supertype of it. //! +//! ### Tracing +//! +//! This module is instrumented with debug- and trace-level `tracing` messages. You can set the +//! `TY_LOG` environment variable to see this output when testing locally. `tracing` log messages +//! typically have a `target` field, which is the name of the module the message appears in — in +//! this case, `ty_python_semantic::types::constraints`. We add additional detail to these targets, +//! in case you only want to debug parts of the implementation. For instance, if you want to debug +//! how we construct sequent maps, you could use +//! +//! ```sh +//! env TY_LOG=ty_python_semantic::types::constraints::SequentMap=trace ty check ... +//! ``` +//! //! [bdd]: https://en.wikipedia.org/wiki/Binary_decision_diagram use std::cell::RefCell; @@ -750,9 +763,10 @@ impl<'db> ConstrainedTypeVar<'db> { /// Terminal nodes (`false` and `true`) have their own dedicated enum variants. The /// [`Interior`][InteriorNode] variant represents interior nodes. /// -/// BDD nodes are _reduced_, which means that there are no duplicate nodes (which we handle via -/// Salsa interning), and that there are no redundant nodes, with `if_true` and `if_false` edges -/// that point at the same node. +/// BDD nodes are _quasi-reduced_, which means that there are no duplicate nodes (which we handle +/// via Salsa interning). Unlike the typical BDD representation, which is (fully) reduced, we do +/// allow redundant nodes, with `if_true` and `if_false` edges that point at the same node. That +/// means that our BDDs "remember" all of the individual constraints that they were created with. /// /// BDD nodes are also _ordered_, meaning that every path from the root of a BDD to a terminal node /// visits variables in the same order. [`ConstrainedTypeVar::ordering`] defines the variable @@ -765,7 +779,7 @@ enum Node<'db> { } impl<'db> Node<'db> { - /// Creates a new BDD node, ensuring that it is fully reduced. + /// Creates a new BDD node, ensuring that it is quasi-reduced. fn new( db: &'db dyn Db, constraint: ConstrainedTypeVar<'db>, @@ -780,9 +794,6 @@ impl<'db> Node<'db> { root_constraint.ordering(db) > constraint.ordering(db) }) ); - if if_true == if_false { - return if_true; - } Self::Interior(InteriorNode::new(db, constraint, if_true, if_false)) } @@ -854,7 +865,7 @@ impl<'db> Node<'db> { // impossible paths, and so we treat them as passing the "always satisfied" check. let constraint = interior.constraint(db); let true_always_satisfied = path - .walk_edge(map, constraint.when_true(), |path, _| { + .walk_edge(db, map, constraint.when_true(), |path, _| { interior .if_true(db) .is_always_satisfied_inner(db, map, path) @@ -865,7 +876,7 @@ impl<'db> Node<'db> { } // Ditto for the if_false branch - path.walk_edge(map, constraint.when_false(), |path, _| { + path.walk_edge(db, map, constraint.when_false(), |path, _| { interior .if_false(db) .is_always_satisfied_inner(db, map, path) @@ -903,7 +914,7 @@ impl<'db> Node<'db> { // impossible paths, and so we treat them as passing the "never satisfied" check. let constraint = interior.constraint(db); let true_never_satisfied = path - .walk_edge(map, constraint.when_true(), |path, _| { + .walk_edge(db, map, constraint.when_true(), |path, _| { interior.if_true(db).is_never_satisfied_inner(db, map, path) }) .unwrap_or(true); @@ -912,7 +923,7 @@ impl<'db> Node<'db> { } // Ditto for the if_false branch - path.walk_edge(map, constraint.when_false(), |path, _| { + path.walk_edge(db, map, constraint.when_false(), |path, _| { interior .if_false(db) .is_never_satisfied_inner(db, map, path) @@ -934,8 +945,15 @@ impl<'db> Node<'db> { /// Returns the `or` or union of two BDDs. fn or(self, db: &'db dyn Db, other: Self) -> Self { match (self, other) { - (Node::AlwaysTrue, _) | (_, Node::AlwaysTrue) => Node::AlwaysTrue, + (Node::AlwaysTrue, Node::AlwaysTrue) => Node::AlwaysTrue, (Node::AlwaysFalse, other) | (other, Node::AlwaysFalse) => other, + (Node::AlwaysTrue, Node::Interior(interior)) + | (Node::Interior(interior), Node::AlwaysTrue) => Node::new( + db, + interior.constraint(db), + Node::AlwaysTrue, + Node::AlwaysTrue, + ), (Node::Interior(a), Node::Interior(b)) => { // OR is commutative, which lets us halve the cache requirements let (a, b) = if b.0 < a.0 { (b, a) } else { (a, b) }; @@ -947,8 +965,15 @@ impl<'db> Node<'db> { /// Returns the `and` or intersection of two BDDs. fn and(self, db: &'db dyn Db, other: Self) -> Self { match (self, other) { - (Node::AlwaysFalse, _) | (_, Node::AlwaysFalse) => Node::AlwaysFalse, + (Node::AlwaysFalse, Node::AlwaysFalse) => Node::AlwaysFalse, (Node::AlwaysTrue, other) | (other, Node::AlwaysTrue) => other, + (Node::AlwaysFalse, Node::Interior(interior)) + | (Node::Interior(interior), Node::AlwaysFalse) => Node::new( + db, + interior.constraint(db), + Node::AlwaysFalse, + Node::AlwaysFalse, + ), (Node::Interior(a), Node::Interior(b)) => { // AND is commutative, which lets us halve the cache requirements let (a, b) = if b.0 < a.0 { (b, a) } else { (a, b) }; @@ -1727,7 +1752,7 @@ impl<'db> InteriorNode<'db> { // we're about to remove. If so, we need to "remember" them by AND-ing them in with the // corresponding branch. let if_true = path - .walk_edge(map, self_constraint.when_true(), |path, new_range| { + .walk_edge(db, map, self_constraint.when_true(), |path, new_range| { let branch = self .if_true(db) .abstract_one_inner(db, should_remove, map, path); @@ -1744,7 +1769,7 @@ impl<'db> InteriorNode<'db> { }) .unwrap_or(Node::AlwaysFalse); let if_false = path - .walk_edge(map, self_constraint.when_false(), |path, new_range| { + .walk_edge(db, map, self_constraint.when_false(), |path, new_range| { let branch = self .if_false(db) .abstract_one_inner(db, should_remove, map, path); @@ -1764,13 +1789,13 @@ impl<'db> InteriorNode<'db> { } else { // Otherwise, we abstract the if_false/if_true edges recursively. let if_true = path - .walk_edge(map, self_constraint.when_true(), |path, _| { + .walk_edge(db, map, self_constraint.when_true(), |path, _| { self.if_true(db) .abstract_one_inner(db, should_remove, map, path) }) .unwrap_or(Node::AlwaysFalse); let if_false = path - .walk_edge(map, self_constraint.when_false(), |path, _| { + .walk_edge(db, map, self_constraint.when_false(), |path, _| { self.if_false(db) .abstract_one_inner(db, should_remove, map, path) }) @@ -1820,6 +1845,11 @@ impl<'db> InteriorNode<'db> { heap_size=ruff_memory_usage::heap_size, )] fn sequent_map(self, db: &'db dyn Db) -> SequentMap<'db> { + tracing::debug!( + target: "ty_python_semantic::types::constraints::SequentMap", + constraints = %Node::Interior(self).display(db), + "create sequent map", + ); let mut map = SequentMap::default(); Node::Interior(self).for_each_constraint(db, &mut |constraint| { map.add(db, constraint); @@ -2225,9 +2255,6 @@ impl<'db> ConstraintAssignment<'db> { } } - // Keep this for future debugging needs, even though it's not currently used when rendering - // constraint sets. - #[expect(dead_code)] fn display(self, db: &'db dyn Db) -> impl Display { struct DisplayConstraintAssignment<'db> { constraint: ConstraintAssignment<'db>, @@ -2304,18 +2331,29 @@ impl<'db> SequentMap<'db> { continue; } - // Otherwise, check this constraint against all of the other ones we've seen so far, seeing + // First see if we can create any sequents from the constraint on its own. + tracing::trace!( + target: "ty_python_semantic::types::constraints::SequentMap", + constraint = %constraint.display(db), + "add sequents for constraint", + ); + self.add_sequents_for_single(db, constraint); + + // Then check this constraint against all of the other ones we've seen so far, seeing // if they're related to each other. let processed = std::mem::take(&mut self.processed); for other in &processed { if constraint != *other { + tracing::trace!( + target: "ty_python_semantic::types::constraints::SequentMap", + left = %constraint.display(db), + right = %other.display(db), + "add sequents for constraint pair", + ); self.add_sequents_for_pair(db, constraint, *other); } } self.processed = processed; - - // And see if we can create any sequents from the constraint on its own. - self.add_sequents_for_single(db, constraint); } } @@ -2339,8 +2377,14 @@ impl<'db> SequentMap<'db> { } } - fn add_single_tautology(&mut self, ante: ConstrainedTypeVar<'db>) { - self.single_tautologies.insert(ante); + fn add_single_tautology(&mut self, db: &'db dyn Db, ante: ConstrainedTypeVar<'db>) { + if self.single_tautologies.insert(ante) { + tracing::debug!( + target: "ty_python_semantic::types::constraints::SequentMap", + sequent = %format_args!("¬{} → false", ante.display(db)), + "add sequent", + ); + } } fn add_pair_impossibility( @@ -2349,8 +2393,16 @@ impl<'db> SequentMap<'db> { ante1: ConstrainedTypeVar<'db>, ante2: ConstrainedTypeVar<'db>, ) { - self.pair_impossibilities - .insert(Self::pair_key(db, ante1, ante2)); + if self + .pair_impossibilities + .insert(Self::pair_key(db, ante1, ante2)) + { + tracing::debug!( + target: "ty_python_semantic::types::constraints::SequentMap", + sequent = %format_args!("{} ∧ {} → false", ante1.display(db), ante2.display(db)), + "add sequent", + ); + } } fn add_pair_implication( @@ -2364,24 +2416,50 @@ impl<'db> SequentMap<'db> { if ante1.implies(db, post) || ante2.implies(db, post) { return; } - self.pair_implications + if self + .pair_implications .entry(Self::pair_key(db, ante1, ante2)) .or_default() - .insert(post); + .insert(post) + { + tracing::debug!( + target: "ty_python_semantic::types::constraints::SequentMap", + sequent = %format_args!( + "{} ∧ {} → {}", + ante1.display(db), + ante2.display(db), + post.display(db), + ), + "add sequent", + ); + } } fn add_single_implication( &mut self, + db: &'db dyn Db, ante: ConstrainedTypeVar<'db>, post: ConstrainedTypeVar<'db>, ) { if ante == post { return; } - self.single_implications + if self + .single_implications .entry(ante) .or_default() - .insert(post); + .insert(post) + { + tracing::debug!( + target: "ty_python_semantic::types::constraints::SequentMap", + sequent = %format_args!( + "{} → {}", + ante.display(db), + post.display(db), + ), + "add sequent", + ); + } } fn add_sequents_for_single(&mut self, db: &'db dyn Db, constraint: ConstrainedTypeVar<'db>) { @@ -2390,7 +2468,7 @@ impl<'db> SequentMap<'db> { let lower = constraint.lower(db); let upper = constraint.upper(db); if lower.is_never() && upper.is_object() { - self.add_single_tautology(constraint); + self.add_single_tautology(db, constraint); return; } @@ -2427,7 +2505,7 @@ impl<'db> SequentMap<'db> { _ => return, }; - self.add_single_implication(constraint, post_constraint); + self.add_single_implication(db, constraint, post_constraint); self.enqueue_constraint(post_constraint); } @@ -2589,25 +2667,50 @@ impl<'db> SequentMap<'db> { // elements. (For instance, when processing `T ≤ τ₁ & τ₂` and `T ≤ τ₂ & τ₁`, these clauses // would add sequents for `(T ≤ τ₁ & τ₂) → (T ≤ τ₂ & τ₁)` and vice versa.) if left_constraint.implies(db, right_constraint) { - self.add_single_implication(left_constraint, right_constraint); + tracing::debug!( + target: "ty_python_semantic::types::constraints::SequentMap", + left = %left_constraint.display(db), + right = %right_constraint.display(db), + "left implies right", + ); + self.add_single_implication(db, left_constraint, right_constraint); } if right_constraint.implies(db, left_constraint) { - self.add_single_implication(right_constraint, left_constraint); + tracing::debug!( + target: "ty_python_semantic::types::constraints::SequentMap", + left = %left_constraint.display(db), + right = %right_constraint.display(db), + "right implies left", + ); + self.add_single_implication(db, right_constraint, left_constraint); } match left_constraint.intersect(db, right_constraint) { Some(intersection_constraint) => { + tracing::debug!( + target: "ty_python_semantic::types::constraints::SequentMap", + left = %left_constraint.display(db), + right = %right_constraint.display(db), + intersection = %intersection_constraint.display(db), + "left and right overlap", + ); self.add_pair_implication( db, left_constraint, right_constraint, intersection_constraint, ); - self.add_single_implication(intersection_constraint, left_constraint); - self.add_single_implication(intersection_constraint, right_constraint); + self.add_single_implication(db, intersection_constraint, left_constraint); + self.add_single_implication(db, intersection_constraint, right_constraint); self.enqueue_constraint(intersection_constraint); } None => { + tracing::debug!( + target: "ty_python_semantic::types::constraints::SequentMap", + left = %left_constraint.display(db), + right = %right_constraint.display(db), + "left and right are disjoint", + ); self.add_pair_impossibility(db, left_constraint, right_constraint); } } @@ -2710,6 +2813,7 @@ impl<'db> PathAssignments<'db> { /// the path we're on. fn walk_edge( &mut self, + db: &'db dyn Db, map: &SequentMap<'db>, assignment: ConstraintAssignment<'db>, f: impl FnOnce(&mut Self, Range) -> R, @@ -2720,7 +2824,17 @@ impl<'db> PathAssignments<'db> { let start = self.assignments.len(); // Add the new assignment and anything we can derive from it. - let result = if self.add_assignment(map, assignment).is_err() { + tracing::trace!( + target: "ty_python_semantic::types::constraints::PathAssignment", + before = %format_args!( + "[{}]", + self.assignments[..start].iter().map(|assignment| assignment.display(db)).format(", "), + ), + edge = %assignment.display(db), + "walk edge", + ); + let found_conflict = self.add_assignment(db, map, assignment); + let result = if found_conflict.is_err() { // If that results in the path now being impossible due to a contradiction, return // without invoking the callback. None @@ -2730,6 +2844,14 @@ impl<'db> PathAssignments<'db> { // if that happens, `start..end` will mark the assignments that were added by the // `add_assignment` call above — that is, the new assignment for this edge along with // the derived information we inferred from it. + tracing::trace!( + target: "ty_python_semantic::types::constraints::PathAssignment", + new = %format_args!( + "[{}]", + self.assignments[start..].iter().map(|assignment| assignment.display(db)).format(", "), + ), + "new assignments", + ); let end = self.assignments.len(); Some(f(self, start..end)) }; @@ -2749,12 +2871,22 @@ impl<'db> PathAssignments<'db> { /// to become invalid, due to a contradiction, returns a [`PathAssignmentConflict`] error. fn add_assignment( &mut self, + db: &'db dyn Db, map: &SequentMap<'db>, assignment: ConstraintAssignment<'db>, ) -> Result<(), PathAssignmentConflict> { // First add this assignment. If it causes a conflict, return that as an error. If we've // already know this assignment holds, just return. if self.assignments.contains(&assignment.negated()) { + tracing::trace!( + target: "ty_python_semantic::types::constraints::PathAssignment", + assignment = %assignment.display(db), + facts = %format_args!( + "[{}]", + self.assignments.iter().map(|assignment| assignment.display(db)).format(", "), + ), + "found contradiction", + ); return Err(PathAssignmentConflict); } if !self.assignments.insert(assignment) { @@ -2770,6 +2902,15 @@ impl<'db> PathAssignments<'db> { if self.assignment_holds(ante.when_false()) { // The sequent map says (ante1) is always true, and the current path asserts that // it's false. + tracing::trace!( + target: "ty_python_semantic::types::constraints::PathAssignment", + ante = %ante.display(db), + facts = %format_args!( + "[{}]", + self.assignments.iter().map(|assignment| assignment.display(db)).format(", "), + ), + "found contradiction", + ); return Err(PathAssignmentConflict); } } @@ -2779,6 +2920,16 @@ impl<'db> PathAssignments<'db> { { // The sequent map says (ante1 ∧ ante2) is an impossible combination, and the // current path asserts that both are true. + tracing::trace!( + target: "ty_python_semantic::types::constraints::PathAssignment", + ante1 = %ante1.display(db), + ante2 = %ante2.display(db), + facts = %format_args!( + "[{}]", + self.assignments.iter().map(|assignment| assignment.display(db)).format(", "), + ), + "found contradiction", + ); return Err(PathAssignmentConflict); } } @@ -2788,7 +2939,7 @@ impl<'db> PathAssignments<'db> { if self.assignment_holds(ante1.when_true()) && self.assignment_holds(ante2.when_true()) { - self.add_assignment(map, post.when_true())?; + self.add_assignment(db, map, post.when_true())?; } } } @@ -2796,7 +2947,7 @@ impl<'db> PathAssignments<'db> { for (ante, posts) in &map.single_implications { for post in posts { if self.assignment_holds(ante.when_true()) { - self.add_assignment(map, post.when_true())?; + self.add_assignment(db, map, post.when_true())?; } } } @@ -2885,10 +3036,13 @@ impl<'db> SatisfiedClause<'db> { } fn display(&self, db: &'db dyn Db) -> String { + if self.constraints.is_empty() { + return String::from("always"); + } + // This is a bit heavy-handed, but we need to output the constraints in a consistent order // even though Salsa IDs are assigned non-deterministically. This Display output is only // used in test cases, so we don't need to over-optimize it. - let mut constraints: Vec<_> = self .constraints .iter() @@ -3015,7 +3169,7 @@ impl<'db> SatisfiedClauses<'db> { // used in test cases, so we don't need to over-optimize it. if self.clauses.is_empty() { - return String::from("always"); + return String::from("never"); } let mut clauses: Vec<_> = self .clauses @@ -3120,8 +3274,20 @@ impl<'db> GenericContext<'db> { db: &'db dyn Db, constraints: ConstraintSet<'db>, ) -> Result, ()> { + tracing::debug!( + target: "ty_python_semantic::types::constraints::specialize_constrained", + generic_context = %self.display_full(db), + constraints = %constraints.node.display(db), + "create specialization for constraint set", + ); + // If the constraint set is cyclic, don't even try to construct a specialization. if constraints.is_cyclic(db) { + tracing::error!( + target: "ty_python_semantic::types::constraints::specialize_constrained", + constraints = %constraints.node.display(db), + "constraint set is cyclic", + ); // TODO: Better error return Err(()); } @@ -3134,6 +3300,11 @@ impl<'db> GenericContext<'db> { .fold(constraints.node, |constraints, bound_typevar| { constraints.and(db, bound_typevar.valid_specializations(db)) }); + tracing::debug!( + target: "ty_python_semantic::types::constraints::specialize_constrained", + valid = %abstracted.display(db), + "limited to valid specializations", + ); // Then we find all of the "representative types" for each typevar in the constraint set. let mut error_occurred = false; @@ -3152,10 +3323,24 @@ impl<'db> GenericContext<'db> { let mut unconstrained = false; let mut greatest_lower_bound = Type::Never; let mut least_upper_bound = Type::object(); - abstracted.find_representative_types(db, bound_typevar.identity(db), |bounds| { + let identity = bound_typevar.identity(db); + tracing::trace!( + target: "ty_python_semantic::types::constraints::specialize_constrained", + bound_typevar = %identity.display(db), + abstracted = %abstracted.retain_one(db, identity).display(db), + "find specialization for typevar", + ); + abstracted.find_representative_types(db, identity, |bounds| { satisfied = true; match bounds { Some((lower_bound, upper_bound)) => { + tracing::trace!( + target: "ty_python_semantic::types::constraints::specialize_constrained", + bound_typevar = %identity.display(db), + lower_bound = %lower_bound.display(db), + upper_bound = %upper_bound.display(db), + "found representative type", + ); greatest_lower_bound = UnionType::from_elements(db, [greatest_lower_bound, lower_bound]); least_upper_bound = @@ -3171,6 +3356,11 @@ impl<'db> GenericContext<'db> { // for this constraint set. if !satisfied { // TODO: Construct a useful error here + tracing::debug!( + target: "ty_python_semantic::types::constraints::specialize_constrained", + bound_typevar = %identity.display(db), + "typevar cannot be satisfied", + ); error_occurred = true; return None; } @@ -3178,18 +3368,36 @@ impl<'db> GenericContext<'db> { // The BDD is satisfiable, but the typevar is unconstrained, then we use `None` to tell // specialize_recursive to fall back on the typevar's default. if unconstrained { + tracing::debug!( + target: "ty_python_semantic::types::constraints::specialize_constrained", + bound_typevar = %identity.display(db), + "typevar is unconstrained", + ); return None; } // If `lower ≰ upper`, then there is no type that satisfies all of the paths in the // BDD. That's an ambiguous specialization, as described above. if !greatest_lower_bound.is_subtype_of(db, least_upper_bound) { + tracing::debug!( + target: "ty_python_semantic::types::constraints::specialize_constrained", + bound_typevar = %identity.display(db), + greatest_lower_bound = %greatest_lower_bound.display(db), + least_upper_bound = %least_upper_bound.display(db), + "typevar bounds are incompatible", + ); error_occurred = true; return None; } // Of all of the types that satisfy all of the paths in the BDD, we choose the // "largest" one (i.e., "closest to `object`") as the specialization. + tracing::debug!( + target: "ty_python_semantic::types::constraints::specialize_constrained", + bound_typevar = %identity.display(db), + specialization = %least_upper_bound.display(db), + "found specialization for typevar", + ); Some(least_upper_bound) }); @@ -3215,18 +3423,32 @@ mod tests { fn test_display_graph_output() { let expected = indoc! {r#" (T = str) - ┡━₁ (U = str) - │ ┡━₁ always - │ └─₀ (U = bool) - │ ┡━₁ always - │ └─₀ never + ┡━₁ (T = bool) + │ ┡━₁ (U = str) + │ │ ┡━₁ (U = bool) + │ │ │ ┡━₁ always + │ │ │ └─₀ always + │ │ └─₀ (U = bool) + │ │ ┡━₁ always + │ │ └─₀ never + │ └─₀ (U = str) + │ ┡━₁ (U = bool) + │ │ ┡━₁ always + │ │ └─₀ always + │ └─₀ (U = bool) + │ ┡━₁ always + │ └─₀ never └─₀ (T = bool) ┡━₁ (U = str) - │ ┡━₁ always + │ ┡━₁ (U = bool) + │ │ ┡━₁ always + │ │ └─₀ always │ └─₀ (U = bool) │ ┡━₁ always │ └─₀ never - └─₀ never + └─₀ (U = str) + ┡━₁ never + └─₀ never "#} .trim_end(); From d6e472f29751a5703c089417ff51641b03ddef4b Mon Sep 17 00:00:00 2001 From: David Peter Date: Wed, 3 Dec 2025 16:40:11 +0100 Subject: [PATCH 10/10] [ty] Reachability constraints: minor documentation fixes (#21774) --- .../resources/mdtest/import/star.md | 2 +- .../src/semantic_index/predicate.rs | 24 +++++++++---------- .../reachability_constraints.rs | 10 ++++---- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/crates/ty_python_semantic/resources/mdtest/import/star.md b/crates/ty_python_semantic/resources/mdtest/import/star.md index adef7f91d3..54d2050259 100644 --- a/crates/ty_python_semantic/resources/mdtest/import/star.md +++ b/crates/ty_python_semantic/resources/mdtest/import/star.md @@ -715,7 +715,7 @@ reveal_type(Y) # revealed: Unknown # The `*` import is not considered a redefinition # of the global variable `Z` in this module, as the symbol in -# the `a` module is in a branch that is statically known +# the `exporter` module is in a branch that is statically known # to be dead code given the `python-version` configuration. # Thus this still reveals `Literal[True]`. reveal_type(Z) # revealed: Literal[True] diff --git a/crates/ty_python_semantic/src/semantic_index/predicate.rs b/crates/ty_python_semantic/src/semantic_index/predicate.rs index 7f96c13630..abefcc34b4 100644 --- a/crates/ty_python_semantic/src/semantic_index/predicate.rs +++ b/crates/ty_python_semantic/src/semantic_index/predicate.rs @@ -166,10 +166,10 @@ impl<'db> PatternPredicate<'db> { } } -/// A "placeholder predicate" that is used to model the fact that the boundness of a -/// (possible) definition or declaration caused by a `*` import cannot be fully determined -/// until type-inference time. This is essentially the same as a standard reachability constraint, -/// so we reuse the [`Predicate`] infrastructure to model it. +/// A "placeholder predicate" that is used to model the fact that the boundness of a (possible) +/// definition or declaration caused by a `*` import cannot be fully determined until type- +/// inference time. This is essentially the same as a standard reachability constraint, so we reuse +/// the [`Predicate`] infrastructure to model it. /// /// To illustrate, say we have a module `exporter.py` like so: /// @@ -183,14 +183,14 @@ impl<'db> PatternPredicate<'db> { /// ```py /// A = 1 /// -/// from importer import * +/// from exporter import * /// ``` /// -/// Since we cannot know whether or not is true at semantic-index time, -/// we record a definition for `A` in `b.py` as a result of the `from a import *` -/// statement, but place a predicate on it to record the fact that we don't yet -/// know whether this definition will be visible from all control-flow paths or not. -/// Essentially, we model `b.py` as something similar to this: +/// Since we cannot know whether or not is true at semantic-index time, we record +/// a definition for `A` in `importer.py` as a result of the `from exporter import *` statement, +/// but place a predicate on it to record the fact that we don't yet know whether this definition +/// will be visible from all control-flow paths or not. Essentially, we model `importer.py` as +/// something similar to this: /// /// ```py /// A = 1 @@ -199,8 +199,8 @@ impl<'db> PatternPredicate<'db> { /// from a import A /// ``` /// -/// At type-check time, the placeholder predicate for the `A` definition is evaluated by -/// attempting to resolve the `A` symbol in `a.py`'s global namespace: +/// At type-check time, the placeholder predicate for the `A` definition is evaluated by attempting +/// to resolve the `A` symbol in `exporter.py`'s global namespace: /// - If it resolves to a definitely bound symbol, then the predicate resolves to [`Truthiness::AlwaysTrue`] /// - If it resolves to an unbound symbol, then the predicate resolves to [`Truthiness::AlwaysFalse`] /// - If it resolves to a possibly bound symbol, then the predicate resolves to [`Truthiness::Ambiguous`] diff --git a/crates/ty_python_semantic/src/semantic_index/reachability_constraints.rs b/crates/ty_python_semantic/src/semantic_index/reachability_constraints.rs index 76ec1a70f0..9da8bbe87a 100644 --- a/crates/ty_python_semantic/src/semantic_index/reachability_constraints.rs +++ b/crates/ty_python_semantic/src/semantic_index/reachability_constraints.rs @@ -3,7 +3,7 @@ //! During semantic index building, we record so-called reachability constraints that keep track //! of a set of conditions that need to apply in order for a certain statement or expression to //! be reachable from the start of the scope. As an example, consider the following situation where -//! we have just processed two `if`-statements: +//! we have just processed an `if`-statement: //! ```py //! if test: //! @@ -101,13 +101,13 @@ //! //! ``` //! If we would not record any constraints at the branching point, we would have an `always-true` -//! reachability for the no-loop branch, and a `always-false` reachability for the branch which enters -//! the loop. Merging those would lead to a reachability of `always-true OR always-false = always-true`, +//! reachability for the no-loop branch, and a `always-true` reachability for the branch which enters +//! the loop. Merging those would lead to a reachability of `always-true OR always-true = always-true`, //! i.e. we would consider the end of the scope to be unconditionally reachable, which is not correct. //! //! Recording an ambiguous constraint at the branching point modifies the constraints in both branches to -//! `always-true AND ambiguous = ambiguous` and `always-false AND ambiguous = always-false`, respectively. -//! Merging these two using OR correctly leads to `ambiguous` for the end-of-scope reachability. +//! `always-true AND ambiguous = ambiguous`. Merging these two using OR correctly leads to `ambiguous` for +//! the end-of-scope reachability. //! //! //! ## Reachability constraints and bindings