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.

Sign up for GitHub

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

Jump to bottom

[red-knot] Pure instance variables declared in class body #15515

Merged

sharkdp merged 2 commits into main from david/pure-instance-variables

Jan 17, 2025

Contributor

sharkdp commented Jan 15, 2025 •

edited

Loading

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 a variable: ClassVar[str] into str without raising any diagnostic (though I'm not sure if Unknown would be better here than str). So I'm also fine with keeping this open until ClassVar support is also implemented.

What the PR currently does is the following:

Adds partial support for pure instance variables declared in the class body, i.e. this case:

class C:
    variable1: str = "a"
    variable2 = "b"

reveal_type(C().variable1)  # str
reveal_type(C().variable2)  # Unknown | Literal["b"]

Adds property as a known class to query for @property decorators
Splits up various @Todo(instance attributes) cases into sub-categories.

Test Plan

Modified existing MD tests.

sharkdp added the red-knot label

sharkdp force-pushed the david/pure-instance-variables branch from 915cc09 to 77cd8c3 Compare

January 15, 2025 21:50

This comment was marked as resolved.

Sign in to view

sharkdp force-pushed the david/pure-instance-variables branch 5 times, most recently from c52a570 to ca07032 Compare

January 16, 2025 21:50

sharkdp marked this pull request as ready for review

January 16, 2025 21:51

sharkdp requested review from carljm, MichaReiser and AlexWaygood as code owners

January 16, 2025 21:51

sharkdp commented

View reviewed changes

crates/red_knot_python_semantic/resources/mdtest/attributes.md Show resolved Hide resolved

sharkdp commented

View reviewed changes

crates/red_knot_python_semantic/resources/mdtest/attributes.md

    
              # 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]

Contributor Author

sharkdp Jan 16, 2025

Is it correct that we want Unknown | Literal[1] for the instance attribute access, but `Literal[1] for the class variable access? (above)

Contributor

carljm Jan 16, 2025

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.)

Contributor Author

sharkdp Jan 17, 2025

Ok, I added an additional test with a TODO for this case.

sharkdp commented

View reviewed changes

crates/red_knot_python_semantic/resources/mdtest/attributes.md

Comment on lines +459 to +465

		reveal_type((2).bit_length) # revealed: @Todo(bound method)
		reveal_type((2).denominator) # revealed: @Todo(@property)

Contributor Author

sharkdp Jan 16, 2025

https://github.com/python/typeshed/blob/2a461a2f7931dd58df8346aca626f5bc048e8455/stdlib/builtins.pyi#L243-L246

carljm approved these changes

View reviewed changes

Contributor

carljm left a comment

Looks good! I don't see a problem with landing this as-is.

crates/red_knot_python_semantic/resources/mdtest/attributes.md

    
              # 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]

Contributor

carljm Jan 16, 2025

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.)

crates/red_knot_python_semantic/resources/mdtest/attributes.md Show resolved Hide resolved

crates/red_knot_python_semantic/src/types.rs Outdated

@@ @@ -2308,6 +2305,7 @@ impl<'db> Type<'db> { @@
                               invalid_expressions: smallvec::smallvec![InvalidTypeExpression::BareAnnotated],
                               fallback_type: Type::unknown(),
                           }),
+                          Type::KnownInstance(KnownInstanceType::ClassVar) => Ok(Type::unknown()),

Contributor

carljm Jan 16, 2025

This should be a TODO, no?

Contributor Author

sharkdp Jan 17, 2025

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.

Contributor

carljm Jan 17, 2025

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.

MichaReiser approved these changes

View reviewed changes

sharkdp force-pushed the david/pure-instance-variables branch from ca07032 to f46cefd Compare

January 17, 2025 09:25

sharkdp added 2 commits

January 17, 2025 10:40


          [red-knot] Pure instance variables declared in class body

950f46c


          Add TODO comment and two additional tests

b8c920a

sharkdp force-pushed the david/pure-instance-variables branch from f46cefd to b8c920a Compare

January 17, 2025 09:40

sharkdp merged commit 6771b8e into main

21 checks passed

sharkdp deleted the david/pure-instance-variables branch

January 17, 2025 09:48

AlexWaygood reviewed

View reviewed changes

Member

AlexWaygood left a comment

Very nice!

crates/red_knot_python_semantic/src/types.rs

Comment on lines +4021 to +4028

+                                      // 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()
+                                      }

Member

AlexWaygood Jan 17, 2025

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

dcreager added a commit that referenced this pull request


          Merge branch 'main' into dcreager/generate-ast

01bac61

* 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)

sharkdp mentioned this pull request

[red-knot] instance attributes #14164

Open

17 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels