fluent diagnostics can easily overwrite each others' fields/args

[oli-obk]

Currently it is neither an error for diagnostic structs to have fields of the same name as subdiagnostics used in that same struct, nor is it a runtime error to specify arg("foo", ...) twice with the same arg name. This caused some confusion in const eval errors where the main error had an instance field/arg and the stack frame that got printed had an instance per frame

Originally posted by @RalfJung in #142015 (comment)

[xizheyin]

I'd like to try sovling this issue.
@rustbot claim

[xizheyin]

There are some issues that need to be clarified. I did some experiments. I inserted the debug_assert macro in the arg and found a lot of related issues.
For example, for ActualImplExplNotes, it has the same field trait_path in each variant, and TraitPlaceholderMismatch stores an array of it, so each insertion of ActualImplExplNotes overwrites the previously inserted trait_path. So I think just checking for duplicate fields may not be enough, and it seems like we would have to modify the way we diagnose derive to be compatible with this similar diagnosis.

But I'm thinking this might cause the compiler size to increase a lot. For example, open up space to store trait_path for each variant in the array.

• ActualImplExplNotes:
  

  rust/compiler/rustc_trait_selection/src/errors.rs

  Lines 802 to 907 in 1bb3352

  	pub enum ActualImplExplNotes<'tcx> {
  	#[note(trait_selection_actual_impl_expl_expected_signature_two)]
  	ExpectedSignatureTwo {
  	leading_ellipsis: bool,
  	ty_or_sig: TyOrSig<'tcx>,
  	trait_path: Highlighted<'tcx, TraitRefPrintOnlyTraitPath<'tcx>>,
  	lifetime_1: usize,
  	lifetime_2: usize,
  	},
  	#[note(trait_selection_actual_impl_expl_expected_signature_any)]
  	ExpectedSignatureAny {
  	leading_ellipsis: bool,
  	ty_or_sig: TyOrSig<'tcx>,
  	trait_path: Highlighted<'tcx, TraitRefPrintOnlyTraitPath<'tcx>>,
  	lifetime_1: usize,
  	},
  	#[note(trait_selection_actual_impl_expl_expected_signature_some)]
  	ExpectedSignatureSome {
  	leading_ellipsis: bool,
  	ty_or_sig: TyOrSig<'tcx>,
  	trait_path: Highlighted<'tcx, TraitRefPrintOnlyTraitPath<'tcx>>,
  	lifetime_1: usize,
  	},
  	#[note(trait_selection_actual_impl_expl_expected_signature_nothing)]
  	ExpectedSignatureNothing {
  	leading_ellipsis: bool,
  	ty_or_sig: TyOrSig<'tcx>,
  	trait_path: Highlighted<'tcx, TraitRefPrintOnlyTraitPath<'tcx>>,
  	},
  	#[note(trait_selection_actual_impl_expl_expected_passive_two)]
  	ExpectedPassiveTwo {
  	leading_ellipsis: bool,
  	ty_or_sig: TyOrSig<'tcx>,
  	trait_path: Highlighted<'tcx, TraitRefPrintOnlyTraitPath<'tcx>>,
  	lifetime_1: usize,
  	lifetime_2: usize,
  	},
  	#[note(trait_selection_actual_impl_expl_expected_passive_any)]
  	ExpectedPassiveAny {
  	leading_ellipsis: bool,
  	ty_or_sig: TyOrSig<'tcx>,
  	trait_path: Highlighted<'tcx, TraitRefPrintOnlyTraitPath<'tcx>>,
  	lifetime_1: usize,
  	},
  	#[note(trait_selection_actual_impl_expl_expected_passive_some)]
  	ExpectedPassiveSome {
  	leading_ellipsis: bool,
  	ty_or_sig: TyOrSig<'tcx>,
  	trait_path: Highlighted<'tcx, TraitRefPrintOnlyTraitPath<'tcx>>,
  	lifetime_1: usize,
  	},
  	#[note(trait_selection_actual_impl_expl_expected_passive_nothing)]
  	ExpectedPassiveNothing {
  	leading_ellipsis: bool,
  	ty_or_sig: TyOrSig<'tcx>,
  	trait_path: Highlighted<'tcx, TraitRefPrintOnlyTraitPath<'tcx>>,
  	},
  	#[note(trait_selection_actual_impl_expl_expected_other_two)]
  	ExpectedOtherTwo {
  	leading_ellipsis: bool,
  	ty_or_sig: TyOrSig<'tcx>,
  	trait_path: Highlighted<'tcx, TraitRefPrintOnlyTraitPath<'tcx>>,
  	lifetime_1: usize,
  	lifetime_2: usize,
  	},
  	#[note(trait_selection_actual_impl_expl_expected_other_any)]
  	ExpectedOtherAny {
  	leading_ellipsis: bool,
  	ty_or_sig: TyOrSig<'tcx>,
  	trait_path: Highlighted<'tcx, TraitRefPrintOnlyTraitPath<'tcx>>,
  	lifetime_1: usize,
  	},
  	#[note(trait_selection_actual_impl_expl_expected_other_some)]
  	ExpectedOtherSome {
  	leading_ellipsis: bool,
  	ty_or_sig: TyOrSig<'tcx>,
  	trait_path: Highlighted<'tcx, TraitRefPrintOnlyTraitPath<'tcx>>,
  	lifetime_1: usize,
  	},
  	#[note(trait_selection_actual_impl_expl_expected_other_nothing)]
  	ExpectedOtherNothing {
  	leading_ellipsis: bool,
  	ty_or_sig: TyOrSig<'tcx>,
  	trait_path: Highlighted<'tcx, TraitRefPrintOnlyTraitPath<'tcx>>,
  	},
  	#[note(trait_selection_actual_impl_expl_but_actually_implements_trait)]
  	ButActuallyImplementsTrait {
  	trait_path: Highlighted<'tcx, TraitRefPrintOnlyTraitPath<'tcx>>,
  	has_lifetime: bool,
  	lifetime: usize,
  	},
  	#[note(trait_selection_actual_impl_expl_but_actually_implemented_for_ty)]
  	ButActuallyImplementedForTy {
  	trait_path: Highlighted<'tcx, TraitRefPrintOnlyTraitPath<'tcx>>,
  	has_lifetime: bool,
  	lifetime: usize,
  	ty: String,
  	},
  	#[note(trait_selection_actual_impl_expl_but_actually_ty_implements)]
  	ButActuallyTyImplements {
  	trait_path: Highlighted<'tcx, TraitRefPrintOnlyTraitPath<'tcx>>,
  	has_lifetime: bool,
  	lifetime: usize,
  	ty: String,
  	},
  	}

