-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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] support for typing.reveal_type #13384
Changes from all commits
d71a7ee
275c499
10d269e
41e6b94
6df5c5c
f0ace0e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -238,6 +238,8 @@ pub enum Type<'db> { | |||||||||||||||||
None, | ||||||||||||||||||
/// a specific function object | ||||||||||||||||||
Function(FunctionType<'db>), | ||||||||||||||||||
/// The `typing.reveal_type` function, which has special `__call__` behavior. | ||||||||||||||||||
RevealTypeFunction(FunctionType<'db>), | ||||||||||||||||||
/// a specific module object | ||||||||||||||||||
Module(File), | ||||||||||||||||||
/// a specific class object | ||||||||||||||||||
|
@@ -324,14 +326,16 @@ impl<'db> Type<'db> { | |||||||||||||||||
|
||||||||||||||||||
pub const fn into_function_type(self) -> Option<FunctionType<'db>> { | ||||||||||||||||||
match self { | ||||||||||||||||||
Type::Function(function_type) => Some(function_type), | ||||||||||||||||||
Type::Function(function_type) | Type::RevealTypeFunction(function_type) => { | ||||||||||||||||||
Some(function_type) | ||||||||||||||||||
} | ||||||||||||||||||
_ => None, | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
Comment on lines
327
to
334
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess the answer is probably no because the function is called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this makes ruff/crates/red_knot_python_semantic/src/types.rs Lines 332 to 335 in 4eb849a
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No, there's no such code. And I think we generally want any external code to ideally transparently treat This is another really good reason to store the inner
I think this makes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Well, you'll at least need to change the message in the ruff/crates/red_knot_python_semantic/src/types.rs Lines 332 to 335 in 4eb849a
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call! Fixed the message. |
||||||||||||||||||
|
||||||||||||||||||
pub fn expect_function(self) -> FunctionType<'db> { | ||||||||||||||||||
self.into_function_type() | ||||||||||||||||||
.expect("Expected a Type::Function variant") | ||||||||||||||||||
.expect("Expected a variant wrapping a FunctionType") | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
pub const fn into_int_literal_type(self) -> Option<i64> { | ||||||||||||||||||
|
@@ -367,6 +371,16 @@ impl<'db> Type<'db> { | |||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
pub fn is_stdlib_symbol(&self, db: &'db dyn Db, module_name: &str, name: &str) -> bool { | ||||||||||||||||||
match self { | ||||||||||||||||||
Type::Class(class) => class.is_stdlib_symbol(db, module_name, name), | ||||||||||||||||||
Type::Function(function) | Type::RevealTypeFunction(function) => { | ||||||||||||||||||
function.is_stdlib_symbol(db, module_name, name) | ||||||||||||||||||
} | ||||||||||||||||||
_ => false, | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
/// Return true if this type is [assignable to] type `target`. | ||||||||||||||||||
/// | ||||||||||||||||||
/// [assignable to]: https://typing.readthedocs.io/en/latest/spec/concepts.html#the-assignable-to-or-consistent-subtyping-relation | ||||||||||||||||||
|
@@ -436,7 +450,7 @@ impl<'db> Type<'db> { | |||||||||||||||||
// TODO: attribute lookup on None type | ||||||||||||||||||
Type::Unknown | ||||||||||||||||||
} | ||||||||||||||||||
Type::Function(_) => { | ||||||||||||||||||
Type::Function(_) | Type::RevealTypeFunction(_) => { | ||||||||||||||||||
// TODO: attribute lookup on function type | ||||||||||||||||||
Type::Unknown | ||||||||||||||||||
} | ||||||||||||||||||
|
@@ -482,26 +496,39 @@ impl<'db> Type<'db> { | |||||||||||||||||
/// | ||||||||||||||||||
/// Returns `None` if `self` is not a callable type. | ||||||||||||||||||
#[must_use] | ||||||||||||||||||
pub fn call(&self, db: &'db dyn Db) -> Option<Type<'db>> { | ||||||||||||||||||
fn call(self, db: &'db dyn Db, arg_types: &[Type<'db>]) -> CallOutcome<'db> { | ||||||||||||||||||
match self { | ||||||||||||||||||
Type::Function(function_type) => Some(function_type.return_type(db)), | ||||||||||||||||||
// TODO validate typed call arguments vs callable signature | ||||||||||||||||||
Type::Function(function_type) => CallOutcome::callable(function_type.return_type(db)), | ||||||||||||||||||
Type::RevealTypeFunction(function_type) => CallOutcome::revealed( | ||||||||||||||||||
function_type.return_type(db), | ||||||||||||||||||
*arg_types.first().unwrap_or(&Type::Unknown), | ||||||||||||||||||
), | ||||||||||||||||||
|
||||||||||||||||||
// TODO annotated return type on `__new__` or metaclass `__call__` | ||||||||||||||||||
Type::Class(class) => Some(Type::Instance(*class)), | ||||||||||||||||||
Type::Class(class) => CallOutcome::callable(Type::Instance(class)), | ||||||||||||||||||
|
||||||||||||||||||
// TODO: handle classes which implement the Callable protocol | ||||||||||||||||||
Type::Instance(_instance_ty) => Some(Type::Unknown), | ||||||||||||||||||
// TODO: handle classes which implement the `__call__` protocol | ||||||||||||||||||
Type::Instance(_instance_ty) => CallOutcome::callable(Type::Unknown), | ||||||||||||||||||
|
||||||||||||||||||
// `Any` is callable, and its return type is also `Any`. | ||||||||||||||||||
Type::Any => Some(Type::Any), | ||||||||||||||||||
Type::Any => CallOutcome::callable(Type::Any), | ||||||||||||||||||
|
||||||||||||||||||
Type::Unknown => Some(Type::Unknown), | ||||||||||||||||||
Type::Unknown => CallOutcome::callable(Type::Unknown), | ||||||||||||||||||
|
||||||||||||||||||
// TODO: union and intersection types, if they reduce to `Callable` | ||||||||||||||||||
Type::Union(_) => Some(Type::Unknown), | ||||||||||||||||||
Type::Intersection(_) => Some(Type::Unknown), | ||||||||||||||||||
Type::Union(union) => CallOutcome::union( | ||||||||||||||||||
self, | ||||||||||||||||||
union | ||||||||||||||||||
.elements(db) | ||||||||||||||||||
.iter() | ||||||||||||||||||
.map(|elem| elem.call(db, arg_types)) | ||||||||||||||||||
.collect::<Box<[CallOutcome<'db>]>>(), | ||||||||||||||||||
), | ||||||||||||||||||
|
||||||||||||||||||
_ => None, | ||||||||||||||||||
// TODO: intersection types | ||||||||||||||||||
Type::Intersection(_) => CallOutcome::callable(Type::Unknown), | ||||||||||||||||||
|
||||||||||||||||||
_ => CallOutcome::not_callable(self), | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
|
@@ -513,7 +540,7 @@ impl<'db> Type<'db> { | |||||||||||||||||
/// for y in x: | ||||||||||||||||||
/// pass | ||||||||||||||||||
/// ``` | ||||||||||||||||||
fn iterate(&self, db: &'db dyn Db) -> IterationOutcome<'db> { | ||||||||||||||||||
fn iterate(self, db: &'db dyn Db) -> IterationOutcome<'db> { | ||||||||||||||||||
if let Type::Tuple(tuple_type) = self { | ||||||||||||||||||
return IterationOutcome::Iterable { | ||||||||||||||||||
element_ty: UnionType::from_elements(db, &**tuple_type.elements(db)), | ||||||||||||||||||
|
@@ -526,18 +553,22 @@ impl<'db> Type<'db> { | |||||||||||||||||
|
||||||||||||||||||
let dunder_iter_method = iterable_meta_type.member(db, "__iter__"); | ||||||||||||||||||
if !dunder_iter_method.is_unbound() { | ||||||||||||||||||
let Some(iterator_ty) = dunder_iter_method.call(db) else { | ||||||||||||||||||
let CallOutcome::Callable { | ||||||||||||||||||
return_ty: iterator_ty, | ||||||||||||||||||
} = dunder_iter_method.call(db, &[]) | ||||||||||||||||||
else { | ||||||||||||||||||
return IterationOutcome::NotIterable { | ||||||||||||||||||
not_iterable_ty: *self, | ||||||||||||||||||
not_iterable_ty: self, | ||||||||||||||||||
}; | ||||||||||||||||||
}; | ||||||||||||||||||
|
||||||||||||||||||
let dunder_next_method = iterator_ty.to_meta_type(db).member(db, "__next__"); | ||||||||||||||||||
return dunder_next_method | ||||||||||||||||||
.call(db) | ||||||||||||||||||
.call(db, &[]) | ||||||||||||||||||
.return_ty(db) | ||||||||||||||||||
.map(|element_ty| IterationOutcome::Iterable { element_ty }) | ||||||||||||||||||
.unwrap_or(IterationOutcome::NotIterable { | ||||||||||||||||||
not_iterable_ty: *self, | ||||||||||||||||||
not_iterable_ty: self, | ||||||||||||||||||
}); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
|
@@ -550,10 +581,11 @@ impl<'db> Type<'db> { | |||||||||||||||||
let dunder_get_item_method = iterable_meta_type.member(db, "__getitem__"); | ||||||||||||||||||
|
||||||||||||||||||
dunder_get_item_method | ||||||||||||||||||
.call(db) | ||||||||||||||||||
.call(db, &[]) | ||||||||||||||||||
.return_ty(db) | ||||||||||||||||||
.map(|element_ty| IterationOutcome::Iterable { element_ty }) | ||||||||||||||||||
.unwrap_or(IterationOutcome::NotIterable { | ||||||||||||||||||
not_iterable_ty: *self, | ||||||||||||||||||
not_iterable_ty: self, | ||||||||||||||||||
}) | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
|
@@ -573,6 +605,7 @@ impl<'db> Type<'db> { | |||||||||||||||||
Type::BooleanLiteral(_) | ||||||||||||||||||
| Type::BytesLiteral(_) | ||||||||||||||||||
| Type::Function(_) | ||||||||||||||||||
| Type::RevealTypeFunction(_) | ||||||||||||||||||
| Type::Instance(_) | ||||||||||||||||||
| Type::Module(_) | ||||||||||||||||||
| Type::IntLiteral(_) | ||||||||||||||||||
|
@@ -595,7 +628,7 @@ impl<'db> Type<'db> { | |||||||||||||||||
Type::BooleanLiteral(_) => builtins_symbol_ty(db, "bool"), | ||||||||||||||||||
Type::BytesLiteral(_) => builtins_symbol_ty(db, "bytes"), | ||||||||||||||||||
Type::IntLiteral(_) => builtins_symbol_ty(db, "int"), | ||||||||||||||||||
Type::Function(_) => types_symbol_ty(db, "FunctionType"), | ||||||||||||||||||
Type::Function(_) | Type::RevealTypeFunction(_) => types_symbol_ty(db, "FunctionType"), | ||||||||||||||||||
Type::Module(_) => types_symbol_ty(db, "ModuleType"), | ||||||||||||||||||
Type::None => typeshed_symbol_ty(db, "NoneType"), | ||||||||||||||||||
// TODO not accurate if there's a custom metaclass... | ||||||||||||||||||
|
@@ -619,6 +652,152 @@ impl<'db> From<&Type<'db>> for Type<'db> { | |||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
#[derive(Debug, Clone, PartialEq, Eq)] | ||||||||||||||||||
enum CallOutcome<'db> { | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find the name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have any plans to address this. My document was just listing out pros and cons as I saw them of different options I'd considered in working on this, so as to be able to refer back to them later. My conclusion for now was to not do a context object, and stick with this style. It's very possible that we later decide it makes sense to do a type inference context object, but that's not clear to me yet and not something I plan to do in the short term. So if you don't like the name "Outcome" then we should brainstorm alternative name options, and we can change both |
||||||||||||||||||
Callable { | ||||||||||||||||||
return_ty: Type<'db>, | ||||||||||||||||||
}, | ||||||||||||||||||
RevealType { | ||||||||||||||||||
return_ty: Type<'db>, | ||||||||||||||||||
revealed_ty: Type<'db>, | ||||||||||||||||||
}, | ||||||||||||||||||
NotCallable { | ||||||||||||||||||
not_callable_ty: Type<'db>, | ||||||||||||||||||
}, | ||||||||||||||||||
Union { | ||||||||||||||||||
called_ty: Type<'db>, | ||||||||||||||||||
outcomes: Box<[CallOutcome<'db>]>, | ||||||||||||||||||
}, | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
impl<'db> CallOutcome<'db> { | ||||||||||||||||||
/// Create a new `CallOutcome::Callable` with given return type. | ||||||||||||||||||
fn callable(return_ty: Type<'db>) -> CallOutcome { | ||||||||||||||||||
CallOutcome::Callable { return_ty } | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
/// Create a new `CallOutcome::NotCallable` with given not-callable type. | ||||||||||||||||||
fn not_callable(not_callable_ty: Type<'db>) -> CallOutcome { | ||||||||||||||||||
CallOutcome::NotCallable { not_callable_ty } | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
/// Create a new `CallOutcome::RevealType` with given revealed and return types. | ||||||||||||||||||
fn revealed(return_ty: Type<'db>, revealed_ty: Type<'db>) -> CallOutcome<'db> { | ||||||||||||||||||
CallOutcome::RevealType { | ||||||||||||||||||
return_ty, | ||||||||||||||||||
revealed_ty, | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
/// Create a new `CallOutcome::Union` with given wrapped outcomes. | ||||||||||||||||||
fn union(called_ty: Type<'db>, outcomes: impl Into<Box<[CallOutcome<'db>]>>) -> CallOutcome { | ||||||||||||||||||
CallOutcome::Union { | ||||||||||||||||||
called_ty, | ||||||||||||||||||
outcomes: outcomes.into(), | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
/// Get the return type of the call, or `None` if not callable. | ||||||||||||||||||
fn return_ty(&self, db: &'db dyn Db) -> Option<Type<'db>> { | ||||||||||||||||||
match self { | ||||||||||||||||||
Self::Callable { return_ty } => Some(*return_ty), | ||||||||||||||||||
Self::RevealType { | ||||||||||||||||||
return_ty, | ||||||||||||||||||
revealed_ty: _, | ||||||||||||||||||
} => Some(*return_ty), | ||||||||||||||||||
Self::NotCallable { not_callable_ty: _ } => None, | ||||||||||||||||||
Self::Union { | ||||||||||||||||||
outcomes, | ||||||||||||||||||
called_ty: _, | ||||||||||||||||||
} => outcomes | ||||||||||||||||||
.iter() | ||||||||||||||||||
// If all outcomes are NotCallable, we return None; if some outcomes are callable | ||||||||||||||||||
// and some are not, we return a union including Unknown. | ||||||||||||||||||
.fold(None, |acc, outcome| { | ||||||||||||||||||
let ty = outcome.return_ty(db); | ||||||||||||||||||
match (acc, ty) { | ||||||||||||||||||
(None, None) => None, | ||||||||||||||||||
(None, Some(ty)) => Some(UnionBuilder::new(db).add(ty)), | ||||||||||||||||||
(Some(builder), ty) => Some(builder.add(ty.unwrap_or(Type::Unknown))), | ||||||||||||||||||
} | ||||||||||||||||||
}) | ||||||||||||||||||
.map(UnionBuilder::build), | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
/// Get the return type of the call, emitting diagnostics if needed. | ||||||||||||||||||
fn unwrap_with_diagnostic<'a>( | ||||||||||||||||||
&self, | ||||||||||||||||||
db: &'db dyn Db, | ||||||||||||||||||
node: ast::AnyNodeRef, | ||||||||||||||||||
builder: &'a mut TypeInferenceBuilder<'db>, | ||||||||||||||||||
) -> Type<'db> { | ||||||||||||||||||
match self { | ||||||||||||||||||
Self::Callable { return_ty } => *return_ty, | ||||||||||||||||||
Self::RevealType { | ||||||||||||||||||
return_ty, | ||||||||||||||||||
revealed_ty, | ||||||||||||||||||
} => { | ||||||||||||||||||
builder.add_diagnostic( | ||||||||||||||||||
node, | ||||||||||||||||||
"revealed-type", | ||||||||||||||||||
format_args!("Revealed type is '{}'.", revealed_ty.display(db)), | ||||||||||||||||||
); | ||||||||||||||||||
*return_ty | ||||||||||||||||||
} | ||||||||||||||||||
Self::NotCallable { not_callable_ty } => { | ||||||||||||||||||
builder.add_diagnostic( | ||||||||||||||||||
node, | ||||||||||||||||||
"call-non-callable", | ||||||||||||||||||
format_args!( | ||||||||||||||||||
"Object of type '{}' is not callable.", | ||||||||||||||||||
not_callable_ty.display(db) | ||||||||||||||||||
), | ||||||||||||||||||
); | ||||||||||||||||||
Type::Unknown | ||||||||||||||||||
} | ||||||||||||||||||
Self::Union { | ||||||||||||||||||
outcomes, | ||||||||||||||||||
called_ty, | ||||||||||||||||||
} => { | ||||||||||||||||||
let mut not_callable = vec![]; | ||||||||||||||||||
let mut union_builder = UnionBuilder::new(db); | ||||||||||||||||||
for outcome in &**outcomes { | ||||||||||||||||||
let return_ty = if let Self::NotCallable { not_callable_ty } = outcome { | ||||||||||||||||||
not_callable.push(*not_callable_ty); | ||||||||||||||||||
Type::Unknown | ||||||||||||||||||
} else { | ||||||||||||||||||
outcome.unwrap_with_diagnostic(db, node, builder) | ||||||||||||||||||
}; | ||||||||||||||||||
union_builder = union_builder.add(return_ty); | ||||||||||||||||||
} | ||||||||||||||||||
match not_callable[..] { | ||||||||||||||||||
[] => {} | ||||||||||||||||||
[elem] => builder.add_diagnostic( | ||||||||||||||||||
node, | ||||||||||||||||||
"call-non-callable", | ||||||||||||||||||
format_args!( | ||||||||||||||||||
"Union element '{}' of type '{}' is not callable.", | ||||||||||||||||||
elem.display(db), | ||||||||||||||||||
called_ty.display(db) | ||||||||||||||||||
), | ||||||||||||||||||
), | ||||||||||||||||||
_ => builder.add_diagnostic( | ||||||||||||||||||
node, | ||||||||||||||||||
"call-non-callable", | ||||||||||||||||||
format_args!( | ||||||||||||||||||
"Union elements {} of type '{}' are not callable.", | ||||||||||||||||||
not_callable.display(db), | ||||||||||||||||||
called_ty.display(db) | ||||||||||||||||||
), | ||||||||||||||||||
), | ||||||||||||||||||
} | ||||||||||||||||||
union_builder.build() | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||||||||||||||||||
enum IterationOutcome<'db> { | ||||||||||||||||||
Iterable { element_ty: Type<'db> }, | ||||||||||||||||||
|
@@ -654,6 +833,14 @@ pub struct FunctionType<'db> { | |||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
impl<'db> FunctionType<'db> { | ||||||||||||||||||
/// Return true if this is a standard library function with given module name and name. | ||||||||||||||||||
pub(crate) fn is_stdlib_symbol(self, db: &'db dyn Db, module_name: &str, name: &str) -> bool { | ||||||||||||||||||
name == self.name(db) | ||||||||||||||||||
&& file_to_module(db, self.definition(db).file(db)).is_some_and(|module| { | ||||||||||||||||||
module.search_path().is_standard_library() && module.name() == module_name | ||||||||||||||||||
}) | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
pub fn has_decorator(self, db: &dyn Db, decorator: Type<'_>) -> bool { | ||||||||||||||||||
self.decorators(db).contains(&decorator) | ||||||||||||||||||
} | ||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to store the entire
FunctionType
forRevealType
? Don't we know its exact signature?Also, kind of funny to have this as a dedicated type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, agree with Micha that there's not much use storing inner data for this variant, since we already have the complexity of having an extra variant (wrapping inner data doesn't really save us any complexity, I don't think), everything about
reveal_type
's special behaviour will have to be hardcoded in by us anyway, and the signature is known and guaranteed to never change.It surprised me initially that this was the approach used, since conceptually it's really the symbol of
reveal_type
that's special rather than the type ofreveal_type
. But I suppose that unless it's represented as a separate variant here, it's hard forType::Call
to operate differently when applied torevealed_type
as opposed to other functions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have this as a distinct Type variant?
Conceptually, singleton types (which include Class and Function) are already the same thing as "a symbol" -- they represent a single Python class or function object, defined at a single location, and these types allow us to track this identity across imports (including with aliasing), assignments, etc. So identifying
reveal_type
via the type system is a natural fit; I don't really see what the alternative would even be.In principle, we wouldn't need the extra variant for this;
Type::Function
has all the information we need already. But practically speaking, it would be a bad idea for performance if we had to add an extra check "wait, is this functionreveal_type
??" at every single call site of any function;is_stdlib_symbol
is not free, differentiating an enum tag is much much cheaper. Using a distinct variant allows us to make theis_stdlib_symbol
check only at function-definition time, which is much less frequent than function-call time.Should we store the inner
FunctionType
insideType::RevealTypeFunction
?I don't have super-strong feelings here, but I think the overall cost-benefit for doing this is positive.
The cost is very low; a
FunctionType
is just a u32 id, andRevealTypeFunction
is not going to be a super common type. I would venture to say the cost of doing this is effectively zero.And the benefit is not zero. We don't have to hardcode everything about
RevealTypeFunction
. If we store the innerFunctionType
, we can in almost every case just fall back to the normalFunction
handling, and only add specialreveal_type
behavior where it is actually special. For instance, I would rather not hardcode call-signature validation, I'd rather share as much as possible withType::Function
.So yes, we could skip storing the inner
FunctionType
, and instead hardcode literally everything aboutRevealTypeFunction
. But I don't see why we'd want to do this, when we don't have to.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternative would be this diff, right? All the red-knot tests pass if I apply this diff to
main
, and to me it seems much simpler conceptually. Perhaps it adds a performance cost, though, because you have to check whether the function is thereveal_type
function every time you runType::Call
.Diff demonstrating an implementation without a new `Type` variant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, yeah -- that's what I addressed in my second paragraph. I guess my first paragraph didn't make much sense / wasn't relevant, because you're not suggesting not using types at all.
This approach is definitely simpler, but I considered it a performance nonstarter to add the overhead of
is_stdlib_symbol
to every Type::call. Perhaps that's premature optimization.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree the code maintenance is a bigger concern as we accumulate lots of these.
If the contained data inside enum variants is large, that will dominate the overall size of the enum. But in this case the contained data is small (usually just a u32 id; I think
IntLiteral
is currently our largest variant, as it contains ani64
), so the need to find niches to discriminate the enum variants themselves can be relevant. Though in practice in this case there just aren't any niches available in ani64
, so the enum requires another entire 8 bytes for the discriminant (doubling the overall size ofType
to 16 bytes), and we are certainly not going to reach 2^64 variants 😆There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe a
FunctionType
instance is actually just au32
, though, since it's a salsa-tracked struct — right? So adding a boolean field toFunctionType
would increase the size of the underlying data stored in the salsa cache for eachFunctionType
instance, but wouldn't actually increase the size of theType
enum...?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's right. I never mentioned the size of the Type enum in relation to adding a boolean to FunctionType. I only brought up the size of the Type enum in relation to having a ton of extra Type variants in the future, and it's probably not even really relevant there.
Making every FunctionType bigger is less bad than making the Type enum bigger would be, but we will have quite a large number of FunctionType in the db.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, that was a sloppy misreading of what you said there!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries!