refactor: create multiple HIR items for imports #51425

QuietMisdreavus · 2018-06-07T22:36:20Z

When lowering use statements into HIR, they get a Def of the thing they're pointing at. This is great for things that need to know what was just pulled into scope. However, this is a bit misleading, because a use statement can pull things from multiple namespaces if their names collide. This is a problem for rustdoc, because if there are a module and a function with the same name (for example) then it will only document the module import, because that's that the lowered use statement points to.

The current version of this PR does the following:

Whenever the resolver comes across a use statement, it loads the definitions into a new import_map instead of the existing def_map. This keeps the resolutions per-namespace so that all the target definitions are available.
When lowering use statements, it looks up the resolutions in the import_map and creates multiple Items if there is more than one resolution.
To ensure the NodeIds are properly tracked in the lowered module, they need to be created in the AST, and pulled out as needed if multiple resolutions are available.

Fixes #34843

eddyb · 2018-06-07T22:38:27Z

r? @petrochenkov cc @jseyfried @nikomatsakis

QuietMisdreavus · 2018-06-08T00:50:46Z

Lol of course the test that breaks is a rustdoc test. The test seems to make sure that we rename re-exports to have the name of the export, rather than the original struct name. I'm taking a look...

QuietMisdreavus · 2018-06-08T20:31:27Z

It looks like... travis passed?! I suppose it's ready for people to take a look.

One place i'd like to make sure about is save-analysis. Right now the get_path_def function is set up so that wherever it was pulling a Def out of a Path, it now just scans the path.defs to pull the first valid def out (the last remaining place this behavior exists). I'm concerned about whether it would be valid to have this behavior, or what ramifications it would have to make this function return PerNS<Def> instead, to bring the multi-Def behavior into save-analysis. I know vanishingly little about how to wedge this change into the actual save-analysis output, though, nor what needs to happen to support that.

QuietMisdreavus · 2018-06-09T00:23:26Z

cc @nrc about the save-analysis questions up-thread. This is a compiler refactor that should be extended to save-analysis way better than i'm doing it right now, but doing that may require messing with the output format. I'm not familiar with save-analysis, so i don't know how involved the change will be.

petrochenkov · 2018-06-09T15:54:11Z

Will review today/tomorrow.

petrochenkov · 2018-06-10T08:31:13Z

I don't think this generally goes into the right direction.
This PR creates a lot of "impossible state" and forces all paths to deal with per-namespace definitions while they are relevant only to non-glob imports.
This is especially true for HIR in which it's questionable whether use items even need to exist (they are used only by rustdoc and also stability pass that needs to be moved to AST / name resolution stage).

I think we can go from the end goal here:

Multiple import resolutions are needed by rustdoc (and also stability checking which is currently buggy and only checks the type namespace for stability).
Rustdoc is HIR based, so we need keep multiple resolutions in HIR, but keeping them for each Path is undesirable.
The solution is to generate several ItemUse HIR items from a single UseTreeKind::Simple AST items during lowering, one for each present namespace.
If resolver can't provide information for this import duplication during lowering, then it should be extended in some minimal way, e.g. with one more table "import's NodeId -> Set" in Resolver filled only for imports, I think this should be enough.

I want to do a similar refactoring in resolve and desugar single import use a::b::c; into multiple paths use a::b::c::{{type}}; use a::b::c::{{value}}; use a::b::c::{{macro}}; each with a single potential resolution, before running import resolution. (As opposed to lumping several resolution into a single PerNS for a single import path).
The solution described above would fit better into this plan.

eddyb · 2018-06-10T09:56:13Z

@petrochenkov I suppose that makes sense given that most paths only can access one NS.
How do you feel about being able to later tell that a name was available but in the wrong namespace? Also, do you think per-segment Def is valuable (since I was recently talking to @nikomatsakis about this), more so than using PerNS<Def> for paths?

In terms of further expanding uses in HIR, would it converge to something similar to ExportMap?

petrochenkov · 2018-06-10T15:28:44Z

@eddyb

How do you feel about being able to later tell that a name was available but in the wrong namespace?

This is already kinda done locally in resolve (see fn resolve_qpath_anywhere) so we can report errors like expected value, found macro, but unexpected resolutions are then replaced with Def::Err so all later stages have to deal only with a single erroneous case.
What use these resolutions would have if the error was already reported in resolve?

Also, do you think per-segment Def is valuable

They would allow to fix issues like #15702 (but not #23937) while keeping the stability pass in its current place and not moving it to resolve. I'd rather prefer to move it to resolve to fix #23937 as well.
Otherwise I don't know how they'd be useful for already resolved paths (for unresolved paths we already have QPath::TypeRelative). Some extra diagnostics perhaps?

In terms of further expanding uses in HIR, would it converge to something similar to ExportMap?