• TraitPlaceholderMismatch
  

  rust/compiler/rustc_trait_selection/src/errors.rs

  Lines 991 to 1007 in 1bb3352

  	#[derive(Diagnostic)]
  	#[diag(trait_selection_trait_placeholder_mismatch)]
  	pub struct TraitPlaceholderMismatch<'tcx> {
  	#[primary_span]
  	pub span: Span,
  	#[label(trait_selection_label_satisfy)]
  	pub satisfy_span: Option<Span>,
  	#[label(trait_selection_label_where)]
  	pub where_span: Option<Span>,
  	#[label(trait_selection_label_dup)]
  	pub dup_span: Option<Span>,
  	pub def_id: String,
  	pub trait_def_id: String,
  	#[subdiagnostic]
  	pub actual_impl_expl_notes: Vec<ActualImplExplNotes<'tcx>>,
  	}

[oli-obk]

hmm yea, we have a similar situation in const eval that caused this issue in the first place. The reason it works with subdiagnostics is that we immediately render after processing each element in the vec. So maybe the solution is to preserve the args before going into subdiagnostics and to reset them after each subdiagnostic to the baseline args.

[xizheyin]

After reading through the code, I think the core of the problem is that both Diagnostic and all Subdiagnostics insert their arguments into the same argument map (args) during the construction phase, before the diagnostic is actually emitted. This means:

• If a Subdiagnostic has a field with the same name as a field in the main Diagnostic (or as another Subdiagnostic), the value will overwrite the previous one in the map.
• By the time we call emit(), only the last value for each argument name is kept, and all previous values are lost.
• So, even if we try to “save and restore” the argument map (as in the “snapshot” idea), it doesn’t help, because the overwriting already happened during construction, not during emission.

In other words, the current implementation is not “render as you go” (where we could snapshot/restore args between subdiagnostics), but rather “build everything, then render once”.
This means that multiple subdiagnostics with the same argument name will always conflict, and only the last one wins.

To really fix this, we’d need to either:

1. Change the system to render each subdiagnostic immediately after its arguments are set (so snapshots make sense), or
2. Make argument names unique per subdiagnostic (e.g. by namespacing), or
3. Redesign how arguments are stored and resolved during rendering. For example, Diagnostic and each SubDiagnostic have their own arguments map.

1 and 2 may partially solve the problem, 3 may totally solve this problem but increase the size of rustc binary.

Please point it out if I misunderstood anything.

[oli-obk]

Oh that's very surprising. I'm guessing either it's so subtle that no one noticed or that for most subdiagnostics the overlapping fields have the same values.

cc @davidtwco which of options 2/3 do you think is best?

[xizheyin]

2 would result in rewriting a field for each variant in this example #142031 (comment), and it would be tricky in a Vec of SubDiagnostics, for example, one field of an ExampleDiagnostic is Vec, and each arg name in the ExampleSubDiagnostic is fixed, it will repeatedly cover the arg with the same name, so it doesn't fully solve our problem.

3 seems to be more ergonomic, and the behavior of the code can be consistent with the structure we defined. But I'm not quite sure why it wasn't designed that way initially.

[oli-obk]

3 seems to be more ergonomic, and the behavior of the code can be consistent with the structure we defined. But I'm not quite sure why it wasn't designed that way initially.

maybe subdiagnostics were meant to be able to access the fields of the main diagnostic, which is a bit in conflict with reusing subdiagnostics, similar to the problem you describe for 2.
But it should be fine under 3. by supplying the outer diagnostic's args to the subdiagnostic renderer

[xizheyin]

bingo, if that's the case, I think using something like a symbol table that takes into account distinguishing between different levels of scope is the best way to go.

[xizheyin]

So what we need to do is:

1. set up a new symbol table for each SubDiagnostic that can also share arg with the Diagnostic.
2. not allow SubDiagnostic to have arg with the same name as Diagnostic.

This avoids the issue originally raised in the issue, and doesn't get in the way of the issue in the comments.