Skip to content

Commit b200108

Browse files
matthiaskrgrmatthiaskrgr
authored
Rollup merge of #122373 - surechen:fix_121331, r=petrochenkov
Fix the conflict problem between the diagnostics fixes of lint `unnecessary_qualification` and `unused_imports` fixes #121331 For an `item` that triggers lint unnecessary_qualification, if the `use item` which imports this item is also trigger unused import, fixing the two lints at the same time may lead to the problem that the `item` cannot be found. This PR will avoid reporting lint unnecessary_qualification when conflict occurs. r? ``@petrochenkov``
2 parents 54a5a49 + 1a81a94 commit b200108

17 files changed

+269
-42
lines changed

Diff for: ‎compiler/rustc_lint_defs/src/builtin.rs

+1
Original file line numberDiff line numberDiff line change
@@ -557,6 +557,7 @@ declare_lint! {
557557
/// fn main() {
558558
/// use foo::bar;
559559
/// foo::bar();
560+
/// bar();
560561
/// }
561562
/// ```
562563
///

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

+56-4
Original file line numberDiff line numberDiff line change
@@ -23,18 +23,19 @@
2323
// - `check_unused` finally emits the diagnostics based on the data generated
2424
// in the last step
2525

26-
use crate::imports::ImportKind;
26+
use crate::imports::{Import, ImportKind};
2727
use crate::module_to_string;
2828
use crate::Resolver;
2929

30-
use crate::NameBindingKind;
30+
use crate::{LexicalScopeBinding, NameBindingKind};
3131
use rustc_ast as ast;
3232
use rustc_ast::visit::{self, Visitor};
3333
use rustc_data_structures::fx::{FxHashMap, FxIndexMap, FxIndexSet};
3434
use rustc_data_structures::unord::UnordSet;
3535
use rustc_errors::{pluralize, MultiSpan};
3636
use rustc_hir::def::{DefKind, Res};
37-
use rustc_session::lint::builtin::{MACRO_USE_EXTERN_CRATE, UNUSED_EXTERN_CRATES, UNUSED_IMPORTS};
37+
use rustc_session::lint::builtin::{MACRO_USE_EXTERN_CRATE, UNUSED_EXTERN_CRATES};
38+
use rustc_session::lint::builtin::{UNUSED_IMPORTS, UNUSED_QUALIFICATIONS};
3839
use rustc_session::lint::BuiltinLintDiag;
3940
use rustc_span::symbol::{kw, Ident};
4041
use rustc_span::{Span, DUMMY_SP};
@@ -514,8 +515,59 @@ impl Resolver<'_, '_> {
514515
}
515516
}
516517

518+
let mut redundant_imports = UnordSet::default();
517519
for import in check_redundant_imports {
518-
self.check_for_redundant_imports(import);
520+
if self.check_for_redundant_imports(import)
521+
&& let Some(id) = import.id()
522+
{
523+
redundant_imports.insert(id);
524+
}
525+
}
526+
527+
// The lint fixes for unused_import and unnecessary_qualification may conflict.
528+
// Deleting both unused imports and unnecessary segments of an item may result
529+
// in the item not being found.
530+
for unn_qua in &self.potentially_unnecessary_qualifications {
531+
if let LexicalScopeBinding::Item(name_binding) = unn_qua.binding
532+
&& let NameBindingKind::Import { import, .. } = name_binding.kind
533+
&& (is_unused_import(import, &unused_imports)
534+
|| is_redundant_import(import, &redundant_imports))
535+
{
536+
continue;
537+
}
538+
539+
self.lint_buffer.buffer_lint_with_diagnostic(
540+
UNUSED_QUALIFICATIONS,
541+
unn_qua.node_id,
542+
unn_qua.path_span,
543+
"unnecessary qualification",
544+
BuiltinLintDiag::UnusedQualifications { removal_span: unn_qua.removal_span },
545+
);
546+
}
547+
548+
fn is_redundant_import(
549+
import: Import<'_>,
550+
redundant_imports: &UnordSet<ast::NodeId>,
551+
) -> bool {
552+
if let Some(id) = import.id()
553+
&& redundant_imports.contains(&id)
554+
{
555+
return true;
556+
}
557+
false
558+
}
559+
560+
fn is_unused_import(
561+
import: Import<'_>,
562+
unused_imports: &FxIndexMap<ast::NodeId, UnusedImport>,
563+
) -> bool {
564+
if let Some(unused_import) = unused_imports.get(&import.root_id)
565+
&& let Some(id) = import.id()
566+
&& unused_import.unused.contains(&id)
567+
{
568+
return true;
569+
}
570+
false
519571
}
520572
}
521573
}

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