If stability pass is moved, then it should be possible to reduce imports in HIR to ExportMap, unless rustdoc needs to display glob imports precisely or some import extensions like this are adopted.

nrc · 2018-06-11T18:01:07Z

@QuietMisdreavus changing the output of save-analysis to have mutiple defs per ref would not be too hard, you'd just change the data format (in rls-data) and how that is read (in rls-analysis), any tools rely on the rls-analysis crate not the data directly.

However, I think that is only the right approach for imports. For all other refs, by the time we get to generating save-analysis there should only be one name in play since we know which namespace we're using by type checking at the latest (iiuc).

QuietMisdreavus · 2018-06-11T18:37:31Z

The solution is to generate several ItemUse HIR items from a single UseTreeKind::Simple AST items during lowering, one for each present namespace.

I started with this approach before moving to the one in this PR. (It still started with changing DefMap to have a PerNS<Option<PathResolution>> in it, so that foundation probably needs to be changed, based on the rest of the discussion here.)

It seems like you're proposing some new kind of map in the resolver just for the use statements, and using that to lower use statements instead of the regular DefMap? I'm not sure i totally follow the rest of the discussion.

petrochenkov · 2018-06-11T19:04:28Z

@QuietMisdreavus

It seems like you're proposing some new kind of map in the resolver just for the use statements, and using that to lower use statements instead of the regular DefMap?

Yes, an additional table should get the job done.
Resolutions for imports shouldn't probably even get into def_map, only into this new table.

bors · 2018-06-12T00:40:12Z

☔ The latest upstream changes (presumably #50617) made this pull request unmergeable. Please resolve the merge conflicts.

QuietMisdreavus · 2018-06-14T21:40:36Z

Okay, i think i have something. I've just force-pushed an update that takes a different approach, more like @petrochenkov's suggestion. I had to modify the AST for use-trees to hold some new NodeIds, so that the new HIR items could be properly referenced later on during type collection. This should hopefully be less breaking than the previous change.

rust-highfive · 2018-06-14T22:43:34Z

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[01:04:20] failures:
[01:04:20] 
[01:04:20] ---- [rustdoc] rustdoc/pub-use-extern-macros.rs stdout ----
[01:04:20] 
[01:04:20] error: htmldocck failed!
[01:04:20] status: exit code: 1
[01:04:20] command: "/usr/bin/python2.7" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/pub-use-extern-macros" "/checkout/src/test/rustdoc/pub-use-extern-macros.rs"
[01:04:20] ------------------------------------------
[01:04:20] 
[01:04:20] ------------------------------------------
[01:04:20] stderr:
[01:04:20] stderr:
[01:04:20] ------------------------------------------
[01:04:20] 18: @!has check failed
[01:04:20]  `XPATH PATTERN` did not match
[01:04:20]  // @!has pub_use_extern_macros/index.html '//code' 'pub use macros::bar;'
[01:04:20] 22: @!has check failed
[01:04:20]  `XPATH PATTERN` did not match
[01:04:20]  // @!has pub_use_extern_macros/index.html '//code' 'pub use macros::baz;'
[01:04:20] Encountered 2 errors
[01:04:20] 
[01:04:20] ------------------------------------------
[01:04:20] 
[01:04:20] 
[01:04:20] thread '[rustdoc] rustdoc/pub-use-extern-macros.rs' panicked at 'explicit panic', too" "--android-cross-path" "" "--color" "always"
[01:04:20] 
[01:04:20] 
[01:04:20] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:04:20] Build completed unsuccessfully in 0:23:13
[01:04:20] Build completed unsuccessfully in 0:23:13
[01:04:20] Makefile:58: recipe for target 'check' failed
[01:04:20] make: *** [check] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0f9317b2
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:0c8c789d:start=1529016211635082647,finish=1529016211642102837,duration=7020190
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0db6743a
$ head -30 ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
head: cannot open ‘./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers’ for reading: No such file or directory
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:08466530
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

oli-obk · 2018-06-14T23:16:49Z

src/librustc/hir/lowering.rs

@@ -571,6 +574,21 @@ impl<'a> LoweringContext<'a> {
        })
    }

