[red-knot] Pure instance variables declared in class body#15515
[red-knot] Pure instance variables declared in class body#15515
Conversation
sharkdp
force-pushed
the
david/pure-instance-variables
branch
from
915cc09 to
77cd8c3
Compare
This comment was marked as resolved.
This comment was marked as resolved.
sharkdp
force-pushed
the
david/pure-instance-variables
branch
5 times, most recently
from
c52a570 to
ca07032
Compare
sharkdp
requested review from
AlexWaygood,
MichaReiser and
carljm
as code owners
| # TODO: should be `str` | ||
| reveal_type(c_instance.variable_with_class_default) # revealed: @Todo(instance attributes) | ||
| reveal_type(c_instance.variable_with_class_default1) # revealed: str | ||
| reveal_type(c_instance.variable_with_class_default2) # revealed: Unknown | Literal[1] |
There was a problem hiding this comment.
Is it correct that we want Unknown | Literal[1] for the instance attribute access, but `Literal[1] for the class variable access? (above)
There was a problem hiding this comment.
No, I think we should probably add the union with Unknown for class attributes also; the same principles apply. (Though perhaps less strongly in practice, since mutation of class attributes is much less common -- but pyright and mypy both allow it, and we should too since it's allowed at runtime.)
There was a problem hiding this comment.
Ok, I added an additional test with a TODO for this case.
| reveal_type((2).bit_length) # revealed: @Todo(bound method) | ||
| reveal_type((2).denominator) # revealed: @Todo(@property) |
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Looks good! I don't see a problem with landing this as-is.
| # TODO: should be `str` | ||
| reveal_type(c_instance.variable_with_class_default) # revealed: @Todo(instance attributes) | ||
| reveal_type(c_instance.variable_with_class_default1) # revealed: str | ||
| reveal_type(c_instance.variable_with_class_default2) # revealed: Unknown | Literal[1] |
There was a problem hiding this comment.
No, I think we should probably add the union with Unknown for class attributes also; the same principles apply. (Though perhaps less strongly in practice, since mutation of class attributes is much less common -- but pyright and mypy both allow it, and we should too since it's allowed at runtime.)
| invalid_expressions: smallvec::smallvec![InvalidTypeExpression::BareAnnotated], | ||
| fallback_type: Type::unknown(), | ||
| }), | ||
| Type::KnownInstance(KnownInstanceType::ClassVar) => Ok(Type::unknown()), |
There was a problem hiding this comment.
Not exactly sure what you meant by that comment, but I added the following TODO comment:
// TODO: A bare `ClassVar` should rather be treated as if the symbol was not
// declared at all.There was a problem hiding this comment.
Sorry, I should have spelled out what (in my mind) the TODO is. I don't think the added comment is quite right, because a bare ClassVar is not equivalent to "not annotated". It means "this attribute is a pure ClassVar (that is, not settable on instances) but its declared type must be inferred from the RHS and unioned with Unknown." The latter part of this is equivalent to "not annotated" but the former part is not.
sharkdp
force-pushed
the
david/pure-instance-variables
branch
from
ca07032 to
f46cefd
Compare
sharkdp
force-pushed
the
david/pure-instance-variables
branch
from
f46cefd to
b8c920a
Compare
| // TODO: Eventually, we are going to process all decorators correctly. This is | ||
| // just a temporary heuristic to provide a broad categorization into properties | ||
| // and non-property methods. | ||
| if function.has_decorator(db, KnownClass::Property.to_class_literal(db)) { | ||
| todo_type!("@property").into() | ||
| } else { | ||
| todo_type!("bound method").into() | ||
| } |
There was a problem hiding this comment.
I'm really hopeful that we can keep our special-casing of properties to a minimum if we have a solid implementation of the descriptor protocol, since they really are just descriptor instances at the end of the day. But this seems fine for now, since properties are by far the most common descriptors around, and this improves our TODO messages, making it clear why we infer bad types for various @property attributes for the time being
* main: [red-knot] Inline `SubclassOfType::as_instance_type_of_metaclass()` (#15556) [`flake8-comprehensions`] strip parentheses around generators in `unnecessary-generator-set` (`C401`) (#15553) [`pylint`] Implement `redefined-slots-in-subclass` (`W0244`) (#9640) [`flake8-bugbear`] Do not raise error if keyword argument is present and target-python version is less or equals than 3.9 (`B903`) (#15549) [red-knot] `type[T]` is disjoint from `type[S]` if the metaclass of `T` is disjoint from the metaclass of `S` (#15547) [red-knot] Pure instance variables declared in class body (#15515) Update snapshots of #15507 with new annotated snipetts rendering (#15546) [`pylint`] Do not report methods with only one `EM101`-compatible `raise` (`PLR6301`) (#15507) Fix unstable f-string formatting for expressions containing a trailing comma (#15545) Support `knot.toml` files in project discovery (#15505) Add support for configuring knot in `pyproject.toml` files (#15493) Fix bracket spacing for single-element tuples in f-string expressions (#15537) [`flake8-simplify`] Do not emit diagnostics for expressions inside string type annotations (`SIM222`, `SIM223`) (#15405) [`flake8-pytest-style`] Do not emit diagnostics for empty `for` loops (`PT012`, `PT031`) (#15542)
4 participants
Summary
This is a small, tentative step towards the bigger goal of understanding instance attributes. I could imagine that this might not be desirable as a (mergeable) first iteration, as it could potentially turn some
@Todo(instance attributes)types into concrete types that are not correct. For example, we currently turn the instance attribute access to avariable: ClassVar[str]intostrwithout raising any diagnostic (though I'm not sure ifUnknownwould be better here thanstr). So I'm also fine with keeping this open untilClassVarsupport is also implemented.What the PR currently does is the following:
propertyas a known class to query for@propertydecorators@Todo(instance attributes)cases into sub-categories.Test Plan
Modified existing MD tests.