Skip to content

[red-knot] Reduce usage of From<Type> implementations when working with Symbols #16076

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Feb 11, 2025
Merged

[red-knot] Reduce usage of From<Type> implementations when working with Symbols #16076

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5762d31
[red-knot] Reduce usage of `From<Type>` implementations when working …
AlexWaygood Feb 10, 2025
426472b
not so smart
AlexWaygood Feb 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion crates/red_knot_python_semantic/src/symbol.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::{
types::{Type, UnionType},
types::{todo_type, Type, UnionType},
Db,
};

Expand Down Expand Up @@ -33,6 +33,17 @@ pub(crate) enum Symbol<'db> {
}

impl<'db> Symbol<'db> {
/// Smart constructor that creates a `Symbol` with boundness [`Boundness::Bound`].
pub(crate) fn bound(ty: impl Into<Type<'db>>) -> Self {
Symbol::Type(ty.into(), Boundness::Bound)
}

/// Smart constructor that creates a [`Symbol`] with a [`crate::types::TodoType`] type
/// and boundness [`Boundness::Bound`].
pub(crate) fn todo(message: &'static str) -> Self {
Symbol::Type(todo_type!(message), Boundness::Bound)
}

pub(crate) fn is_unbound(&self) -> bool {
matches!(self, Symbol::Unbound)
}
Expand Down
118 changes: 62 additions & 56 deletions crates/red_knot_python_semantic/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ fn symbol<'db>(db: &'db dyn Db, scope: ScopeId<'db>, name: &str) -> Symbol<'db>
}
// Symbol is possibly undeclared and (possibly) bound
Symbol::Type(inferred_ty, boundness) => Symbol::Type(
UnionType::from_elements(db, [inferred_ty, declared_ty].iter().copied()),
UnionType::from_elements(db, [inferred_ty, declared_ty]),
boundness,
),
}
Expand All @@ -160,7 +160,7 @@ fn symbol<'db>(db: &'db dyn Db, scope: ScopeId<'db>, name: &str) -> Symbol<'db>
Err((declared_ty, _)) => {
// Intentionally ignore conflicting declared types; that's not our problem,
// it's the problem of the module we are importing from.
declared_ty.inner_type().into()
Symbol::bound(declared_ty.inner_type())
}
}

Expand Down Expand Up @@ -188,18 +188,15 @@ fn symbol<'db>(db: &'db dyn Db, scope: ScopeId<'db>, name: &str) -> Symbol<'db>
&& file_to_module(db, scope.file(db))
.is_some_and(|module| module.is_known(KnownModule::Typing))
{
return Symbol::Type(Type::BooleanLiteral(true), Boundness::Bound);
return Symbol::bound(Type::BooleanLiteral(true));
}
if name == "platform"
&& file_to_module(db, scope.file(db))
.is_some_and(|module| module.is_known(KnownModule::Sys))
{
match Program::get(db).python_platform(db) {
crate::PythonPlatform::Identifier(platform) => {
return Symbol::Type(
Type::StringLiteral(StringLiteralType::new(db, platform.as_str())),
Boundness::Bound,
);
return Symbol::bound(Type::string_literal(db, platform.as_str()));
}
crate::PythonPlatform::All => {
// Fall through to the looked up type
Expand Down Expand Up @@ -402,9 +399,16 @@ fn symbol_from_bindings<'db>(
/// If we look up the declared type of `variable` in the scope of class `C`, we will get
/// the type `int`, a "declaredness" of [`Boundness::PossiblyUnbound`], and the information
/// that this comes with a [`TypeQualifiers::CLASS_VAR`] type qualifier.
#[derive(Debug)]
pub(crate) struct SymbolAndQualifiers<'db>(Symbol<'db>, TypeQualifiers);

impl SymbolAndQualifiers<'_> {
/// Smart constructor to create a [`SymbolAndQualifiers`] instance with a [`TodoType`] type
/// and no qualifiers.
fn todo(message: &'static str) -> Self {
Self(Symbol::todo(message), TypeQualifiers::empty())
}

fn is_class_var(&self) -> bool {
self.1.contains(TypeQualifiers::CLASS_VAR)
}
Expand All @@ -420,12 +424,6 @@ impl<'db> From<Symbol<'db>> for SymbolAndQualifiers<'db> {
}
}

impl<'db> From<Type<'db>> for SymbolAndQualifiers<'db> {
fn from(ty: Type<'db>) -> Self {
SymbolAndQualifiers(ty.into(), TypeQualifiers::empty())
}
}

/// The result of looking up a declared type from declarations; see [`symbol_from_declarations`].
type SymbolFromDeclarationsResult<'db> =
Result<SymbolAndQualifiers<'db>, (TypeAndQualifiers<'db>, Box<[Type<'db>]>)>;
Expand Down Expand Up @@ -561,6 +559,11 @@ macro_rules! todo_type {
$crate::types::TodoType::Message($message),
))
};
($message:ident) => {
$crate::types::Type::Dynamic($crate::types::DynamicType::Todo(
$crate::types::TodoType::Message($message),
))
};
}