+    fn expect_full_def_from_use(&mut self, id: NodeId) -> Vec<Def> {


This could return an impl Iterator<Item=Def> by not doing the is_empty dance which is repeated at the use site of this function anyway.

Oh, it turns out i can! I was scarred because i tried doing that to a version of lower_path (which i wound up taking out when we moved to the AST modification) and wound up with exotic lifetime errors due to the introduction of a &ast::Path with an unaccounted lifetime. But this one doesn't have to use anything external for its mapping, so it works out.

oli-obk · 2018-06-14T23:17:20Z

src/librustc/hir/lowering.rs

+                let mut defs = self.expect_full_def_from_use(id).into_iter();
+                // we want to return *something* from this function, so hang onto the first item
+                // for later
+                let mut ret_def = defs.next().unwrap_or(Def::Err);


use site duplication here

oli-obk · 2018-06-14T23:22:00Z

src/librustc/hir/lowering.rs

+                    let vis = vis.clone();
+                    let name = name.clone();
+                    let span = path.span;
+                    self.resolver.definitions().create_def_with_parent(


It could now be possible to not create def ids here, but instead in

rust/src/librustc/hir/map/def_collector.rs

Line 131 in 538fea5

fn visit_use_tree(&mut self, use_tree: &'a UseTree, id: NodeId, _nested: bool) {

with the create_def method. I'm not sure if it's possible, because you'd already need to know how many definitions you are going to need

I'm guessing it's problematic to create all the defs ahead of time and leave them potentially unused? Is there a way to pull them back out later (or mark them used in a way that won't affect anything)?

Yes, that's what I worry about, too.

petrochenkov · 2018-06-16T20:44:38Z

src/libsyntax/ast.rs

+    ///
+    /// The extra `NodeId`s are for HIR lowering, when additional statements are created for each
+    /// namespace.
+    Simple(Option<Ident>, NodeId, NodeId),


Is this really necessary?
Can't we create HIR items with fresh NodeIds "out of thin air"?

While possible, we'd need to insert a cache for the NodeId of the Simple variant to the generated NodeIds and carry that one around in lowering.

The reason for that is that you need the same ids in https://github.com/rust-lang/rust/pull/51425/files#diff-64b696b0ef6ad44140e973801ed82b25R2742 and https://github.com/rust-lang/rust/pull/51425/files#diff-64b696b0ef6ad44140e973801ed82b25R2409

I tried creating the NodeIds during lowering, but they weren't referenced by the module after lowering, so type collection hit an error when it tried to account for these items. Unless there's an easier way to reference the new Items and their IDs when the module's ItemIds are lowered, this is what @oli-obk came up with.

petrochenkov · 2018-06-16T20:45:23Z

PR description also needs an update.

QuietMisdreavus · 2018-06-16T21:07:28Z

I've updated the PR description.

petrochenkov · 2018-06-17T08:41:43Z

@bors r+

bors · 2018-06-17T08:41:44Z

📌 Commit 903e2c8 has been approved by petrochenkov

bors · 2018-06-17T09:48:16Z

⌛ Testing commit 903e2c8 with merge 594b05d...

…=petrochenkov refactor: create multiple HIR items for imports When lowering `use` statements into HIR, they get a `Def` of the thing they're pointing at. This is great for things that need to know what was just pulled into scope. However, this is a bit misleading, because a `use` statement can pull things from multiple namespaces if their names collide. This is a problem for rustdoc, because if there are a module and a function with the same name (for example) then it will only document the module import, because that's that the lowered `use` statement points to. The current version of this PR does the following: * Whenever the resolver comes across a `use` statement, it loads the definitions into a new `import_map` instead of the existing `def_map`. This keeps the resolutions per-namespace so that all the target definitions are available. * When lowering `use` statements, it looks up the resolutions in the `import_map` and creates multiple `Item`s if there is more than one resolution. * To ensure the `NodeId`s are properly tracked in the lowered module, they need to be created in the AST, and pulled out as needed if multiple resolutions are available. Fixes #34843

bors · 2018-06-17T11:53:51Z

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 594b05d to master...

rustdoc: import cross-crate macros alongside everything else The thrilling conclusion of the cross-crate macro saga in rustdoc! After #51425 made sure we saw all the namespaces of an import (and prevented us from losing the `vec!` macro in std's documentation), here is the PR to handle cross-crate macro re-exports at the same time as everything else. This way, attributes like `#[doc(hidden)]` and `#[doc(no_inline)]` can be used to control how the documentation for these macros is seen, rather than rustdoc inlining every macro every time. Fixes #50647

@QuietMisdreavus

rustdoc: Hide struct and enum variant constructor imports This is fallout from #51425. The duplicate variant imports can be seen [here](https://doc.rust-lang.org/nightly/std/prelude/v1/index.html) for example. This is fixing a regression so could be backported to beta. r? @QuietMisdreavus

rust-highfive assigned estebank Jun 7, 2018

This comment has been minimized.

Sign in to view

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 7, 2018

rust-highfive assigned petrochenkov and unassigned estebank Jun 7, 2018

This comment has been minimized.

Sign in to view

QuietMisdreavus changed the title ~~[WIP] refactor: resolve/store Defs per-namespace~~ refactor: resolve/store Defs per-namespace Jun 8, 2018

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 10, 2018

QuietMisdreavus force-pushed the thats-def-a-namespace-there branch from 72f9e04 to df8c30d Compare June 14, 2018 21:36

QuietMisdreavus changed the title ~~refactor: resolve/store Defs per-namespace~~ refactor: create multiple HIR items for imports Jun 14, 2018

QuietMisdreavus added 3 commits June 14, 2018 17:47

create multiple HIR items for a use statement

122b5b4

rustdoc: discard struct ctors from imports

16cfa5c

add test for issue 34843

02121f3

QuietMisdreavus force-pushed the thats-def-a-namespace-there branch from df8c30d to 02121f3 Compare June 14, 2018 22:47

oli-obk reviewed Jun 14, 2018

View reviewed changes

turn expect_full_def_from_use into an iterator

903e2c8

petrochenkov reviewed Jun 16, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 17, 2018

bors mentioned this pull request Jun 17, 2018

The Great Generics Generalisation: HIR Edition #48149

Merged

bors merged commit 903e2c8 into rust-lang:master Jun 17, 2018

QuietMisdreavus deleted the thats-def-a-namespace-there branch June 17, 2018 15:20

This was referenced Jun 17, 2018

rustdoc: import cross-crate macros alongside everything else #51611

Merged

rustdoc: pub use for macros is inlined too eagerly #50647

Closed

mati865 mentioned this pull request Jun 18, 2018

Version bump rust-lang/rust-clippy#2851

Merged

dtolnay mentioned this pull request Jun 20, 2018

Cargo doc regression in crates that include #![deny(unused_imports)] #51661

Closed

QuietMisdreavus mentioned this pull request Jun 23, 2018

Function is not linked in docs if is has the same name as a struct and is re-exported #37342

Closed

ollie27 mentioned this pull request Jul 9, 2018

rustdoc: Hide struct and enum variant constructor imports #52196

Merged

eddyb mentioned this pull request Jul 31, 2018

2018 edition allows ungated access to unstable crates #52489

Closed

petrochenkov mentioned this pull request Oct 24, 2018

Keep resolved defs in path prefixes and emit them in save-analysis #54145

Merged

petrochenkov mentioned this pull request Nov 22, 2018

Issue 56128 segment id ice nightly #56143

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: create multiple HIR items for imports #51425

refactor: create multiple HIR items for imports #51425

QuietMisdreavus commented Jun 7, 2018 •

edited

Loading

This comment has been minimized.

eddyb commented Jun 7, 2018

This comment has been minimized.

This comment has been minimized.

QuietMisdreavus commented Jun 8, 2018

QuietMisdreavus commented Jun 8, 2018

QuietMisdreavus commented Jun 9, 2018

petrochenkov commented Jun 9, 2018

petrochenkov commented Jun 10, 2018

eddyb commented Jun 10, 2018

petrochenkov commented Jun 10, 2018

nrc commented Jun 11, 2018

QuietMisdreavus commented Jun 11, 2018

petrochenkov commented Jun 11, 2018

bors commented Jun 12, 2018

QuietMisdreavus commented Jun 14, 2018

rust-highfive commented Jun 14, 2018

oli-obk Jun 14, 2018

QuietMisdreavus Jun 15, 2018

oli-obk Jun 14, 2018

oli-obk Jun 14, 2018

QuietMisdreavus Jun 15, 2018

oli-obk Jun 15, 2018

petrochenkov Jun 16, 2018

oli-obk Jun 16, 2018

QuietMisdreavus Jun 16, 2018

petrochenkov commented Jun 16, 2018

QuietMisdreavus commented Jun 16, 2018

petrochenkov commented Jun 17, 2018

bors commented Jun 17, 2018

bors commented Jun 17, 2018

bors commented Jun 17, 2018

refactor: create multiple HIR items for imports #51425

refactor: create multiple HIR items for imports #51425

Conversation

QuietMisdreavus commented Jun 7, 2018 • edited Loading

This comment has been minimized.

eddyb commented Jun 7, 2018

This comment has been minimized.

This comment has been minimized.

QuietMisdreavus commented Jun 8, 2018

QuietMisdreavus commented Jun 8, 2018

QuietMisdreavus commented Jun 9, 2018

petrochenkov commented Jun 9, 2018

petrochenkov commented Jun 10, 2018

eddyb commented Jun 10, 2018

petrochenkov commented Jun 10, 2018

nrc commented Jun 11, 2018

QuietMisdreavus commented Jun 11, 2018

petrochenkov commented Jun 11, 2018

bors commented Jun 12, 2018

QuietMisdreavus commented Jun 14, 2018

rust-highfive commented Jun 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Jun 16, 2018

QuietMisdreavus commented Jun 16, 2018

petrochenkov commented Jun 17, 2018

bors commented Jun 17, 2018

bors commented Jun 17, 2018

bors commented Jun 17, 2018

QuietMisdreavus commented Jun 7, 2018 •

edited

Loading