[red-knot] more ergonomic and efficient handling of known builtin classes #13615

Slyces · 2024-10-03T17:37:02Z

Summary

This PR introduces a new enumeration BuiltinType that serves 2 purposes:

df16f21: give a common syntax for the convenience shortcuts to get a builtin instance
- For that purpose, the enum doesn't need to be exhaustive - just to have the types we often need
c98ce74: save if a class is a builtin on creation, to save time on call when we need to check for custom behaviour (str(...), bool(...), ...)

I think for the first purpose that's mainly a syntax preference, you should be able to tell fairly fast if you prefer it that way.
For the second, that's mainly an optimisation that we might need once we handle more specific behaviour for builtin used as callable - this could clearly wait until we implement more of those.

Test Plan

No tests were added, but the existing test suite is enough to check if this introduced a regression.

This enumeration allows us to have shorter syntax for very common builtin types. This mainly allows to gather convenience methods under one common syntax (one place to look for).

On `ClassType` creation, check if the class is a common builtin and save that information for later use during inference. This will allow us to save time on every call for builtin class/functions (e.g. `str`,`int`,`float`,`bool`, ...)

github-actions · 2024-10-03T17:55:54Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

crates/red_knot_python_semantic/src/types.rs

…n builtin

carljm

This looks great as a general direction! A few nits and thoughts on naming/API.

carljm · 2024-10-04T17:25:40Z

crates/red_knot_python_semantic/src/types.rs

+/// Non-exhaustive enumeration of builtin types to allow for easier syntax when interacting with
+/// the most common builtin types (e.g. int, str, ...).
+///
+/// Feel free to expend this enum if you ever find yourself using the same builtin type in multiple


Suggested change

/// Feel free to expend this enum if you ever find yourself using the same builtin type in multiple

/// Feel free to expand this enum if you ever find yourself using the same builtin type in multiple

carljm · 2024-10-04T17:26:44Z

crates/red_knot_python_semantic/src/types.rs

