Skip to content
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] Fix narrowing for '… is not …' type guards, add '… is …' type guards #13758

Merged
merged 11 commits into from
Oct 15, 2024
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# Narrowing for `is` conditionals

## `is None`

```py
x = None if flag else 1
sharkdp marked this conversation as resolved.
Show resolved Hide resolved

if x is None:
# TODO the following should be simplified to 'None'
reveal_type(x) # revealed: None | Literal[1] & None

reveal_type(x) # revealed: None | Literal[1]
```

## `is` for other types

```py
class A:
...

x = A()
y = x if flag else None

if y is x:
# TODO the following should be simplified to 'A'
reveal_type(y) # revealed: A | None & A

reveal_type(y) # revealed: A | None
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
# Narrowing for `is not` conditionals

## `is not None`

The type guard removes `None` from the union type:

```py
x = None if flag else 1

if x is not None:
reveal_type(x) # revealed: Literal[1]

reveal_type(x) # revealed: None | Literal[1]
```

## `is not` for other singleton types

```py
x = True if flag else False
reveal_type(x) # revealed: bool

if x is not False:
# TODO the following should be `Literal[True]`
reveal_type(x) # revealed: bool & ~Literal[False]
```

## `is not` for non-singleton types

Non-singleton types should *not* narrow the type: two instances of a
non-singleton class may occupy different addresses in memory even if
they compare equal.

```py
x = [1]
y = [1]

if x is not y:
# TODO: should include type parameter: list[int]
reveal_type(x) # revealed: list
sharkdp marked this conversation as resolved.
Show resolved Hide resolved
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Narrowing for `match` statements

## Single `match` pattern

```py
x = None if flag else 1
reveal_type(x) # revealed: None | Literal[1]

y = 0

match x:
case None:
y = x

# TODO intersection simplification: should be just Literal[0] | None
reveal_type(y) # revealed: Literal[0] | None | Literal[1] & None
```
sharkdp marked this conversation as resolved.
Show resolved Hide resolved
76 changes: 76 additions & 0 deletions crates/red_knot_python_semantic/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,58 @@ impl<'db> Type<'db> {
self == other
}

/// Return true if there is just a single inhabitant for this type.
///
/// 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, db: &'db dyn Db) -> bool {
match self {
Type::Any
| Type::Never
| Type::Unknown
| Type::Todo
| Type::Unbound
| Type::Module(..)
sharkdp marked this conversation as resolved.
Show resolved Hide resolved
| Type::Instance(..) // TODO some instance types can be singleton types (EllipsisType, NotImplementedType)
| Type::IntLiteral(..)
| Type::StringLiteral(..)
| Type::BytesLiteral(..)
| Type::LiteralString => {
// Note: The literal types included in this pattern are not true singletons.
// There can be multiple Python objects (at different memory locations) that
// are both of type Literal[345], for example.
false
}
Type::None | Type::BooleanLiteral(_) | Type::Function(..) | Type::Class(..) => true,
Type::Tuple(tuple) => {
// We deliberately deviate from the language specification [1] here and claim
// that the empty tuple type is a singleton type. The reasoning is that `()`
// is often used as a sentinel value in user code. Declaring the empty tuple to
// be of singleton type allows us to narrow types in `is not ()` conditionals.
//
// [1] https://docs.python.org/3/reference/expressions.html#parenthesized-forms
tuple.elements(db).is_empty()
Comment on lines +489 to +495
Copy link
Contributor

@carljm carljm Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we'll want to stick with this conclusion, given the response to https://discuss.python.org/t/should-we-specify-in-the-language-reference-that-the-empty-tuple-is-a-singleton/67957/7 -- in particular the fact that is comparisons with tuples are now a syntax warning in recent Python versions, and that there are Python implementations (GraalPy) for which the empty tuple is not a singleton. But we can leave it for now, I don't think it's particularly critical either way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah. Though strictly speaking the narrowing is still safe on all popular Python implementations, and probably always will be. But probably not worth carving out a special case for, given the language appears to be actively discouraging it now.

}
Type::Union(..) => {
// A single-element union, where the sole element was a singleton, would itself
// be a singleton type. However, unions with length < 2 should never appear in
// our model due to [`UnionBuilder::build`].
false
}
Type::Intersection(..) => {
// Intersection types are hard to analyze. The following types are technically
// all singleton types, but it is not straightforward to compute this. Again,
// we simply return false.
//
// bool & ~Literal[False]`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should (but don't yet) simplify this to just Literal[True] in intersection builder.

// None & (None | int)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type also cannot occur in our representation, because we normalize to disjunctive normal form (union of intersections), so this would become (None & None) | (None & int) and then None | (None & int) and then (this step not yet implemented) None | Never and thus just None.

// (A | B) & (B | C) with A, B, C disjunct and B a singleton
Copy link
Contributor

@carljm carljm Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already always normalize this to (A & B) | (A & C) | (B & B) | (B & C) -> (A & B) | (A & C) | B | (B & C), which should (though I'm not sure if it would yet?) become (A & C) | B, which (if we know A and C are disjoint -- this part not yet implemented) should become just B -- so then would return true from this method :)

//
false
}
}
}

