[red-knot] record dependencies when resolving across imports #11176
Conversation
|
| #[tracing::instrument(level = "trace", skip(db))] | ||
| pub fn infer_symbol_type<Db>(db: &mut Db, file_id: FileId, symbol_id: SymbolId) -> Type | ||
| pub fn infer_symbol_type<Db>(db: &Db, file_id: FileId, symbol_id: SymbolId) -> Type |
There was a problem hiding this comment.
Should we add some documentation explaining what the file_id and symbol_id paramters are?
| // TODO integrate this into module and symbol-resolution APIs (requiring a | ||
| // "requester" argument) so that it doesn't have to be remembered | ||
| db.jar().type_store.record_symbol_dependency( | ||
| (file_id, symbol_id), |
There was a problem hiding this comment.
I think I already commented this on the other PR. I think it would be nice if we had a struct that combines file_id and symbol_id. I find that clearer than unnamed two-element tuples. We may then even change the infer_symbol_type method to take such an instance as the argument instead of a file id and symbol id pair.
There was a problem hiding this comment.
Yep, agreed, will push that new type in this PR.
| .insert(to); | ||
| } | ||
|
|
||
| fn record_module_dependency(&self, from: (FileId, SymbolId), to: ModuleName) { |
There was a problem hiding this comment.
Should this track the ModuleId instead of the ModuleName? For exampel, what if the module name remains unchanged, but it now resolves to a different module:
- Before:
namespace1/foo/bar.pyandnamespace2/foo/baz.pywherefoohas no__init__.pyand theModuleNameisfoo.baz - After: Add a
__init__.py tonamespace1/foo, nowfoo.bazno longer resolves.
Even without this limitation. Storing ModuleIds would be cheaper over ModuleNames. However, ModuleIds have the downside that they're less stable. So storing ModuleNames might, after all, be desired.
There was a problem hiding this comment.
The case that you outline (same module name referring to a different file) is precisely why I think this needs to store a ModuleName. The dependency here is on "whatever this module name resolves to" -- if we have a dependency on foo.bar, and foo/bar.py doesn't change, but foo/bar/__init__.py is added so foo.bar now resolves there, we must invalidate the dependency on foo.bar. So the dependency is on the module name, not on the file ID.
Whether it could be on the module ID instead (i.e. type Module), or not, is unclear to me. That depends whether we guarantee stable 1:1 mapping between FileId and Module, or between ModuleName and Module (or neither?). If it's the latter (stable 1:1 mapping between ModuleName and Module), then I could store a dependency to Module here and it would be a little cheaper.
There was a problem hiding this comment.
The ModuleId mapping is stable between ModuleName and ModuleId. Although it is not guaranteed to be stable across runs (but that's not your concern, but something that the persistent caching layer must patch up).
There was a problem hiding this comment.
Hmm, maybe it would work out to keep this dependency to FileId, too? As long as in the case where a file is shadowed by a different file for the same module name, we always remove the now-shadowed file, invalidating all its data?
There was a problem hiding this comment.
We might. However, I'm also happy to use ModuleId when we know that it always comes from a Module. It gives the id more semantic meaning.
|
I don't think I'm going to merge this as-is, want to redo it based on further discussion, and also depends on the salsa-or-no-salsa decision. |
Summary
Record dependencies when resolving type of a symbol that depends on type of another symbol.
If a symbol in another module can't be resolved, then we record a dependency on the entire module (if it changes, maybe then the type can be resolved, so we would need to invalidate.)
Also corrects unnecessary mutable references to db/jar/type_store. (I know this was done in another outstanding PR too, but it was bugging me so I did it here too; I don't mind rebasing if the other one lands first.)
Test Plan
cargo test