Improve suggestions for importing out-of-scope traits reexported as `_` #91412

compiler-errors · 2021-12-01T02:33:28Z

Fix up the parent_map query to prefer visible parents that don't export items with the name _.
- I'm not sure if I modified this query properly. Not sure if we want to check for other idents than kw::Underscore.
- This also has the side-effect of not doing BFS on any modules re-exported as _, but I think that's desirable here too (or else we get suggestions for paths like a::_::b like in this doctest example).
Bail in try_print_visible_def_path if the def_id is re-exported as _, which will fall back to printing the def-id's real (definition) path.
- Side-effect of this is that we print paths that are not actually public, but it seems we already sometimes suggest useing paths that are private anyways. See this doctest example for demonstration of current behavior.
Suggest a glob-import (for example my_library::prelude::*) if the trait in question is only pub-exported as _, as a fallback.
```
use foo::bar::prelude::*; // trait MyTrait
```
- I think this is good fallback behavior to suggest instead of doing nothing. Thanks to the original issue filer for suggesting this.

I was somewhat opinionated about behaviors in this PR, and I'm totally open to limiting the impact of my changes or only landing parts of this. Happy to receive feedback if there are better ways to the same end.

Fixes #86035

rust-highfive · 2021-12-01T02:33:35Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jackh726 (or someone else) soon.

Please see the contribution instructions for more information.

jplatte · 2021-12-01T08:43:43Z

Not sure if we want to check for other idents than kw::Underscore.

What about #[doc(hidden)] modules that export the symbol out symbols that are themselves #[doc(hidden)]? Are those accounted for already?

compiler-errors · 2021-12-01T17:51:27Z

Not sure if we want to check for other idents than kw::Underscore.

What about #[doc(hidden)] modules that export the symbol out symbols that are themselves #[doc(hidden)]? Are those accounted for already?

I don't think this factors in doc attributes... Not sure what the precedent on rustc using doc attributes for diagnostics is, so we might want to fix this in a separate issue.

bors · 2021-12-02T15:30:30Z

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

bors · 2021-12-03T20:07:36Z

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

compiler-errors · 2021-12-14T06:15:39Z

(rebased onto master)

jackh726

I'm not really familiar with this code and not comfortable doing a final review, but I left some initial comments.

src/test/ui/imports/auxiliary/unnamed_pub_trait_source.rs

src/test/ui/imports/unnamed_pub_trait.stderr

compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs

jackh726 · 2021-12-18T02:35:48Z

compiler/rustc_middle/src/ty/print/pretty.rs

