Skip to content

Commit 95561b3

Browse files
borsbors
committed
Auto merge of rust-lang#94584 - pnkfelix:inject-use-suggestion-sites, r=ekuber
More robust fallback for `use` suggestion Our old way to suggest where to add `use`s would first look for pre-existing `use`s in the relevant crate/module, and if there are *no* uses, it would fallback on trying to use another item as the basis for the suggestion. But this was fragile, as illustrated in issue rust-lang#87613 This PR instead identifies span of the first token after any inner attributes, and uses *that* as the fallback for the `use` suggestion. Fix rust-lang#87613
2 parents 9842048 + 8f4c6b0 commit 95561b3

File tree

27 files changed

+291
-102
lines changed

27 files changed

+291
-102
lines changed

Diff for: ‎compiler/rustc_ast/src/ast.rs

+16-2
Original file line numberDiff line numberDiff line change
@@ -510,7 +510,7 @@ pub struct WhereEqPredicate {
510510
pub struct Crate {
511511
pub attrs: Vec<Attribute>,
512512
pub items: Vec<P<Item>>,
513-
pub span: Span,
513+
pub spans: ModSpans,
514514
/// Must be equal to `CRATE_NODE_ID` after the crate root is expanded, but may hold
515515
/// expansion placeholders or an unassigned value (`DUMMY_NODE_ID`) before that.
516516
pub id: NodeId,
@@ -2317,11 +2317,25 @@ pub enum ModKind {
23172317
/// or with definition outlined to a separate file `mod foo;` and already loaded from it.
23182318
/// The inner span is from the first token past `{` to the last token until `}`,
23192319
/// or from the first to the last token in the loaded file.
2320-
Loaded(Vec<P<Item>>, Inline, Span),
2320+
Loaded(Vec<P<Item>>, Inline, ModSpans),
23212321
/// Module with definition outlined to a separate file `mod foo;` but not yet loaded from it.
23222322
Unloaded,
23232323
}
23242324

2325+
#[derive(Copy, Clone, Encodable, Decodable, Debug)]
2326+
pub struct ModSpans {
2327+
/// `inner_span` covers the body of the module; for a file module, its the whole file.
2328+
/// For an inline module, its the span inside the `{ ... }`, not including the curly braces.
2329+
pub inner_span: Span,
2330+
pub inject_use_span: Span,
2331+
}
2332+
2333+
impl Default for ModSpans {
2334+
fn default() -> ModSpans {
2335+
ModSpans { inner_span: Default::default(), inject_use_span: Default::default() }
2336+
}
2337+
}
2338+
23252339
/// Foreign module declaration.
23262340
///
23272341
/// E.g., `extern { .. }` or `extern "C" { .. }`.

Diff for: ‎compiler/rustc_ast/src/mut_visit.rs

+7-4
Original file line numberDiff line numberDiff line change
@@ -1009,8 +1009,9 @@ pub fn noop_visit_item_kind<T: MutVisitor>(kind: &mut ItemKind, vis: &mut T) {
10091009
ItemKind::Mod(unsafety, mod_kind) => {
10101010
visit_unsafety(unsafety, vis);
10111011
match mod_kind {
1012-
ModKind::Loaded(items, _inline, inner_span) => {
1012+
ModKind::Loaded(items, _inline, ModSpans { inner_span, inject_use_span }) => {
10131013
vis.visit_span(inner_span);
1014+
vis.visit_span(inject_use_span);
10141015
items.flat_map_in_place(|item| vis.flat_map_item(item));
10151016
}
10161017
ModKind::Unloaded => {}
@@ -1121,11 +1122,13 @@ pub fn noop_visit_fn_header<T: MutVisitor>(header: &mut FnHeader, vis: &mut T) {
11211122
}
11221123

11231124
pub fn noop_visit_crate<T: MutVisitor>(krate: &mut Crate, vis: &mut T) {
1124-
let Crate { attrs, items, span, id, is_placeholder: _ } = krate;
1125+
let Crate { attrs, items, spans, id, is_placeholder: _ } = krate;
11251126
vis.visit_id(id);
11261127
visit_attrs(attrs, vis);
11271128
items.flat_map_in_place(|item| vis.flat_map_item(item));
1128-
vis.visit_span(span);
1129+
let ModSpans { inner_span, inject_use_span } = spans;
1130+
vis.visit_span(inner_span);
1131+
vis.visit_span(inject_use_span);
11291132
}
11301133

11311134
// Mutates one item into possibly many items.
@@ -1558,7 +1561,7 @@ impl DummyAstNode for Crate {
15581561
Crate {
15591562
attrs: Default::default(),
15601563
items: Default::default(),
1561-
span: Default::default(),
1564+
spans: Default::default(),
15621565
id: DUMMY_NODE_ID,
15631566
is_placeholder: Default::default(),
15641567
}

Diff for: ‎compiler/rustc_ast_lowering/src/item.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -291,7 +291,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
291291
})
292292
}
293293
ItemKind::Mod(_, ref mod_kind) => match mod_kind {
294-
ModKind::Loaded(items, _, inner_span) => {
294+
ModKind::Loaded(items, _, ModSpans { inner_span, inject_use_span: _ }) => {
295295
hir::ItemKind::Mod(self.lower_mod(items, *inner_span))
296296
}
297297
ModKind::Unloaded => panic!("`mod` items should have been loaded by now"),

Diff for: ‎compiler/rustc_ast_lowering/src/lib.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -452,7 +452,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
452452
visit::walk_crate(&mut item::ItemLowerer { lctx: &mut self }, c);
453453

454454
self.with_hir_id_owner(CRATE_NODE_ID, |lctx| {
455-
let module = lctx.lower_mod(&c.items, c.span);
455+
let module = lctx.lower_mod(&c.items, c.spans.inner_span);
456456
lctx.lower_attrs(hir::CRATE_HIR_ID, &c.attrs);
457457
hir::OwnerNode::Crate(lctx.arena.alloc(module))
458458
});

Diff for: ‎compiler/rustc_builtin_macros/src/test_harness.rs

+3-2
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ impl<'a> MutVisitor for TestHarnessGenerator<'a> {
112112
fn visit_crate(&mut self, c: &mut ast::Crate) {
113113
let prev_tests = mem::take(&mut self.tests);
114114
noop_visit_crate(c, self);
115-
self.add_test_cases(ast::CRATE_NODE_ID, c.span, prev_tests);
115+
self.add_test_cases(ast::CRATE_NODE_ID, c.spans.inner_span, prev_tests);
116116

117117
// Create a main function to run our tests
118118
c.items.push(mk_main(&mut self.cx));
@@ -129,7 +129,8 @@ impl<'a> MutVisitor for TestHarnessGenerator<'a> {
129129

130130
// We don't want to recurse into anything other than mods, since
131131
// mods or tests inside of functions will break things
132-
if let ast::ItemKind::Mod(_, ModKind::Loaded(.., span)) = item.kind {
132+
if let ast::ItemKind::Mod(_, ModKind::Loaded(.., ref spans)) = item.kind {
133+
let ast::ModSpans { inner_span: span, inject_use_span: _ } = *spans;
133134
let prev_tests = mem::take(&mut self.tests);
134135
noop_visit_item_kind(&mut item.kind, self);
135136
self.add_test_cases(item.id, span, prev_tests);

Diff for: ‎compiler/rustc_expand/src/base.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ impl Annotatable {
6767
Annotatable::Param(ref p) => p.span,
6868
Annotatable::FieldDef(ref sf) => sf.span,
6969
Annotatable::Variant(ref v) => v.span,
70-
Annotatable::Crate(ref c) => c.span,
70+
Annotatable::Crate(ref c) => c.spans.inner_span,
7171
}
7272
}
7373

Diff for: ‎compiler/rustc_expand/src/expand.rs

+5-5
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,8 @@ use rustc_ast::token;
1212
use rustc_ast::tokenstream::TokenStream;
1313
use rustc_ast::visit::{self, AssocCtxt, Visitor};
1414
use rustc_ast::{AssocItemKind, AstLike, AstLikeWrapper, AttrStyle, ExprKind, ForeignItemKind};
15-
use rustc_ast::{Inline, ItemKind, MacArgs, MacStmtStyle, MetaItemKind, ModKind, NestedMetaItem};
16-
use rustc_ast::{NodeId, PatKind, StmtKind, TyKind};
15+
use rustc_ast::{Inline, ItemKind, MacArgs, MacStmtStyle, MetaItemKind, ModKind};
16+
use rustc_ast::{NestedMetaItem, NodeId, PatKind, StmtKind, TyKind};
1717
use rustc_ast_pretty::pprust;
1818
use rustc_data_structures::map_in_place::MapInPlace;
1919
use rustc_data_structures::sync::Lrc;
@@ -364,7 +364,7 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
364364
}
365365

366366
pub fn expand_crate(&mut self, krate: ast::Crate) -> ast::Crate {
367-
let file_path = match self.cx.source_map().span_to_filename(krate.span) {
367+
let file_path = match self.cx.source_map().span_to_filename(krate.spans.inner_span) {
368368
FileName::Real(name) => name
369369
.into_local_path()
370370
.expect("attempting to resolve a file path in an external file"),
@@ -1091,7 +1091,7 @@ impl InvocationCollectorNode for P<ast::Item> {
10911091
ModKind::Unloaded => {
10921092
// We have an outline `mod foo;` so we need to parse the file.
10931093
let old_attrs_len = attrs.len();
1094-
let ParsedExternalMod { items, inner_span, file_path, dir_path, dir_ownership } =
1094+
let ParsedExternalMod { items, spans, file_path, dir_path, dir_ownership } =
10951095
parse_external_mod(
10961096
&ecx.sess,
10971097
ident,
@@ -1112,7 +1112,7 @@ impl InvocationCollectorNode for P<ast::Item> {
11121112
);
11131113
}
11141114

1115-
*mod_kind = ModKind::Loaded(items, Inline::No, inner_span);
1115+
*mod_kind = ModKind::Loaded(items, Inline::No, spans);
11161116
node.attrs = attrs;
11171117
if node.attrs.len() > old_attrs_len {
11181118
// If we loaded an out-of-line module and added some inner attributes,

Diff for: ‎compiler/rustc_expand/src/module.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use crate::base::ModuleData;
22
use rustc_ast::ptr::P;
3-
use rustc_ast::{token, Attribute, Inline, Item};
3+
use rustc_ast::{token, Attribute, Inline, Item, ModSpans};
44
use rustc_errors::{struct_span_err, DiagnosticBuilder, ErrorGuaranteed};
55
use rustc_parse::new_parser_from_file;
66
use rustc_parse::validate_attr;
@@ -28,7 +28,7 @@ pub struct ModulePathSuccess {
2828

2929
crate struct ParsedExternalMod {
3030
pub items: Vec<P<Item>>,
31-
pub inner_span: Span,
31+
pub spans: ModSpans,
3232
pub file_path: PathBuf,
3333
pub dir_path: PathBuf,
3434
pub dir_ownership: DirOwnership,
@@ -69,13 +69,13 @@ crate fn parse_external_mod(
6969
(items, inner_span, mp.file_path)
7070
};
7171
// (1) ...instead, we return a dummy module.
72-
let (items, inner_span, file_path) =
72+
let (items, spans, file_path) =
7373
result.map_err(|err| err.report(sess, span)).unwrap_or_default();
7474

7575
// Extract the directory path for submodules of the module.
7676
let dir_path = file_path.parent().unwrap_or(&file_path).to_owned();
7777

78-
ParsedExternalMod { items, inner_span, file_path, dir_path, dir_ownership }
78+
ParsedExternalMod { items, spans, file_path, dir_path, dir_ownership }
7979
}
8080

8181
crate fn mod_dir_path(

Diff for: ‎compiler/rustc_expand/src/placeholders.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ pub fn placeholder(
4949
AstFragmentKind::Crate => AstFragment::Crate(ast::Crate {
5050
attrs: Default::default(),
5151
items: Default::default(),
52-
span,
52+
spans: ast::ModSpans { inner_span: span, ..Default::default() },
5353
id,
5454
is_placeholder: true,
5555
}),

Diff for: ‎compiler/rustc_metadata/src/creader.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -899,7 +899,7 @@ impl<'a> CrateLoader<'a> {
899899

900900
fn report_unused_deps(&mut self, krate: &ast::Crate) {
901901
// Make a point span rather than covering the whole file
902-
let span = krate.span.shrink_to_lo();
902+
let span = krate.spans.inner_span.shrink_to_lo();
903903
// Complain about anything left over
904904
for (name, entry) in self.sess.opts.externs.iter() {
905905
if let ExternLocation::FoundInLibrarySearchDirectories = entry.location {

Diff for: ‎compiler/rustc_parse/src/lib.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -331,7 +331,7 @@ pub fn fake_token_stream(sess: &ParseSess, nt: &Nonterminal) -> TokenStream {
331331
pub fn fake_token_stream_for_crate(sess: &ParseSess, krate: &ast::Crate) -> TokenStream {
332332
let source = pprust::crate_to_string_for_macros(krate);
333333
let filename = FileName::macro_expansion_source_code(&source);
334-
parse_stream_from_source_str(filename, source, sess, Some(krate.span))
334+
parse_stream_from_source_str(filename, source, sess, Some(krate.spans.inner_span))
335335
}
336336

337337
pub fn parse_cfg_attr(

Diff for: ‎compiler/rustc_parse/src/parser/item.rs

+7-4
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,8 @@ use tracing::debug;
2727
impl<'a> Parser<'a> {
2828
/// Parses a source module as a crate. This is the main entry point for the parser.
2929
pub fn parse_crate_mod(&mut self) -> PResult<'a, ast::Crate> {
30-
let (attrs, items, span) = self.parse_mod(&token::Eof)?;
31-
Ok(ast::Crate { attrs, items, span, id: DUMMY_NODE_ID, is_placeholder: false })
30+
let (attrs, items, spans) = self.parse_mod(&token::Eof)?;
31+
Ok(ast::Crate { attrs, items, spans, id: DUMMY_NODE_ID, is_placeholder: false })
3232
}
3333

3434
/// Parses a `mod <foo> { ... }` or `mod <foo>;` item.
@@ -52,10 +52,11 @@ impl<'a> Parser<'a> {
5252
pub fn parse_mod(
5353
&mut self,
5454
term: &TokenKind,
55-
) -> PResult<'a, (Vec<Attribute>, Vec<P<Item>>, Span)> {
55+
) -> PResult<'a, (Vec<Attribute>, Vec<P<Item>>, ModSpans)> {
5656
let lo = self.token.span;
5757
let attrs = self.parse_inner_attributes()?;
5858

59+
let post_attr_lo = self.token.span;
5960
let mut items = vec![];
6061
while let Some(item) = self.parse_item(ForceCollect::No)? {
6162
items.push(item);
@@ -72,7 +73,9 @@ impl<'a> Parser<'a> {
7273
}
7374
}
7475

75-
Ok((attrs, items, lo.to(self.prev_token.span)))
76+
let inject_use_span = post_attr_lo.data().with_hi(post_attr_lo.lo());
77+
let mod_spans = ModSpans { inner_span: lo.to(self.prev_token.span), inject_use_span };
78+
Ok((attrs, items, mod_spans))
7679
}
7780
}
7881

Diff for: ‎compiler/rustc_resolve/src/lib.rs

+50-55
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,6 @@ use rustc_span::{Span, DUMMY_SP};
7171
use smallvec::{smallvec, SmallVec};
7272
use std::cell::{Cell, RefCell};
7373
use std::collections::BTreeSet;
74-
use std::ops::ControlFlow;
7574
use std::{cmp, fmt, iter, mem, ptr};
7675
use tracing::debug;
7776

@@ -315,74 +314,70 @@ impl<'a> From<&'a ast::PathSegment> for Segment {
315314
}
316315
}
317316

317+
#[derive(Debug)]
318318
struct UsePlacementFinder {
319319
target_module: NodeId,
320-
span: Option<Span>,
321-
found_use: bool,
320+
first_legal_span: Option<Span>,
321+
first_use_span: Option<Span>,
322322
}
323323

324324
impl UsePlacementFinder {
325325
fn check(krate: &Crate, target_module: NodeId) -> (Option<Span>, bool) {
326-
let mut finder = UsePlacementFinder { target_module, span: None, found_use: false };
327-
if let ControlFlow::Continue(..) = finder.check_mod(&krate.items, CRATE_NODE_ID) {
328-
visit::walk_crate(&mut finder, krate);
329-
}
330-
(finder.span, finder.found_use)
331-
}
332-
333-
fn check_mod(&mut self, items: &[P<ast::Item>], node_id: NodeId) -> ControlFlow<()> {
334-
if self.span.is_some() {
335-
return ControlFlow::Break(());
336-
}
337-
if node_id != self.target_module {
338-
return ControlFlow::Continue(());
339-
}
340-
// find a use statement
341-
for item in items {
342-
match item.kind {
343-
ItemKind::Use(..) => {
344-
// don't suggest placing a use before the prelude
345-
// import or other generated ones
346-
if !item.span.from_expansion() {
347-
self.span = Some(item.span.shrink_to_lo());
348-
self.found_use = true;
349-
return ControlFlow::Break(());
350-
}
351-
}
352-
// don't place use before extern crate
353-
ItemKind::ExternCrate(_) => {}
354-
// but place them before the first other item
355-
_ => {
356-
if self.span.map_or(true, |span| item.span < span)
357-
&& !item.span.from_expansion()
358-
{
359-
self.span = Some(item.span.shrink_to_lo());
360-
// don't insert between attributes and an item
361-
// find the first attribute on the item
362-
// FIXME: This is broken for active attributes.
363-
for attr in &item.attrs {
364-
if !attr.span.is_dummy()
365-
&& self.span.map_or(true, |span| attr.span < span)
366-
{
367-
self.span = Some(attr.span.shrink_to_lo());
368-
}
369-
}
370-
}
371-
}
326+
let mut finder =
327+
UsePlacementFinder { target_module, first_legal_span: None, first_use_span: None };
328+
finder.visit_crate(krate);
329+
if let Some(use_span) = finder.first_use_span {
330+
(Some(use_span), true)
331+
} else {
332+
(finder.first_legal_span, false)
333+
}
334+
}
335+
}
336+
337+
fn is_span_suitable_for_use_injection(s: Span) -> bool {
338+
// don't suggest placing a use before the prelude
339+
// import or other generated ones
340+
!s.from_expansion()
341+
}
342+
343+
fn search_for_any_use_in_items(items: &[P<ast::Item>]) -> Option<Span> {
344+
for item in items {
345+
if let ItemKind::Use(..) = item.kind {
346+
if is_span_suitable_for_use_injection(item.span) {
347+
return Some(item.span.shrink_to_lo());
372348
}
373349
}
374-
ControlFlow::Continue(())
375350
}
351+
return None;
376352
}
377353

378354
impl<'tcx> Visitor<'tcx> for UsePlacementFinder {
355+
fn visit_crate(&mut self, c: &Crate) {
356+
if self.target_module == CRATE_NODE_ID {
357+
let inject = c.spans.inject_use_span;
358+
if is_span_suitable_for_use_injection(inject) {
359+
self.first_legal_span = Some(inject);
360+
}
361+
self.first_use_span = search_for_any_use_in_items(&c.items);
362+
return;
363+
} else {
364+
visit::walk_crate(self, c);
365+
}
366+
}
367+
379368
fn visit_item(&mut self, item: &'tcx ast::Item) {
380-
if let ItemKind::Mod(_, ModKind::Loaded(items, ..)) = &item.kind {
381-
if let ControlFlow::Break(..) = self.check_mod(items, item.id) {
369+
if self.target_module == item.id {
370+
if let ItemKind::Mod(_, ModKind::Loaded(items, _inline, mod_spans)) = &item.kind {
371+
let inject = mod_spans.inject_use_span;
372+
if is_span_suitable_for_use_injection(inject) {
373+
self.first_legal_span = Some(inject);
374+
}
375+
self.first_use_span = search_for_any_use_in_items(items);
382376
return;
383377
}
378+
} else {
379+
visit::walk_item(self, item);
384380
}
385-
visit::walk_item(self, item);
386381
}
387382
}
388383

@@ -1282,7 +1277,7 @@ impl<'a> Resolver<'a> {
12821277
None,
12831278
ModuleKind::Def(DefKind::Mod, root_def_id, kw::Empty),
12841279
ExpnId::root(),
1285-
krate.span,
1280+
krate.spans.inner_span,
12861281
session.contains_name(&krate.attrs, sym::no_implicit_prelude),
12871282
&mut module_map,
12881283
);
@@ -1295,7 +1290,7 @@ impl<'a> Resolver<'a> {
12951290
&mut FxHashMap::default(),
12961291
);
12971292

1298-
let definitions = Definitions::new(session.local_stable_crate_id(), krate.span);
1293+
let definitions = Definitions::new(session.local_stable_crate_id(), krate.spans.inner_span);
12991294
let root = definitions.get_root_def();
13001295

13011296
let mut visibilities = FxHashMap::default();

0 commit comments

Comments
 (0)