+7-4
Original file line numberDiff line numberDiff line change
@@ -1306,7 +1306,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
13061306
None
13071307
}
13081308

1309-
pub(crate) fn check_for_redundant_imports(&mut self, import: Import<'a>) {
1309+
pub(crate) fn check_for_redundant_imports(&mut self, import: Import<'a>) -> bool {
13101310
// This function is only called for single imports.
13111311
let ImportKind::Single {
13121312
source, target, ref source_bindings, ref target_bindings, id, ..
@@ -1317,12 +1317,12 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
13171317

13181318
// Skip if the import is of the form `use source as target` and source != target.
13191319
if source != target {
1320-
return;
1320+
return false;
13211321
}
13221322

13231323
// Skip if the import was produced by a macro.
13241324
if import.parent_scope.expansion != LocalExpnId::ROOT {
1325-
return;
1325+
return false;
13261326
}
13271327

13281328
// Skip if we are inside a named module (in contrast to an anonymous
@@ -1332,7 +1332,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
13321332
if import.used.get() == Some(Used::Other)
13331333
|| self.effective_visibilities.is_exported(self.local_def_id(id))
13341334
{
1335-
return;
1335+
return false;
13361336
}
13371337

13381338
let mut is_redundant = true;
@@ -1375,7 +1375,10 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
13751375
format!("the item `{source}` is imported redundantly"),
13761376
BuiltinLintDiag::RedundantImport(redundant_spans, source),
13771377
);
1378+
return true;
13781379
}
1380+
1381+
false
13791382
}
13801383

13811384
fn resolve_glob_import(&mut self, import: Import<'a>) {

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

+17-12
Original file line numberDiff line numberDiff line change
@@ -580,6 +580,15 @@ impl MaybeExported<'_> {
580580
}
581581
}
582582

583+
/// Used for recording UnnecessaryQualification.
584+
#[derive(Debug)]
585+
pub(crate) struct UnnecessaryQualification<'a> {
586+
pub binding: LexicalScopeBinding<'a>,
587+
pub node_id: NodeId,
588+
pub path_span: Span,
589+
pub removal_span: Span,
590+
}
591+
583592
#[derive(Default)]
584593
struct DiagMetadata<'ast> {
585594
/// The current trait's associated items' ident, used for diagnostic suggestions.
@@ -4654,20 +4663,16 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
46544663
let ns = if i + 1 == path.len() { ns } else { TypeNS };
46554664
let res = self.r.partial_res_map.get(&seg.id?)?.full_res()?;
46564665
let binding = self.resolve_ident_in_lexical_scope(seg.ident, ns, None, None)?;
4657-
4658-
(res == binding.res()).then_some(seg)
4666+
(res == binding.res()).then_some((seg, binding))
46594667
});
46604668

4661-
if let Some(unqualified) = unqualified {
4662-
self.r.lint_buffer.buffer_lint_with_diagnostic(
4663-
lint::builtin::UNUSED_QUALIFICATIONS,
4664-
finalize.node_id,
4665-
finalize.path_span,
4666-
"unnecessary qualification",
4667-
lint::BuiltinLintDiag::UnusedQualifications {
4668-
removal_span: path[0].ident.span.until(unqualified.ident.span),
4669-
},
4670-
);
4669+
if let Some((seg, binding)) = unqualified {
4670+
self.r.potentially_unnecessary_qualifications.push(UnnecessaryQualification {
4671+
binding,
4672+
node_id: finalize.node_id,
4673+
path_span: finalize.path_span,
4674+
removal_span: path[0].ident.span.until(seg.ident.span),
4675+
});
46714676
}
46724677
}
46734678
}

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

+5-2
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ use std::fmt;
6868

6969
use diagnostics::{ImportSuggestion, LabelSuggestion, Suggestion};
7070
use imports::{Import, ImportData, ImportKind, NameResolution};
71-
use late::{HasGenericParams, PathSource, PatternSource};
71+
use late::{HasGenericParams, PathSource, PatternSource, UnnecessaryQualification};
7272
use macros::{MacroRulesBinding, MacroRulesScope, MacroRulesScopeRef};
7373

