From 88d9bb191bdf0d3742204c516d3e0de276a5dfe7 Mon Sep 17 00:00:00 2001 From: David Peter Date: Mon, 4 Nov 2024 14:00:05 +0100 Subject: [PATCH] [red-knot] Remove `Type::None` (#14024) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Removes `Type::None` in favor of `KnownClass::NoneType.to_instance(…)`. closes #13670 ## Performance There is a -4% performance regression on our red-knot benchmark. This is due to the fact that we now have to import `_typeshed` as a module, and infer types. ## Test Plan Existing tests pass. --- crates/red_knot_python_semantic/src/types.rs | 79 ++++++++++++------- .../src/types/builder.rs | 16 ++-- .../src/types/display.rs | 8 +- .../src/types/infer.rs | 12 ++- .../src/types/narrow.rs | 4 +- 5 files changed, 72 insertions(+), 47 deletions(-) diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index 75eb6688f5d4c3..fd4d34e96843d9 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -308,8 +308,6 @@ pub enum Type<'db> { /// Unknown type (either no annotation, or some kind of type error). /// Equivalent to Any, or possibly to object in strict mode Unknown, - /// The None object -- TODO remove this in favor of Instance(types.NoneType) - None, /// Temporary type for symbols that can't be inferred yet because of missing implementations. /// Behaves equivalently to `Any`. /// @@ -564,10 +562,17 @@ impl<'db> Type<'db> { } /// Return true if this type is equivalent to type `other`. - pub(crate) fn is_equivalent_to(self, _db: &'db dyn Db, other: Type<'db>) -> bool { + pub(crate) fn is_equivalent_to(self, db: &'db dyn Db, other: Type<'db>) -> bool { // TODO equivalent but not identical structural types, differently-ordered unions and // intersections, other cases? + + // TODO: The following is a workaround that is required to unify the two different + // versions of `NoneType` in typeshed. This should not be required anymore once we + // understand `sys.version_info` branches. self == other + || matches!((self, other), + (Type::Instance(self_class), Type::Instance(target_class)) + if self_class.is_known(db, KnownClass::NoneType) && target_class.is_known(db, KnownClass::NoneType)) } /// Return true if this type and `other` have no common elements. @@ -603,8 +608,7 @@ impl<'db> Type<'db> { } ( - left @ (Type::None - | Type::BooleanLiteral(..) + left @ (Type::BooleanLiteral(..) | Type::IntLiteral(..) | Type::StringLiteral(..) | Type::BytesLiteral(..) @@ -612,8 +616,7 @@ impl<'db> Type<'db> { | Type::FunctionLiteral(..) | Type::ModuleLiteral(..) | Type::ClassLiteral(..)), - right @ (Type::None - | Type::BooleanLiteral(..) + right @ (Type::BooleanLiteral(..) | Type::IntLiteral(..) | Type::StringLiteral(..) | Type::BytesLiteral(..) @@ -623,13 +626,20 @@ impl<'db> Type<'db> { | Type::ClassLiteral(..)), ) => left != right, - (Type::None, Type::Instance(class_type)) | (Type::Instance(class_type), Type::None) => { + (Type::Instance(class_none), Type::Instance(class_other)) + | (Type::Instance(class_other), Type::Instance(class_none)) + if class_none.is_known(db, KnownClass::NoneType) => + { !matches!( - class_type.known(db), + class_other.known(db), Some(KnownClass::NoneType | KnownClass::Object) ) } - (Type::None, _) | (_, Type::None) => true, + (Type::Instance(class_none), _) | (_, Type::Instance(class_none)) + if class_none.is_known(db, KnownClass::NoneType) => + { + true + } (Type::BooleanLiteral(..), Type::Instance(class_type)) | (Type::Instance(class_type), Type::BooleanLiteral(..)) => !matches!( @@ -687,8 +697,9 @@ impl<'db> Type<'db> { (Type::Instance(..), Type::Instance(..)) => { // TODO: once we have support for `final`, there might be some cases where - // we can determine that two types are disjoint. For non-final classes, we - // return false (multiple inheritance). + // we can determine that two types are disjoint. Once we do this, some cases + // above (e.g. NoneType) can be removed. For non-final classes, we return + // false (multiple inheritance). // TODO: is there anything specific to do for instances of KnownClass::Type? @@ -726,13 +737,12 @@ impl<'db> Type<'db> { /// /// Note: This function aims to have no false positives, but might return `false` /// for more complicated types that are actually singletons. - pub(crate) fn is_singleton(self) -> bool { + pub(crate) fn is_singleton(self, db: &'db dyn Db) -> bool { match self { Type::Any | Type::Never | Type::Unknown | Type::Todo - | Type::Instance(..) // TODO some instance types can be singleton types (EllipsisType, NotImplementedType) | Type::IntLiteral(..) | Type::StringLiteral(..) | Type::BytesLiteral(..) @@ -743,7 +753,14 @@ impl<'db> Type<'db> { // are both of type Literal[345], for example. false } - Type::None | Type::BooleanLiteral(_) | Type::FunctionLiteral(..) | Type::ClassLiteral(..) | Type::ModuleLiteral(..) => true, + Type::BooleanLiteral(_) + | Type::FunctionLiteral(..) + | Type::ClassLiteral(..) + | Type::ModuleLiteral(..) => true, + Type::Instance(class) => { + // TODO some more instance types can be singleton types (EllipsisType, NotImplementedType) + matches!(class.known(db), Some(KnownClass::NoneType)) + } Type::Tuple(..) => { // The empty tuple is a singleton on CPython and PyPy, but not on other Python // implementations such as GraalPy. Its *use* as a singleton is discouraged and @@ -774,8 +791,7 @@ impl<'db> Type<'db> { /// Return true if this type is non-empty and all inhabitants of this type compare equal. pub(crate) fn is_single_valued(self, db: &'db dyn Db) -> bool { match self { - Type::None - | Type::FunctionLiteral(..) + Type::FunctionLiteral(..) | Type::ModuleLiteral(..) | Type::ClassLiteral(..) | Type::IntLiteral(..) @@ -835,10 +851,6 @@ impl<'db> Type<'db> { Type::Todo.into() } Type::Unknown => Type::Unknown.into(), - Type::None => { - // TODO: attribute lookup on None type - Type::Todo.into() - } Type::FunctionLiteral(_) => { // TODO: attribute lookup on function type Type::Todo.into() @@ -963,7 +975,6 @@ impl<'db> Type<'db> { fn bool(&self, db: &'db dyn Db) -> Truthiness { match self { Type::Any | Type::Todo | Type::Never | Type::Unknown => Truthiness::Ambiguous, - Type::None => Truthiness::AlwaysFalse, Type::FunctionLiteral(_) => Truthiness::AlwaysTrue, Type::ModuleLiteral(_) => Truthiness::AlwaysTrue, Type::ClassLiteral(_) => { @@ -971,10 +982,15 @@ impl<'db> Type<'db> { // More info in https://docs.python.org/3/library/stdtypes.html#truth-value-testing Truthiness::Ambiguous } - Type::Instance(_) => { + Type::Instance(class) => { // TODO: lookup `__bool__` and `__len__` methods on the instance's class // More info in https://docs.python.org/3/library/stdtypes.html#truth-value-testing - Truthiness::Ambiguous + // For now, we only special-case some builtin classes + if class.is_known(db, KnownClass::NoneType) { + Truthiness::AlwaysFalse + } else { + Truthiness::Ambiguous + } } Type::Union(union) => { let union_elements = union.elements(db); @@ -1164,11 +1180,15 @@ impl<'db> Type<'db> { | Type::StringLiteral(_) | Type::SliceLiteral(_) | Type::Tuple(_) - | Type::LiteralString - | Type::None => Type::Unknown, + | Type::LiteralString => Type::Unknown, } } + /// The type `NoneType` / `None` + pub fn none(db: &'db dyn Db) -> Type<'db> { + KnownClass::NoneType.to_instance(db) + } + /// Given a type that is assumed to represent an instance of a class, /// return a type that represents that class itself. #[must_use] @@ -1184,7 +1204,6 @@ impl<'db> Type<'db> { Type::FunctionLiteral(_) => KnownClass::FunctionType.to_class(db), Type::ModuleLiteral(_) => KnownClass::ModuleType.to_class(db), Type::Tuple(_) => KnownClass::Tuple.to_class(db), - Type::None => KnownClass::NoneType.to_class(db), // TODO not accurate if there's a custom metaclass... Type::ClassLiteral(_) => KnownClass::Type.to_class(db), // TODO can we do better here? `type[LiteralString]`? @@ -2026,7 +2045,7 @@ mod tests { match self { Ty::Never => Type::Never, Ty::Unknown => Type::Unknown, - Ty::None => Type::None, + Ty::None => Type::none(db), Ty::Any => Type::Any, Ty::Todo => Type::Todo, Ty::IntLiteral(n) => Type::IntLiteral(n), @@ -2266,7 +2285,7 @@ mod tests { fn is_singleton(from: Ty) { let db = setup_db(); - assert!(from.into_type(&db).is_singleton()); + assert!(from.into_type(&db).is_singleton(&db)); } #[test_case(Ty::None)] @@ -2304,7 +2323,7 @@ mod tests { fn is_not_singleton(from: Ty) { let db = setup_db(); - assert!(!from.into_type(&db).is_singleton()); + assert!(!from.into_type(&db).is_singleton(&db)); } #[test_case(Ty::IntLiteral(1); "is_int_literal_truthy")] diff --git a/crates/red_knot_python_semantic/src/types/builder.rs b/crates/red_knot_python_semantic/src/types/builder.rs index f36bb4d2e6d824..2fe9504e6fe6b1 100644 --- a/crates/red_knot_python_semantic/src/types/builder.rs +++ b/crates/red_knot_python_semantic/src/types/builder.rs @@ -675,8 +675,8 @@ mod tests { fn build_intersection_self_negation() { let db = setup_db(); let ty = IntersectionBuilder::new(&db) - .add_positive(Type::None) - .add_negative(Type::None) + .add_positive(Type::none(&db)) + .add_negative(Type::none(&db)) .build(); assert_eq!(ty, Type::Never); @@ -686,18 +686,18 @@ mod tests { fn build_intersection_simplify_negative_never() { let db = setup_db(); let ty = IntersectionBuilder::new(&db) - .add_positive(Type::None) + .add_positive(Type::none(&db)) .add_negative(Type::Never) .build(); - assert_eq!(ty, Type::None); + assert_eq!(ty, Type::none(&db)); } #[test] fn build_intersection_simplify_positive_never() { let db = setup_db(); let ty = IntersectionBuilder::new(&db) - .add_positive(Type::None) + .add_positive(Type::none(&db)) .add_positive(Type::Never) .build(); @@ -709,14 +709,14 @@ mod tests { let db = setup_db(); let ty = IntersectionBuilder::new(&db) - .add_negative(Type::None) + .add_negative(Type::none(&db)) .add_positive(Type::IntLiteral(1)) .build(); assert_eq!(ty, Type::IntLiteral(1)); let ty = IntersectionBuilder::new(&db) .add_positive(Type::IntLiteral(1)) - .add_negative(Type::None) + .add_negative(Type::none(&db)) .build(); assert_eq!(ty, Type::IntLiteral(1)); } @@ -875,7 +875,7 @@ mod tests { let db = setup_db(); let t1 = Type::IntLiteral(1); - let t2 = Type::None; + let t2 = Type::none(&db); let ty = IntersectionBuilder::new(&db) .add_positive(t1) diff --git a/crates/red_knot_python_semantic/src/types/display.rs b/crates/red_knot_python_semantic/src/types/display.rs index 376340dc5d49f6..bea1f1646fbd25 100644 --- a/crates/red_knot_python_semantic/src/types/display.rs +++ b/crates/red_knot_python_semantic/src/types/display.rs @@ -6,7 +6,7 @@ use ruff_db::display::FormatterJoinExtension; use ruff_python_ast::str::Quote; use ruff_python_literal::escape::AsciiEscape; -use crate::types::{IntersectionType, Type, UnionType}; +use crate::types::{IntersectionType, KnownClass, Type, UnionType}; use crate::Db; use rustc_hash::FxHashMap; @@ -64,7 +64,9 @@ impl Display for DisplayRepresentation<'_> { Type::Any => f.write_str("Any"), Type::Never => f.write_str("Never"), Type::Unknown => f.write_str("Unknown"), - Type::None => f.write_str("None"), + Type::Instance(class) if class.is_known(self.db, KnownClass::NoneType) => { + f.write_str("None") + } // `[Type::Todo]`'s display should be explicit that is not a valid display of // any other type Type::Todo => f.write_str("@Todo"), @@ -380,7 +382,7 @@ mod tests { global_symbol(&db, mod_file, "bar").expect_type(), global_symbol(&db, mod_file, "B").expect_type(), Type::BooleanLiteral(true), - Type::None, + Type::none(&db), ]; let union = UnionType::from_elements(&db, union_elements).expect_union(); let display = format!("{}", union.display(&db)); diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index c07472c5bbe85e..43ad1ab30cb7b4 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -1149,7 +1149,12 @@ impl<'db> TypeInferenceBuilder<'db> { if exit_ty .call( self.db, - &[context_manager_ty, Type::None, Type::None, Type::None], + &[ + context_manager_ty, + Type::none(self.db), + Type::none(self.db), + Type::none(self.db), + ], ) .return_ty_result( self.db, @@ -1877,7 +1882,7 @@ impl<'db> TypeInferenceBuilder<'db> { fn infer_expression(&mut self, expression: &ast::Expr) -> Type<'db> { let ty = match expression { - ast::Expr::NoneLiteral(ast::ExprNoneLiteral { range: _ }) => Type::None, + ast::Expr::NoneLiteral(ast::ExprNoneLiteral { range: _ }) => Type::none(self.db), ast::Expr::NumberLiteral(literal) => self.infer_number_literal_expression(literal), ast::Expr::BooleanLiteral(literal) => self.infer_boolean_literal_expression(literal), ast::Expr::StringLiteral(literal) => self.infer_string_literal_expression(literal), @@ -3582,7 +3587,6 @@ impl<'db> TypeInferenceBuilder<'db> { Err(_) => SliceArg::Unsupported, }, Some(Type::BooleanLiteral(b)) => SliceArg::Arg(Some(i32::from(b))), - Some(Type::None) => SliceArg::Arg(None), Some(Type::Instance(class)) if class.is_known(self.db, KnownClass::NoneType) => { SliceArg::Arg(None) } @@ -3686,7 +3690,7 @@ impl<'db> TypeInferenceBuilder<'db> { .to_instance(self.db) } - ast::Expr::NoneLiteral(_literal) => Type::None, + ast::Expr::NoneLiteral(_literal) => Type::none(self.db), // TODO: parse the expression and check whether it is a string annotation. // https://typing.readthedocs.io/en/latest/spec/annotations.html#string-annotations diff --git a/crates/red_knot_python_semantic/src/types/narrow.rs b/crates/red_knot_python_semantic/src/types/narrow.rs index 57798a9427ea20..f3d276f17236a0 100644 --- a/crates/red_knot_python_semantic/src/types/narrow.rs +++ b/crates/red_knot_python_semantic/src/types/narrow.rs @@ -242,7 +242,7 @@ impl<'db> NarrowingConstraintsBuilder<'db> { match if is_positive { *op } else { op.negate() } { ast::CmpOp::IsNot => { - if rhs_ty.is_singleton() { + if rhs_ty.is_singleton(self.db) { let ty = IntersectionBuilder::new(self.db) .add_negative(rhs_ty) .build(); @@ -316,7 +316,7 @@ impl<'db> NarrowingConstraintsBuilder<'db> { let symbol = self.symbols().symbol_id_by_name(id).unwrap(); let ty = match pattern.value { - ast::Singleton::None => Type::None, + ast::Singleton::None => Type::none(self.db), ast::Singleton::True => Type::BooleanLiteral(true), ast::Singleton::False => Type::BooleanLiteral(false), };