Cyclic0007/ruff
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[red-knot] Add diagnostic for class-object access to pure instance variables (#16036)

Browse Source 
## Summary

Add a diagnostic if a pure instance variable is accessed on a class object. For example

```py
class C:
    instance_only: str

    def __init__(self):
        self.instance_only = "a"

# error: Attribute `instance_only` can only be accessed on instances, not on the class object `Literal[C]` itself.
C.instance_only
```


---------

Co-authored-by: David Peter <mail@david-peter.de>
This commit is contained in:
Mike Perlov
2025-02-24 09:17:16 -05:00
committed by GitHub
parent e7a6c19e3a
commit 68991d09a8
8 changed files with 246 additions and 136 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
crates/red_knot_python_semantic/resources/mdtest/annotations/string.md
View File

@@ -116,8 +116,8 @@ MyType = int
class Aliases:
MyType = str
forward: "MyType"
not_forward: MyType
forward: "MyType" = "value"
not_forward: MyType = "value"
reveal_type(Aliases.forward) # revealed: str
reveal_type(Aliases.not_forward) # revealed: str

33
crates/red_knot_python_semantic/resources/mdtest/attributes.md
View File

@@ -54,13 +54,12 @@ c_instance.declared_and_bound = False
# error: [invalid-assignment] "Object of type `Literal["incompatible"]` is not assignable to attribute `declared_and_bound` of type `bool`"
c_instance.declared_and_bound = "incompatible"
# TODO: we already show an error here but the message might be improved?
# mypy shows no error here, but pyright raises "reportAttributeAccessIssue"
# error: [unresolved-attribute] "Type `Literal[C]` has no attribute `inferred_from_value`"
# error: [unresolved-attribute] "Attribute `inferred_from_value` can only be accessed on instances, not on the class object `Literal[C]` itself."
reveal_type(C.inferred_from_value) # revealed: Unknown
# TODO: this should be an error (pure instance variables cannot be accessed on the class)
# mypy shows no error here, but pyright raises "reportAttributeAccessIssue"
# error: [invalid-attribute-access] "Cannot assign to instance attribute `inferred_from_value` from the class object `Literal[C]`"
C.inferred_from_value = "overwritten on class"
# This assignment is fine:
@@ -90,13 +89,13 @@ c_instance = C()
reveal_type(c_instance.declared_and_bound) # revealed: str | None
# TODO: we currently plan to emit a diagnostic here. Note that both mypy
# and pyright show no error in this case! So we may reconsider this in
# the future, if it turns out to produce too many false positives.
reveal_type(C.declared_and_bound) # revealed: str | None
# Note that both mypy and pyright show no error in this case! So we may reconsider this in
# the future, if it turns out to produce too many false positives. We currently emit:
# error: [unresolved-attribute] "Attribute `declared_and_bound` can only be accessed on instances, not on the class object `Literal[C]` itself."
reveal_type(C.declared_and_bound) # revealed: Unknown
# TODO: same as above. We plan to emit a diagnostic here, even if both mypy
# and pyright allow this.
# Same as above. Mypy and pyright do not show an error here.
# error: [invalid-attribute-access] "Cannot assign to instance attribute `declared_and_bound` from the class object `Literal[C]`"
C.declared_and_bound = "overwritten on class"
# error: [invalid-assignment] "Object of type `Literal[1]` is not assignable to attribute `declared_and_bound` of type `str | None`"
@@ -116,11 +115,11 @@ c_instance = C()
reveal_type(c_instance.only_declared) # revealed: str
# TODO: mypy and pyright do not show an error here, but we plan to emit a diagnostic.
# The type could be changed to 'Unknown' if we decide to emit an error?
reveal_type(C.only_declared) # revealed: str
# Mypy and pyright do not show an error here. We treat this as a pure instance variable.
# error: [unresolved-attribute] "Attribute `only_declared` can only be accessed on instances, not on the class object `Literal[C]` itself."
reveal_type(C.only_declared) # revealed: Unknown
# TODO: mypy and pyright do not show an error here, but we plan to emit one.
# error: [invalid-attribute-access] "Cannot assign to instance attribute `only_declared` from the class object `Literal[C]`"
C.only_declared = "overwritten on class"
```
@@ -191,11 +190,10 @@ reveal_type(c_instance.declared_only) # revealed: bytes
reveal_type(c_instance.declared_and_bound) # revealed: bool
# TODO: We already show an error here, but the message might be improved?
# error: [unresolved-attribute]
# error: [unresolved-attribute] "Attribute `inferred_from_value` can only be accessed on instances, not on the class object `Literal[C]` itself."
reveal_type(C.inferred_from_value) # revealed: Unknown
# TODO: this should be an error
# error: [invalid-attribute-access] "Cannot assign to instance attribute `inferred_from_value` from the class object `Literal[C]`"
C.inferred_from_value = "overwritten on class"
```
@@ -598,6 +596,9 @@ C.class_method()
# error: [unresolved-attribute]
reveal_type(C.pure_class_variable) # revealed: Unknown
# TODO: should be no error when descriptor protocol is supported
# and the assignment is properly attributed to the class method.
# error: [invalid-attribute-access] "Cannot assign to instance attribute `pure_class_variable` from the class object `Literal[C]`"
C.pure_class_variable = "overwritten on class"
# TODO: should be `Unknown | Literal["value set in class method"]` or

2
crates/red_knot_python_semantic/resources/mdtest/expression/boolean.md
View File

@@ -145,7 +145,7 @@ def test(cond: bool):
```py
def test(cond: bool):
class NotBoolable:
__bool__: int
__bool__ = None
a = 10 if cond else NotBoolable()

12
crates/red_knot_python_semantic/resources/mdtest/scopes/eager.md
View File

@@ -341,10 +341,12 @@ annotation are looked up lazily, even if they occur in an eager scope.
### Eager annotations in a Python file
```py
from typing import ClassVar
x = int
class C:
var: x
var: ClassVar[x]
reveal_type(C.var) # revealed: int
@@ -356,10 +358,12 @@ x = str
```py
from __future__ import annotations
from typing import ClassVar
x = int
class C:
var: x
var: ClassVar[x]
reveal_type(C.var) # revealed: Unknown | str
@@ -369,10 +373,12 @@ x = str
### Deferred annotations in a stub file
```pyi
from typing import ClassVar
x = int
class C:
var: x
var: ClassVar[x]
reveal_type(C.var) # revealed: Unknown | str

207
crates/red_knot_python_semantic/src/symbol.rs
View File

@@ -184,6 +184,40 @@ pub(crate) fn symbol<'db>(db: &'db dyn Db, scope: ScopeId<'db>, name: &str) -> S
symbol_impl(db, scope, name, RequiresExplicitReExport::No)
}
/// Infer the public type of a class symbol (its type as seen from outside its scope) in the given
/// `scope`.
pub(crate) fn class_symbol<'db>(db: &'db dyn Db, scope: ScopeId<'db>, name: &str) -> Symbol<'db> {
symbol_table(db, scope)
.symbol_id_by_name(name)
.map(|symbol| {
let symbol_and_quals = symbol_by_id(db, scope, symbol, RequiresExplicitReExport::No);
if symbol_and_quals.is_class_var() {
// For declared class vars we do not need to check if they have bindings,
// we just trust the declaration.
return symbol_and_quals.0;
}
if let SymbolAndQualifiers(Symbol::Type(ty, _), _) = symbol_and_quals {
// Otherwise, we need to check if the symbol has bindings
let use_def = use_def_map(db, scope);
let bindings = use_def.public_bindings(symbol);
let inferred =
symbol_from_bindings_impl(db, bindings, RequiresExplicitReExport::No);
// TODO: we should not need to calculate inferred type second time. This is a temporary
// solution until the notion of Boundness and Declaredness is split. See #16036, #16264
match inferred {
Symbol::Unbound => Symbol::Unbound,
Symbol::Type(_, boundness) => Symbol::Type(ty, boundness),
}
} else {
Symbol::Unbound
}
})
.unwrap_or(Symbol::Unbound)
}
/// Infers the public type of an explicit module-global symbol as seen from within the same file.
///
/// Note that all global scopes also include various "implicit globals" such as `__name__`,
@@ -348,7 +382,7 @@ pub(crate) type SymbolFromDeclarationsResult<'db> =
/// that this comes with a [`CLASS_VAR`] type qualifier.
///
/// [`CLASS_VAR`]: crate::types::TypeQualifiers::CLASS_VAR
#[derive(Debug)]
#[derive(Debug, Clone, PartialEq, Eq, salsa::Update)]
pub(crate) struct SymbolAndQualifiers<'db>(pub(crate) Symbol<'db>, pub(crate) TypeQualifiers);
impl SymbolAndQualifiers<'_> {
@@ -364,11 +398,6 @@ impl SymbolAndQualifiers<'_> {
pub(crate) fn is_class_var(&self) -> bool {
self.1.contains(TypeQualifiers::CLASS_VAR)
}
/// Returns `true` if the symbol has a `Final` type qualifier.
pub(crate) fn is_final(&self) -> bool {
self.1.contains(TypeQualifiers::FINAL)
}
}
impl<'db> From<Symbol<'db>> for SymbolAndQualifiers<'db> {
@@ -377,6 +406,91 @@ impl<'db> From<Symbol<'db>> for SymbolAndQualifiers<'db> {
}
}
#[salsa::tracked]
fn symbol_by_id<'db>(
db: &'db dyn Db,
scope: ScopeId<'db>,
symbol_id: ScopedSymbolId,
requires_explicit_reexport: RequiresExplicitReExport,
) -> SymbolAndQualifiers<'db> {
let use_def = use_def_map(db, scope);
// If the symbol is declared, the public type is based on declarations; otherwise, it's based
// on inference from bindings.
let declarations = use_def.public_declarations(symbol_id);
let declared = symbol_from_declarations_impl(db, declarations, requires_explicit_reexport);
match declared {
// Symbol is declared, trust the declared type
Ok(symbol_and_quals @ SymbolAndQualifiers(Symbol::Type(_, Boundness::Bound), _)) => {
symbol_and_quals
}
// Symbol is possibly declared
Ok(SymbolAndQualifiers(Symbol::Type(declared_ty, Boundness::PossiblyUnbound), quals)) => {
let bindings = use_def.public_bindings(symbol_id);
let inferred = symbol_from_bindings_impl(db, bindings, requires_explicit_reexport);
let symbol = match inferred {
// Symbol is possibly undeclared and definitely unbound
Symbol::Unbound => {
// TODO: We probably don't want to report `Bound` here. This requires a bit of
// design work though as we might want a different behavior for stubs and for
// normal modules.
Symbol::Type(declared_ty, Boundness::Bound)
}
// Symbol is possibly undeclared and (possibly) bound
Symbol::Type(inferred_ty, boundness) => Symbol::Type(
UnionType::from_elements(db, [inferred_ty, declared_ty]),
boundness,
),
};
SymbolAndQualifiers(symbol, quals)
}
// Symbol is undeclared, return the union of `Unknown` with the inferred type
Ok(SymbolAndQualifiers(Symbol::Unbound, _)) => {
let bindings = use_def.public_bindings(symbol_id);
let inferred = symbol_from_bindings_impl(db, bindings, requires_explicit_reexport);
// `__slots__` is a symbol with special behavior in Python's runtime. It can be
// modified externally, but those changes do not take effect. We therefore issue
// a diagnostic if we see it being modified externally. In type inference, we
// can assign a "narrow" type to it even if it is not *declared*. This means, we
// do not have to call [`widen_type_for_undeclared_public_symbol`].
let is_considered_non_modifiable =
symbol_table(db, scope).symbol(symbol_id).name() == "__slots__";
widen_type_for_undeclared_public_symbol(db, inferred, is_considered_non_modifiable)
.into()
}
// Symbol has conflicting declared types
Err((declared_ty, _)) => {
// Intentionally ignore conflicting declared types; that's not our problem,
// it's the problem of the module we are importing from.
SymbolAndQualifiers(
Symbol::bound(declared_ty.inner_type()),
declared_ty.qualifiers(),
)
}
}
// TODO (ticket: https://github.com/astral-sh/ruff/issues/14297) Our handling of boundness
// currently only depends on bindings, and ignores declarations. This is inconsistent, since
// we only look at bindings if the symbol may be undeclared. Consider the following example:
// ```py
// x: int
//
// if flag:
// y: int
// else
// y = 3
// ```
// If we import from this module, we will currently report `x` as a definitely-bound symbol
// (even though it has no bindings at all!) but report `y` as possibly-unbound (even though
// every path has either a binding or a declaration for it.)
}
/// Implementation of [`symbol`].
fn symbol_impl<'db>(
db: &'db dyn Db,
@@ -384,85 +498,6 @@ fn symbol_impl<'db>(
name: &str,
requires_explicit_reexport: RequiresExplicitReExport,
) -> Symbol<'db> {
#[salsa::tracked]
fn symbol_by_id<'db>(
db: &'db dyn Db,
scope: ScopeId<'db>,
symbol_id: ScopedSymbolId,
requires_explicit_reexport: RequiresExplicitReExport,
) -> Symbol<'db> {
let use_def = use_def_map(db, scope);
// If the symbol is declared, the public type is based on declarations; otherwise, it's based
// on inference from bindings.
let declarations = use_def.public_declarations(symbol_id);
let declared = symbol_from_declarations_impl(db, declarations, requires_explicit_reexport);
let is_final = declared.as_ref().is_ok_and(SymbolAndQualifiers::is_final);
let declared = declared.map(|SymbolAndQualifiers(symbol, _)| symbol);
match declared {
// Symbol is declared, trust the declared type
Ok(symbol @ Symbol::Type(_, Boundness::Bound)) => symbol,
// Symbol is possibly declared
Ok(Symbol::Type(declared_ty, Boundness::PossiblyUnbound)) => {
let bindings = use_def.public_bindings(symbol_id);
let inferred = symbol_from_bindings_impl(db, bindings, requires_explicit_reexport);
match inferred {
// Symbol is possibly undeclared and definitely unbound
Symbol::Unbound => {
// TODO: We probably don't want to report `Bound` here. This requires a bit of
// design work though as we might want a different behavior for stubs and for
// normal modules.
Symbol::Type(declared_ty, Boundness::Bound)
}
// Symbol is possibly undeclared and (possibly) bound
Symbol::Type(inferred_ty, boundness) => Symbol::Type(
UnionType::from_elements(db, [inferred_ty, declared_ty]),
boundness,
),
}
}
// Symbol is undeclared, return the union of `Unknown` with the inferred type
Ok(Symbol::Unbound) => {
let bindings = use_def.public_bindings(symbol_id);
let inferred = symbol_from_bindings_impl(db, bindings, requires_explicit_reexport);
// `__slots__` is a symbol with special behavior in Python's runtime. It can be
// modified externally, but those changes do not take effect. We therefore issue
// a diagnostic if we see it being modified externally. In type inference, we
// can assign a "narrow" type to it even if it is not *declared*. This means, we
// do not have to call [`widen_type_for_undeclared_public_symbol`].
let is_considered_non_modifiable =
is_final || symbol_table(db, scope).symbol(symbol_id).name() == "__slots__";
widen_type_for_undeclared_public_symbol(db, inferred, is_considered_non_modifiable)
}
// Symbol has conflicting declared types
Err((declared_ty, _)) => {
// Intentionally ignore conflicting declared types; that's not our problem,
// it's the problem of the module we are importing from.
Symbol::bound(declared_ty.inner_type())
}
}
// TODO (ticket: https://github.com/astral-sh/ruff/issues/14297) Our handling of boundness
// currently only depends on bindings, and ignores declarations. This is inconsistent, since
// we only look at bindings if the symbol may be undeclared. Consider the following example:
// ```py
// x: int
//
// if flag:
// y: int
// else
// y = 3
// ```
// If we import from this module, we will currently report `x` as a definitely-bound symbol
// (even though it has no bindings at all!) but report `y` as possibly-unbound (even though
// every path has either a binding or a declaration for it.)
}
let _span = tracing::trace_span!("symbol", ?name).entered();
// We don't need to check for `typing_extensions` here, because `typing_extensions.TYPE_CHECKING`
@@ -489,7 +524,7 @@ fn symbol_impl<'db>(
symbol_table(db, scope)
.symbol_id_by_name(name)
.map(|symbol| symbol_by_id(db, scope, symbol, requires_explicit_reexport))
.map(|symbol| symbol_by_id(db, scope, symbol, requires_explicit_reexport).0)
.unwrap_or(Symbol::Unbound)
}

