Skip to content

Commit

Permalink
[red-knot] Remove Type::None (#14024)
Browse files Browse the repository at this point in the history
## 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.
  • Loading branch information
sharkdp authored Nov 4, 2024
1 parent e302c2d commit 88d9bb1
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 47 deletions.
79 changes: 49 additions & 30 deletions crates/red_knot_python_semantic/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
///
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -603,17 +608,15 @@ impl<'db> Type<'db> {
}

(
left @ (Type::None
| Type::BooleanLiteral(..)
left @ (Type::BooleanLiteral(..)
| Type::IntLiteral(..)
| Type::StringLiteral(..)
| Type::BytesLiteral(..)
| Type::SliceLiteral(..)
| Type::FunctionLiteral(..)
| Type::ModuleLiteral(..)
| Type::ClassLiteral(..)),
right @ (Type::None
| Type::BooleanLiteral(..)
right @ (Type::BooleanLiteral(..)
| Type::IntLiteral(..)
| Type::StringLiteral(..)
| Type::BytesLiteral(..)
Expand All @@ -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!(
Expand Down Expand Up @@ -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?

Expand Down Expand Up @@ -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(..)
Expand All @@ -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
Expand Down Expand Up @@ -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(..)
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -963,18 +975,22 @@ 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(_) => {
// TODO: lookup `__bool__` and `__len__` methods on the class's metaclass
// 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);
Expand Down Expand Up @@ -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]
Expand All @@ -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]`?
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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)]
Expand Down Expand Up @@ -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")]
Expand Down
16 changes: 8 additions & 8 deletions crates/red_knot_python_semantic/src/types/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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();

Expand All @@ -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));
}
Expand Down Expand Up @@ -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)
Expand Down
8 changes: 5 additions & 3 deletions crates/red_knot_python_semantic/src/types/display.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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"),
Expand Down Expand Up @@ -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));
Expand Down
12 changes: 8 additions & 4 deletions crates/red_knot_python_semantic/src/types/infer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions crates/red_knot_python_semantic/src/types/narrow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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),
};
Expand Down

0 comments on commit 88d9bb1

Please sign in to comment.