[red-knot] Token system to avoid cross-module query dependencies

[MichaReiser]

Summary

This PR introduces a token based system to reduce accidental cross-module query dependencies.

The basic idea is that we create a custom accessor for tracked struct fields that return an AstNodeRef and require an extra query_file: File argument.
The query_file is the file of the enclosing query and we compare it against the tracked struct's scope to ensure it is the same.

The basic idea here is: You should probably not access the node if don't know in which context your query runs or wrap your code in an extra query.

Getting the file should generally be easy. E.g. TypeInferenceBuilder has a file method.

This system doesn't prevent abuse. It's normally easy to get the correct file. E.g. you can call definition.scope(db).file(db) to get the file
but let's not do that ;).

The main downside of this approach is that it's sort of annoying. But I take that for extra peace of mind.

This should mitigate/fix #15949

Alternative approach

I considered changing AstNodeRef::node to take a File instead. But that quickly became annoying because we'd lose the Deref and Debug would also need a workaround. It also isn't AstNodeRef::node that's the problem, it's accessing the field on the tracked struct. That's why I opted for the custom-accessor on the tracked struct approach.

The ideal solution is to solve this problem by restructuring our modules and enforce it with visibility constraints but that's a bit more work.

[AlexWaygood]

Suggested change

	/// It acts as a token of prove that we aren't accessing an AST node from a different file
	/// than in which the current enclosing Salsa query (which would lead to cross-file dependencies).
	/// It acts as a token of proof that we aren't accessing an AST node from a different file
	/// to the one from which the current enclosing Salsa query is being called.
	/// Doing so would lead to cross-file dependencies, hurting incremental computation.

[AlexWaygood]

Suggested change

	/// It acts as a token of prove that we aren't accessing an AST node from a different file
	/// than in which the current enclosing Salsa query (which would lead to cross-file dependencies).
	/// It acts as a token of proof that we aren't accessing an AST node from a different file
	/// to the one from which the current enclosing Salsa query is being called.
	/// Doing so would lead to cross-file dependencies, hurting incremental computation.

[AlexWaygood]

Suggested change

	/// `query_file` is the file for which the current query performs type inference.
	/// It acts as a token of prove that we aren't accessing an AST node from a different file
	/// than in which the current enclosing Salsa query (which would lead to cross-file dependencies).
	/// `query_file` is the file for which the current query performs type inference.
	/// It acts as a token of proof that we aren't accessing an AST node from a different file
	/// to the one from which the current enclosing Salsa query is being called.
	/// Doing so would lead to cross-file dependencies, hurting incremental computation.

[AlexWaygood]

Suggested change

	/// It acts as a token of prove that we aren't accessing an AST node from a different file
	/// than in which the current enclosing Salsa query (which would lead to cross-file dependencies).
	/// It acts as a token of proof that we aren't accessing an AST node from a different file
	/// to the one from which the current enclosing Salsa query is being called.
	/// Doing so would lead to cross-file dependencies, hurting incremental computation.

[AlexWaygood]

Suggested change

	/// It acts as a token of prove that we aren't accessing an AST node from a different file
	/// than in which the current enclosing Salsa query (which would lead to cross-file dependencies).
	/// It acts as a token of proof that we aren't accessing an AST node from a different file
	/// to the one from which the current enclosing Salsa query is being called.
	/// Doing so would lead to cross-file dependencies, hurting incremental computation.

[MichaReiser]

One caveat. This doesn't protect us fully from cross-module dependencies. For example, a function can still call semantic_index (or ast_ids) and introduce a cross-module dependency that way. But I think it helps a little?

[carljm]

I have mixed feelings about this. It feels like it's not protecting the boundary at the layer where we need to protect it in order to get the results we actually want (as mentioned above about semantic_index and ast_ids), and it introduces a fair amount of "noise" in order to provide protection that isn't quite at the right layer (which means I would still want to explore other solutions to #15949).

Exactly where the "right" layer is is still an open question: it could be set at "anything below Type", which would be I think pretty conceptually clean but might result in too many queries needed? But it seems clear that it ought to be set "above" semantic index at the very least.

If you feel this alone would have avoided a significant number of the errors we've made in this area in the past, I'm not opposed to merging it for now and considering other approaches later.

[MichaReiser]

If you feel this alone would have avoided a significant number of the errors we've made in this area in the past, I'm not opposed to merging it for now and considering other approaches later.

I do think it would have caught the cross-module use in Class::implicit_instance_attribute and in visibility constraint's evaluate.

But I agree that it's not as good as I first thought it would be.