7474
use crate::effective_visibilities::EffectiveVisibilitiesVisitor;
@@ -372,7 +372,7 @@ impl<'a> From<&'a ast::PathSegment> for Segment {
372372
/// This refers to the thing referred by a name. The difference between `Res` and `Item` is that
373373
/// items are visible in their whole block, while `Res`es only from the place they are defined
374374
/// forward.
375-
#[derive(Debug)]
375+
#[derive(Debug, Copy, Clone)]
376376
enum LexicalScopeBinding<'a> {
377377
Item(NameBinding<'a>),
378378
Res(Res),
@@ -1105,6 +1105,8 @@ pub struct Resolver<'a, 'tcx> {
11051105

11061106
potentially_unused_imports: Vec<Import<'a>>,
11071107

1108+
potentially_unnecessary_qualifications: Vec<UnnecessaryQualification<'a>>,
1109+
11081110
/// Table for mapping struct IDs into struct constructor IDs,
11091111
/// it's not used during normal resolution, only for better error reporting.
11101112
/// Also includes of list of each fields visibility
@@ -1464,6 +1466,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
14641466
local_macro_def_scopes: FxHashMap::default(),
14651467
name_already_seen: FxHashMap::default(),
14661468
potentially_unused_imports: Vec::new(),
1469+
potentially_unnecessary_qualifications: Default::default(),
14671470
struct_constructors: Default::default(),
14681471
unused_macros: Default::default(),
14691472
unused_macro_rules: Default::default(),

Diff for: ‎tests/ui/lint/lint-qualification.fixed

+5-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
//@ run-rustfix
22
#![deny(unused_qualifications)]
3+
#![deny(unused_imports)]
34
#![allow(deprecated, dead_code)]
45

56
mod foo {
@@ -21,8 +22,10 @@ fn main() {
2122
//~^ ERROR: unnecessary qualification
2223
//~| ERROR: unnecessary qualification
2324

24-
use std::fmt;
25-
let _: fmt::Result = Ok(()); //~ ERROR: unnecessary qualification
25+
26+
//~^ ERROR: unused import: `std::fmt`
27+
let _: std::fmt::Result = Ok(());
28+
// don't report unnecessary qualification because fix(#122373) for issue #121331
2629

2730
let _ = <bool as Default>::default(); // issue #121999
2831
//~^ ERROR: unnecessary qualification

Diff for: ‎tests/ui/lint/lint-qualification.rs

+4-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
//@ run-rustfix
22
#![deny(unused_qualifications)]
3+
#![deny(unused_imports)]
34
#![allow(deprecated, dead_code)]
45

56
mod foo {
@@ -22,7 +23,9 @@ fn main() {
2223
//~| ERROR: unnecessary qualification
2324

2425
use std::fmt;
25-
let _: std::fmt::Result = Ok(()); //~ ERROR: unnecessary qualification
26+
//~^ ERROR: unused import: `std::fmt`
27+
let _: std::fmt::Result = Ok(());
28+
// don't report unnecessary qualification because fix(#122373) for issue #121331
2629

2730
let _ = <bool as ::std::default::Default>::default(); // issue #121999
2831
//~^ ERROR: unnecessary qualification

Diff for: ‎tests/ui/lint/lint-qualification.stderr

+15-15
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: unnecessary qualification
2-
--> $DIR/lint-qualification.rs:11:5
2+
--> $DIR/lint-qualification.rs:12:5
33
|
44
LL | foo::bar();
55
| ^^^^^^^^
@@ -16,7 +16,7 @@ LL + bar();
1616
|
1717

1818
error: unnecessary qualification
19-
--> $DIR/lint-qualification.rs:12:5
19+
--> $DIR/lint-qualification.rs:13:5
2020
|
2121
LL | crate::foo::bar();
2222
| ^^^^^^^^^^^^^^^
@@ -28,7 +28,7 @@ LL + bar();
2828
|
2929

3030
error: unnecessary qualification
31-
--> $DIR/lint-qualification.rs:17:13
31+
--> $DIR/lint-qualification.rs:18:13
3232
|
3333
LL | let _ = std::string::String::new();
3434
| ^^^^^^^^^^^^^^^^^^^^^^^^
@@ -40,7 +40,7 @@ LL + let _ = String::new();
4040
|
4141

4242
error: unnecessary qualification
43-
--> $DIR/lint-qualification.rs:18:13
43+
--> $DIR/lint-qualification.rs:19:13
4444
|
4545
LL | let _ = ::std::env::current_dir();
4646
| ^^^^^^^^^^^^^^^^^^^^^^^
@@ -52,7 +52,7 @@ LL + let _ = std::env::current_dir();
5252
|
5353

5454
error: unnecessary qualification
55-
--> $DIR/lint-qualification.rs:20:12
55+
--> $DIR/lint-qualification.rs:21:12
5656
|
5757
LL | let _: std::vec::Vec<String> = std::vec::Vec::<String>::new();
5858
| ^^^^^^^^^^^^^^^^^^^^^
@@ -64,7 +64,7 @@ LL + let _: Vec<String> = std::vec::Vec::<String>::new();
6464
|
6565

6666
error: unnecessary qualification
67-
--> $DIR/lint-qualification.rs:20:36
67+
--> $DIR/lint-qualification.rs:21:36
6868
|
6969
LL | let _: std::vec::Vec<String> = std::vec::Vec::<String>::new();
7070
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -75,20 +75,20 @@ LL - let _: std::vec::Vec<String> = std::vec::Vec::<String>::new();
7575
LL + let _: std::vec::Vec<String> = Vec::<String>::new();
7676
|
7777

78-
error: unnecessary qualification
79-
--> $DIR/lint-qualification.rs:25:12
80-
|
81-
LL | let _: std::fmt::Result = Ok(());
82-
| ^^^^^^^^^^^^^^^^
78+
error: unused import: `std::fmt`
79+
--> $DIR/lint-qualification.rs:25:9
8380
|
84-
help: remove the unnecessary path segments
81+
LL | use std::fmt;
82+
| ^^^^^^^^
8583
|
86-
LL - let _: std::fmt::Result = Ok(());
87-
LL + let _: fmt::Result = Ok(());
84+
note: the lint level is defined here
85+
--> $DIR/lint-qualification.rs:3:9
8886
|
87+
LL | #![deny(unused_imports)]
88+
| ^^^^^^^^^^^^^^
8989

9090
error: unnecessary qualification
91-
--> $DIR/lint-qualification.rs:27:13
91+
--> $DIR/lint-qualification.rs:30:13
9292
|
9393
LL | let _ = <bool as ::std::default::Default>::default(); // issue #121999
9494
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Diff for: ‎tests/ui/lint/unnecessary-qualification/lint-unnecessary-qualification-issue-121331.fixed

+50
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
//@ run-rustfix
2+
//@ edition:2021
3+
#![deny(unused_qualifications)]
4+
#![deny(unused_imports)]
5+
#![feature(coroutines, coroutine_trait)]
6+
7+
use std::ops::{
8+
Coroutine,
9+
CoroutineState::{self},
10+
//~^ ERROR unused import: `*`
11+
};
12+
use std::pin::Pin;
13+
14+
#[allow(dead_code)]
15+
fn finish<T>(mut amt: usize, mut t: T) -> T::Return
16+
where T: Coroutine<(), Yield = ()> + Unpin,
17+
{
18+
loop {
19+
match Pin::new(&mut t).resume(()) {
20+
CoroutineState::Yielded(()) => amt = amt.checked_sub(1).unwrap(),
21+
CoroutineState::Complete(ret) => {
22+
assert_eq!(amt, 0);
23+
return ret
24+
}
25+
}
26+
}
27+
}
28+
29+
30+
mod foo {
31+
pub fn bar() {}
32+
}
33+
34+
pub fn main() {
35+
36+
use foo::bar;
37+
bar();
38+
//~^ ERROR unnecessary qualification
39+
bar();
40+
41+
// The item `use std::string::String` is imported redundantly.
42+
// Suppress `unused_imports` reporting, otherwise the fixed file will report an error
43+
#[allow(unused_imports)]
44+
use std::string::String;
45+
let s = String::new();
46+
let y = std::string::String::new();
47+
// unnecessary qualification
48+
println!("{} {}", s, y);
49+
50+
}

0 commit comments

Comments
 (0)