#[cfg(not(debug_assertions))]
Expand All @@ -571,6 +574,9 @@ macro_rules! todo_type {
($message:literal) => {
$crate::types::Type::Dynamic($crate::types::DynamicType::Todo(crate::types::TodoType))
};
($message:ident) => {
$crate::types::Type::Dynamic($crate::types::DynamicType::Todo(crate::types::TodoType))
};
}

pub(crate) use todo_type;
Expand Down Expand Up @@ -1684,17 +1690,17 @@ impl<'db> Type<'db> {
#[must_use]
pub(crate) fn member(&self, db: &'db dyn Db, name: &str) -> Symbol<'db> {
if name == "__class__" {
return self.to_meta_type(db).into();
return Symbol::bound(self.to_meta_type(db));
}

match self {
Type::Dynamic(_) => self.into(),
Type::Dynamic(_) => Symbol::bound(self),

Type::Never => todo_type!("attribute lookup on Never").into(),
Type::Never => Symbol::todo("attribute lookup on Never"),

Type::FunctionLiteral(_) => match name {
"__get__" => todo_type!("`__get__` method on functions").into(),
"__call__" => todo_type!("`__call__` method on functions").into(),
"__get__" => Symbol::todo("`__get__` method on functions"),
"__call__" => Symbol::todo("`__call__` method on functions"),
_ => KnownClass::FunctionType.to_instance(db).member(db, name),
},

Expand All @@ -1707,12 +1713,12 @@ impl<'db> Type<'db> {
Type::KnownInstance(known_instance) => known_instance.member(db, name),

Type::Instance(InstanceType { class }) => match (class.known(db), name) {
(Some(KnownClass::VersionInfo), "major") => {
Type::IntLiteral(Program::get(db).python_version(db).major.into()).into()
}
(Some(KnownClass::VersionInfo), "minor") => {
Type::IntLiteral(Program::get(db).python_version(db).minor.into()).into()
}
(Some(KnownClass::VersionInfo), "major") => Symbol::bound(Type::IntLiteral(
Program::get(db).python_version(db).major.into(),
)),
(Some(KnownClass::VersionInfo), "minor") => Symbol::bound(Type::IntLiteral(
Program::get(db).python_version(db).minor.into(),
)),
_ => {
let SymbolAndQualifiers(symbol, _) = class.instance_member(db, name);
symbol
Expand Down Expand Up @@ -1758,30 +1764,30 @@ impl<'db> Type<'db> {
Type::Intersection(_) => {
// TODO perform the get_member on each type in the intersection
// TODO return the intersection of those results
todo_type!("Attribute access on `Intersection` types").into()
Symbol::todo("Attribute access on `Intersection` types")
}

Type::IntLiteral(_) => match name {
"real" | "numerator" => self.into(),
"real" | "numerator" => Symbol::bound(self),
// TODO more attributes could probably be usefully special-cased
_ => KnownClass::Int.to_instance(db).member(db, name),
},

Type::BooleanLiteral(bool_value) => match name {
"real" | "numerator" => Type::IntLiteral(i64::from(*bool_value)).into(),
"real" | "numerator" => Symbol::bound(Type::IntLiteral(i64::from(*bool_value))),
_ => KnownClass::Bool.to_instance(db).member(db, name),
},

Type::StringLiteral(_) => {
// TODO defer to `typing.LiteralString`/`builtins.str` methods
// from typeshed's stubs
todo_type!("Attribute access on `StringLiteral` types").into()
Symbol::todo("Attribute access on `StringLiteral` types")
}

Type::LiteralString => {
// TODO defer to `typing.LiteralString`/`builtins.str` methods
// from typeshed's stubs
todo_type!("Attribute access on `LiteralString` types").into()
Symbol::todo("Attribute access on `LiteralString` types")
}

Type::BytesLiteral(_) => KnownClass::Bytes.to_instance(db).member(db, name),
Expand All @@ -1793,13 +1799,13 @@ impl<'db> Type<'db> {

Type::Tuple(_) => {
// TODO: implement tuple methods
todo_type!("Attribute access on heterogeneous tuple types").into()
Symbol::todo("Attribute access on heterogeneous tuple types")
}

Type::AlwaysTruthy | Type::AlwaysFalsy => match name {
"__bool__" => {
// TODO should be `Callable[[], Literal[True/False]]`
todo_type!("`__bool__` for `AlwaysTruthy`/`AlwaysFalsy` Type variants").into()
Symbol::todo("`__bool__` for `AlwaysTruthy`/`AlwaysFalsy` Type variants")
}
_ => KnownClass::Object.to_instance(db).member(db, name),
},
Expand Down Expand Up @@ -2524,18 +2530,6 @@ impl<'db> From<&Type<'db>> for Type<'db> {
}
}

impl<'db> From<Type<'db>> for Symbol<'db> {
fn from(value: Type<'db>) -> Self {
Symbol::Type(value, Boundness::Bound)
}
}

impl<'db> From<&Type<'db>> for Symbol<'db> {
fn from(value: &Type<'db>) -> Self {
Self::from(*value)
}
}

#[derive(Copy, Clone, Debug, Eq, Hash, PartialEq)]
pub enum DynamicType {
// An explicitly annotated `typing.Any`
Expand Down Expand Up @@ -2568,7 +2562,7 @@ impl std::fmt::Display for DynamicType {

bitflags! {
/// Type qualifiers that appear in an annotation expression.
#[derive(Copy, Clone, Debug, Eq, PartialEq)]
#[derive(Copy, Clone, Debug, Eq, PartialEq, Default)]
pub(crate) struct TypeQualifiers: u8 {
/// `typing.ClassVar`
const CLASS_VAR = 1 << 0;
Expand All @@ -2595,6 +2589,14 @@ impl<'db> TypeAndQualifiers<'db> {
Self { inner, qualifiers }
}

/// Smart constructor to create a [`TypeAndQualifiers`] instance with type `Unknown` and no qualifiers.
pub(crate) fn unknown() -> Self {
Self {
inner: Type::unknown(),
qualifiers: TypeQualifiers::empty(),
}
}

/// Forget about type qualifiers and only return the inner type.
pub(crate) fn inner_type(&self) -> Type<'db> {
self.inner
Expand Down Expand Up @@ -3326,7 +3328,7 @@ impl<'db> KnownInstanceType<'db> {
(Self::TypeAliasType(alias), "__name__") => Type::string_literal(db, alias.name(db)),
_ => return self.instance_fallback(db).member(db, name),
};
ty.into()
Symbol::bound(ty)
}
}

Expand Down Expand Up @@ -3784,8 +3786,7 @@ impl<'db> ModuleLiteralType<'db> {
full_submodule_name.extend(&submodule_name);
if imported_submodules.contains(&full_submodule_name) {
if let Some(submodule) = resolve_module(db, &full_submodule_name) {
let submodule_ty = Type::module_literal(db, importing_file, submodule);
return Symbol::Type(submodule_ty, Boundness::Bound);
return Symbol::bound(Type::module_literal(db, importing_file, submodule));
}
}
}
Expand Down Expand Up @@ -4114,7 +4115,7 @@ impl<'db> Class<'db> {
pub(crate) fn class_member(self, db: &'db dyn Db, name: &str) -> Symbol<'db> {
if name == "__mro__" {
let tuple_elements = self.iter_mro(db).map(Type::from);
return TupleType::from_elements(db, tuple_elements).into();
return Symbol::bound(TupleType::from_elements(db, tuple_elements));
}

for superclass in self.iter_mro(db) {
Expand Down Expand Up @@ -4154,7 +4155,9 @@ impl<'db> Class<'db> {
for superclass in self.iter_mro(db) {
match superclass {
ClassBase::Dynamic(_) => {
return todo_type!("instance attribute on class with dynamic base").into();
return SymbolAndQualifiers::todo(
"instance attribute on class with dynamic base",
);
}
ClassBase::Class(class) => {
if let member @ SymbolAndQualifiers(Symbol::Type(_, _), _) =
Expand Down Expand Up @@ -4204,7 +4207,7 @@ impl<'db> Class<'db> {
.and_then(|assignments| assignments.get(name))
else {
if inferred_type_from_class_body.is_some() {
return union_of_inferred_types.build().into();
return Symbol::bound(union_of_inferred_types.build());
}
return Symbol::Unbound;
};
Expand All @@ -4221,7 +4224,7 @@ impl<'db> Class<'db> {
let annotation_ty = infer_expression_type(db, *annotation);

// TODO: check if there are conflicting declarations
return annotation_ty.into();
return Symbol::bound(annotation_ty);
}
AttributeAssignment::Unannotated { value } => {
// We found an un-annotated attribute assignment of the form:
Expand Down Expand Up @@ -4261,7 +4264,7 @@ impl<'db> Class<'db> {
}
}

union_of_inferred_types.build().into()
Symbol::bound(union_of_inferred_types.build())
}

/// A helper function for `instance_member` that looks up the `name` attribute only on
Expand Down Expand Up @@ -4290,12 +4293,12 @@ impl<'db> Class<'db> {
// just a temporary heuristic to provide a broad categorization into properties
// and non-property methods.
if function.has_decorator(db, KnownClass::Property.to_class_literal(db)) {
todo_type!("@property").into()
SymbolAndQualifiers::todo("@property")
} else {
todo_type!("bound method").into()
SymbolAndQualifiers::todo("bound method")
}
} else {
SymbolAndQualifiers(Symbol::Type(declared_ty, Boundness::Bound), qualifiers)
SymbolAndQualifiers(Symbol::bound(declared_ty), qualifiers)
}
}
Ok(SymbolAndQualifiers(Symbol::Unbound, _)) => {
Expand All @@ -4310,7 +4313,10 @@ impl<'db> Class<'db> {
}
Err((declared_ty, _conflicting_declarations)) => {
// There are conflicting declarations for this attribute in the class body.
SymbolAndQualifiers(declared_ty.inner_type().into(), declared_ty.qualifiers())
SymbolAndQualifiers(
Symbol::bound(declared_ty.inner_type()),
declared_ty.qualifiers(),
)
}
}
} else {
Expand Down
Loading
Loading