-
Notifications
You must be signed in to change notification settings - Fork 892
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.
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
[red-knot] record dependencies when resolving across imports #11176
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,10 +6,8 @@ use crate::types::Type; | |
use crate::FileId; | ||
use ruff_python_ast::AstNode; | ||
|
||
// TODO this should not take a &mut db, it should be a query, not a mutation. This means we'll need | ||
// to use interior mutability in TypeStore instead, and avoid races in populating the cache. | ||
#[tracing::instrument(level = "trace", skip(db))] | ||
MichaReiser marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add some documentation explaining what the |
||
where | ||
Db: SemanticDb + HasJar<SemanticJar>, | ||
{ | ||
|
@@ -36,15 +34,27 @@ where | |
// TODO relative imports | ||
assert!(matches!(level, 0)); | ||
let module_name = ModuleName::new(module.as_ref().expect("TODO relative imports")); | ||
if let Some(module) = db.resolve_module(module_name) { | ||
if let Some(module) = db.resolve_module(module_name.clone()) { | ||
let remote_file_id = module.path(db).file(); | ||
let remote_symbols = db.symbol_table(remote_file_id); | ||
if let Some(remote_symbol_id) = remote_symbols.root_symbol_id_by_name(name) { | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I already commented this on the other PR. I think it would be nice if we had a struct that combines There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, agreed, will push that new type in this PR. |
||
(remote_file_id, remote_symbol_id), | ||
); | ||
db.infer_symbol_type(remote_file_id, remote_symbol_id) | ||
} else { | ||
db.jar() | ||
.type_store | ||
.record_module_dependency((file_id, symbol_id), module_name); | ||
Type::Unknown | ||
} | ||
} else { | ||
db.jar() | ||
.type_store | ||
.record_module_dependency((file_id, symbol_id), module_name); | ||
Type::Unknown | ||
} | ||
} | ||
|
@@ -60,7 +70,7 @@ where | |
let ast = parsed.ast(); | ||
let node = node_key.resolve_unwrap(ast.as_any_node_ref()); | ||
|
||
let store = &mut db.jar_mut().type_store; | ||
let store = &db.jar().type_store; | ||
let ty = Type::Class(store.add_class(file_id, &node.name.id)); | ||
store.cache_node_type(file_id, *node_key.erased(), ty); | ||
ty | ||
|
@@ -69,10 +79,9 @@ where | |
_ => todo!("other kinds of definitions"), | ||
}; | ||
|
||
db.jar_mut() | ||
db.jar() | ||
.type_store | ||
.cache_symbol_type(file_id, symbol_id, ty); | ||
// TODO record dependencies | ||
ty | ||
} | ||
|
||
|
@@ -112,7 +121,7 @@ mod tests { | |
fn follow_import_to_class() -> std::io::Result<()> { | ||
let TestCase { | ||
src, | ||
mut db, | ||
db, | ||
temp_dir: _temp_dir, | ||
} = create_test()?; | ||
|
||
|
@@ -132,10 +141,24 @@ mod tests { | |
|
||
let ty = db.infer_symbol_type(a_file, d_sym); | ||
|
||
let b_file = db | ||
.resolve_module(ModuleName::new("b")) | ||
.expect("module should be found") | ||
.path(&db) | ||
.file(); | ||
let b_syms = db.symbol_table(b_file); | ||
let c_sym = b_syms | ||
.root_symbol_id_by_name("C") | ||
.expect("C symbol should be found"); | ||
|
||
let jar = HasJar::<SemanticJar>::jar(&db); | ||
|
||
assert!(matches!(ty, Type::Class(_))); | ||
assert_eq!(format!("{}", ty.display(&jar.type_store)), "Literal[C]"); | ||
assert_eq!( | ||
jar.type_store.get_module(a_file).symbol_dependencies[&d_sym], | ||
[(b_file, c_sym)].iter().copied().collect() | ||
); | ||
Ok(()) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this track the
ModuleId
instead of theModuleName
? For exampel, what if the module name remains unchanged, but it now resolves to a different module:namespace1/foo/bar.py
andnamespace2/foo/baz.py
wherefoo
has no__init__.py
and theModuleName
isfoo.baz
__init__
.py tonamespace1/foo
, nowfoo.baz
no longer resolves.Even without this limitation. Storing
ModuleId
s would be cheaper overModuleName
s. However,ModuleId
s have the downside that they're less stable. So storingModuleName
s might, after all, be desired.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 onfoo.bar
, andfoo/bar.py
doesn't change, butfoo/bar/__init__.py
is added sofoo.bar
now resolves there, we must invalidate the dependency onfoo.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 betweenFileId
andModule
, or betweenModuleName
andModule
(or neither?). If it's the latter (stable 1:1 mapping betweenModuleName
andModule
), then I could store a dependency toModule
here and it would be a little cheaper.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
ModuleId
mapping is stable betweenModuleName
andModuleId
. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might. However, I'm also happy to use
ModuleId
when we know that it always comes from aModule
. It gives the id more semantic meaning.