remove find_use_placement

A more robust solution to finding where to place use suggestions was added. The algorithm uses the AST to find the span for the suggestion so we pass this span down to the HIR during lowering and use it. Signed-off-by: Miguel Guarniz <[email protected]>
RalfJung · Mar 31, 2022 · 8c2353b · 8c2353b
1 parent 0677edc
commit 8c2353b
Show file tree

Hide file tree

Showing 12 changed files with 57 additions and 148 deletions.
diff --git a/compiler/rustc_ast_lowering/src/index.rs b/compiler/rustc_ast_lowering/src/index.rs
@@ -52,7 +52,9 @@ pub(super) fn index_hir<'hir>(
     };
 
     match item {
-        OwnerNode::Crate(citem) => collector.visit_mod(&citem, citem.inner, hir::CRATE_HIR_ID),
+        OwnerNode::Crate(citem) => {
+            collector.visit_mod(&citem, citem.spans.inner_span, hir::CRATE_HIR_ID)
+        }
         OwnerNode::Item(item) => collector.visit_item(item),
         OwnerNode::TraitItem(item) => collector.visit_trait_item(item),
         OwnerNode::ImplItem(item) => collector.visit_impl_item(item),

diff --git a/compiler/rustc_ast_lowering/src/item.rs b/compiler/rustc_ast_lowering/src/item.rs
@@ -124,7 +124,7 @@ impl<'a, 'hir> ItemLowerer<'a, 'hir> {
         debug_assert_eq!(self.resolver.local_def_id(CRATE_NODE_ID), CRATE_DEF_ID);
 
         self.with_lctx(CRATE_NODE_ID, |lctx| {
-            let module = lctx.lower_mod(&c.items, c.spans.inner_span);
+            let module = lctx.lower_mod(&c.items, &c.spans);
             lctx.lower_attrs(hir::CRATE_HIR_ID, &c.attrs);
             hir::OwnerNode::Crate(lctx.arena.alloc(module))
         })
@@ -186,9 +186,12 @@ impl<'a, 'hir> ItemLowerer<'a, 'hir> {
 }
 
 impl<'hir> LoweringContext<'_, 'hir> {
-    pub(super) fn lower_mod(&mut self, items: &[P<Item>], inner: Span) -> hir::Mod<'hir> {
+    pub(super) fn lower_mod(&mut self, items: &[P<Item>], spans: &ModSpans) -> hir::Mod<'hir> {
         hir::Mod {
-            inner: self.lower_span(inner),
+            spans: hir::ModSpans {
+                inner_span: self.lower_span(spans.inner_span),
+                inject_use_span: self.lower_span(spans.inject_use_span),
+            },
             item_ids: self.arena.alloc_from_iter(items.iter().flat_map(|x| self.lower_item_ref(x))),
         }
     }
@@ -308,8 +311,8 @@ impl<'hir> LoweringContext<'_, 'hir> {
                 })
             }
             ItemKind::Mod(_, ref mod_kind) => match mod_kind {
-                ModKind::Loaded(items, _, ModSpans { inner_span, inject_use_span: _ }) => {
-                    hir::ItemKind::Mod(self.lower_mod(items, *inner_span))
+                ModKind::Loaded(items, _, spans) => {
+                    hir::ItemKind::Mod(self.lower_mod(items, spans))
                 }
                 ModKind::Unloaded => panic!("`mod` items should have been loaded by now"),
             },

diff --git a/compiler/rustc_hir/src/hir.rs b/compiler/rustc_hir/src/hir.rs
@@ -2522,11 +2522,17 @@ impl FnRetTy<'_> {
 
 #[derive(Encodable, Debug, HashStable_Generic)]
 pub struct Mod<'hir> {
+    pub spans: ModSpans,
+    pub item_ids: &'hir [ItemId],
+}
+
+#[derive(Copy, Clone, Debug, HashStable_Generic, Encodable)]
+pub struct ModSpans {
     /// A span from the first token past `{` to the last token until `}`.
     /// For `mod foo;`, the inner span ranges from the first token
     /// to the last token in the external file.
-    pub inner: Span,
-    pub item_ids: &'hir [ItemId],
+    pub inner_span: Span,
+    pub inject_use_span: Span,
 }
 
 #[derive(Debug, HashStable_Generic)]
