-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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] Add support for typing.ClassVar
#15550
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me from a salsa/rust perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
crates/red_knot_python_semantic/resources/mdtest/type_qualifiers/classvar.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/type_qualifiers/classvar.md
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
crates/red_knot_python_semantic/resources/mdtest/type_qualifiers/classvar.md
Show resolved
Hide resolved
|
||
## Conflicting type qualifiers | ||
|
||
We currently ignore conflicting qualifiers and simply union them, which is more conservative than |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be a diagnostic? But it isn't in pyright (which has the same behavior you implemented here).
It is in mypy, but for different reasons than it would be for us, since mypy just generally prohibits re-declaration.
Not sure, don't feel clear enough about it to even suggest a TODO comment. We can always add this diagnostic if we decide we need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "currently" was my way to try and describe that this is probably an open design decision. It feels to me like a not very common edge case, so in spirit of "every lint rule generates maintenance work", I'll leave this as-is for now, but it should be straightforward to add a lint for this later if we want to.
@@ -246,7 +247,7 @@ pub(crate) fn binding_ty<'db>(db: &'db dyn Db, definition: Definition<'db>) -> T | |||
} | |||
|
|||
/// Infer the type of a declaration. | |||
fn declaration_ty<'db>(db: &'db dyn Db, definition: Definition<'db>) -> Type<'db> { | |||
fn declaration_ty<'db>(db: &'db dyn Db, definition: Definition<'db>) -> TypeAndQualifiers<'db> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: is it valuable to maintain an invariant that a variable named *_ty
contains a Type
, and a function named *_ty
returns a Type
? If so, should we establish a suffix like *_tyq
for a "type and qualifiers"? Or is this all too pedantic and veering unnecessarily into Hungarian notation? (I don't feel strongly.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid I broke this invariant before, and never cleaned it up. For example, bindings_ty
returns a Symbol
. Most call sites that call functions returning Symbol
/TypeAndQualifiers
are only interested in the contained type, but it's probably still better to clean up those names. I opened #15569 as a follow-up task for me.
ExprContext::Store => { | ||
let value_ty = self.infer_expression(value); | ||
|
||
if let (ast::ExprContext::Store, Type::Instance(instance)) = (ctx, value_ty) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're already inside a match arm for ctx
being Store
, why do we need to match on ctx
again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching that. I previously had this code block in an entirely different place (where the match on ctx
was necessary) and forgot to remove it when I moved it here.
With type narrowing, clippy could have been able to catch this 😄
Summary
Add support for
typing.ClassVar
, i.e. emit a diagnostic in this scenario:Test Plan
typing.ClassVar
qualifierattributes.md