[red-knot] Fix Stack overflow in Type::bool #15843

mishamsk · 2025-01-31T04:05:45Z

Summary

This PR adds Type::call_bound method for calls that should follow descriptor protocol calling convention. The PR is intentionally shallow in scope and only fixes #15672

Couple of obvious things that weren't done:

Switch to call_bound everywhere it should be used
Address the fact, that red_knot resolves __bool__ = bool as a Union, which includes Type::Dynamic and hence fails to infer that the truthiness is always false for such a class (I've added a todo comment in mdtests)
Doesn't try to invent a new type for descriptors, although I have a gut feeling it may be more convenient in the end, instead of doing method lookup each time like I did in call_bound

Test Plan

extended mdtests with 2 examples from the issue
cargo neatest run

use `Type::call_bound` in bool evaluation. fixes astral-sh#15672

carljm

Thank you for the PR! I think the call_bound abstraction makes sense, and is needed to handle this case correctly. Some inline comments.

carljm · 2025-02-03T21:59:33Z

crates/red_knot_python_semantic/resources/mdtest/unary/not.md

-    # TODO: The `type[bool]` declaration here is a workaround to avoid running into
-    # https://github.com/astral-sh/ruff/issues/15672
-    __bool__: type[bool] = bool
+    __bool__ = bool


I don't think this case is important enough to test in two different places.

I think it belongs better where you've put it (in truthiness.md). But really, most of this file should be moved over to truthiness.md, this file should really just test type negation and nothing else. It would probably be better to either move all the instance __bool__ tests over at once, or none of them.

Best would probably be to move them in a separate PR, rather than in this one.

sure, agree. I erred on the side of caution and didn't want to impose my view on how tests should be structured. At least not with my 4th PR ;-)

I removed this specific case from not.md and left it only in the truthiness.md file. I can move the rest in a separate follow-up or as a flyby with other stuff

That would be great if you want to do this! I recommend separate PR, there's no real cost to more smaller PRs, and it can make it a lot easier to review this kind of change if it's not combined with functional changes.

carljm · 2025-02-03T22:02:34Z

crates/red_knot_python_semantic/src/types.rs

+                self.call(db, &arguments.with_self(*receiver_ty))
+            }
+
+            object_ty @ (Type::Instance(_) | Type::ClassLiteral(_)) => {


This should handle Type::SubclassOf as well -- but so should Type::call, and it doesn't either. So this can probably be left to a separate PR to fix.

Happy to do that in a follow-up. See my comment regarding descriptor protocol below

Sure, that would be great! I created #15948

crates/red_knot_python_semantic/resources/mdtest/type_properties/truthiness.md

carljm · 2025-02-03T22:22:41Z

crates/red_knot_python_semantic/src/types.rs

+                match object_ty.member(db, "__get__") {
+                    Symbol::Type(get_method, _) => {
+                        let descr_result = match receiver_ty {
+                            Type::Instance(_) => {
+                                let receiver_class = receiver_ty.to_meta_type(db);
+
+                                get_method.call(
+                                    db,
+                                    &CallArguments::positional([*receiver_ty, receiver_class]),
+                                )
+                            }
+                            Type::ClassLiteral(_) => get_method.call(
+                                db,
+                                &CallArguments::positional([
+                                    KnownClass::NoneType.to_instance(db),
+                                    object_ty,
+                                ]),
+                            ),
+                            _ => unreachable!(),
+                        };
+
+                        match descr_result {
+                            CallOutcome::Callable { binding } => {
+                                binding.return_type().call(db, arguments)
+                            }
+                            _ => descr_result,
+                        }
+                    }
+                    Symbol::Unbound => {
+                        // Not a descriptor, so call without passing the instance
+                        self.call(db, arguments)
+                    }
+                }


Since we don't have tests for descriptor protocol (and this PR doesn't add any), and we may want to design our descriptor protocol support a bit more holistically (not just specific to bound calls), I think we should not add support for it here. Instead we should just simplify this code to assume non-descriptor, with a TODO comment for descriptor support.

Suggested change

match object_ty.member(db, "__get__") {

Symbol::Type(get_method, _) => {

let descr_result = match receiver_ty {

Type::Instance(_) => {

let receiver_class = receiver_ty.to_meta_type(db);

get_method.call(

db,

&CallArguments::positional([*receiver_ty, receiver_class]),

)

}

Type::ClassLiteral(_) => get_method.call(

db,

&CallArguments::positional([

KnownClass::NoneType.to_instance(db),

object_ty,

]),

),

_ => unreachable!(),

};

match descr_result {

CallOutcome::Callable { binding } => {

binding.return_type().call(db, arguments)

}

_ => descr_result,

}

}

Symbol::Unbound => {

// Not a descriptor, so call without passing the instance

self.call(db, arguments)

}

}

// TODO descriptor protocol. For now, assume non-descriptor and call without `self` argument.

self.call(db, arguments)

I'd love to work on the descriptor protocol as a whole, would be fun and will allow me to better review more of the red_knot codebase. If you are fine with that, I can open a separate draft PR and outline a plan.

One obvious place to start would be:

ruff/crates/red_knot_python_semantic/src/types.rs