@@ -3024,8 +3030,8 @@ impl<'hir> OwnerNode<'hir> {
             OwnerNode::Item(Item { span, .. })
             | OwnerNode::ForeignItem(ForeignItem { span, .. })
             | OwnerNode::ImplItem(ImplItem { span, .. })
-            | OwnerNode::TraitItem(TraitItem { span, .. })
-            | OwnerNode::Crate(Mod { inner: span, .. }) => *span,
+            | OwnerNode::TraitItem(TraitItem { span, .. }) => *span,
+            OwnerNode::Crate(Mod { spans: ModSpans { inner_span, .. }, .. }) => *inner_span,
         }
     }
 

diff --git a/compiler/rustc_middle/src/hir/map/mod.rs b/compiler/rustc_middle/src/hir/map/mod.rs
@@ -586,7 +586,7 @@ impl<'hir> Map<'hir> {
             Some(OwnerNode::Item(&Item { span, kind: ItemKind::Mod(ref m), .. })) => {
                 (m, span, hir_id)
             }
-            Some(OwnerNode::Crate(item)) => (item, item.inner, hir_id),
+            Some(OwnerNode::Crate(item)) => (item, item.spans.inner_span, hir_id),
             node => panic!("not a module: {:?}", node),
         }
     }
@@ -1014,7 +1014,7 @@ impl<'hir> Map<'hir> {
             Node::Infer(i) => i.span,
             Node::Visibility(v) => bug!("unexpected Visibility {:?}", v),
             Node::Local(local) => local.span,
-            Node::Crate(item) => item.inner,
+            Node::Crate(item) => item.spans.inner_span,
         };
         Some(span)
     }

diff --git a/compiler/rustc_save_analysis/src/dump_visitor.rs b/compiler/rustc_save_analysis/src/dump_visitor.rs
@@ -1095,11 +1095,11 @@ impl<'tcx> DumpVisitor<'tcx> {
 
         let sm = self.tcx.sess.source_map();
         let krate_mod = self.tcx.hir().root_module();
-        let filename = sm.span_to_filename(krate_mod.inner);
+        let filename = sm.span_to_filename(krate_mod.spans.inner_span);
         let data_id = id_from_hir_id(id, &self.save_ctxt);
         let children =
             krate_mod.item_ids.iter().map(|i| id_from_def_id(i.def_id.to_def_id())).collect();
-        let span = self.span_from_span(krate_mod.inner);
+        let span = self.span_from_span(krate_mod.spans.inner_span);
         let attrs = self.tcx.hir().attrs(id);
 
         self.dumper.dump_def(

diff --git a/compiler/rustc_save_analysis/src/lib.rs b/compiler/rustc_save_analysis/src/lib.rs
@@ -282,7 +282,7 @@ impl<'tcx> SaveContext<'tcx> {
                 let qualname = format!("::{}", self.tcx.def_path_str(def_id));
 
                 let sm = self.tcx.sess.source_map();
-                let filename = sm.span_to_filename(m.inner);
+                let filename = sm.span_to_filename(m.spans.inner_span);
 
                 filter!(self.span_utils, item.ident.span);
 

diff --git a/compiler/rustc_typeck/src/check/method/suggest.rs b/compiler/rustc_typeck/src/check/method/suggest.rs
@@ -7,7 +7,7 @@ use rustc_errors::{
     pluralize, struct_span_err, Applicability, Diagnostic, DiagnosticBuilder, ErrorGuaranteed,
 };
 use rustc_hir as hir;
-use rustc_hir::def_id::{DefId, LocalDefId};
+use rustc_hir::def_id::DefId;
 use rustc_hir::lang_items::LangItem;
 use rustc_hir::{ExprKind, Node, QPath};
 use rustc_infer::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind};
@@ -1473,12 +1473,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         }
     }
 
-    fn suggest_use_candidates(
-        &self,
-        err: &mut Diagnostic,
-        mut msg: String,
-        candidates: Vec<DefId>,
-    ) {
+    fn suggest_use_candidates(&self, err: &mut Diagnostic, msg: String, candidates: Vec<DefId>) {
         let parent_map = self.tcx.visible_parent_map(());
 
         // Separate out candidates that must be imported with a glob, because they are named `_`
@@ -1502,80 +1497,28 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         });
 
         let module_did = self.tcx.parent_module(self.body_id);
-        let (span, found_use) = find_use_placement(self.tcx, module_did);
-        if let Some(span) = span {
-            let path_strings = candidates.iter().map(|trait_did| {
-                // Produce an additional newline to separate the new use statement
-                // from the directly following item.
-                let additional_newline = if found_use { "" } else { "\n" };
-                format!(
-                    "use {};\n{}",
-                    with_crate_prefix!(self.tcx.def_path_str(*trait_did)),
-                    additional_newline
-                )
-            });
+        let (module, _, _) = self.tcx.hir().get_module(module_did);
+        let span = module.spans.inject_use_span;
 
-            let glob_path_strings = globs.iter().map(|trait_did| {
-                let parent_did = parent_map.get(trait_did).unwrap();
+        let path_strings = candidates.iter().map(|trait_did| {
+            format!("use {};\n", with_crate_prefix!(self.tcx.def_path_str(*trait_did)),)
+        });
 
-                // Produce an additional newline to separate the new use statement
-                // from the directly following item.
-                let additional_newline = if found_use { "" } else { "\n" };
-                format!(
-                    "use {}::*; // trait {}\n{}",
-                    with_crate_prefix!(self.tcx.def_path_str(*parent_did)),
-                    self.tcx.item_name(*trait_did),
-                    additional_newline
-                )
-            });
+        let glob_path_strings = globs.iter().map(|trait_did| {
+            let parent_did = parent_map.get(trait_did).unwrap();
+            format!(
+                "use {}::*; // trait {}\n",
+                with_crate_prefix!(self.tcx.def_path_str(*parent_did)),
+                self.tcx.item_name(*trait_did),
+            )
+        });
 
-            err.span_suggestions(
-                span,
-                &msg,
-                path_strings.chain(glob_path_strings),
-                Applicability::MaybeIncorrect,
-            );
-        } else {
-            let limit = if candidates.len() + globs.len() == 5 { 5 } else { 4 };
-            for (i, trait_did) in candidates.iter().take(limit).enumerate() {
-                if candidates.len() + globs.len() > 1 {
-                    msg.push_str(&format!(
-                        "\ncandidate #{}: `use {};`",
-                        i + 1,
-                        with_crate_prefix!(self.tcx.def_path_str(*trait_did))
-                    ));
-                } else {
-                    msg.push_str(&format!(
-                        "\n`use {};`",
-                        with_crate_prefix!(self.tcx.def_path_str(*trait_did))
-                    ));
-                }
-            }
-            for (i, trait_did) in
-                globs.iter().take(limit.saturating_sub(candidates.len())).enumerate()
-            {
-                let parent_did = parent_map.get(trait_did).unwrap();
-
-                if candidates.len() + globs.len() > 1 {
-                    msg.push_str(&format!(
-                        "\ncandidate #{}: `use {}::*; // trait {}`",
-                        candidates.len() + i + 1,
-                        with_crate_prefix!(self.tcx.def_path_str(*parent_did)),
-                        self.tcx.item_name(*trait_did),
-                    ));
-                } else {
-                    msg.push_str(&format!(
-                        "\n`use {}::*; // trait {}`",
-                        with_crate_prefix!(self.tcx.def_path_str(*parent_did)),
-                        self.tcx.item_name(*trait_did),
-                    ));
-                }
-            }
-            if candidates.len() > limit {
-                msg.push_str(&format!("\nand {} others", candidates.len() + globs.len() - limit));
-            }
-            err.note(&msg);
-        }
+        err.span_suggestions(
+            span,
+            &msg,
+            path_strings.chain(glob_path_strings),
+            Applicability::MaybeIncorrect,
+        );
     }
 
     fn suggest_valid_traits(
@@ -2100,53 +2043,6 @@ pub fn all_traits(tcx: TyCtxt<'_>) -> Vec<TraitInfo> {
     tcx.all_traits().map(|def_id| TraitInfo { def_id }).collect()
 }
 
-fn find_use_placement<'tcx>(tcx: TyCtxt<'tcx>, target_module: LocalDefId) -> (Option<Span>, bool) {
-    // FIXME(#94854): this code uses an out-of-date method for inferring a span
-    // to suggest. It would be better to thread the ModSpans from the AST into
-    // the HIR, and then use that to drive the suggestion here.
-
-    let mut span = None;
-    let mut found_use = false;
-    let (module, _, _) = tcx.hir().get_module(target_module);
-
-    // Find a `use` statement.
-    for &item_id in module.item_ids {
-        let item = tcx.hir().item(item_id);
-        match item.kind {
-            hir::ItemKind::Use(..) => {
-                // Don't suggest placing a `use` before the prelude
-                // import or other generated ones.
-                if !item.span.from_expansion() {
-                    span = Some(item.span.shrink_to_lo());
-                    found_use = true;
-                    break;
-                }
-            }
-            // Don't place `use` before `extern crate`...
-            hir::ItemKind::ExternCrate(_) => {}
-            // ...but do place them before the first other item.
-            _ => {
-                if span.map_or(true, |span| item.span < span) {
-                    if !item.span.from_expansion() {
-                        span = Some(item.span.shrink_to_lo());
-                        // Don't insert between attributes and an item.
-                        let attrs = tcx.hir().attrs(item.hir_id());
-                        // Find the first attribute on the item.
-                        // FIXME: This is broken for active attributes.
-                        for attr in attrs {
-                            if !attr.span.is_dummy() && span.map_or(true, |span| attr.span < span) {
-                                span = Some(attr.span.shrink_to_lo());
-                            }
-                        }
-                    }
-                }
-            }
-        }
-    }
-
-    (span, found_use)
-}
-
 fn print_disambiguation_help<'tcx>(
     item_name: Ident,
     args: Option<&'tcx [hir::Expr<'tcx>]>,

diff --git a/src/librustdoc/html/render/span_map.rs b/src/librustdoc/html/render/span_map.rs
@@ -119,11 +119,14 @@ impl<'tcx> Visitor<'tcx> for SpanMapVisitor<'tcx> {
     fn visit_mod(&mut self, m: &'tcx Mod<'tcx>, span: Span, id: HirId) {
         // To make the difference between "mod foo {}" and "mod foo;". In case we "import" another
         // file, we want to link to it. Otherwise no need to create a link.
-        if !span.overlaps(m.inner) {
+        if !span.overlaps(m.spans.inner_span) {
             // Now that we confirmed it's a file import, we want to get the span for the module
             // name only and not all the "mod foo;".
             if let Some(Node::Item(item)) = self.tcx.hir().find(id) {
-                self.matches.insert(item.ident.span, LinkFromSrc::Local(clean::Span::new(m.inner)));
+                self.matches.insert(
+                    item.ident.span,
+                    LinkFromSrc::Local(clean::Span::new(m.spans.inner_span)),
+                );
             }
         }
         intravisit::walk_mod(self, m, id);

diff --git a/src/librustdoc/visit_ast.rs b/src/librustdoc/visit_ast.rs
@@ -154,7 +154,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
         m: &'tcx hir::Mod<'tcx>,
         name: Symbol,
     ) -> Module<'tcx> {
-        let mut om = Module::new(name, id, m.inner);
+        let mut om = Module::new(name, id, m.spans.inner_span);
         let def_id = self.cx.tcx.hir().local_def_id(id).to_def_id();
         // Keep track of if there were any private modules in the path.
         let orig_inside_public_path = self.inside_public_path;

diff --git a/src/test/ui/imports/overlapping_pub_trait.rs b/src/test/ui/imports/overlapping_pub_trait.rs
@@ -4,10 +4,10 @@
  * This crate declares two public paths, `m::Tr` and `prelude::_`. Make sure we prefer the former.
  */
 extern crate overlapping_pub_trait_source;
+//~^ HELP the following trait is implemented but not in scope; perhaps add a `use` for it:
+//~| SUGGESTION overlapping_pub_trait_source::m::Tr
 
 fn main() {
-    //~^ HELP the following trait is implemented but not in scope; perhaps add a `use` for it:
-    //~| SUGGESTION overlapping_pub_trait_source::m::Tr
     use overlapping_pub_trait_source::S;
     S.method();
     //~^ ERROR no method named `method` found for struct `S` in the current scope [E0599]

diff --git a/src/test/ui/imports/unnamed_pub_trait.rs b/src/test/ui/imports/unnamed_pub_trait.rs
@@ -5,10 +5,10 @@
  * importing it by name, and instead we suggest importing it by glob.
  */
 extern crate unnamed_pub_trait_source;
+//~^ HELP the following trait is implemented but not in scope; perhaps add a `use` for it:
+//~| SUGGESTION unnamed_pub_trait_source::prelude::*; // trait Tr
 
 fn main() {
-    //~^ HELP the following trait is implemented but not in scope; perhaps add a `use` for it:
-    //~| SUGGESTION unnamed_pub_trait_source::prelude::*; // trait Tr
     use unnamed_pub_trait_source::S;
     S.method();
     //~^ ERROR no method named `method` found for struct `S` in the current scope [E0599]

diff --git a/src/test/ui/suggestions/use-placement-typeck.fixed b/src/test/ui/suggestions/use-placement-typeck.fixed
@@ -7,7 +7,6 @@
 #![allow(unused)]
 
 use m::Foo;
-
 fn main() {
     let s = m::S;
     s.abc(); //~ ERROR no method named `abc`