save-analysis: add parents to imports #46540
Conversation
kennytm
added
the
S-waiting-on-review
|
review ping for you @nrc ! |
| @@ -1233,10 +1233,13 @@ impl<'l, 'tcx: 'l, 'll, O: DumpOutput + 'll> DumpVisitor<'l, 'tcx, 'll, O> { | |||
| fn process_use_tree(&mut self, | |||
| use_tree: &'l ast::UseTree, | |||
| id: NodeId, | |||
| parent_item: &'l ast::Item, | |||
| root_item: &'l ast::Item, | |||
There was a problem hiding this comment.
Could you add a comment to the function here explaining what the root_item is please? Also perhaps what a use_tree is.
| let access = access_from!(self.save_ctxt, root_item); | ||
| let parent = self.save_ctxt.tcx.hir.opt_local_def_id(id) | ||
| .and_then(|id| self.save_ctxt.tcx.parent_def_id(id)) | ||
| .map(::id_from_def_id); |
There was a problem hiding this comment.
I'm not clear on what the tcx regards as the parent for a nested import - is it the enclosing module? Or the outer import? It seems like Rustdoc always wants the former. Which I would imagine is either the root_item or the parent of the root_item. But perhaps id is the id of the root item? Or the tcx always gives the module?
Anyway, could you explain what you think is going on here please?
There was a problem hiding this comment.
cc @pietroalbini, who might have some ideas on how to best handle this. My intuition was that we would want to store the parent imports as the parents, but I think that we would have to then store the nested parents as well. I'm not sure that rls-data has the right struct to represent this.
For now, it probably makes sense to just store the enclosing module as the parent.
There was a problem hiding this comment.
The parent_def_id of a nested import should be the parent of the enclosing thing, regardless of the nesting level (no defs are added for the use item, and no extra parents are set while creating the use tree's defs).
I don't see why the parent imports should be stored, since each import already has the full path in it, but I'm not familiar with rls at all so I don't know how it uses those things.
There was a problem hiding this comment.
I think that logically rustdoc wants a list imports and the module they are imported into. I.e., the imports are flattened for the RLS.
The parent here is the module (or function or block, whatever) where the import is declared and is the context into which the imported name is added. It's not 'parent import'.
So, it sounds like we are doing the right thing here.
| @@ -1399,6 +1404,7 @@ impl<'l, 'tcx: 'l, 'll, O: DumpOutput + 'll> Visitor<'l> for DumpVisitor<'l, 'tc | |||
| span, | |||
| name: item.ident.to_string(), | |||
| value: String::new(), | |||
| parent: None, | |||
There was a problem hiding this comment.
I would have thought you'd want the module with the extern crate in would be the parent here, why is that not the case?
|
@nrc Comments addressed. |
|
Thanks! @bors: r+ |
|
📌 Commit f93c10e has been approved by |
|
🔒 Merge conflict |
euclio
force-pushed
the
import-parents
branch
from
f93c10e to
6321349
Compare
|
Rebased and fixed the merge conflict in Cargo.lock. |
|
@bors r=nrc |
|
📌 Commit 6321349 has been approved by |
|
🔒 Merge conflict |
euclio
force-pushed
the
import-parents
branch
from
6321349 to
b82d280
Compare
|
Rebased again, conflict in Cargo.lock. |
|
@bors r=nrc |
|
📌 Commit b82d280 has been approved by |
save-analysis: add parents to imports This PR populates the `parent` field added to `Import` in `rls-data` 0.14. I'm not quite sure if I handled nested imports' parents correctly: this is a new feature to me. r? @nrc cc rust-dev-tools/rls-analysis#123
|
☀️ Test successful - status-appveyor, status-travis |
This PR populates the
parentfield added toImportinrls-data0.14.I'm not quite sure if I handled nested imports' parents correctly: this is a new feature to me.
r? @nrc
cc rust-dev-tools/rls-analysis#123