11
crates/red_knot_python_semantic/src/types/class.rs
View File

@@ -5,8 +5,8 @@ use crate::{
symbol::ScopeId, symbol_table, use_def_map,
},
symbol::{
known_module_symbol, symbol, symbol_from_bindings, symbol_from_declarations, LookupError,
LookupResult, Symbol, SymbolAndQualifiers,
class_symbol, known_module_symbol, symbol_from_bindings, symbol_from_declarations,
LookupError, LookupResult, Symbol, SymbolAndQualifiers,
},
types::{
definition_expression_type, CallArguments, CallError, MetaclassCandidate, TupleType,
@@ -363,14 +363,15 @@ impl<'db> Class<'db> {
}
}
/// Returns the inferred type of the class member named `name`.
/// Returns the inferred type of the class member named `name`. Only bound members
/// or those marked as ClassVars are considered.
///
/// Returns [`Symbol::Unbound`] if `name` cannot be found in this class's scope
/// directly. Use [`Class::class_member`] if you require a method that will
/// traverse through the MRO until it finds the member.
pub(crate) fn own_class_member(self, db: &'db dyn Db, name: &str) -> Symbol<'db> {
let scope = self.body_scope(db);
symbol(db, scope, name)
let body_scope = self.body_scope(db);
class_symbol(db, body_scope, name)
}
/// Returns the `name` attribute of an instance of this class.

106
crates/red_knot_python_semantic/src/types/infer.rs
View File

@@ -52,7 +52,7 @@ use crate::semantic_index::SemanticIndex;
use crate::symbol::{
builtins_module_scope, builtins_symbol, explicit_global_symbol,
module_type_implicit_global_symbol, symbol, symbol_from_bindings, symbol_from_declarations,
typing_extensions_symbol, LookupError,
typing_extensions_symbol, Boundness, LookupError,
};
use crate::types::call::{Argument, CallArguments, UnionCallError};
use crate::types::diagnostic::{
@@ -68,18 +68,18 @@ use crate::types::diagnostic::{
use crate::types::mro::MroErrorKind;
use crate::types::unpacker::{UnpackResult, Unpacker};
use crate::types::{
class::MetaclassErrorKind, todo_type, Boundness, Class, DynamicType, FunctionType,
InstanceType, IntersectionBuilder, IntersectionType, KnownClass, KnownFunction,
KnownInstanceType, MetaclassCandidate, SliceLiteralType, SubclassOfType, Symbol,
SymbolAndQualifiers, Truthiness, TupleType, Type, TypeAliasType, TypeAndQualifiers,
TypeArrayDisplay, TypeQualifiers, TypeVarBoundOrConstraints, TypeVarInstance, UnionBuilder,
UnionType,
class::MetaclassErrorKind, todo_type, Class, DynamicType, FunctionType, InstanceType,
IntersectionBuilder, IntersectionType, KnownClass, KnownFunction, KnownInstanceType,
MetaclassCandidate, SliceLiteralType, SubclassOfType, Symbol, SymbolAndQualifiers, Truthiness,
TupleType, Type, TypeAliasType, TypeAndQualifiers, TypeArrayDisplay, TypeQualifiers,
TypeVarBoundOrConstraints, TypeVarInstance, UnionBuilder, UnionType,
};
use crate::unpack::Unpack;
use crate::util::subscript::{PyIndex, PySlice};
use crate::Db;
use super::call::CallError;
use super::class_base::ClassBase;
use super::context::{InNoTypeCheck, InferContext, WithDiagnostics};
use super::diagnostic::{
report_index_out_of_bounds, report_invalid_exception_caught, report_invalid_exception_cause,
@@ -3712,6 +3712,32 @@ impl<'db> TypeInferenceBuilder<'db> {
.member(db, &attr.id)
.unwrap_with_diagnostic(|lookup_error| match lookup_error {
LookupError::Unbound => {
let bound_on_instance = match value_type {
Type::ClassLiteral(class) => {
!class.class().instance_member(db, attr).0.is_unbound()
}
Type::SubclassOf(subclass_of @ SubclassOfType { .. }) => {
match subclass_of.subclass_of() {
ClassBase::Class(class) => {
!class.instance_member(db, attr).0.is_unbound()
}
ClassBase::Dynamic(_) => unreachable!("Attribute lookup on a dynamic `SubclassOf` type should always return a bound symbol"),
}
}
_ => false,
};
if bound_on_instance {
self.context.report_lint(
&UNRESOLVED_ATTRIBUTE,
attribute,
format_args!(
"Attribute `{}` can only be accessed on instances, not on the class object `{}` itself.",
attr.id,
value_type.display(db)
),
);
} else {
self.context.report_lint(
&UNRESOLVED_ATTRIBUTE,
attribute,
@@ -3721,7 +3747,9 @@ impl<'db> TypeInferenceBuilder<'db> {
attr.id
),
);
Type::unknown()
}
Type::unknown()
}
LookupError::PossiblyUnbound(type_when_bound) => {
self.context.report_lint(
@@ -3751,22 +3779,54 @@ impl<'db> TypeInferenceBuilder<'db> {
ExprContext::Store => {
let value_ty = self.infer_expression(value);
let symbol = if let Type::Instance(instance) = value_ty {
let instance_member = instance.class().instance_member(self.db(), attr);
if instance_member.is_class_var() {
self.context.report_lint(
&INVALID_ATTRIBUTE_ACCESS,
attribute,
format_args!(
"Cannot assign to ClassVar `{attr}` from an instance of type `{ty}`",
ty = value_ty.display(self.db()),
),
);
}
let symbol = match value_ty {
Type::Instance(instance) => {
let instance_member = instance.class().instance_member(self.db(), attr);
if instance_member.is_class_var() {
self.context.report_lint(
&INVALID_ATTRIBUTE_ACCESS,
attribute,
format_args!(
"Cannot assign to ClassVar `{attr}` from an instance of type `{ty}`",
ty = value_ty.display(self.db()),
),
);
}
instance_member.0
} else {
value_ty.member(self.db(), attr)
instance_member.0
}
Type::ClassLiteral(_) | Type::SubclassOf(_) => {
let class_member = value_ty.member(self.db(), attr);
if class_member.is_unbound() {
if let Some(class) = match value_ty {
Type::ClassLiteral(class) => Some(class.class()),
Type::SubclassOf(subclass_of @ SubclassOfType { .. }) => {
match subclass_of.subclass_of() {
ClassBase::Class(class) => Some(class),
ClassBase::Dynamic(_) => unreachable!("Attribute lookup on a dynamic `SubclassOf` type should always return a bound symbol"),
}
}
_ => None,
} {
let instance_member = class.instance_member(self.db(), attr);
// Attribute is declared or bound on instance. Forbid access from the class object
if !instance_member.0.is_unbound() {
self.context.report_lint(
&INVALID_ATTRIBUTE_ACCESS,
attribute,
format_args!(
"Cannot assign to instance attribute `{attr}` from the class object `{ty}`",
ty = value_ty.display(self.db()),
));
}
}
}
class_member
}
_ => value_ty.member(self.db(), attr),
};
// TODO: The unbound-case might also yield a diagnostic, but we can not activate