From d5ef0d3e265f8049401b184cf98dc4cc2acbdf33 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Wed, 23 Apr 2025 12:20:16 -0500 Subject: [PATCH 01/12] [red-knot] Check for invalid overload usages --- .../resources/mdtest/overloads.md | 12 +- ...ds_-_Invalid_-_At_least_two_overloads.snap | 57 +++++++++ crates/red_knot_python_semantic/src/types.rs | 4 + .../src/types/diagnostic.rs | 44 +++++++ .../src/types/infer.rs | 111 +++++++++++++++++- knot.schema.json | 10 ++ 6 files changed, 233 insertions(+), 5 deletions(-) create mode 100644 crates/red_knot_python_semantic/resources/mdtest/snapshots/overloads.md_-_Overloads_-_Invalid_-_At_least_two_overloads.snap diff --git a/crates/red_knot_python_semantic/resources/mdtest/overloads.md b/crates/red_knot_python_semantic/resources/mdtest/overloads.md index 764b6a44123ed..b4a5955ec71da 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/overloads.md +++ b/crates/red_knot_python_semantic/resources/mdtest/overloads.md @@ -309,16 +309,22 @@ reveal_type(func("")) # revealed: Literal[""] ### At least two overloads + + At least two `@overload`-decorated definitions must be present. ```py from typing import overload -# TODO: error @overload -def func(x: int) -> int: ... -def func(x: int | str) -> int | str: +def func1(x: int) -> int: ... +# error: [invalid-overload] +def func1(x: int | str) -> int | str: return x + +@overload +# error: [invalid-overload] +def func2(x: int) -> int: ... ``` ### Overload without an implementation diff --git a/crates/red_knot_python_semantic/resources/mdtest/snapshots/overloads.md_-_Overloads_-_Invalid_-_At_least_two_overloads.snap b/crates/red_knot_python_semantic/resources/mdtest/snapshots/overloads.md_-_Overloads_-_Invalid_-_At_least_two_overloads.snap new file mode 100644 index 0000000000000..c015a77290550 --- /dev/null +++ b/crates/red_knot_python_semantic/resources/mdtest/snapshots/overloads.md_-_Overloads_-_Invalid_-_At_least_two_overloads.snap @@ -0,0 +1,57 @@ +--- +source: crates/red_knot_test/src/lib.rs +expression: snapshot +--- +--- +mdtest name: overloads.md - Overloads - Invalid - At least two overloads +mdtest path: crates/red_knot_python_semantic/resources/mdtest/overloads.md +--- + +# Python source files + +## mdtest_snippet.py + +``` + 1 | from typing import overload + 2 | + 3 | @overload + 4 | def func1(x: int) -> int: ... + 5 | # error: [invalid-overload] + 6 | def func1(x: int | str) -> int | str: + 7 | return x + 8 | + 9 | @overload +10 | # error: [invalid-overload] +11 | def func2(x: int) -> int: ... +``` + +# Diagnostics + +``` +error: lint:invalid-overload: Function `func1` requires at least two overloads + --> /src/mdtest_snippet.py:4:5 + | +3 | @overload +4 | def func1(x: int) -> int: ... + | ----- Only one overload defined here +5 | # error: [invalid-overload] +6 | def func1(x: int | str) -> int | str: + | ^^^^^ +7 | return x + | + +``` + +``` +error: lint:invalid-overload: Function `func2` requires at least two overloads + --> /src/mdtest_snippet.py:11:5 + | + 9 | @overload +10 | # error: [invalid-overload] +11 | def func2(x: int) -> int: ... + | ----- + | | + | Only one overload defined here + | + +``` diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index 335e4d8e1c13f..5abfb2a191410 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -6396,6 +6396,10 @@ impl<'db> FunctionType<'db> { Type::BoundMethod(BoundMethodType::new(db, self, self_instance)) } + pub(crate) fn node(self, db: &'db dyn Db) -> &'db ast::StmtFunctionDef { + self.body_scope(db).node(db).expect_function() + } + /// Returns the [`FileRange`] of the function's name. pub fn focus_range(self, db: &dyn Db) -> FileRange { FileRange::new( diff --git a/crates/red_knot_python_semantic/src/types/diagnostic.rs b/crates/red_knot_python_semantic/src/types/diagnostic.rs index 63d5a18e4baef..f56300489085d 100644 --- a/crates/red_knot_python_semantic/src/types/diagnostic.rs +++ b/crates/red_knot_python_semantic/src/types/diagnostic.rs @@ -37,6 +37,7 @@ pub(crate) fn register_lints(registry: &mut LintRegistryBuilder) { registry.register_lint(&INVALID_EXCEPTION_CAUGHT); registry.register_lint(&INVALID_LEGACY_TYPE_VARIABLE); registry.register_lint(&INVALID_METACLASS); + registry.register_lint(&INVALID_OVERLOAD); registry.register_lint(&INVALID_PARAMETER_DEFAULT); registry.register_lint(&INVALID_PROTOCOL); registry.register_lint(&INVALID_RAISE); @@ -447,6 +448,49 @@ declare_lint! { } } +declare_lint! { + /// ## What it does + /// Checks for various invalid `@overload` usages. + /// + /// ## Why is this bad? + /// The `@overload` decorator is used to define functions and methods that accepts different + /// combinations of arguments and return different types based on the arguments passed. This is + /// mainly beneficial for type checkers. But, if the `@overload` usage is invalid, the type + /// checker may not be able to provide correct type information. + /// + /// ## Example + /// + /// Defining only one overload: + /// + /// ```py + /// from typing import overload + /// + /// @overload + /// def foo(x: int) -> int: ... + /// def foo(x: int | None) -> int | None: + /// return x + /// ``` + /// + /// Or, not providing an implementation for the overloaded definition: + /// + /// ```py + /// from typing import overload + /// + /// @overload + /// def foo() -> None: ... + /// @overload + /// def foo(x: int) -> int: ... + /// ``` + /// + /// ## References + /// - [Python documentation: `@overload`](https://docs.python.org/3/library/typing.html#typing.overload) + pub(crate) static INVALID_OVERLOAD = { + summary: "detects invalid `@overload` usages", + status: LintStatus::preview("1.0.0"), + default_level: Level::Error, + } +} + declare_lint! { /// ## What it does /// Checks for default values that can't be assigned to the parameter's annotated type. diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index db5b2a53197f0..bafddf497d7dc 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -101,8 +101,8 @@ use super::diagnostic::{ report_invalid_exception_raised, report_invalid_type_checking_constant, report_non_subscriptable, report_possibly_unresolved_reference, report_runtime_check_against_non_runtime_checkable_protocol, report_slice_step_size_zero, - report_unresolved_reference, INVALID_METACLASS, INVALID_PROTOCOL, REDUNDANT_CAST, - STATIC_ASSERT_ERROR, SUBCLASS_OF_FINAL_CLASS, TYPE_ASSERTION_FAILURE, + report_unresolved_reference, INVALID_METACLASS, INVALID_OVERLOAD, INVALID_PROTOCOL, + REDUNDANT_CAST, STATIC_ASSERT_ERROR, SUBCLASS_OF_FINAL_CLASS, TYPE_ASSERTION_FAILURE, }; use super::slots::check_class_slots; use super::string_annotation::{ @@ -524,6 +524,29 @@ pub(super) struct TypeInferenceBuilder<'db> { /// The returned types and their corresponding ranges of the region, if it is a function body. return_types_and_ranges: Vec>, + /// A set of functions that have been defined **and** called in this region. + /// + /// This is a set because the same function could be called multiple times in the same region. + /// This is mainly used in [`check_overloaded_functions`] to check an overloaded function that + /// is shadowed by a function with the same name in this scope but has been called before. For + /// example: + /// + /// ```py + /// from typing import overload + /// + /// @overload + /// def foo() -> None: ... + /// @overload + /// def foo(x: int) -> int: ... + /// def foo(x: int | None) -> int | None: return x + /// + /// foo() # An overloaded function that was defined in this scope have been called + /// + /// def foo(x: int) -> int: + /// return x + /// ``` + called_functions: FxHashSet>, + /// The deferred state of inferring types of certain expressions within the region. /// /// This is different from [`InferenceRegion::Deferred`] which works on the entire definition @@ -556,6 +579,7 @@ impl<'db> TypeInferenceBuilder<'db> { index, region, return_types_and_ranges: vec![], + called_functions: FxHashSet::default(), deferred_state: DeferredExpressionState::None, types: TypeInference::empty(scope), } @@ -718,6 +742,7 @@ impl<'db> TypeInferenceBuilder<'db> { // TODO: Only call this function when diagnostics are enabled. self.check_class_definitions(); + self.check_overloaded_functions(); } /// Iterate over all class definitions to check that the definition will not cause an exception @@ -952,6 +977,82 @@ impl<'db> TypeInferenceBuilder<'db> { } } + /// Check the overloaded functions in this scope. + /// + /// This only checks the overloaded functions that are: + /// 1. Visible publicly at the end of this scope + /// 2. Or, defined and called in this scope + /// + /// For (1), this has the consequence of not checking an overloaded function that is being + /// shadowed by another function with the same name in this scope. + fn check_overloaded_functions(&mut self) { + let mut functions_to_check = self.called_functions.clone(); + + // Collect all the unique overloaded function symbols in this scope. This requires a set + // because an overloaded function uses the same symbol for each of the overloads and the + // implementation. + let overloaded_function_symbols: FxHashSet<_> = self + .types + .declarations + .iter() + .filter_map(|(definition, ty)| { + // Filter out function literals that result from anything other than a function + // definition e.g., imports. + if !matches!(definition.kind(self.db()), DefinitionKind::Function(_)) { + return None; + } + let function = ty.inner_type().into_function_literal()?; + if function.has_known_decorator(self.db(), FunctionDecorators::OVERLOAD) { + Some(definition.symbol(self.db())) + } else { + None + } + }) + .collect(); + + let use_def = self + .index + .use_def_map(self.scope().file_scope_id(self.db())); + + for symbol in overloaded_function_symbols { + if let Symbol::Type(Type::FunctionLiteral(function), Boundness::Bound) = + symbol_from_bindings(self.db(), use_def.public_bindings(symbol)) + { + // Extend the functions that we need to check with the publicly visible overloaded + // function. This is always going to be either the implementation or the last + // overload if the implementation doesn't exists. + functions_to_check.insert(function); + } + } + + for function in functions_to_check { + let Some(overloaded) = function.to_overloaded(self.db()) else { + continue; + }; + + let function_node = function.node(self.db()); + + if overloaded.overloads.len() < 2 { + if let Some(builder) = self + .context + .report_lint(&INVALID_OVERLOAD, &function_node.name) + { + let mut diagnostic = builder.into_diagnostic(format_args!( + "Function `{}` requires at least two overloads", + &function_node.name + )); + if let Some(first_overload) = overloaded.overloads.first() { + diagnostic.annotate( + self.context + .secondary(first_overload.focus_range(self.db())) + .message(format_args!("Only one overload defined here")), + ); + } + } + } + } + } + fn infer_region_definition(&mut self, definition: Definition<'db>) { match definition.kind(self.db()) { DefinitionKind::Function(function) => { @@ -4298,6 +4399,12 @@ impl<'db> TypeInferenceBuilder<'db> { let mut call_arguments = Self::parse_arguments(arguments); let callable_type = self.infer_expression(func); + if let Type::FunctionLiteral(function) = callable_type { + if function.definition(self.db()).scope(self.db()) == self.scope() { + self.called_functions.insert(function); + } + } + // It might look odd here that we emit an error for class-literals but not `type[]` types. // But it's deliberate! The typing spec explicitly mandates that `type[]` types can be called // even though class-literals cannot. This is because even though a protocol class `SomeProtocol` diff --git a/knot.schema.json b/knot.schema.json index 66e0a5b2e304a..aeda31fd530bc 100644 --- a/knot.schema.json +++ b/knot.schema.json @@ -470,6 +470,16 @@ } ] }, + "invalid-overload": { + "title": "detects invalid `@overload` usages", + "description": "## What it does\nChecks for various invalid `@overload` usages.\n\n## Why is this bad?\nThe `@overload` decorator is used to define functions and methods that accepts different\ncombinations of arguments and return different types based on the arguments passed. This is\nmainly beneficial for type checkers. But, if the `@overload` usage is invalid, the type\nchecker may not be able to provide correct type information.\n\n## Example\n\nDefining only one overload:\n\n```py\nfrom typing import overload\n\n@overload\ndef foo(x: int) -> int: ...\ndef foo(x: int | None) -> int | None:\n return x\n```\n\nOr, not providing an implementation for the overloaded definition:\n\n```py\nfrom typing import overload\n\n@overload\ndef foo() -> None: ...\n@overload\ndef foo(x: int) -> int: ...\n```\n\n## References\n- [Python documentation: `@overload`](https://docs.python.org/3/library/typing.html#typing.overload)", + "default": "error", + "oneOf": [ + { + "$ref": "#/definitions/Level" + } + ] + }, "invalid-parameter-default": { "title": "detects default values that can't be assigned to the parameter's annotated type", "description": "## What it does\nChecks for default values that can't be assigned to the parameter's annotated type.\n\n## Why is this bad?\nTODO #14889", From d4f485f58291463b5ad50e8ea0b02f0f29d56ebf Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Thu, 24 Apr 2025 19:13:52 -0500 Subject: [PATCH 02/12] Avoid cross-module AST dependency --- crates/red_knot_python_semantic/src/types/infer.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index bafddf497d7dc..f4ced65eac04e 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -997,7 +997,7 @@ impl<'db> TypeInferenceBuilder<'db> { .iter() .filter_map(|(definition, ty)| { // Filter out function literals that result from anything other than a function - // definition e.g., imports. + // definition e.g., imports which would create a cross-module AST dependency. if !matches!(definition.kind(self.db()), DefinitionKind::Function(_)) { return None; } @@ -1030,9 +1030,8 @@ impl<'db> TypeInferenceBuilder<'db> { continue; }; - let function_node = function.node(self.db()); - if overloaded.overloads.len() < 2 { + let function_node = function.node(self.db()); if let Some(builder) = self .context .report_lint(&INVALID_OVERLOAD, &function_node.name) @@ -4400,7 +4399,13 @@ impl<'db> TypeInferenceBuilder<'db> { let callable_type = self.infer_expression(func); if let Type::FunctionLiteral(function) = callable_type { - if function.definition(self.db()).scope(self.db()) == self.scope() { + // Make sure that the `function.definition` is only called when the function is defined + // in the same file as the one we're currently inferring the types for. This is because + // the `definition` method accesses the semantic index, which could create a + // cross-module AST dependency. + if function.body_scope(self.db()).file(self.db()) == self.file() + && function.definition(self.db()).scope(self.db()) == self.scope() + { self.called_functions.insert(function); } } From 8c05f1f248e8564c65761f6d4d7ab81012b07b92 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Thu, 24 Apr 2025 19:26:48 -0500 Subject: [PATCH 03/12] Fix pre-commit --- .../resources/mdtest/overloads.md | 1 + ...ds_-_Invalid_-_At_least_two_overloads.snap | 30 ++++++++++--------- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/crates/red_knot_python_semantic/resources/mdtest/overloads.md b/crates/red_knot_python_semantic/resources/mdtest/overloads.md index b4a5955ec71da..fcf08390290f8 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/overloads.md +++ b/crates/red_knot_python_semantic/resources/mdtest/overloads.md @@ -318,6 +318,7 @@ from typing import overload @overload def func1(x: int) -> int: ... + # error: [invalid-overload] def func1(x: int | str) -> int | str: return x diff --git a/crates/red_knot_python_semantic/resources/mdtest/snapshots/overloads.md_-_Overloads_-_Invalid_-_At_least_two_overloads.snap b/crates/red_knot_python_semantic/resources/mdtest/snapshots/overloads.md_-_Overloads_-_Invalid_-_At_least_two_overloads.snap index c015a77290550..9618364806c61 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/snapshots/overloads.md_-_Overloads_-_Invalid_-_At_least_two_overloads.snap +++ b/crates/red_knot_python_semantic/resources/mdtest/snapshots/overloads.md_-_Overloads_-_Invalid_-_At_least_two_overloads.snap @@ -16,13 +16,14 @@ mdtest path: crates/red_knot_python_semantic/resources/mdtest/overloads.md 2 | 3 | @overload 4 | def func1(x: int) -> int: ... - 5 | # error: [invalid-overload] - 6 | def func1(x: int | str) -> int | str: - 7 | return x - 8 | - 9 | @overload -10 | # error: [invalid-overload] -11 | def func2(x: int) -> int: ... + 5 | + 6 | # error: [invalid-overload] + 7 | def func1(x: int | str) -> int | str: + 8 | return x + 9 | +10 | @overload +11 | # error: [invalid-overload] +12 | def func2(x: int) -> int: ... ``` # Diagnostics @@ -34,21 +35,22 @@ error: lint:invalid-overload: Function `func1` requires at least two overloads 3 | @overload 4 | def func1(x: int) -> int: ... | ----- Only one overload defined here -5 | # error: [invalid-overload] -6 | def func1(x: int | str) -> int | str: +5 | +6 | # error: [invalid-overload] +7 | def func1(x: int | str) -> int | str: | ^^^^^ -7 | return x +8 | return x | ``` ``` error: lint:invalid-overload: Function `func2` requires at least two overloads - --> /src/mdtest_snippet.py:11:5 + --> /src/mdtest_snippet.py:12:5 | - 9 | @overload -10 | # error: [invalid-overload] -11 | def func2(x: int) -> int: ... +10 | @overload +11 | # error: [invalid-overload] +12 | def func2(x: int) -> int: ... | ----- | | | Only one overload defined here From b7b0e22e5c3c59e20ee8728397a9626bcb57f135 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Thu, 24 Apr 2025 19:30:59 -0500 Subject: [PATCH 04/12] Fix docs --- crates/red_knot_python_semantic/src/types/infer.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index f4ced65eac04e..33f1593ca2268 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -545,6 +545,8 @@ pub(super) struct TypeInferenceBuilder<'db> { /// def foo(x: int) -> int: /// return x /// ``` + /// + /// [`check_overloaded_functions`]: TypeInferenceBuilder::check_overloaded_functions called_functions: FxHashSet>, /// The deferred state of inferring types of certain expressions within the region. From 9c446b274c8980b9b7ecc3c57b804091392a0f4a Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Fri, 25 Apr 2025 08:56:51 -0500 Subject: [PATCH 05/12] Use union instead of cloning --- crates/red_knot_python_semantic/src/types/infer.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 33f1593ca2268..794fce1e657ef 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -988,8 +988,6 @@ impl<'db> TypeInferenceBuilder<'db> { /// For (1), this has the consequence of not checking an overloaded function that is being /// shadowed by another function with the same name in this scope. fn check_overloaded_functions(&mut self) { - let mut functions_to_check = self.called_functions.clone(); - // Collect all the unique overloaded function symbols in this scope. This requires a set // because an overloaded function uses the same symbol for each of the overloads and the // implementation. @@ -1016,6 +1014,8 @@ impl<'db> TypeInferenceBuilder<'db> { .index .use_def_map(self.scope().file_scope_id(self.db())); + let mut public_functions = FxHashSet::default(); + for symbol in overloaded_function_symbols { if let Symbol::Type(Type::FunctionLiteral(function), Boundness::Bound) = symbol_from_bindings(self.db(), use_def.public_bindings(symbol)) @@ -1023,17 +1023,17 @@ impl<'db> TypeInferenceBuilder<'db> { // Extend the functions that we need to check with the publicly visible overloaded // function. This is always going to be either the implementation or the last // overload if the implementation doesn't exists. - functions_to_check.insert(function); + public_functions.insert(function); } } - for function in functions_to_check { + for function in self.called_functions.union(&public_functions) { let Some(overloaded) = function.to_overloaded(self.db()) else { continue; }; if overloaded.overloads.len() < 2 { - let function_node = function.node(self.db()); + let function_node = function.node(self.db(), self.file()); if let Some(builder) = self .context .report_lint(&INVALID_OVERLOAD, &function_node.name) From d6a72272fc48fed5a196a0c4dbfb3b3704da56aa Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Fri, 25 Apr 2025 08:57:00 -0500 Subject: [PATCH 06/12] Add some docs --- crates/red_knot_python_semantic/src/types.rs | 25 ++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index 5abfb2a191410..721625abaadea 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -6396,8 +6396,22 @@ impl<'db> FunctionType<'db> { Type::BoundMethod(BoundMethodType::new(db, self, self_instance)) } - pub(crate) fn node(self, db: &'db dyn Db) -> &'db ast::StmtFunctionDef { - self.body_scope(db).node(db).expect_function() + /// Returns the AST node for this function. + /// + /// # Panics + /// + /// Panics if the file passed in does not match the one where this function is defined. + pub(crate) fn node(self, db: &'db dyn Db, file: File) -> &'db ast::StmtFunctionDef { + let body_scope = self.body_scope(db); + + assert_eq!( + file, + body_scope.file(db), + "FunctionType::node() must be called with the same file as the one where \ + the function is defined." + ); + + body_scope.node(db).expect_function() } /// Returns the [`FileRange`] of the function's name. @@ -6415,6 +6429,13 @@ impl<'db> FunctionType<'db> { ) } + /// Returns the [`Definition`] of this function. + /// + /// ## Warning + /// + /// This uses the semantic index to find the definition of the function. This means that if the + /// calling query is not in the same file as this function is defined in, then this will create + /// a cross-module dependency which might lead to cache invalidation. pub(crate) fn definition(self, db: &'db dyn Db) -> Definition<'db> { let body_scope = self.body_scope(db); let index = semantic_index(db, body_scope.file(db)); From 735e5ec7087c8a750799be5be517bfc28c8b112d Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Fri, 25 Apr 2025 17:27:23 -0500 Subject: [PATCH 07/12] Address review comments --- ...d_-_Overloads_-_Invalid_-_At_least_two_overloads.snap | 4 ++-- crates/red_knot_python_semantic/src/types.rs | 9 +++------ crates/red_knot_python_semantic/src/types/infer.rs | 2 +- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/crates/red_knot_python_semantic/resources/mdtest/snapshots/overloads.md_-_Overloads_-_Invalid_-_At_least_two_overloads.snap b/crates/red_knot_python_semantic/resources/mdtest/snapshots/overloads.md_-_Overloads_-_Invalid_-_At_least_two_overloads.snap index 9618364806c61..a1660a89237ba 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/snapshots/overloads.md_-_Overloads_-_Invalid_-_At_least_two_overloads.snap +++ b/crates/red_knot_python_semantic/resources/mdtest/snapshots/overloads.md_-_Overloads_-_Invalid_-_At_least_two_overloads.snap @@ -29,7 +29,7 @@ mdtest path: crates/red_knot_python_semantic/resources/mdtest/overloads.md # Diagnostics ``` -error: lint:invalid-overload: Function `func1` requires at least two overloads +error: lint:invalid-overload: Overloaded function `func1` requires at least two overloads --> /src/mdtest_snippet.py:4:5 | 3 | @overload @@ -45,7 +45,7 @@ error: lint:invalid-overload: Function `func1` requires at least two overloads ``` ``` -error: lint:invalid-overload: Function `func2` requires at least two overloads +error: lint:invalid-overload: Overloaded function `func2` requires at least two overloads --> /src/mdtest_snippet.py:12:5 | 10 | @overload diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index 721625abaadea..7ea0adaa0787d 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -6397,14 +6397,10 @@ impl<'db> FunctionType<'db> { } /// Returns the AST node for this function. - /// - /// # Panics - /// - /// Panics if the file passed in does not match the one where this function is defined. pub(crate) fn node(self, db: &'db dyn Db, file: File) -> &'db ast::StmtFunctionDef { let body_scope = self.body_scope(db); - assert_eq!( + debug_assert_eq!( file, body_scope.file(db), "FunctionType::node() must be called with the same file as the one where \ @@ -6435,7 +6431,8 @@ impl<'db> FunctionType<'db> { /// /// This uses the semantic index to find the definition of the function. This means that if the /// calling query is not in the same file as this function is defined in, then this will create - /// a cross-module dependency which might lead to cache invalidation. + /// a cross-module dependency directly on the full AST which will lead to cache + /// over-invalidation. pub(crate) fn definition(self, db: &'db dyn Db) -> Definition<'db> { let body_scope = self.body_scope(db); let index = semantic_index(db, body_scope.file(db)); diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 794fce1e657ef..45be975f7926f 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -1039,7 +1039,7 @@ impl<'db> TypeInferenceBuilder<'db> { .report_lint(&INVALID_OVERLOAD, &function_node.name) { let mut diagnostic = builder.into_diagnostic(format_args!( - "Function `{}` requires at least two overloads", + "Overloaded function `{}` requires at least two overloads", &function_node.name )); if let Some(first_overload) = overloaded.overloads.first() { From 31b590cc07b7befce9e3a74aec8e4bc43d14162f Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Sun, 27 Apr 2025 16:58:44 -0500 Subject: [PATCH 08/12] Use `.as_slice` --- crates/red_knot_python_semantic/src/types/infer.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 45be975f7926f..c4fe257f1de12 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -1032,7 +1032,7 @@ impl<'db> TypeInferenceBuilder<'db> { continue; }; - if overloaded.overloads.len() < 2 { + if let [single_overload] = overloaded.overloads.as_slice() { let function_node = function.node(self.db(), self.file()); if let Some(builder) = self .context @@ -1042,13 +1042,11 @@ impl<'db> TypeInferenceBuilder<'db> { "Overloaded function `{}` requires at least two overloads", &function_node.name )); - if let Some(first_overload) = overloaded.overloads.first() { - diagnostic.annotate( - self.context - .secondary(first_overload.focus_range(self.db())) - .message(format_args!("Only one overload defined here")), - ); - } + diagnostic.annotate( + self.context + .secondary(single_overload.focus_range(self.db())) + .message(format_args!("Only one overload defined here")), + ); } } } From 749cf382c4c50e2c56abf27f26762bfdc38d397e Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Sun, 27 Apr 2025 18:12:41 -0500 Subject: [PATCH 09/12] Move non-implementation snippet to stub file --- .../resources/mdtest/overloads.md | 10 +++- ...ds_-_Invalid_-_At_least_two_overloads.snap | 60 ++++++++++--------- .../src/types/infer.rs | 5 +- 3 files changed, 43 insertions(+), 32 deletions(-) diff --git a/crates/red_knot_python_semantic/resources/mdtest/overloads.md b/crates/red_knot_python_semantic/resources/mdtest/overloads.md index fcf08390290f8..4bbef31b1d735 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/overloads.md +++ b/crates/red_knot_python_semantic/resources/mdtest/overloads.md @@ -317,15 +317,19 @@ At least two `@overload`-decorated definitions must be present. from typing import overload @overload -def func1(x: int) -> int: ... +def func(x: int) -> int: ... # error: [invalid-overload] -def func1(x: int | str) -> int | str: +def func(x: int | str) -> int | str: return x +``` + +```pyi +from typing import overload @overload # error: [invalid-overload] -def func2(x: int) -> int: ... +def func(x: int) -> int: ... ``` ### Overload without an implementation diff --git a/crates/red_knot_python_semantic/resources/mdtest/snapshots/overloads.md_-_Overloads_-_Invalid_-_At_least_two_overloads.snap b/crates/red_knot_python_semantic/resources/mdtest/snapshots/overloads.md_-_Overloads_-_Invalid_-_At_least_two_overloads.snap index a1660a89237ba..8f93a74f662f5 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/snapshots/overloads.md_-_Overloads_-_Invalid_-_At_least_two_overloads.snap +++ b/crates/red_knot_python_semantic/resources/mdtest/snapshots/overloads.md_-_Overloads_-_Invalid_-_At_least_two_overloads.snap @@ -12,48 +12,54 @@ mdtest path: crates/red_knot_python_semantic/resources/mdtest/overloads.md ## mdtest_snippet.py ``` - 1 | from typing import overload - 2 | - 3 | @overload - 4 | def func1(x: int) -> int: ... - 5 | - 6 | # error: [invalid-overload] - 7 | def func1(x: int | str) -> int | str: - 8 | return x - 9 | -10 | @overload -11 | # error: [invalid-overload] -12 | def func2(x: int) -> int: ... +1 | from typing import overload +2 | +3 | @overload +4 | def func(x: int) -> int: ... +5 | +6 | # error: [invalid-overload] +7 | def func(x: int | str) -> int | str: +8 | return x +``` + +## mdtest_snippet.pyi + +``` +1 | from typing import overload +2 | +3 | @overload +4 | # error: [invalid-overload] +5 | def func(x: int) -> int: ... ``` # Diagnostics ``` -error: lint:invalid-overload: Overloaded function `func1` requires at least two overloads +error: lint:invalid-overload: Overloaded function `func` requires at least two overloads --> /src/mdtest_snippet.py:4:5 | 3 | @overload -4 | def func1(x: int) -> int: ... - | ----- Only one overload defined here +4 | def func(x: int) -> int: ... + | ---- Only one overload defined here 5 | 6 | # error: [invalid-overload] -7 | def func1(x: int | str) -> int | str: - | ^^^^^ +7 | def func(x: int | str) -> int | str: + | ^^^^ 8 | return x | ``` ``` -error: lint:invalid-overload: Overloaded function `func2` requires at least two overloads - --> /src/mdtest_snippet.py:12:5 - | -10 | @overload -11 | # error: [invalid-overload] -12 | def func2(x: int) -> int: ... - | ----- - | | - | Only one overload defined here - | +error: lint:invalid-overload: Overloaded function `func` requires at least two overloads + --> /src/mdtest_snippet.pyi:5:5 + | +3 | @overload +4 | # error: [invalid-overload] +5 | def func(x: int) -> int: ... + | ---- + | | + | Only one overload defined here + | ``` diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index c4fe257f1de12..748863deb7e8c 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -418,7 +418,7 @@ impl<'db> TypeInference<'db> { .copied() .or(self.cycle_fallback_type) .expect( - "definition should belong to this TypeInference region and + "definition should belong to this TypeInference region and \ TypeInferenceBuilder should have inferred a type for it", ) } @@ -430,7 +430,7 @@ impl<'db> TypeInference<'db> { .copied() .or(self.cycle_fallback_type.map(Into::into)) .expect( - "definition should belong to this TypeInference region and + "definition should belong to this TypeInference region and \ TypeInferenceBuilder should have inferred a type for it", ) } @@ -1032,6 +1032,7 @@ impl<'db> TypeInferenceBuilder<'db> { continue; }; + // Check that the overloaded function has at least two overloads if let [single_overload] = overloaded.overloads.as_slice() { let function_node = function.node(self.db(), self.file()); if let Some(builder) = self From 7efe7dfaa99cfa4204e6c45bbaa89146bc57409b Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Mon, 28 Apr 2025 08:57:37 -0500 Subject: [PATCH 10/12] Add `FunctionType::file` API --- crates/red_knot_python_semantic/src/types.rs | 17 +++++++++++------ .../red_knot_python_semantic/src/types/infer.rs | 2 +- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index 7ea0adaa0787d..df018024fc625 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -6375,6 +6375,13 @@ pub struct FunctionType<'db> { #[salsa::tracked] impl<'db> FunctionType<'db> { + /// Returns the [`File`] in which this function is defined. + pub(crate) fn file(self, db: &'db dyn Db) -> File { + // NOTE: Do not use `self.definition(db).file(db)` here, as that could create a + // cross-module dependency on the full AST. + self.body_scope(db).file(db) + } + pub(crate) fn has_known_decorator(self, db: &dyn Db, decorator: FunctionDecorators) -> bool { self.decorators(db).contains(decorator) } @@ -6398,29 +6405,27 @@ impl<'db> FunctionType<'db> { /// Returns the AST node for this function. pub(crate) fn node(self, db: &'db dyn Db, file: File) -> &'db ast::StmtFunctionDef { - let body_scope = self.body_scope(db); - debug_assert_eq!( file, - body_scope.file(db), + self.file(db), "FunctionType::node() must be called with the same file as the one where \ the function is defined." ); - body_scope.node(db).expect_function() + self.body_scope(db).node(db).expect_function() } /// Returns the [`FileRange`] of the function's name. pub fn focus_range(self, db: &dyn Db) -> FileRange { FileRange::new( - self.body_scope(db).file(db), + self.file(db), self.body_scope(db).node(db).expect_function().name.range, ) } pub fn full_range(self, db: &dyn Db) -> FileRange { FileRange::new( - self.body_scope(db).file(db), + self.file(db), self.body_scope(db).node(db).expect_function().range, ) } diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 748863deb7e8c..f34fa5a4af78d 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -4404,7 +4404,7 @@ impl<'db> TypeInferenceBuilder<'db> { // in the same file as the one we're currently inferring the types for. This is because // the `definition` method accesses the semantic index, which could create a // cross-module AST dependency. - if function.body_scope(self.db()).file(self.db()) == self.file() + if function.file(self.db()) == self.file() && function.definition(self.db()).scope(self.db()) == self.scope() { self.called_functions.insert(function); From 8a219b5b8fcf8265d91c863d07be3b72317637d4 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Tue, 29 Apr 2025 08:44:49 -0500 Subject: [PATCH 11/12] Update snapshot after rebasing on main --- ...ads.md_-_Overloads_-_Invalid_-_At_least_two_overloads.snap | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/red_knot_python_semantic/resources/mdtest/snapshots/overloads.md_-_Overloads_-_Invalid_-_At_least_two_overloads.snap b/crates/red_knot_python_semantic/resources/mdtest/snapshots/overloads.md_-_Overloads_-_Invalid_-_At_least_two_overloads.snap index 8f93a74f662f5..246dff3c952ba 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/snapshots/overloads.md_-_Overloads_-_Invalid_-_At_least_two_overloads.snap +++ b/crates/red_knot_python_semantic/resources/mdtest/snapshots/overloads.md_-_Overloads_-_Invalid_-_At_least_two_overloads.snap @@ -36,7 +36,7 @@ mdtest path: crates/red_knot_python_semantic/resources/mdtest/overloads.md ``` error: lint:invalid-overload: Overloaded function `func` requires at least two overloads - --> /src/mdtest_snippet.py:4:5 + --> src/mdtest_snippet.py:4:5 | 3 | @overload 4 | def func(x: int) -> int: ... @@ -52,7 +52,7 @@ error: lint:invalid-overload: Overloaded function `func` requires at least two o ``` error: lint:invalid-overload: Overloaded function `func` requires at least two overloads - --> /src/mdtest_snippet.pyi:5:5 + --> src/mdtest_snippet.pyi:5:5 | 3 | @overload 4 | # error: [invalid-overload] From 382b15bb176055486b0780e0ce8f2f2a1d4d8e26 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Tue, 29 Apr 2025 11:15:01 -0500 Subject: [PATCH 12/12] Avoid checking if the inferred type is not in the file that we're checking --- crates/red_knot_python_semantic/src/types/infer.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index f34fa5a4af78d..f6bfe29b48c74 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -1020,6 +1020,12 @@ impl<'db> TypeInferenceBuilder<'db> { if let Symbol::Type(Type::FunctionLiteral(function), Boundness::Bound) = symbol_from_bindings(self.db(), use_def.public_bindings(symbol)) { + if function.file(self.db()) != self.file() { + // If the function is not in this file, we don't need to check it. + // https://github.com/astral-sh/ruff/pull/17609#issuecomment-2839445740 + continue; + } + // Extend the functions that we need to check with the publicly visible overloaded // function. This is always going to be either the implementation or the last // overload if the implementation doesn't exists.