Rollup merge of #95194 - kckeiks:update-algo-in-find-use-placement, r=pnkfelix

remove find_use_placement

A more robust solution to finding where to place use suggestions was added in #94584.
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 instead of calling `find_use_placement`

Fixes #94941

Author: Dylan-DPC

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),

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"),
             },

compiler/rustc_hir/src/hir.rs

@@ -2557,11 +2557,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)]
@@ -3059,8 +3065,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,
         }
     }
 

compiler/rustc_middle/src/hir/map/mod.rs

@@ -584,7 +584,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),
         }
     }
@@ -1012,7 +1012,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)
     }

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(

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);
 

compiler/rustc_typeck/src/check/method/suggest.rs

@@ -8,7 +8,7 @@ use rustc_errors::{
     MultiSpan,
 };
 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(
@@ -2106,53 +2049,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>]>,

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);

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;

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]