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] Handle possibly-unbound instance members #16363

Merged
merged 4 commits into from
Feb 25, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
62 changes: 62 additions & 0 deletions crates/red_knot_python_semantic/resources/mdtest/attributes.md
Original file line number Diff line number Diff line change
Expand Up @@ -783,6 +783,9 @@ def _(flag1: bool, flag2: bool):

# error: [possibly-unbound-attribute] "Attribute `x` on type `Literal[C1, C2, C3]` is possibly unbound"
reveal_type(C.x) # revealed: Unknown | Literal[1, 3]

# error: [possibly-unbound-attribute] "Attribute `x` on type `C1 | C2 | C3` is possibly unbound"
reveal_type(C().x) # revealed: Unknown | Literal[1, 3]
```

### Possibly-unbound within a class
Expand All @@ -806,6 +809,28 @@ def _(flag: bool, flag1: bool, flag2: bool):

# error: [possibly-unbound-attribute] "Attribute `x` on type `Literal[C1, C2, C3]` is possibly unbound"
reveal_type(C.x) # revealed: Unknown | Literal[1, 2, 3]

# Note: we might want to consider ignoring possibly-unbound diagnostics for instance attributes eventually,
# see the "Possibly unbound/undeclared instance attribute" section below.
# error: [possibly-unbound-attribute] "Attribute `x` on type `C1 | C2 | C3` is possibly unbound"
reveal_type(C().x) # revealed: Unknown | Literal[1, 2, 3]
```

### Possibly-unbound within gradual types

```py
from typing import Any

def _(flag: bool):
class Base:
x: Any

class Derived(Base):
if flag:
# Redeclaring `x` with a more static type is okay in terms of LSP.
x: int

reveal_type(Derived().x) # revealed: int | Any
```

### Attribute possibly unbound on a subclass but not on a superclass
Expand All @@ -820,6 +845,8 @@ def _(flag: bool):
x = 2

reveal_type(Bar.x) # revealed: Unknown | Literal[2, 1]

reveal_type(Bar().x) # revealed: Unknown | Literal[2, 1]
```

### Attribute possibly unbound on a subclass and on a superclass
Expand All @@ -836,6 +863,41 @@ def _(flag: bool):

# error: [possibly-unbound-attribute]
reveal_type(Bar.x) # revealed: Unknown | Literal[2, 1]

# error: [possibly-unbound-attribute]
reveal_type(Bar().x) # revealed: Unknown | Literal[2, 1]
```

### Possibly unbound/undeclared instance attribute

#### Possibly unbound and undeclared

```py
def _(flag: bool):
class Foo:
if flag:
x: int

def __init(self):
if flag:
self.x = 1

# error: [possibly-unbound-attribute]
reveal_type(Foo().x) # revealed: int
```

#### Possibly unbound

```py
def _(flag: bool):
class Foo:
def __init(self):
if flag:
self.x = 1

# Emitting a diagnostic in a case like this is not something we support, and it's unclear
# if we ever will (or want to)
Comment on lines +898 to +899
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We came to the same conclusion in an earlier PR. This is also mentioned higher up in this test suite. And mypy/pyright don't emit a diagnostic for this either (which is also mentioned higher up). This test is only added here for easier reference / completeness.

reveal_type(Foo().x) # revealed: Unknown | Literal[1]
```

### Attribute access on `Any`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,24 @@ c = C()
c.a = 2
```

and similarly here:

```py
class Base:
a: ClassVar[int] = 1

class Derived(Base):
if flag():
a: int

reveal_type(Derived.a) # revealed: int

d = Derived()

# error: [invalid-attribute-access]
d.a = 2
```

## Too many arguments

```py
Expand Down
4 changes: 4 additions & 0 deletions crates/red_knot_python_semantic/src/types/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ impl<'db> UnionBuilder<'db> {
}
}

pub(crate) fn is_empty(&self) -> bool {
self.elements.is_empty()
}

