Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[red-knot] Prefix Type::call and dunder_call with try #16261

Merged
merged 2 commits into from
Feb 20, 2025

Conversation

MichaReiser
Copy link
Member

Summary

This PR prefixes the Type::call and dunder_call methods with try to make it more explicit that they return a Result
and add call and dunder_call methods that return the return-type without a Result. These new methods are intended
to be used outside of type checking.

This follows an existing pattern:

We also have Class::try_mro() and Class::mro(), which are similar. And Class::try_metaclass() / Class::metaclass()

The main motivation for this change is #16238 because it is somewhat common to call bool outside
of type checking -- Making it desirable to have to dedicated methods for it. Splitting the methods also helps improving performance
because we can skip some analysis when we don't care about errors.

Test Plan

cargo test

Verified

This commit was signed with the committer’s verified signature.
MichaReiser Micha Reiser
@MichaReiser MichaReiser added the red-knot Multi-file analysis & type inference label Feb 19, 2025
@MichaReiser MichaReiser changed the title Prefix Type::call and dunder_call with try [red-knot] Prefix Type::call and dunder_call with try Feb 19, 2025
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The renaming of the existing methods to Type::try_call(), Type::try_call_bound and Type::try_call_dunder makes sense to me. I'm not sure I see the motivation for adding the unused Type::call(), Type::call_dunder and Type::call_bound methods, though. They seem like they should be fairly easy to add if we ever need them?

@@ -2693,52 +2725,8 @@ pub enum KnownInstanceType<'db> {
}

impl<'db> KnownInstanceType<'db> {
pub const fn as_str(self) -> &'static str {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

@MichaReiser
Copy link
Member Author

I think it helps establish the pattern and it more clearly communicates the idea. It also provides a place to document where they should be used. My last argument is that I'm often too lazy to add methods like this if I have to use them the first time and I only add them if I rewrote the same code like 10 times. Maybe that's a good thing but I want to take that burden from future me.

But I don't feel strongly. Removing them just means that we'll have some op try_op pairs whereas other methods don't. Which seems inconsistent (I like symmetry, you know ;))

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I lean towards leaving out the unused methods until we need them, since we should usually encourage people to use the methods that return Results in most contexts anyway. But I also don't feel too strongly :-)

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the pattern, but I also think that we should omit methods that we don't currently need (and IMO it is not clear if we will ever need them.) I think the existence of try_call already strongly implies the pattern even if call doesn't exist (yet).

This will cause more rebase pain for @sharkdp on the descriptors PR :)

Verified

This commit was signed with the committer’s verified signature.
MichaReiser Micha Reiser
@MichaReiser MichaReiser enabled auto-merge (squash) February 20, 2025 09:02
@MichaReiser MichaReiser merged commit fb09d63 into main Feb 20, 2025
20 checks passed
@MichaReiser MichaReiser deleted the micha/rename-type-methods branch February 20, 2025 09:05
Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants