Skip to content

Add runtime check to avoid overwrite arg in Diag #142724

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
8 changes: 7 additions & 1 deletion compiler/rustc_borrowck/src/diagnostics/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1254,8 +1254,14 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
&& !spans.is_empty()
{
let mut span: MultiSpan = spans.clone().into();
err.arg("ty", param_ty.to_string());
let msg = err.dcx.eagerly_translate_to_string(
fluent::borrowck_moved_a_fn_once_in_call_def,
err.args.iter(),
);
err.remove_arg("ty");
for sp in spans {
span.push_span_label(sp, fluent::borrowck_moved_a_fn_once_in_call_def);
span.push_span_label(sp, msg.clone());
}
span.push_span_label(
fn_call_span,
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_builtin_macros/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -672,6 +672,7 @@ impl Subdiagnostic for FormatUnusedArg {
fn add_to_diag<G: EmissionGuarantee>(self, diag: &mut Diag<'_, G>) {
diag.arg("named", self.named);
let msg = diag.eagerly_translate(crate::fluent_generated::builtin_macros_format_unused_arg);
diag.remove_arg("named");
diag.span_label(self.span, msg);
}
}
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_const_eval/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,9 @@ impl Subdiagnostic for FrameNote {
span.push_span_label(self.span, fluent::const_eval_frame_note_last);
}
let msg = diag.eagerly_translate(fluent::const_eval_frame_note);
diag.remove_arg("times");
diag.remove_arg("where_");
diag.remove_arg("instance");
diag.span_note(span, msg);
}
}
Expand Down
33 changes: 32 additions & 1 deletion compiler/rustc_errors/src/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,9 @@ pub struct DiagInner {
pub suggestions: Suggestions,
pub args: DiagArgMap,

// This is used to store args and restore them after a subdiagnostic is rendered.
pub reserved_args: DiagArgMap,

/// This is not used for highlighting or rendering any error message. Rather, it can be used
/// as a sort key to sort a buffer of diagnostics. By default, it is the primary span of
/// `span` if there is one. Otherwise, it is `DUMMY_SP`.
Expand Down Expand Up @@ -319,6 +322,7 @@ impl DiagInner {
children: vec![],
suggestions: Suggestions::Enabled(vec![]),
args: Default::default(),
reserved_args: Default::default(),
sort_span: DUMMY_SP,
is_lint: None,
long_ty_path: None,
Expand Down Expand Up @@ -390,7 +394,28 @@ impl DiagInner {
}

pub(crate) fn arg(&mut self, name: impl Into<DiagArgName>, arg: impl IntoDiagArg) {
self.args.insert(name.into(), arg.into_diag_arg(&mut self.long_ty_path));
let name = name.into();
let value = arg.into_diag_arg(&mut self.long_ty_path);
// This assertion is to avoid subdiagnostics overwriting an existing diagnostic arg.
debug_assert!(
!self.args.contains_key(&name) || self.args.get(&name) == Some(&value),
"arg {} already exists",
name
);
self.args.insert(name, value);
}

pub fn remove_arg(&mut self, name: &str) {
self.args.swap_remove(name);
}

pub fn store_args(&mut self) {
self.reserved_args = self.args.clone();
}

pub fn restore_args(&mut self) {
self.args = self.reserved_args.clone();
self.reserved_args.clear();
}

/// Fields used for Hash, and PartialEq trait.
Expand Down Expand Up @@ -1423,6 +1448,12 @@ impl<'a, G: EmissionGuarantee> Diag<'a, G> {
self.downgrade_to_delayed_bug();
self.emit()
}

pub fn remove_arg(&mut self, name: &str) {
if let Some(diag) = self.diag.as_mut() {
diag.remove_arg(name);
}
}
}

/// Destructor bomb: every `Diag` must be consumed (emitted, cancelled, etc.)
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_hir_analysis/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ pub(crate) enum AssocItemNotFoundSugg<'a> {
SimilarInOtherTrait {
#[primary_span]
span: Span,
trait_name: &'a str,
assoc_kind: &'static str,
suggested_name: Symbol,
},
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_hir_analysis/src/hir_ty_lowering/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,7 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
// change the associated item.
err.sugg = Some(errors::AssocItemNotFoundSugg::SimilarInOtherTrait {
span: assoc_ident.span,
trait_name: &trait_name,
assoc_kind: assoc_kind_str,
suggested_name,
});
Expand Down
5 changes: 3 additions & 2 deletions compiler/rustc_lint/src/levels.rs
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,8 @@ impl<'s, P: LintLevelsProvider> LintLevelsBuilder<'s, P> {
let name = lint_name.as_str();
let suggestion = RenamedLintSuggestion::WithoutSpan { replace };
let requested_level = RequestedLevel { level, lint_name };
let lint = RenamedLintFromCommandLine { name, suggestion, requested_level };
let lint =
RenamedLintFromCommandLine { name, replace, suggestion, requested_level };
self.emit_lint(RENAMED_AND_REMOVED_LINTS, lint);
}
CheckLintNameResult::Removed(ref reason) => {
Expand Down Expand Up @@ -824,7 +825,7 @@ impl<'s, P: LintLevelsProvider> LintLevelsBuilder<'s, P> {
RenamedLintSuggestion::WithSpan { suggestion: sp, replace };
let name =
tool_ident.map(|tool| format!("{tool}::{name}")).unwrap_or(name);
let lint = RenamedLint { name: name.as_str(), suggestion };
let lint = RenamedLint { name: name.as_str(), replace, suggestion };
self.emit_span_lint(RENAMED_AND_REMOVED_LINTS, sp.into(), lint);
}

Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_lint/src/lifetime_syntax.rs
Original file line number Diff line number Diff line change
Expand Up @@ -422,12 +422,12 @@ fn build_mismatch_suggestion(
lifetime_name: &str,
infos: &[&Info<'_>],
) -> lints::MismatchedLifetimeSyntaxesSuggestion {
let lifetime_name = lifetime_name.to_owned();
let lifetime_name_sugg = lifetime_name.to_owned();

let suggestions = infos.iter().map(|info| info.suggestion(&lifetime_name)).collect();

lints::MismatchedLifetimeSyntaxesSuggestion::Explicit {
lifetime_name,
lifetime_name_sugg,
suggestions,
tool_only: false,
}
Expand Down
11 changes: 6 additions & 5 deletions compiler/rustc_lint/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1089,6 +1089,7 @@ pub(crate) struct DeprecatedLintNameFromCommandLine<'a> {
#[diag(lint_renamed_lint)]
pub(crate) struct RenamedLint<'a> {
pub name: &'a str,
pub replace: &'a str,
#[subdiagnostic]
pub suggestion: RenamedLintSuggestion<'a>,
}
Expand All @@ -1109,6 +1110,7 @@ pub(crate) enum RenamedLintSuggestion<'a> {
#[diag(lint_renamed_lint)]
pub(crate) struct RenamedLintFromCommandLine<'a> {
pub name: &'a str,
pub replace: &'a str,
#[subdiagnostic]
pub suggestion: RenamedLintSuggestion<'a>,
#[subdiagnostic]
Expand Down Expand Up @@ -3227,7 +3229,7 @@ pub(crate) enum MismatchedLifetimeSyntaxesSuggestion {
},

Explicit {
lifetime_name: String,
lifetime_name_sugg: String,
suggestions: Vec<(Span, String)>,
tool_only: bool,
},
Expand Down Expand Up @@ -3281,13 +3283,12 @@ impl Subdiagnostic for MismatchedLifetimeSyntaxesSuggestion {
);
}

Explicit { lifetime_name, suggestions, tool_only } => {
diag.arg("lifetime_name", lifetime_name);

Explicit { lifetime_name_sugg, suggestions, tool_only } => {
diag.arg("lifetime_name_sugg", lifetime_name_sugg);
let msg = diag.eagerly_translate(
fluent::lint_mismatched_lifetime_syntaxes_suggestion_explicit,
);

diag.remove_arg("lifetime_name_sugg");
diag.multipart_suggestion_with_style(
msg,
suggestions,
Expand Down
30 changes: 25 additions & 5 deletions compiler/rustc_macros/src/diagnostics/subdiagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ impl<'parent, 'a> SubdiagnosticDeriveVariantBuilder<'parent, 'a> {
}

/// Generates the code for a field with no attributes.
fn generate_field_arg(&mut self, binding_info: &BindingInfo<'_>) -> TokenStream {
fn generate_field_arg(&mut self, binding_info: &BindingInfo<'_>) -> (TokenStream, TokenStream) {
let diag = &self.parent.diag;

let field = binding_info.ast();
Expand All @@ -230,12 +230,16 @@ impl<'parent, 'a> SubdiagnosticDeriveVariantBuilder<'parent, 'a> {
let ident = field.ident.as_ref().unwrap();
let ident = format_ident!("{}", ident); // strip `r#` prefix, if present

quote! {
let args = quote! {
#diag.arg(
stringify!(#ident),
#field_binding
);
}
};
let remove_args = quote! {
#diag.remove_arg(stringify!(#ident));
};
(args, remove_args)
}

/// Generates the necessary code for all attributes on a field.
Expand Down Expand Up @@ -600,8 +604,13 @@ impl<'parent, 'a> SubdiagnosticDeriveVariantBuilder<'parent, 'a> {

calls.extend(call);
}

let plain_args: TokenStream = self
let store_args = quote! {
#diag.store_args();
};
let restore_args = quote! {
#diag.restore_args();
};
let (plain_args, remove_args): (TokenStream, TokenStream) = self
.variant
.bindings()
.iter()
Expand All @@ -610,12 +619,23 @@ impl<'parent, 'a> SubdiagnosticDeriveVariantBuilder<'parent, 'a> {
.collect();

let formatting_init = &self.formatting_init;

// For #[derive(Subdiagnostic)]
//
// - Store args of the main diagnostic for later restore.
// - add args of subdiagnostic.
// - Generate the calls, such as note, label, etc.
// - Remove the arguments for allowing Vec<Subdiagnostic> to be used.
// - Restore the arguments for allowing main and subdiagnostic share the same fields.
Ok(quote! {
#init
#formatting_init
#attr_args
#store_args
#plain_args
#calls
#remove_args
#restore_args
Comment on lines +622 to +638
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For #[derive(Subdiagnostic)], I made two changes:

  1. After the subdiagnostic is rendered, remove all args of this subdiagnostic, which allows for usage like Vec<Subdiag>.
  2. Store diag.args before setting arguments, so that you can restore the contents of the main diagnostic after deleting the arguments after subdiagnostic is rendered, to avoid deleting the main diagnostic's arg when they have the same name args.

})
}
}
2 changes: 1 addition & 1 deletion compiler/rustc_metadata/src/dependency_format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ fn add_library(
crate_name: tcx.crate_name(cnum),
non_static_deps: unavailable_as_static
.drain(..)
.map(|cnum| NonStaticCrateDep { crate_name: tcx.crate_name(cnum) })
.map(|cnum| NonStaticCrateDep { crate_name_: tcx.crate_name(cnum) })
.collect(),
rustc_driver_help: linking_to_rustc_driver.then_some(RustcDriverHelp),
});
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_metadata/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ pub struct CrateDepMultiple {
#[derive(Subdiagnostic)]
#[note(metadata_crate_dep_not_static)]
pub struct NonStaticCrateDep {
pub crate_name: Symbol,
pub crate_name_: Symbol,
}

#[derive(Subdiagnostic)]
Expand Down
17 changes: 13 additions & 4 deletions compiler/rustc_mir_build/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -994,14 +994,15 @@ pub(crate) struct PatternNotCovered<'s, 'tcx> {
pub(crate) uncovered: Uncovered,
#[subdiagnostic]
pub(crate) inform: Option<Inform>,
#[label(mir_build_confused)]
pub(crate) interpreted_as_const: Option<Span>,
#[subdiagnostic]
pub(crate) interpreted_as_const_sugg: Option<InterpretedAsConst>,
pub(crate) interpreted_as_const: Option<InterpretedAsConst>,
#[subdiagnostic]
pub(crate) interpreted_as_const_sugg: Option<InterpretedAsConstSugg>,
#[subdiagnostic]
pub(crate) adt_defined_here: Option<AdtDefinedHere<'tcx>>,
#[note(mir_build_privately_uninhabited)]
pub(crate) witness_1_is_privately_uninhabited: bool,
pub(crate) witness_1: String,
#[note(mir_build_pattern_ty)]
pub(crate) _p: (),
pub(crate) pattern_ty: Ty<'tcx>,
Expand All @@ -1016,6 +1017,14 @@ pub(crate) struct PatternNotCovered<'s, 'tcx> {
#[note(mir_build_more_information)]
pub(crate) struct Inform;

#[derive(Subdiagnostic)]
#[label(mir_build_confused)]
pub(crate) struct InterpretedAsConst {
#[primary_span]
pub(crate) span: Span,
pub(crate) variable: String,
}

pub(crate) struct AdtDefinedHere<'tcx> {
pub(crate) adt_def_span: Span,
pub(crate) ty: Ty<'tcx>,
Expand Down Expand Up @@ -1046,7 +1055,7 @@ impl<'tcx> Subdiagnostic for AdtDefinedHere<'tcx> {
applicability = "maybe-incorrect",
style = "verbose"
)]
pub(crate) struct InterpretedAsConst {
pub(crate) struct InterpretedAsConstSugg {
#[primary_span]
pub(crate) span: Span,
pub(crate) variable: String,
Expand Down
7 changes: 5 additions & 2 deletions compiler/rustc_mir_build/src/thir/pattern/check_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -685,8 +685,8 @@ impl<'p, 'tcx> MatchVisitor<'p, 'tcx> {
let span = self.tcx.def_span(def_id);
let variable = self.tcx.item_name(def_id).to_string();
// When we encounter a constant as the binding name, point at the `const` definition.
interpreted_as_const = Some(span);
interpreted_as_const_sugg = Some(InterpretedAsConst { span: pat.span, variable });
interpreted_as_const = Some(InterpretedAsConst { span, variable: variable.clone() });
interpreted_as_const_sugg = Some(InterpretedAsConstSugg { span: pat.span, variable });
} else if let PatKind::Constant { .. } = unpeeled_pat.kind
&& let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(pat.span)
{
Expand Down Expand Up @@ -738,6 +738,8 @@ impl<'p, 'tcx> MatchVisitor<'p, 'tcx> {
false
};

let witness_1 = cx.print_witness_pat(witnesses.get(0).unwrap());

self.error = Err(self.tcx.dcx().emit_err(PatternNotCovered {
span: pat.span,
origin,
Expand All @@ -746,6 +748,7 @@ impl<'p, 'tcx> MatchVisitor<'p, 'tcx> {
interpreted_as_const,
interpreted_as_const_sugg,
witness_1_is_privately_uninhabited,
witness_1,
_p: (),
pattern_ty,
let_suggestion,
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_mir_transform/src/lint_tail_expr_drop_order.rs
Original file line number Diff line number Diff line change
Expand Up @@ -516,8 +516,12 @@ struct LocalLabel<'a> {
/// A custom `Subdiagnostic` implementation so that the notes are delivered in a specific order
impl Subdiagnostic for LocalLabel<'_> {
fn add_to_diag<G: rustc_errors::EmissionGuarantee>(self, diag: &mut rustc_errors::Diag<'_, G>) {
// Becuase parent uses this field , we need to remove it delay before adding it.
diag.remove_arg("name");
diag.arg("name", self.name);
diag.remove_arg("is_generated_name");
diag.arg("is_generated_name", self.is_generated_name);
diag.remove_arg("is_dropped_first_edition_2024");
diag.arg("is_dropped_first_edition_2024", self.is_dropped_first_edition_2024);
let msg = diag.eagerly_translate(crate::fluent_generated::mir_transform_tail_expr_local);
diag.span_label(self.span, msg);
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_passes/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ passes_duplicate_lang_item_crate_depends =
.second_definition_path = second definition in `{$crate_name}` loaded from {$path}

passes_enum_variant_same_name =
it is impossible to refer to the {$descr} `{$dead_name}` because it is shadowed by this enum variant with the same name
it is impossible to refer to the {$dead_descr} `{$dead_name}` because it is shadowed by this enum variant with the same name

passes_export_name =
attribute should be applied to a free function, impl method or static
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_passes/src/dead.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1078,7 +1078,7 @@ impl<'tcx> DeadVisitor<'tcx> {
maybe_enum.variants().iter().find(|i| i.name == dead_item.name)
{
Some(crate::errors::EnumVariantSameName {
descr: tcx.def_descr(dead_item.def_id.to_def_id()),
dead_descr: tcx.def_descr(dead_item.def_id.to_def_id()),
dead_name: dead_item.name,
variant_span: tcx.def_span(variant.def_id),
})
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_passes/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1509,7 +1509,7 @@ pub(crate) struct EnumVariantSameName<'tcx> {
#[primary_span]
pub variant_span: Span,
pub dead_name: Symbol,
pub descr: &'tcx str,
pub dead_descr: &'tcx str,
}

#[derive(Subdiagnostic)]
Expand Down Expand Up @@ -1707,6 +1707,7 @@ impl Subdiagnostic for UnusedVariableStringInterp {
#[derive(LintDiagnostic)]
#[diag(passes_unused_variable_try_ignore)]
pub(crate) struct UnusedVarTryIgnore {
pub name: String,
#[subdiagnostic]
pub sugg: UnusedVarTryIgnoreSugg,
}
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_passes/src/liveness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1743,6 +1743,7 @@ impl<'tcx> Liveness<'_, 'tcx> {
.map(|(_, pat_span, _)| *pat_span)
.collect::<Vec<_>>(),
errors::UnusedVarTryIgnore {
name: name.clone(),
sugg: errors::UnusedVarTryIgnoreSugg {
shorthands,
non_shorthands,
Expand Down
Loading
Loading