+                        return Ok((self, false));
+                    }
+                } else {
+                    // FIXME(compiler-errors): What if we don't have an item corresponding to our


Good question, is there a test case that covers this case?

The previous logic didn't do anything, and it's just a missing diagnostic, right? Maybe we could emit a note that the thing is unimportable?

Yeah, this is more like the visible_parent for the item doesn't actually contain the item at all. This is actually probably a bug! (e.g. maybe the visible_parents_map has a bug).

jackh726 · 2021-12-18T02:37:43Z

compiler/rustc_middle/src/ty/print/pretty.rs

+                    if let Some(reexport) = reexports
+                        .into_iter()
+                        .find(|child| child.vis.is_public() && child.ident.name != kw::Underscore)
+                        .map(|child| child.ident.name)


Do we actually need to collect into a vec?

Good point. Probably not.

compiler/rustc_middle/src/ty/print/pretty.rs

jackh726 · 2021-12-18T02:41:30Z

r? rust-lang/compiler

Not really familiar with this area.

oli-obk · 2021-12-18T06:46:43Z

Maybe we should allow use foo::_; to import just the things reexported as _. So basically like use foo::*, but more targeted.

But that's a language change, we can do the suggestion logic first, as this PR does.

…le name

compiler-errors

Addressed comments.

@rustbot label -S-waiting-on-author +S-waiting-on-review

compiler-errors · 2021-12-21T03:15:43Z

compiler/rustc_middle/src/ty/print/pretty.rs

+                        return Ok((self, false));
+                    }
+                } else {
+                    // FIXME(compiler-errors): What if we don't have an item corresponding to our


Yeah, this is more like the visible_parent for the item doesn't actually contain the item at all. This is actually probably a bug! (e.g. maybe the visible_parents_map has a bug).

compiler-errors · 2021-12-21T03:15:57Z

compiler/rustc_middle/src/ty/print/pretty.rs

+                    if let Some(reexport) = reexports
+                        .into_iter()
+                        .find(|child| child.vis.is_public() && child.ident.name != kw::Underscore)
+                        .map(|child| child.ident.name)


Good point. Probably not.

src/test/ui/imports/unnamed_pub_trait.stderr

oli-obk · 2021-12-21T07:09:35Z

@bors r+ rollup

bors · 2021-12-21T07:09:36Z

📌 Commit 682a342 has been approved by oli-obk

…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#90345 (Stabilise entry_insert) - rust-lang#91412 (Improve suggestions for importing out-of-scope traits reexported as `_`) - rust-lang#91770 (Suggest adding a `#[cfg(test)]` to to a test module) - rust-lang#91823 (Make `PTR::as_ref` and similar methods `const`.) - rust-lang#92127 (Move duplicates removal when generating results instead of when displaying them) - rust-lang#92129 (JoinHandle docs: add missing 'the') - rust-lang#92131 (Sync rustc_codegen_cranelift) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

In-line · 2021-12-22T14:14:58Z

Hey, why #87349 was benchmarked, while this one not?
cc: @estebank

oli-obk · 2021-12-22T14:17:23Z

It wasn't benchmarked because it was rolled up. The rollup PR mentioned above was benchmarked

rust-highfive assigned jackh726 Dec 1, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 1, 2021

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 1, 2021

compiler-errors force-pushed the issue-86035 branch from ab9cd46 to 4a1b1b3 Compare December 3, 2021 04:11

compiler-errors force-pushed the issue-86035 branch from 4a1b1b3 to 906787b Compare December 14, 2021 06:15

jackh726 requested changes Dec 18, 2021

View reviewed changes

rust-highfive assigned oli-obk and unassigned jackh726 Dec 18, 2021

compiler-errors added 5 commits December 20, 2021 19:50

Test for issue-86035

48dab5c

Prefer visibility paths where items are not named _

1f2cf1e

Bail if printing item named _ in try_print_visible_def_path

c327627

Suggest glob-import if we need to import a trait, but it has no visib…

f835085

…le name

Fixup tests for issue-86035

682a342

compiler-errors force-pushed the issue-86035 branch from 906787b to 682a342 Compare December 21, 2021 03:53

compiler-errors commented Dec 21, 2021

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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 21, 2021

matthiaskrgr mentioned this pull request Dec 21, 2021

Rollup of 7 pull requests #92152

Merged

bors merged commit af09d24 into rust-lang:master Dec 21, 2021

rustbot added this to the 1.59.0 milestone Dec 21, 2021

compiler-errors deleted the issue-86035 branch November 2, 2022 02:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve suggestions for importing out-of-scope traits reexported as `_` #91412

Improve suggestions for importing out-of-scope traits reexported as `_` #91412

compiler-errors commented Dec 1, 2021 •

edited

Loading

rust-highfive commented Dec 1, 2021

jplatte commented Dec 1, 2021

compiler-errors commented Dec 1, 2021

bors commented Dec 2, 2021

bors commented Dec 3, 2021

compiler-errors commented Dec 14, 2021

jackh726 left a comment

jackh726 Dec 18, 2021

oli-obk Dec 18, 2021

compiler-errors Dec 21, 2021

jackh726 Dec 18, 2021

compiler-errors Dec 21, 2021

jackh726 commented Dec 18, 2021

oli-obk commented Dec 18, 2021

compiler-errors left a comment

compiler-errors Dec 21, 2021

compiler-errors Dec 21, 2021

oli-obk commented Dec 21, 2021

bors commented Dec 21, 2021

In-line commented Dec 22, 2021

oli-obk commented Dec 22, 2021

Improve suggestions for importing out-of-scope traits reexported as _ #91412

Improve suggestions for importing out-of-scope traits reexported as _ #91412

Conversation

compiler-errors commented Dec 1, 2021 • edited Loading

rust-highfive commented Dec 1, 2021

jplatte commented Dec 1, 2021

compiler-errors commented Dec 1, 2021

bors commented Dec 2, 2021

bors commented Dec 3, 2021

compiler-errors commented Dec 14, 2021

jackh726 left a comment

Choose a reason for hiding this comment

jackh726 Dec 18, 2021

Choose a reason for hiding this comment

oli-obk Dec 18, 2021

Choose a reason for hiding this comment

compiler-errors Dec 21, 2021

Choose a reason for hiding this comment

jackh726 Dec 18, 2021

Choose a reason for hiding this comment

compiler-errors Dec 21, 2021

Choose a reason for hiding this comment

jackh726 commented Dec 18, 2021

oli-obk commented Dec 18, 2021

compiler-errors left a comment

Choose a reason for hiding this comment

compiler-errors Dec 21, 2021

Choose a reason for hiding this comment

compiler-errors Dec 21, 2021

Choose a reason for hiding this comment

oli-obk commented Dec 21, 2021

bors commented Dec 21, 2021

In-line commented Dec 22, 2021

oli-obk commented Dec 22, 2021

Improve suggestions for importing out-of-scope traits reexported as `_` #91412

Improve suggestions for importing out-of-scope traits reexported as `_` #91412

compiler-errors commented Dec 1, 2021 •

edited

Loading