/// Resolve a member access of a type.
///
/// For example, if `foo` is `Type::Instance(<Bar>)`,
Expand Down Expand Up @@ -1514,6 +1566,7 @@ mod tests {
enum Ty {
Never,
Unknown,
None,
sharkdp marked this conversation as resolved.
Show resolved Hide resolved
Any,
IntLiteral(i64),
BoolLiteral(bool),
Expand All @@ -1530,6 +1583,7 @@ mod tests {
match self {
Ty::Never => Type::Never,
Ty::Unknown => Type::Unknown,
Ty::None => Type::None,
Ty::Any => Type::Any,
Ty::IntLiteral(n) => Type::IntLiteral(n),
Ty::StringLiteral(s) => {
Expand Down Expand Up @@ -1618,6 +1672,28 @@ mod tests {
assert!(from.into_type(&db).is_equivalent_to(&db, to.into_type(&db)));
}

#[test_case(Ty::None)]
#[test_case(Ty::BoolLiteral(true))]
#[test_case(Ty::BoolLiteral(false))]
#[test_case(Ty::Tuple(vec![]))]
fn is_singleton(from: Ty) {
let db = setup_db();

assert!(from.into_type(&db).is_singleton(&db));
}

#[test_case(Ty::Never)]
#[test_case(Ty::IntLiteral(345))]
#[test_case(Ty::BuiltinInstance("str"))]
#[test_case(Ty::Union(vec![Ty::IntLiteral(1), Ty::IntLiteral(2)]))]
#[test_case(Ty::Tuple(vec![Ty::None]))]
#[test_case(Ty::Tuple(vec![Ty::None, Ty::BoolLiteral(true)]))]
fn is_not_singleton(from: Ty) {
let db = setup_db();

assert!(!from.into_type(&db).is_singleton(&db));
}

#[test_case(Ty::IntLiteral(1); "is_int_literal_truthy")]
#[test_case(Ty::IntLiteral(-1))]
#[test_case(Ty::StringLiteral("foo"))]
Expand Down
47 changes: 0 additions & 47 deletions crates/red_knot_python_semantic/src/types/infer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5421,53 +5421,6 @@ mod tests {
Ok(())
}

#[test]
fn narrow_not_none() -> anyhow::Result<()> {
let mut db = setup_db();

db.write_dedented(
"/src/a.py",
"
x = None if flag else 1
y = 0
if x is not None:
y = x
",
)?;

assert_public_ty(&db, "/src/a.py", "x", "None | Literal[1]");
assert_public_ty(&db, "/src/a.py", "y", "Literal[0, 1]");

Ok(())
}

#[test]
fn narrow_singleton_pattern() {
let mut db = setup_db();

db.write_dedented(
"/src/a.py",
"
x = None if flag else 1
y = 0
match x:
case None:
y = x
",
)
.unwrap();

// TODO: The correct inferred type should be `Literal[0] | None` but currently the
// simplification logic doesn't account for this. The final type with parenthesis:
// `Literal[0] | None | (Literal[1] & None)`
assert_public_ty(
&db,
"/src/a.py",
"y",
"Literal[0] | None | Literal[1] & None",
);
}

#[test]
fn while_loop() -> anyhow::Result<()> {
let mut db = setup_db();
Expand Down
25 changes: 18 additions & 7 deletions crates/red_knot_python_semantic/src/types/narrow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,13 +155,24 @@ impl<'db> NarrowingConstraintsBuilder<'db> {
let inference = infer_expression_types(self.db, expression);
for (op, comparator) in std::iter::zip(&**ops, &**comparators) {
let comp_ty = inference.expression_ty(comparator.scoped_ast_id(self.db, scope));
if matches!(op, ast::CmpOp::IsNot) {
let ty = IntersectionBuilder::new(self.db)
.add_negative(comp_ty)
.build();
self.constraints.insert(symbol, ty);
};
// TODO other comparison types
match op {
ast::CmpOp::IsNot => {
if comp_ty.is_singleton(self.db) {
let ty = IntersectionBuilder::new(self.db)
.add_negative(comp_ty)
.build();
self.constraints.insert(symbol, ty);
} else {
// Non-singletons cannot be safely narrowed using `is not`
}
}
sharkdp marked this conversation as resolved.
Show resolved Hide resolved
ast::CmpOp::Is => {
self.constraints.insert(symbol, comp_ty);
}
_ => {
// TODO other comparison types
}
}
}
}
}
Expand Down
Loading