[red-knot] Do not emit `invalid-return-type` for abstract functions #16900

InSyncWithFoo · 2025-03-21T16:08:55Z

Summary

Resolves #16895.

abstractmethod is now a KnownFunction. When a function is decorated by abstractmethod or when the parent class inherits directly from Protocol, invalid-return-type won't be emitted for that function.

Test Plan

Markdown tests.

github-actions · 2025-03-21T16:20:28Z

`mypy_primer` results

Changes were detected when running on open source projects

packaging (https://github.com/pypa/packaging)
- error[lint:invalid-return-type] /tmp/mypy_primer/projects/packaging/src/packaging/specifiers.py:46:26: Function can implicitly return `None`, which is not assignable to return type `str`
- error[lint:invalid-return-type] /tmp/mypy_primer/projects/packaging/src/packaging/specifiers.py:53:27: Function can implicitly return `None`, which is not assignable to return type `int`
- error[lint:invalid-return-type] /tmp/mypy_primer/projects/packaging/src/packaging/specifiers.py:59:40: Function can implicitly return `None`, which is not assignable to return type `bool`
- error[lint:invalid-return-type] /tmp/mypy_primer/projects/packaging/src/packaging/specifiers.py:84:71: Function can implicitly return `None`, which is not assignable to return type `bool`
- Found 141 diagnostics
+ Found 137 diagnostics

pyinstrument (https://github.com/joerick/pyinstrument)
- error[lint:invalid-return-type] /tmp/mypy_primer/projects/pyinstrument/pyinstrument/frame.py:47:42: Function can implicitly return `None`, which is not assignable to return type `str`
- Found 288 diagnostics
+ Found 287 diagnostics

rich (https://github.com/Textualize/rich)
- error[lint:invalid-return-type] /tmp/mypy_primer/projects/rich/rich/console.py:264:10: Function can implicitly return `None`, which is not assignable to return type `ConsoleRenderable | RichCast | str`
- error[lint:invalid-return-type] /tmp/mypy_primer/projects/rich/rich/console.py:562:10: Function can implicitly return `None`, which is not assignable to return type `list`
- error[lint:invalid-return-type] /tmp/mypy_primer/projects/rich/rich/layout.py:86:32: Function can implicitly return `None`, which is not assignable to return type `str`
- Found 589 diagnostics
+ Found 586 diagnostics

AlexWaygood · 2025-03-21T16:22:37Z

I don't know if we want to support abstractproperty, abstractclassmethod and abstractstaticmethod. They've all been deprecated for many Python releases, and are unsupported by both mypy and pyright. The support you're adding for them in this PR isn't too much added complexity, but I suspect that full support for these decorators might be a significant amount of complexity. It will be less confusing for users if we don't support them at all rather than partially supporting them.

InSyncWithFoo · 2025-03-21T16:47:18Z

Glad to hear that! Reverted.

crates/red_knot_python_semantic/src/types/infer.rs

MatthewMckee4

Should we add a test for "Function can implicitly return None, which is not assignable to return type {}" is emitted when you have a function with no body outside of class

# error: [invalid-return-type] "Function can implicitly return `None`, which is not assignable to return type `int`"
def f(self) -> int: ...

AlexWaygood

This looks great to me -- thank you!

carljm

Thank you, this is a nice improvement!

crates/red_knot_python_semantic/src/types/infer.rs

carljm · 2025-03-21T19:43:12Z

crates/red_knot_python_semantic/src/types/infer.rs

+            let class_inherits_protocol_directly = (|| -> bool {
+                let current_scope_id = self.scope().file_scope_id(self.db());
+                let current_scope = self.index.scope(current_scope_id);
+                let Some(parent_scope_id) = current_scope.parent() else {
+                    return false;
+                };
+                let parent_scope = self.index.scope(parent_scope_id);
+
+                let NodeWithScopeKind::Class(node_ref) = parent_scope.node() else {
+                    return false;
+                };
+
+                node_ref.bases().iter().any(|base| {
+                    matches!(
+                        self.file_expression_type(base),
+                        Type::KnownInstance(KnownInstanceType::Protocol)
+                    )
+                })
+            })();


In terms of API, rather than doing all this right here in type inference, I think we should have methods Class::is_protocol() -> bool and FunctionType::enclosing_class() -> Option<Class<'db>>.

In terms of how those methods are implemented, I did wonder about an alternate strategy that would record the enclosing-class information for each function in semantic indexing. This would imply that DefinitionKind::Function would include an Option<FileScopeId> which is the body scope of the lexically enclosing class.

But I'm not sure this is really better; it would require extra work in semantic indexing to maintain the "enclosing class" state and clear it appropriately (that is, nested functions inside methods are not themselves methods and should have no enclosing class), and I think the scope-walking code you have here can be made fully correct without too much difficulty.

carljm · 2025-03-21T19:47:36Z

Should we add a test for "Function can implicitly return None, which is not assignable to return type {}" is emitted when you have a function with no body outside of class

I think we should test this, but it seems orthogonal to this PR. If we are lacking a test for this and you want to submit a PR to add it, that would be great!

carljm

Added tests look good! I would still like to see us break up the mass of the new added code by extracting some reusable and meaningfully-named functions.

InSyncWithFoo · 2025-03-23T17:00:51Z

@carljm FunctionType::enclosing_class() seems unhelpful here, since infer_function_body() is trying to figure out that type in the first place. Am I missing something?

carljm · 2025-03-23T17:46:22Z

FunctionType::enclosing_class() seems unhelpful here, since infer_function_body() is trying to figure out that type in the first place. Am I missing something?

infer_function_body is not part of figuring out the FunctionType; checking the function body is separate and only done on demand. So it would be possible to get the FunctionType here by looking up the function Definition and querying its type. But you're right that it may not be the simplest / most ergonomic thing.

My other concern is that we should let Class keep responsibility for interpreting the types of its bases, rather than having ad-hoc code in various places looking directly at the base class elements in the AST. But I'm fine with just adding a TODO for that for now, since it will be natural to do it when actually adding Protocol support.

crates/red_knot_python_semantic/src/types/infer.rs

carljm · 2025-03-23T17:48:55Z

Still halfway tempted to move "find the enclosing class" logic to Scope, but I think we can wait on that until/unless we have a second or third use case for the same logic. (I think it initially struck me as duplication because we had this same code previously, but I think the previous occurrence of it was removed in favor of storing the "defining node" of a scope in the scope itself.)

carljm · 2025-03-23T17:49:17Z

Thanks for the PR!

carljm · 2025-03-23T20:53:01Z

Ok, I found where I had seen this is-method-of-class logic before, and we still have it (and it's currently wrong). See #16928

InSyncWithFoo requested review from carljm, AlexWaygood, sharkdp and dcreager as code owners March 21, 2025 16:08

AlexWaygood added the red-knot label Mar 21, 2025

InSyncWithFoo force-pushed the rk-invalid-return-type branch from a84f4d5 to 393329b Compare March 21, 2025 16:46

MichaReiser reviewed Mar 21, 2025

View reviewed changes

crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved

MichaReiser reviewed Mar 21, 2025

View reviewed changes

crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved

MatthewMckee4 reviewed Mar 21, 2025

View reviewed changes

AlexWaygood approved these changes Mar 21, 2025

View reviewed changes

carljm reviewed Mar 21, 2025

View reviewed changes

InSyncWithFoo added 5 commits March 22, 2025 04:30

[red-knot] Do not emit invalid-return-type for abstract functions

f87684d

Per review

f749fd7

Per review

1f3fd7b

Formatting

35ecb94

Per review

Loading
Loading status checks…

1aa98e6

InSyncWithFoo force-pushed the rk-invalid-return-type branch from e3457da to 1aa98e6 Compare March 22, 2025 04:33

carljm reviewed Mar 23, 2025

View reviewed changes

crates/red_knot_python_semantic/src/types/infer.rs Show resolved Hide resolved

Update crates/red_knot_python_semantic/src/types/infer.rs

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

Loading
Loading status checks…

d023cc3

carljm enabled auto-merge (squash) March 23, 2025 17:49

carljm merged commit 902d86e into astral-sh:main Mar 23, 2025
22 checks passed

InSyncWithFoo deleted the rk-invalid-return-type branch March 23, 2025 17:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[red-knot] Do not emit `invalid-return-type` for abstract functions #16900

[red-knot] Do not emit `invalid-return-type` for abstract functions #16900

InSyncWithFoo commented Mar 21, 2025 •

edited

Loading

github-actions bot commented Mar 21, 2025 •

edited

Loading

AlexWaygood commented Mar 21, 2025

InSyncWithFoo commented Mar 21, 2025

MatthewMckee4 left a comment

AlexWaygood left a comment

carljm left a comment

carljm Mar 21, 2025 •

edited

Loading

carljm commented Mar 21, 2025

carljm left a comment

InSyncWithFoo commented Mar 23, 2025

carljm commented Mar 23, 2025

carljm commented Mar 23, 2025

carljm commented Mar 23, 2025

carljm commented Mar 23, 2025

[red-knot] Do not emit invalid-return-type for abstract functions #16900

[red-knot] Do not emit invalid-return-type for abstract functions #16900

Conversation

InSyncWithFoo commented Mar 21, 2025 • edited Loading

Summary

Test Plan

github-actions bot commented Mar 21, 2025 • edited Loading

mypy_primer results

AlexWaygood commented Mar 21, 2025

InSyncWithFoo commented Mar 21, 2025

MatthewMckee4 left a comment

Choose a reason for hiding this comment

AlexWaygood left a comment

Choose a reason for hiding this comment

carljm left a comment

Choose a reason for hiding this comment

carljm Mar 21, 2025 • edited Loading

Choose a reason for hiding this comment

carljm commented Mar 21, 2025

carljm left a comment

Choose a reason for hiding this comment

InSyncWithFoo commented Mar 23, 2025

carljm commented Mar 23, 2025

carljm commented Mar 23, 2025

carljm commented Mar 23, 2025

carljm commented Mar 23, 2025

[red-knot] Do not emit `invalid-return-type` for abstract functions #16900

[red-knot] Do not emit `invalid-return-type` for abstract functions #16900

InSyncWithFoo commented Mar 21, 2025 •

edited

Loading

github-actions bot commented Mar 21, 2025 •

edited

Loading

`mypy_primer` results

carljm Mar 21, 2025 •

edited

Loading