/// Collapse the union to a single type: `object`.
fn collapse_to_object(mut self) -> Self {
self.elements.clear();
Expand Down
68 changes: 49 additions & 19 deletions crates/red_knot_python_semantic/src/types/class.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use crate::{
},
symbol::{
class_symbol, known_module_symbol, symbol_from_bindings, symbol_from_declarations,
LookupError, LookupResult, Symbol, SymbolAndQualifiers,
Boundness, LookupError, LookupResult, Symbol, SymbolAndQualifiers,
},
types::{
definition_expression_type, CallArguments, CallError, MetaclassCandidate, TupleType,
Expand Down Expand Up @@ -383,6 +383,9 @@ impl<'db> Class<'db> {
///
/// The attribute might also be defined in a superclass of this class.
pub(super) fn instance_member(self, db: &'db dyn Db, name: &str) -> SymbolAndQualifiers<'db> {
let mut union = UnionBuilder::new(db);
let mut union_qualifiers = TypeQualifiers::empty();

for superclass in self.iter_mro(db) {
match superclass {
ClassBase::Dynamic(_) => {
Expand All @@ -391,16 +394,43 @@ impl<'db> Class<'db> {
);
}
ClassBase::Class(class) => {
if let member @ SymbolAndQualifiers(Symbol::Type(_, _), _) =
if let member @ SymbolAndQualifiers(Symbol::Type(ty, boundness), qualifiers) =
class.own_instance_member(db, name)
{
return member;
// TODO: We could raise a diagnostic here if there are conflicting type qualifiers
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably the diagnostic would be raised by an eager Liskov check at the inheritance site, rather than here? But it doesn't hurt to have a TODO here until we do that.

union_qualifiers = union_qualifiers.union(qualifiers);

if boundness == Boundness::Bound {
if union.is_empty() {
// Short-circuit, no need to allocate inside the union builder
return member;
}

return SymbolAndQualifiers(
Symbol::bound(union.add(ty).build()),
union_qualifiers,
);
}

// If we see a possibly-unbound symbol, we need to keep looking
// higher up in the MRO.
union = union.add(ty);
}
}
}
}

SymbolAndQualifiers(Symbol::Unbound, TypeQualifiers::empty())
if union.is_empty() {
SymbolAndQualifiers(Symbol::Unbound, TypeQualifiers::empty())
} else {
// If we have reached this point, we know that we have only seen possibly-unbound symbols.
// This means that the final result is still possibly-unbound.

SymbolAndQualifiers(
Symbol::Type(union.build(), Boundness::PossiblyUnbound),
union_qualifiers,
)
}
}

/// Tries to find declarations/bindings of an instance attribute named `name` that are only
Expand All @@ -409,16 +439,18 @@ impl<'db> Class<'db> {
db: &'db dyn Db,
class_body_scope: ScopeId<'db>,
name: &str,
inferred_type_from_class_body: Option<Type<'db>>,
inferred_from_class_body: &Symbol<'db>,
) -> Symbol<'db> {
// If we do not see any declarations of an attribute, neither in the class body nor in
// any method, we build a union of `Unknown` with the inferred types of all bindings of
// that attribute. We include `Unknown` in that union to account for the fact that the
// attribute might be externally modified.
let mut union_of_inferred_types = UnionBuilder::new(db).add(Type::unknown());
let mut union_boundness = Boundness::Bound;

if let Some(ty) = inferred_type_from_class_body {
union_of_inferred_types = union_of_inferred_types.add(ty);
if let Symbol::Type(ty, boundness) = inferred_from_class_body {
union_of_inferred_types = union_of_inferred_types.add(*ty);
union_boundness = *boundness;
}

let attribute_assignments = attribute_assignments(db, class_body_scope);
Expand All @@ -427,10 +459,10 @@ impl<'db> Class<'db> {
.as_deref()
.and_then(|assignments| assignments.get(name))
else {
if inferred_type_from_class_body.is_some() {
return Symbol::bound(union_of_inferred_types.build());
if inferred_from_class_body.is_unbound() {
return Symbol::Unbound;
}
return Symbol::Unbound;
return Symbol::Type(union_of_inferred_types.build(), union_boundness);
};

for attribute_assignment in attribute_assignments {
Expand Down Expand Up @@ -484,7 +516,7 @@ impl<'db> Class<'db> {
}
}

Symbol::bound(union_of_inferred_types.build())
Symbol::Type(union_of_inferred_types.build(), union_boundness)
}

/// A helper function for `instance_member` that looks up the `name` attribute only on
Expand All @@ -493,7 +525,6 @@ impl<'db> Class<'db> {
// TODO: There are many things that are not yet implemented here:
// - `typing.Final`
// - Proper diagnostics
// - Handling of possibly-undeclared/possibly-unbound attributes

let body_scope = self.body_scope(db);
let table = symbol_table(db, body_scope);
Expand All @@ -504,7 +535,7 @@ impl<'db> Class<'db> {
let declarations = use_def.public_declarations(symbol_id);

match symbol_from_declarations(db, declarations) {
Ok(SymbolAndQualifiers(Symbol::Type(declared_ty, _), qualifiers)) => {
Ok(SymbolAndQualifiers(declared @ Symbol::Type(declared_ty, _), qualifiers)) => {
// The attribute is declared in the class body.

if let Some(function) = declared_ty.into_function_literal() {
Expand All @@ -514,7 +545,7 @@ impl<'db> Class<'db> {
if function.has_known_class_decorator(db, KnownClass::Classmethod)
&& function.decorators(db).len() == 1
{
SymbolAndQualifiers(Symbol::bound(declared_ty), qualifiers)
SymbolAndQualifiers(declared, qualifiers)
} else if function.has_known_class_decorator(db, KnownClass::Property) {
SymbolAndQualifiers::todo("@property")
} else if function.has_known_function_decorator(db, KnownFunction::Overload)
Expand All @@ -523,10 +554,10 @@ impl<'db> Class<'db> {
} else if !function.decorators(db).is_empty() {
SymbolAndQualifiers::todo("decorated method")
} else {
SymbolAndQualifiers(Symbol::bound(declared_ty), qualifiers)
SymbolAndQualifiers(declared, qualifiers)
}
} else {
SymbolAndQualifiers(Symbol::bound(declared_ty), qualifiers)
SymbolAndQualifiers(declared, qualifiers)
}
}
Ok(SymbolAndQualifiers(Symbol::Unbound, _)) => {
Expand All @@ -535,9 +566,8 @@ impl<'db> Class<'db> {

let bindings = use_def.public_bindings(symbol_id);
let inferred = symbol_from_bindings(db, bindings);
let inferred_ty = inferred.ignore_possibly_unbound();

Self::implicit_instance_attribute(db, body_scope, name, inferred_ty).into()
Self::implicit_instance_attribute(db, body_scope, name, &inferred).into()
}
Err((declared_ty, _conflicting_declarations)) => {
// There are conflicting declarations for this attribute in the class body.
Expand All @@ -551,7 +581,7 @@ impl<'db> Class<'db> {
// This attribute is neither declared nor bound in the class body.
// It could still be implicitly defined in a method.

Self::implicit_instance_attribute(db, body_scope, name, None).into()
Self::implicit_instance_attribute(db, body_scope, name, &Symbol::Unbound).into()
}
}

Expand Down
Loading