Implement related_decls tool
#1579
Conversation
fw-immunant
changed the title
related decls toolrelated_decls tool
fw-immunant
force-pushed
the
fw/related-decls
branch
from
987029b to
7234b44
Compare
arguably we should use serde_json for output formatting, but this works for now
fw-immunant
changed the title
related_decls toolrelated_decls tool
this tool only needs file IDs, not their paths
this was originally written before I found the `visit_file_defs` helper
spernsteiner
left a comment
spernsteiner
left a comment
There was a problem hiding this comment.
Would be nice to have an example of what the output looks like, or at least a rough schema. Otherwise this looks like it should work as a starting point.
| Definition::Trait(x) => Some(ModuleDef::Trait(x)), | ||
| Definition::TypeAlias(x) => Some(ModuleDef::TypeAlias(x)), | ||
| Definition::BuiltinType(x) => Some(ModuleDef::BuiltinType(x)), | ||
| _ => None, |
There was a problem hiding this comment.
Unless there's a huge number of cases to ignore, IMO it's better to match each one explicitly instead of using a wildcard case. If an upgrade to rust-analyzer adds new variants, we want to get a compile error here so we don't forget to update this function.
| if let Some(path) = item.canonical_module_path(db) { | ||
| append_module_path_to_string(db, path, edition, &mut out); | ||
| } |
There was a problem hiding this comment.
Maybe worth noting (or debug_assert!ing) that append_module_path_to_string always leaves :: at the end of out, similar to the other write! invocations. This makes it clear that the pop() below is correct regardless of which branches were taken previously.
| // Assume the first file in `vfs` is the crate root. | ||
| let (first_file_id, _) = vfs.iter().next().unwrap(); |
There was a problem hiding this comment.
We should move setup logic like this into rust_util. Currently it's duplicated between this tool and split_ffi. We had to change this logic in split_ffi to handle crates that have both a lib and a bin target (GaloisInc/Tractor-Crisp@ccd9e15) and we'll probably need a similar change here—better to have it in one place so fixes like that can be shared.
| use std::fmt::Write; | ||
| use std::ops::Index; |
There was a problem hiding this comment.
Move these to the top with the other imports
| unfound_paths.retain(|path| { | ||
| if canonical_path.as_ref() == Some(path) || abs_path == *path { |
There was a problem hiding this comment.
Sounds like this is looking for an entry in unfound_paths that exactly matches either abs_path or canonical_path. Could this use two calls to unfound_paths.remove(...) instead (and do the insert if one succeeds)? Calling retain() is O(N).
randomPoison
left a comment
randomPoison
left a comment
There was a problem hiding this comment.
Seems reasonable to me, I left a couple of comments but none of them are blockers.
| let func = match dwb { | ||
| DefWithBody::Function(f) => f, | ||
| _ => panic!("not function!"), | ||
| }; |
There was a problem hiding this comment.
Do we want the tool to recover gracefully when pointed at a definition that's not a function? main already returns a Result, seems like this function could do the same instead of panicking.
| for query in [Query::Uses, Query::UsedItems, Query::FnSignature] { | ||
| match query { | ||
| Query::Uses => { | ||
| let using_items = item_uses(&sema, &items_by_range, module_def); | ||
| let paths = using_items | ||
| .into_iter() | ||
| .map(|module_def| absolute_item_path(&db, module_def, Edition::DEFAULT)) | ||
| .collect::<Vec<_>>(); | ||
| path_info.insert("uses".to_owned(), paths.into()); | ||
| } | ||
| Query::UsedItems => { | ||
| let used_items = items_used_by(&sema, module_def); | ||
| let paths = used_items | ||
| .into_iter() | ||
| .map(|module_def| absolute_item_path(&db, module_def, Edition::DEFAULT)) | ||
| .collect::<Vec<_>>(); | ||
| path_info.insert("used_items".to_owned(), paths.into()); | ||
| } | ||
| Query::FnSignature => { | ||
| let id: Option<ModuleDefId> = module_def.try_into().ok(); | ||
| if let Some(ModuleDefId::FunctionId(func_id)) = id { | ||
| let sig = db.function_signature(func_id); | ||
| path_info.insert( | ||
| "signature".to_owned(), | ||
| pp_function_signature(&db, &*sig).into(), | ||
| ); | ||
| path_info.insert( | ||
| "written_signature".to_owned(), | ||
| function_signature_as_written(&sema, func_id.into()).into(), | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop + match seems odd to me. You're creating an array of query types and iterating over it, but there's no shared code between the three listed cases. It seems like you could get the same behavior without the loop or match, i.e. just trying the three query types in order. I noticed there's a commented out query field in Args, is the intent to make the type of query a user-specified option? Maybe slap a TODO on there if that's the intent.
Fixes #1563.
This builds and works, with the following caveats:
Uses of an item are returned as source locations; these need to be resolved to their immediately containing item.(fixed)Output should be formatted as JSON.(done)impl, etc.), but for now we'll emit both this WIP format and the signature exactly as written at the definition site, under the"written_signature"JSON key.We might want to select only some outputs (uses/used items/fn signature)(consumers can just filter out irrelevant output entries for now), andwe might want to accept multiple paths at once as input(done).