[red-knot] Implicit instance attributes

[sharkdp]

Summary

Add support for implicitly-defined instance attributes, i.e. support type inference for cases like this:

class C:
    def __init__(self) -> None:
        self.x: int = 1
        self.y = None

reveal_type(C().x)  # int
reveal_type(C().y)  # Unknown | None

A lot of things have been intentionally left out of this PR:

• Type inference for self
• Support attributes which are implicitly "declared" via their parameter types (self.x = param)
• Tuple-unpacking attribute assignments: self.x, self.y = …
• Diagnostic for conflicting declared types
• Attributes in statically-known-to-be-false branches are still visible:

  class C:
      def __init__(self) -> None:
          if 2 + 3 < 4:
              self.x: str = "a"
  
  # # TODO: Ideally, this would result in a `unresolved-attribute` error. But mypy and pyright
  # do not support this either (for conditions that can only be resolved to `False` in type
  # inference), so it does not seem to be particularly important.
  reveal_type(C().x)  # revealed: str

Part of: #14164

Benchmarks

Codspeed reports no change in a cold-cache benchmark, and a -1% regression in the incremental benchmark. On black's src folder, I don't see a statistically significant difference between the branches (Reminder to self: always set the CPU frequency scaling governor to performance when benchmarking on a laptop):

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
./red_knot_main check --project /home/shark/black/src	133.7 ± 9.5	126.7	164.7	1.01 ± 0.08
./red_knot_feature check --project /home/shark/black/src	132.2 ± 5.1	118.1	140.9	1.00

Test Plan

Updated and new Markdown tests

[carljm]

Review not complete yet, just submitting partway through

[carljm]

What about assignments to attributes of the first argument of a classmethod or staticmethod?

[sharkdp]

Those are not (properly) supported yet. We do have one pre-existing test for attribute assignments in @classmethods in this file, but we do not yet find these implicitly-declared attributes when calling .member on the class object. With this PR, we're potentially in a weird state where we find those attributes on instances, but not on the class object. If that's acceptable, I'll open an issue and add that to my goals for next week. Otherwise, I can also include it in this PR.

[carljm]

I don't think the functionality scope of this PR needs to be expanded, this can definitely be a follow-up. I was more just thinking that some TODO tests around it would clarify the intention and fit in well with the existing tests in this PR, which are already a mix of "complete" and "TODO". But there's certainly no need for that, the tests can come with the feature.

[sharkdp]

I'll note this down as a task for a follow-up.

[carljm]

I guess at this point we do attribute lookup fully lazily, and without any Salsa caching. I wonder if there's enough work repeated here to make some Salsa caching of attribute types worth it? But we can evaluate this separately.

[MichaReiser]

We may want to have salsa caching here to avoid invalidating types from module a if the class is defined in module b. Or is it guaranteed that this method is only ever called from within the same module (or if it is called across-modules, that it goes through a salsa query?)

[sharkdp]

I think I need some help answering these questions.

Or is it guaranteed that this method is only ever called from within the same module

I don't think so.

or if it is called across-modules, that it goes through a salsa query?

This function is only called from Class::instance_member (via own_instance_member), which in turn can be called from:

• Type::member
• TypeInferenceBuilder::infer_attribute_expression

The former (Type::member) is called from all sorts of different places.

[MichaReiser]

Thanks. Yeah, in that case, I think implicit_instance_attribute or own_instance_member or member should be a salsa query because: semantic_index changes whenever the file where the class is declared changes, and nothing is constraining member calls to be from the same file.

An alternative is to introduce a attribute_assignments(db, class_body_scope) query that returns the assignments just for that scope. That might be cheaper but it still has the benefit that implicit_instance_attribute only gets invalidated when the attribute assignments in that specific scope changed.

We use a similar pattern for use_def_map and symbol_table where those queries return a specific "slice" of the SemanticIndex which is less likely to change.

Let me know if that helps or if you need some more input (also happy to discuss on Discord/sync)

[sharkdp]

For other reviewers: I implemented something that I discussed with Micha.

Removing salsa::tracked from either attribute_assignments or the inline infer_expression_type query makes the newly added test fail.

[carljm]

Ok, finished review, only had one more comment.

This is, as usual, awesome work.

[MichaReiser]

We may want to have salsa caching here to avoid invalidating types from module a if the class is defined in module b. Or is it guaranteed that this method is only ever called from within the same module (or if it is called across-modules, that it goes through a salsa query?)

[MichaReiser]

I wonder if this would be useful to have as a general query and if it should be defined next to infer_expression_types instead.

[sharkdp]

I had a similar thought. It could certainly be used in more places, but I wasn't sure if we want the additional query in those places? I'll note it down as a task and open a small PR after this has been merged.

[carljm]

It is intuitive to me why attribute_assignments should be a query, but it's less intuitive to me why this should be a query. I don't see what additional dependencies are introduced here above those that the underlying infer_expression_types query would have anyway. So I guess this is not about dependencies, but rather about a smaller returned value so backdating can be more effective?

It feels intuitively to me that we would get more caching value with fewer cached memos if we placed the salsa query caching at the level of Class::own_instance_member or Type::member, rather than at such a fine-grained level that does such little work over the underlying infer_expression_types. But we should of course validate any such changes with observed performance differences.

[MichaReiser]

The problem is the expression.node_ref. It accesses the AST node from the module where the class is declared, meaning the enclosing query has to re-run whenever the declaring module changes -- which we don't want.

Doing it here is mainly about avoiding calling a query (and caching the value which is expensive) in cases where we don't need to. Although I admit, I don't have any numbers on whether caching here is a significantly lower number than caching at the own_instance_member level.