[red-knot] Prevent cross-module query dependencies in own_instance_member

[MichaReiser]

Summary

This PR fixes #16172 by using infer_expression_type (which is a salsa query) when evaluating visibility constraints. This should be sufficient to remove any AST reads fromsymbol_from_declarations and symbol_from_bindings.

I did consider making a more local change in own_instance_member but I think that has two downsides:

• We pay the query cost even if the symbol has no visibility constraints (probably common for instance members?)
• It doesn't solve the problem that symbol_from_declarations and symbol_from_bindings are still cross-module, and it's very easy to forget to put the call behind a query because it isn't obvious that the method does cross-file analysis.

Performance

The cold benchmark improves by about 1%, the incremental benchmark by 3%

I first considered removing the #[salsa::tracked] from symbol_by_id because it is no longer necessary for cross-module isolation. However, this PR showed that the version where symbol_by_id actually performs better #16279 (comment)

Test Plan

I added a new test for own_instance_member that assert that own_instance_member isn't re-executed if there's a no-op change in the module that declares the Class. All credit for that test goes to @sharkdp because I used his original test as a template and he helped me come up with an example that contains visibility constraints.

I verified that the test fails on main.

[MichaReiser]

Ha, this actually performs better and it also makes it harder to hold it wrong. I still haven't been able to construct a test case where own_instance_member unnecessarily recomputes...

[MichaReiser]

These accessors are footguns. They make it very easy to introduce dependencies on kind. That's why I removed them.

[AlexWaygood]

I have not reviewed line-by-line in depth but this makes sense to me!

[AlexWaygood]

the amount of setup required here makes me think that we might want to prioritise adding support for incremental tests to mdtest, similar to mypy's test framework (take a look at https://github.com/python/mypy/blob/master/test-data/unit/check-incremental.test)

[MichaReiser]

Yeah, although copy pasting is fine, considering how few we have but it could be something to consider for a more scalable approach on how to avoid cross-file queries.

[AlexWaygood]

yeah, it would only be worth it if we consider that it would be useful to have a lot more tests like this, and we want to make it easier to write those tests.

[carljm]

There was already a design for this in the original mdtest design (and it's in the README as a planned improvement), we just haven't implemented yet. (And the design probably needs to change a bit to avoid using non-rendering tag info strings on code blocks.)

[codspeed-hq]

CodSpeed Performance Report

Merging #16268 will improve performances by 5.46%

_{Comparing micha/untrack-symbol-by-id (c06af23) with main (b385c7d)}

Summary

⚡ 1 improvements
✅ 31 untouched benchmarks

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
⚡	red_knot_check_file[incremental]	5.7 ms	5.4 ms	+5.46%

[MichaReiser]

Okay, keeping the symbol_by_id is by far the best for incremental. A solid 5% perf improvement.

[MichaReiser]

@carljm said he liked it overall. So I'll go ahead and merge it. We may want to keep iterating on how we want to enforce cross-module boundaries. E.g. is it okay that own_instance_members reads a symbol table from another module?