Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(resolve): update the ambiguity glob binding as warning recursively #113099

Merged
merged 2 commits into from
Jul 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions compiler/rustc_errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1871,6 +1871,22 @@ pub fn add_elided_lifetime_in_path_suggestion(
});
}

pub fn report_ambiguity_error<'a, G: EmissionGuarantee>(
db: &mut DiagnosticBuilder<'a, G>,
ambiguity: rustc_lint_defs::AmbiguityErrorDiag,
) {
db.span_label(ambiguity.label_span, ambiguity.label_msg);
db.note(ambiguity.note_msg);
db.span_note(ambiguity.b1_span, ambiguity.b1_note_msg);
for help_msg in ambiguity.b1_help_msgs {
db.help(help_msg);
}
db.span_note(ambiguity.b2_span, ambiguity.b2_note_msg);
for help_msg in ambiguity.b2_help_msgs {
db.help(help_msg);
}
}

#[derive(Clone, Copy, PartialEq, Hash, Debug)]
pub enum TerminalUrl {
No,
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_lint/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -946,6 +946,9 @@ pub trait LintContext: Sized {
Applicability::MachineApplicable,
);
}
BuiltinLintDiagnostics::AmbiguousGlobImports { diag } => {
rustc_errors::report_ambiguity_error(db, diag);
}
BuiltinLintDiagnostics::AmbiguousGlobReexports { name, namespace, first_reexport_span, duplicate_reexport_span } => {
db.span_label(first_reexport_span, format!("the name `{name}` in the {namespace} namespace is first re-exported here"));
db.span_label(duplicate_reexport_span, format!("but the name `{name}` in the {namespace} namespace is also re-exported here"));
Expand Down
44 changes: 44 additions & 0 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3316,6 +3316,7 @@ declare_lint_pass! {
// tidy-alphabetical-start
ABSOLUTE_PATHS_NOT_STARTING_WITH_CRATE,
AMBIGUOUS_ASSOCIATED_ITEMS,
AMBIGUOUS_GLOB_IMPORTS,
AMBIGUOUS_GLOB_REEXPORTS,
ARITHMETIC_OVERFLOW,
ASM_SUB_REGISTER,
Expand Down Expand Up @@ -4405,3 +4406,46 @@ declare_lint! {
Warn,
"unrecognized diagnostic attribute"
}

declare_lint! {
/// The `ambiguous_glob_imports` lint detects glob imports that should report ambiguity
/// errors, but previously didn't do that due to rustc bugs.
///
/// ### Example
///
/// ```rust,compile_fail
///
/// #![deny(ambiguous_glob_imports)]
/// pub fn foo() -> u32 {
/// use sub::*;
/// C
/// }
///
/// mod sub {
/// mod mod1 { pub const C: u32 = 1; }
/// mod mod2 { pub const C: u32 = 2; }
///
/// pub use mod1::*;
/// pub use mod2::*;
/// }
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// Previous versions of Rust compile it successfully because it
/// had lost the ambiguity error when resolve `use sub::mod2::*`.
///
/// This is a [future-incompatible] lint to transition this to a
/// hard error in the future.
///
/// [future-incompatible]: ../index.md#future-incompatible-lints
pub AMBIGUOUS_GLOB_IMPORTS,
Warn,
"detects certain glob imports that require reporting an ambiguity error",
@future_incompatible = FutureIncompatibleInfo {
reason: FutureIncompatibilityReason::FutureReleaseError,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW this FutureIncompatibilityReason will make the warning only show up in locally built crates, but not in cargo's future breakage report that also shows warnings from dependencies. Just making sure that this is a deliberate choice and not an accident. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just assumed all future incompatibility lints are reported for dependencies during review.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that's not the case. #116049 should help clarify (but doesn't change the behavior of any existing lint).

reference: "issue #114095 <https://github.com/rust-lang/rust/issues/114095>",
};
}
18 changes: 18 additions & 0 deletions compiler/rustc_lint_defs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,21 @@ impl<HCX> ToStableHashKey<HCX> for LintId {
}
}

#[derive(Debug)]
pub struct AmbiguityErrorDiag {
pub msg: String,
pub span: Span,
pub label_span: Span,
pub label_msg: String,
pub note_msg: String,
pub b1_span: Span,
pub b1_note_msg: String,
pub b1_help_msgs: Vec<String>,
pub b2_span: Span,
pub b2_note_msg: String,
pub b2_help_msgs: Vec<String>,
}

// This could be a closure, but then implementing derive trait
// becomes hacky (and it gets allocated).
#[derive(Debug)]
Expand Down Expand Up @@ -530,6 +545,9 @@ pub enum BuiltinLintDiagnostics {
vis_span: Span,
ident_span: Span,
},
AmbiguousGlobImports {
diag: AmbiguityErrorDiag,
},
AmbiguousGlobReexports {
/// The name for which collision(s) have occurred.
name: String,
Expand Down
4 changes: 3 additions & 1 deletion compiler/rustc_resolve/src/build_reduced_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ impl<'a, Id: Into<DefId>> ToNameBinding<'a>
arenas.alloc_name_binding(NameBindingData {
kind: NameBindingKind::Module(self.0),
ambiguity: None,
warn_ambiguity: false,
vis: self.1.to_def_id(),
span: self.2,
expansion: self.3,
Expand All @@ -53,6 +54,7 @@ impl<'a, Id: Into<DefId>> ToNameBinding<'a> for (Res, ty::Visibility<Id>, Span,
arenas.alloc_name_binding(NameBindingData {
kind: NameBindingKind::Res(self.0),
ambiguity: None,
warn_ambiguity: false,
vis: self.1.to_def_id(),
span: self.2,
expansion: self.3,
Expand All @@ -69,7 +71,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
{
let binding = def.to_name_binding(self.arenas);
let key = self.new_disambiguated_key(ident, ns);
if let Err(old_binding) = self.try_define(parent, key, binding) {
if let Err(old_binding) = self.try_define(parent, key, binding, false) {
self.report_conflict(parent, ident, ns, old_binding, binding);
}
}
Expand Down
75 changes: 52 additions & 23 deletions compiler/rustc_resolve/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,8 @@ use rustc_ast::{self as ast, Crate, ItemKind, ModKind, NodeId, Path, CRATE_NODE_
use rustc_ast::{MetaItemKind, NestedMetaItem};
use rustc_ast_pretty::pprust;
use rustc_data_structures::fx::FxHashSet;
use rustc_errors::{
pluralize, Applicability, Diagnostic, DiagnosticBuilder, ErrorGuaranteed, MultiSpan,
};
use rustc_errors::{struct_span_err, SuggestionStyle};
use rustc_errors::{pluralize, report_ambiguity_error, struct_span_err, SuggestionStyle};
use rustc_errors::{Applicability, Diagnostic, DiagnosticBuilder, ErrorGuaranteed, MultiSpan};
use rustc_feature::BUILTIN_ATTRIBUTES;
use rustc_hir::def::Namespace::{self, *};
use rustc_hir::def::{self, CtorKind, CtorOf, DefKind, NonMacroAttrKind, PerNS};
Expand All @@ -17,8 +15,9 @@ use rustc_hir::PrimTy;
use rustc_middle::bug;
use rustc_middle::ty::TyCtxt;
use rustc_session::lint::builtin::ABSOLUTE_PATHS_NOT_STARTING_WITH_CRATE;
use rustc_session::lint::builtin::AMBIGUOUS_GLOB_IMPORTS;
use rustc_session::lint::builtin::MACRO_EXPANDED_MACRO_EXPORTS_ACCESSED_BY_ABSOLUTE_PATHS;
use rustc_session::lint::BuiltinLintDiagnostics;
use rustc_session::lint::{AmbiguityErrorDiag, BuiltinLintDiagnostics};
use rustc_session::Session;
use rustc_span::edit_distance::find_best_match_for_name;
use rustc_span::edition::Edition;
Expand Down Expand Up @@ -135,7 +134,23 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
}

for ambiguity_error in &self.ambiguity_errors {
self.report_ambiguity_error(ambiguity_error);
let diag = self.ambiguity_diagnostics(ambiguity_error);
if ambiguity_error.warning {
let NameBindingKind::Import { import, .. } = ambiguity_error.b1.0.kind else {
unreachable!()
};
self.lint_buffer.buffer_lint_with_diagnostic(
AMBIGUOUS_GLOB_IMPORTS,
import.root_id,
ambiguity_error.ident.span,
diag.msg.to_string(),
BuiltinLintDiagnostics::AmbiguousGlobImports { diag },
);
} else {
let mut err = struct_span_err!(self.tcx.sess, diag.span, E0659, "{}", &diag.msg);
report_ambiguity_error(&mut err, diag);
err.emit();
}
}

let mut reported_spans = FxHashSet::default();
Expand Down Expand Up @@ -1540,20 +1555,15 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
}
}

fn report_ambiguity_error(&self, ambiguity_error: &AmbiguityError<'_>) {
let AmbiguityError { kind, ident, b1, b2, misc1, misc2 } = *ambiguity_error;
fn ambiguity_diagnostics(&self, ambiguity_error: &AmbiguityError<'_>) -> AmbiguityErrorDiag {
let AmbiguityError { kind, ident, b1, b2, misc1, misc2, .. } = *ambiguity_error;
let (b1, b2, misc1, misc2, swapped) = if b2.span.is_dummy() && !b1.span.is_dummy() {
// We have to print the span-less alternative first, otherwise formatting looks bad.
(b2, b1, misc2, misc1, true)
} else {
(b1, b2, misc1, misc2, false)
};

let mut err = struct_span_err!(self.tcx.sess, ident.span, E0659, "`{ident}` is ambiguous");
err.span_label(ident.span, "ambiguous name");
err.note(format!("ambiguous because of {}", kind.descr()));

let mut could_refer_to = |b: NameBinding<'_>, misc: AmbiguityErrorMisc, also: &str| {
let could_refer_to = |b: NameBinding<'_>, misc: AmbiguityErrorMisc, also: &str| {
let what = self.binding_description(b, ident, misc == AmbiguityErrorMisc::FromPrelude);
let note_msg = format!("`{ident}` could{also} refer to {what}");

Expand All @@ -1579,16 +1589,35 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
AmbiguityErrorMisc::FromPrelude | AmbiguityErrorMisc::None => {}
}

err.span_note(b.span, note_msg);
for (i, help_msg) in help_msgs.iter().enumerate() {
let or = if i == 0 { "" } else { "or " };
err.help(format!("{}{}", or, help_msg));
}
(
b.span,
note_msg,
help_msgs
.iter()
.enumerate()
.map(|(i, help_msg)| {
let or = if i == 0 { "" } else { "or " };
format!("{}{}", or, help_msg)
})
.collect::<Vec<_>>(),
)
};

could_refer_to(b1, misc1, "");
could_refer_to(b2, misc2, " also");
err.emit();
let (b1_span, b1_note_msg, b1_help_msgs) = could_refer_to(b1, misc1, "");
let (b2_span, b2_note_msg, b2_help_msgs) = could_refer_to(b2, misc2, " also");

AmbiguityErrorDiag {
msg: format!("`{ident}` is ambiguous"),
span: ident.span,
label_span: ident.span,
label_msg: "ambiguous name".to_string(),
note_msg: format!("ambiguous because of {}", kind.descr()),
b1_span,
b1_note_msg,
b1_help_msgs,
b2_span,
b2_note_msg,
b2_help_msgs,
}
}

/// If the binding refers to a tuple struct constructor with fields,
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_resolve/src/ident.rs
Original file line number Diff line number Diff line change
Expand Up @@ -677,6 +677,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
ident: orig_ident,
b1: innermost_binding,
b2: binding,
warning: false,
misc1: misc(innermost_flags),
misc2: misc(flags),
});
Expand Down Expand Up @@ -905,6 +906,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
ident,
b1: binding,
b2: shadowed_glob,
warning: false,
misc1: AmbiguityErrorMisc::None,
misc2: AmbiguityErrorMisc::None,
});
Expand Down
Loading