Lines 4211 to 4219 in 0529ad6

/// A helper function for `instance_member` that looks up the `name` attribute only on

/// this class, not on its superclasses.

fn own_instance_member(self, db: &'db dyn Db, name: &str) -> SymbolAndQualifiers<'db> {

// TODO: There are many things that are not yet implemented here:

// - `typing.Final`

// - Proper diagnostics

// - Handling of possibly-undeclared/possibly-unbound attributes

// - The descriptor protocol

and implement proper handling of property instead of a current "workaround". This would serve as a test. + I can make a special mdtest suite for descriptors in general.

As for my code here, I do not think that there is harm in leaving it, as it will work for cases when __get__ method is defined explicitly already, but since you suggest removing it, I'll just stash it and see if it eventually survives.

Thanks for removing this for now. I agree it's possible that later it may come back in the same or similar form, but I would rather it arrive along with tests exercising it.

I think @sharkdp was planning to tackle descriptors soon, but may be open to you working on that instead, freeing him up to tackle something else? Let's wait for him to respond before you start on it, in case he has already started on it.

It would definitely be good to open an issue with a proposed design first, so we can discuss, before opening a PR; this may save some implementation work.

@carljm @sharkdp I can create an outline with items that needs to be tackled in principle, a proposed step-by-step plan and draft design (or at least some thinking around it). Shall I use discussions or an issue?

That sounds like a great approach! We usually use issues, not discussions. I think @sharkdp was planning to create a set of issues tomorrow for various follow-ups to his recent work on instance attributes, including descriptor protocol as one of those.

@carljm @sharkdp I can create an outline with items that needs to be tackled in principle, a proposed step-by-step plan and draft design (or at least some thinking around it). Shall I use discussions or an issue?

@mishamsk If you want to work on descriptors, we would definitely appreciate it! Starting with a separate design step sounds like a good idea. Please feel free to add comments to this issue: #15966

I will also set up an initial draft for a descriptor test suite that we can detail further while we work on the design and the implementation (Edit: #15972).

In case you want to work on something with a smaller scope first, I created a couple of fine-grained tickets around our understanding of instance and class attributes. They are listed as sub-tasks here and are roughly ordered by priority and inter-dependencies. I marked some of these issues as "help wanted".

@sharkdp thanks a lot for all the links. Helps a ton, I wouldn't have found that tracking issue myself easily.

I'll try to do both - comment on some plan rgd descriptors & pick up one of the issues from #14164 - will do at least of two later today (I am EST)

crates/red_knot_python_semantic/src/types.rs

carljm

Looks great, thank you!

crates/red_knot_python_semantic/resources/mdtest/type_properties/truthiness.md

add Type::call_bound method for calls that invoke descriptor protocol.

81b4b57

use `Type::call_bound` in bool evaluation. fixes astral-sh#15672

mishamsk requested review from carljm, MichaReiser, AlexWaygood and sharkdp as code owners January 31, 2025 04:05

dhruvmanila added the red-knot Multi-file analysis & type inference label Jan 31, 2025

carljm requested changes Feb 3, 2025

View reviewed changes

minor fixes per review comments

1dcf5ac

carljm approved these changes Feb 4, 2025

View reviewed changes

crates/red_knot_python_semantic/resources/mdtest/type_properties/truthiness.md Show resolved Hide resolved

carljm merged commit e154193 into astral-sh:main Feb 4, 2025
21 checks passed

mishamsk deleted the 15672-fix-stack-overflow-in-type-bool-evaluation branch February 4, 2025 23:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[red-knot] Fix Stack overflow in Type::bool #15843

[red-knot] Fix Stack overflow in Type::bool #15843

mishamsk commented Jan 31, 2025

carljm left a comment

carljm Feb 3, 2025

mishamsk Feb 4, 2025

carljm Feb 4, 2025

carljm Feb 3, 2025

mishamsk Feb 4, 2025

carljm Feb 4, 2025

carljm Feb 3, 2025

mishamsk Feb 4, 2025 •

edited

Loading

carljm Feb 4, 2025

mishamsk Feb 4, 2025

carljm Feb 4, 2025

sharkdp Feb 5, 2025 •

edited

Loading

sharkdp Feb 5, 2025

mishamsk Feb 5, 2025

carljm left a comment

	/// A helper function for `instance_member` that looks up the `name` attribute only on
	/// this class, not on its superclasses.
	fn own_instance_member(self, db: &'db dyn Db, name: &str) -> SymbolAndQualifiers<'db> {
	// TODO: There are many things that are not yet implemented here:
	// - `typing.Final`
	// - Proper diagnostics
	// - Handling of possibly-undeclared/possibly-unbound attributes
	// - The descriptor protocol

[red-knot] Fix Stack overflow in Type::bool #15843

[red-knot] Fix Stack overflow in Type::bool #15843

Conversation

mishamsk commented Jan 31, 2025

Summary

Test Plan

carljm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mishamsk Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharkdp Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carljm left a comment

Choose a reason for hiding this comment

mishamsk Feb 4, 2025 •

edited

Loading

sharkdp Feb 5, 2025 •

edited

Loading