+    }
+
+    pub fn to_instance(&self, db: &'db dyn Db) -> Type<'db> {
+        builtins_symbol_ty(db, self.as_str()).to_instance(db)


maybe

Suggested change

builtins_symbol_ty(db, self.as_str()).to_instance(db)

self.to_class(db).to_instance(db)

carljm · 2024-10-04T17:31:09Z

crates/red_knot_python_semantic/src/types.rs

@@ -424,27 +416,35 @@ impl<'db> Type<'db> {
            (Type::Never, _) => true,
            (_, Type::Never) => false,
            (Type::IntLiteral(_), Type::Instance(class))
-                if class.is_stdlib_symbol(db, "builtins", "int") =>
+                if matches!(class.is_builtin(db), Some(BuiltinType::Int)) =>


Can we add a method on ClassType to make this less verbose? I would probably even call it is_builtin:

Suggested change

if matches!(class.is_builtin(db), Some(BuiltinType::Int)) =>

if class.is_builtin(db, BuiltinType::Int) =>

And then rename the is_builtin field to just builtin; is_ prefix suggests a boolean, not an optional enum.

carljm · 2024-10-04T17:46:34Z

crates/red_knot_python_semantic/src/types.rs

@@ -1220,9 +1279,26 @@ pub struct ClassType<'db> {
    definition: Definition<'db>,

    body_scope: ScopeId<'db>,
+
+    is_builtin: Option<BuiltinType>,


As mentioned above, I'd name this just builtin -- is_ prefix suggests a boolean.

carljm · 2024-10-04T17:54:19Z

crates/red_knot_python_semantic/src/types.rs

+/// Feel free to expend this enum if you ever find yourself using the same builtin type in multiple
+/// places.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+pub enum BuiltinType {


Naming nit: let's rename this to BuiltinClass. This actually only handles classes, not all kinds of types (functions are types too).

Speaking of which, I kinda want to unify this PR and FunctionKind, in terms of both naming and API. The one trick there is that we want to represent known functions that aren't actually builtins (e.g. reveal_type). The same may be true in future for other stdlib classes!

So maybe ultimately what we'll actually want is KnownClass and KnownFunction enums, and a known field (which is an Option<KnownClass> or Option<KnownFunction> - I like this Option approach better than FunctionKind::Ordinary), and is_known method, on both ClassType and FunctionType. But this would require generalizing what you have in this PR to not assume builtins module, and instead classify the known functions/classes by module as well. I would be happy with doing all of this now, in this PR, or waiting on it if you prefer to land a simpler version of this PR for now.

cc @AlexWaygood in case you hate my naming/API preferences :)

Hmm, I considered the pros and cons of Option<KnownFunction> vs having FunctionKind::Ordinary while working on my previous PR. I ended up going with FunctionKind::Ordinary because I have a general preference having a flat list of possible states rather than representing possible states using an enum of enums. It makes it easier to count exactly how many possible states there are, and giving the default state a name (Ordinary) rather than using None makes it abundantly clear to new readers of the code what the default state represents. It's also more ergonomic to pattern-match on a flat list rather than a nested enum.

I don't feel too strongly, but a flat enum would be my preference :)

I think clearer naming is the primary reason I prefer Option<KnownFunction>. I don't like the name FunctionKind because it is so unnecessarily generic -- it doesn't clarify anything at all about what characteristic(s) of the function we are actually talking about. What happens when we later need to categorize functions according to some totally different cross-cutting categorization?

It seems clearer to me to have known = Some(KnownFunction) mean it's a known function (and then the enum specifies which one), and known = None to mean "not a known function." I suppose we could still use the KnownFunction name and have KnownFunction::NotKnown or KnownFunction::None -- it just seems weird to me to have a variant of KnownFunction that means... not a known function.

Yes, you're definitely right that FunctionKind is not a great name. And I think you're right that renaming it to KnownFunction addresses some of the concern I had that it wouldn't be obvious what the default state signified. It's pretty obvious that None indicates that it's not a known function!

I'm still not a massive fan of nested enums, but I guess in this case it's okay. Feel free to proceed :)

carljm · 2024-10-04T18:06:20Z

crates/red_knot_python_semantic/src/types.rs

 }

 impl<'db> ClassType<'db> {
+    /// Find if a class is a builtin type.
+    pub fn maybe_builtin(


I don't like the name of this method currently because it implies that if it returns None the class is not a builtin. But that's not necessarily true, since our list of known builtins isn't exhaustive. I'd prefer maybe_known_builtin. Or just maybe_known if we go with the broader refactor/rename to handle any known class, even if it's not a builtin.

Slyces added 2 commits October 3, 2024 19:29

[red-knot] feat: provide a non-exhaustive BuiltinType enum

df16f21

This enumeration allows us to have shorter syntax for very common builtin types. This mainly allows to gather convenience methods under one common syntax (one place to look for).

Slyces requested review from carljm, MichaReiser and AlexWaygood as code owners October 3, 2024 17:37

dhruvmanila reviewed Oct 4, 2024

View reviewed changes

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

fixup! [red-knot] feat: on ClassType creation, save if it's a commo…

00375b4

…n builtin

carljm reviewed Oct 4, 2024

View reviewed changes

carljm added the red-knot Multi-file analysis & type inference label Oct 4, 2024

carljm changed the title ~~Feat/builtins enum~~ [red-knot] more ergonomic and efficient handling of known builtin classes Oct 4, 2024

carljm reviewed Oct 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[red-knot] more ergonomic and efficient handling of known builtin classes #13615

[red-knot] more ergonomic and efficient handling of known builtin classes #13615

Slyces commented Oct 3, 2024

github-actions bot commented Oct 3, 2024 •

edited

Loading

carljm left a comment

carljm Oct 4, 2024

carljm Oct 4, 2024

carljm Oct 4, 2024

carljm Oct 4, 2024

carljm Oct 4, 2024

AlexWaygood Oct 4, 2024 •

edited

Loading

carljm Oct 4, 2024 •

edited

Loading

AlexWaygood Oct 4, 2024

carljm Oct 4, 2024

	/// Feel free to expend this enum if you ever find yourself using the same builtin type in multiple
	/// Feel free to expand this enum if you ever find yourself using the same builtin type in multiple

	builtins_symbol_ty(db, self.as_str()).to_instance(db)
	self.to_class(db).to_instance(db)

	if matches!(class.is_builtin(db), Some(BuiltinType::Int)) =>
	if class.is_builtin(db, BuiltinType::Int) =>

[red-knot] more ergonomic and efficient handling of known builtin classes #13615

Are you sure you want to change the base?

[red-knot] more ergonomic and efficient handling of known builtin classes #13615

Conversation

Slyces commented Oct 3, 2024

Summary

Test Plan

github-actions bot commented Oct 3, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

carljm left a comment

Choose a reason for hiding this comment

carljm Oct 4, 2024

Choose a reason for hiding this comment

carljm Oct 4, 2024

Choose a reason for hiding this comment

carljm Oct 4, 2024

Choose a reason for hiding this comment

carljm Oct 4, 2024

Choose a reason for hiding this comment

carljm Oct 4, 2024

Choose a reason for hiding this comment

AlexWaygood Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

carljm Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

AlexWaygood Oct 4, 2024

Choose a reason for hiding this comment

carljm Oct 4, 2024

Choose a reason for hiding this comment

github-actions bot commented Oct 3, 2024 •

edited

Loading

`ruff-ecosystem` results

AlexWaygood Oct 4, 2024 •

edited

Loading

carljm Oct 4, 2024 •

edited

Loading