Auto merge of #117742 - weiznich:turn_overlapping_diagnostic_options_into_warnings, r=compiler-errors

bors · bors · commit 547ace805166 · 2023-11-18T02:57:09.000Z
Add some additional warnings for duplicated diagnostic items

This commit adds warnings if a user supplies several diagnostic options where we can only apply one of them. We explicitly warn about ignored options here. In addition a small test for these warnings is added.

r? `@compiler-errors`

For now that's the last PR to improve the warnings generated by misused `#[diagnostic::on_unimplemented]` attributes. I'm not sure what needs to be done next to move this closer to stabilization.
diff --git a/compiler/rustc_trait_selection/messages.ftl b/compiler/rustc_trait_selection/messages.ftl
@@ -22,6 +22,10 @@ trait_selection_dump_vtable_entries = vtable entries for `{$trait_ref}`: {$entri
 trait_selection_empty_on_clause_in_rustc_on_unimplemented = empty `on`-clause in `#[rustc_on_unimplemented]`
     .label = empty on-clause here
 
+trait_selection_ignored_diagnostic_option = `{$option_name}` is ignored due to previous definition of `{$option_name}`
+    .other_label = `{$option_name}` is first declared here
+    .label = `{$option_name}` is already declared here
+
 trait_selection_inherent_projection_normalization_overflow = overflow evaluating associated type `{$ty}`
 
 trait_selection_invalid_on_clause_in_rustc_on_unimplemented = invalid `on`-clause in `#[rustc_on_unimplemented]`
diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/on_unimplemented.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/on_unimplemented.rs
@@ -321,7 +321,7 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
 }
 
 #[derive(Clone, Debug)]
-pub struct OnUnimplementedFormatString(Symbol);
+pub struct OnUnimplementedFormatString(Symbol, Span);
 
 #[derive(Debug)]
 pub struct OnUnimplementedDirective {
@@ -350,7 +350,7 @@ pub struct OnUnimplementedNote {
 pub enum AppendConstMessage {
     #[default]
     Default,
-    Custom(Symbol),
+    Custom(Symbol, Span),
 }
 
 #[derive(LintDiagnostic)]
@@ -372,6 +372,35 @@ impl MalformedOnUnimplementedAttrLint {
 #[help]
 pub struct MissingOptionsForOnUnimplementedAttr;
 
+#[derive(LintDiagnostic)]
+#[diag(trait_selection_ignored_diagnostic_option)]
+pub struct IgnoredDiagnosticOption {
+    pub option_name: &'static str,
+    #[label]
+    pub span: Span,
+    #[label(trait_selection_other_label)]
+    pub prev_span: Span,
+}
+
+impl IgnoredDiagnosticOption {
+    fn maybe_emit_warning<'tcx>(
+        tcx: TyCtxt<'tcx>,
+        item_def_id: DefId,
+        new: Option<Span>,
+        old: Option<Span>,
+        option_name: &'static str,
+    ) {
+        if let (Some(new_item), Some(old_item)) = (new, old) {
+            tcx.emit_spanned_lint(
+                UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES,
+                tcx.hir().local_def_id_to_hir_id(item_def_id.expect_local()),
+                new_item,
+                IgnoredDiagnosticOption { span: new_item, prev_span: old_item, option_name },
+            );
+        }
+    }
+}
+
 impl<'tcx> OnUnimplementedDirective {
     fn parse(
         tcx: TyCtxt<'tcx>,
@@ -384,8 +413,9 @@ impl<'tcx> OnUnimplementedDirective {
         let mut errored = None;
         let mut item_iter = items.iter();
 
-        let parse_value = |value_str| {
-            OnUnimplementedFormatString::try_parse(tcx, item_def_id, value_str, span).map(Some)
+        let parse_value = |value_str, value_span| {
+            OnUnimplementedFormatString::try_parse(tcx, item_def_id, value_str, span, value_span)
+                .map(Some)
         };
 
         let condition = if is_root {
@@ -398,7 +428,7 @@ impl<'tcx> OnUnimplementedDirective {
                 .ok_or_else(|| tcx.sess.emit_err(InvalidOnClauseInOnUnimplemented { span }))?;
             attr::eval_condition(cond, &tcx.sess.parse_sess, Some(tcx.features()), &mut |cfg| {
                 if let Some(value) = cfg.value
-                    && let Err(guar) = parse_value(value)
+                    && let Err(guar) = parse_value(value, cfg.span)
                 {
                     errored = Some(guar);
                 }
@@ -417,17 +447,17 @@ impl<'tcx> OnUnimplementedDirective {
         for item in item_iter {
             if item.has_name(sym::message) && message.is_none() {
                 if let Some(message_) = item.value_str() {
-                    message = parse_value(message_)?;
+                    message = parse_value(message_, item.span())?;
                     continue;
                 }
             } else if item.has_name(sym::label) && label.is_none() {
                 if let Some(label_) = item.value_str() {
-                    label = parse_value(label_)?;
+                    label = parse_value(label_, item.span())?;
                     continue;
                 }
             } else if item.has_name(sym::note) {
                 if let Some(note_) = item.value_str() {
-                    if let Some(note) = parse_value(note_)? {
+                    if let Some(note) = parse_value(note_, item.span())? {
                         notes.push(note);
                         continue;
                     }
@@ -437,7 +467,7 @@ impl<'tcx> OnUnimplementedDirective {
                 && !is_diagnostic_namespace_variant
             {
                 if let Some(parent_label_) = item.value_str() {
-                    parent_label = parse_value(parent_label_)?;
+                    parent_label = parse_value(parent_label_, item.span())?;
                     continue;
                 }
             } else if item.has_name(sym::on)
@@ -470,7 +500,7 @@ impl<'tcx> OnUnimplementedDirective {
                 && !is_diagnostic_namespace_variant
             {
                 if let Some(msg) = item.value_str() {
-                    append_const_msg = Some(AppendConstMessage::Custom(msg));
+                    append_const_msg = Some(AppendConstMessage::Custom(msg, item.span()));
                     continue;
                 } else if item.is_word() {
                     append_const_msg = Some(AppendConstMessage::Default);
@@ -519,6 +549,54 @@ impl<'tcx> OnUnimplementedDirective {
                         subcommands.extend(directive.subcommands);
                         let mut notes = aggr.notes;
                         notes.extend(directive.notes);
+                        IgnoredDiagnosticOption::maybe_emit_warning(
+                            tcx,
+                            item_def_id,
+                            directive.message.as_ref().map(|f| f.1),
+                            aggr.message.as_ref().map(|f| f.1),
+                            "message",
+                        );
+                        IgnoredDiagnosticOption::maybe_emit_warning(
+                            tcx,
+                            item_def_id,
+                            directive.label.as_ref().map(|f| f.1),
+                            aggr.label.as_ref().map(|f| f.1),
+                            "label",
+                        );
+                        IgnoredDiagnosticOption::maybe_emit_warning(
+                            tcx,
+                            item_def_id,
+                            directive.condition.as_ref().map(|i| i.span),
+                            aggr.condition.as_ref().map(|i| i.span),
+                            "condition",
+                        );
+                        IgnoredDiagnosticOption::maybe_emit_warning(
+                            tcx,
+                            item_def_id,
+                            directive.parent_label.as_ref().map(|f| f.1),
+                            aggr.parent_label.as_ref().map(|f| f.1),
+                            "parent_label",
+                        );
+                        IgnoredDiagnosticOption::maybe_emit_warning(
+                            tcx,
+                            item_def_id,
+                            directive.append_const_msg.as_ref().and_then(|c| {
+                                if let AppendConstMessage::Custom(_, s) = c {
+                                    Some(*s)
+                                } else {
+                                    None
+                                }
+                            }),
+                            aggr.append_const_msg.as_ref().and_then(|c| {
+                                if let AppendConstMessage::Custom(_, s) = c {
+                                    Some(*s)
+                                } else {
+                                    None
+                                }
+                            }),
+                            "append_const_msg",
+                        );
+
                         Ok(Some(Self {
                             condition: aggr.condition.or(directive.condition),
                             subcommands,
@@ -556,6 +634,7 @@ impl<'tcx> OnUnimplementedDirective {
                         item_def_id,
                         value,
                         attr.span,
+                        attr.span,
                     )?),
                     notes: Vec::new(),
                     parent_label: None,
@@ -633,7 +712,11 @@ impl<'tcx> OnUnimplementedDirective {
                             // `with_no_visible_paths` is also used when generating the options,
                             // so we need to match it here.
                             ty::print::with_no_visible_paths!(
-                                OnUnimplementedFormatString(v).format(tcx, trait_ref, &options_map)
+                                OnUnimplementedFormatString(v, cfg.span).format(
+                                    tcx,
+                                    trait_ref,
+                                    &options_map
+                                )
                             )
                         });
 
@@ -678,8 +761,9 @@ impl<'tcx> OnUnimplementedFormatString {
         item_def_id: DefId,
         from: Symbol,
         err_sp: Span,
+        value_span: Span,
     ) -> Result<Self, ErrorGuaranteed> {
-        let result = OnUnimplementedFormatString(from);
+        let result = OnUnimplementedFormatString(from, value_span);
         result.verify(tcx, item_def_id, err_sp)?;
         Ok(result)
     }
diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs
@@ -2729,7 +2729,7 @@ impl<'tcx> InferCtxtPrivExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
                         Some(format!("{cannot_do_this} in const contexts"))
                     }
                     // overridden post message
-                    (true, Some(AppendConstMessage::Custom(custom_msg))) => {
+                    (true, Some(AppendConstMessage::Custom(custom_msg, _))) => {
                         Some(format!("{cannot_do_this}{custom_msg}"))
                     }
                     // fallback to generic message
diff --git a/tests/ui/diagnostic_namespace/on_unimplemented/ignore_unsupported_options_and_continue_to_use_fallback.rs b/tests/ui/diagnostic_namespace/on_unimplemented/ignore_unsupported_options_and_continue_to_use_fallback.rs
@@ -8,6 +8,8 @@
     note = "custom note"
 )]
 #[diagnostic::on_unimplemented(message = "fallback!!")]
+//~^ `message` is ignored due to previous definition of `message`
+//~| `message` is ignored due to previous definition of `message`
 #[diagnostic::on_unimplemented(label = "fallback label")]
 #[diagnostic::on_unimplemented(note = "fallback note")]
 trait Foo {}
diff --git a/tests/ui/diagnostic_namespace/on_unimplemented/ignore_unsupported_options_and_continue_to_use_fallback.stderr b/tests/ui/diagnostic_namespace/on_unimplemented/ignore_unsupported_options_and_continue_to_use_fallback.stderr
@@ -7,6 +7,15 @@ LL |     if(Self = "()"),
    = help: only `message`, `note` and `label` are allowed as options
    = note: `#[warn(unknown_or_malformed_diagnostic_attributes)]` on by default
 
+warning: `message` is ignored due to previous definition of `message`
+  --> $DIR/ignore_unsupported_options_and_continue_to_use_fallback.rs:10:32
+   |
+LL |     message = "custom message",
+   |     -------------------------- `message` is first declared here
+...
+LL | #[diagnostic::on_unimplemented(message = "fallback!!")]
+   |                                ^^^^^^^^^^^^^^^^^^^^^^ `message` is already declared here
+
 warning: malformed `on_unimplemented` attribute
   --> $DIR/ignore_unsupported_options_and_continue_to_use_fallback.rs:4:5
    |
@@ -16,8 +25,19 @@ LL |     if(Self = "()"),
    = help: only `message`, `note` and `label` are allowed as options
    = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
 
+warning: `message` is ignored due to previous definition of `message`
+  --> $DIR/ignore_unsupported_options_and_continue_to_use_fallback.rs:10:32
+   |
+LL |     message = "custom message",
+   |     -------------------------- `message` is first declared here
+...
+LL | #[diagnostic::on_unimplemented(message = "fallback!!")]
+   |                                ^^^^^^^^^^^^^^^^^^^^^^ `message` is already declared here
+   |
+   = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
+
 error[E0277]: custom message
-  --> $DIR/ignore_unsupported_options_and_continue_to_use_fallback.rs:18:15
+  --> $DIR/ignore_unsupported_options_and_continue_to_use_fallback.rs:20:15
    |
 LL |     takes_foo(());
    |     --------- ^^ fallback label
@@ -28,16 +48,16 @@ LL |     takes_foo(());
    = note: custom note
    = note: fallback note
 help: this trait has no implementations, consider adding one
-  --> $DIR/ignore_unsupported_options_and_continue_to_use_fallback.rs:13:1
+  --> $DIR/ignore_unsupported_options_and_continue_to_use_fallback.rs:15:1
    |
 LL | trait Foo {}
    | ^^^^^^^^^
 note: required by a bound in `takes_foo`
-  --> $DIR/ignore_unsupported_options_and_continue_to_use_fallback.rs:15:22
+  --> $DIR/ignore_unsupported_options_and_continue_to_use_fallback.rs:17:22
    |
 LL | fn takes_foo(_: impl Foo) {}
    |                      ^^^ required by this bound in `takes_foo`
 
-error: aborting due to previous error; 2 warnings emitted
+error: aborting due to previous error; 4 warnings emitted
 
 For more information about this error, try `rustc --explain E0277`.
diff --git a/tests/ui/diagnostic_namespace/on_unimplemented/report_warning_on_duplicated_options.rs b/tests/ui/diagnostic_namespace/on_unimplemented/report_warning_on_duplicated_options.rs
@@ -0,0 +1,25 @@
+#![feature(diagnostic_namespace)]
+
+#[diagnostic::on_unimplemented(
+    message = "first message",
+    label = "first label",
+    note = "custom note"
+)]
+#[diagnostic::on_unimplemented(
+    message = "second message",
+    //~^WARN `message` is ignored due to previous definition of `message`
+    //~|WARN `message` is ignored due to previous definition of `message`
+    label = "second label",
+    //~^WARN `label` is ignored due to previous definition of `label`
+    //~|WARN `label` is ignored due to previous definition of `label`
+    note = "second note"
+)]
+trait Foo {}
+
+
+fn takes_foo(_: impl Foo) {}
+
+fn main() {
+    takes_foo(());
+    //~^ERROR first message
+}
diff --git a/tests/ui/diagnostic_namespace/on_unimplemented/report_warning_on_duplicated_options.stderr b/tests/ui/diagnostic_namespace/on_unimplemented/report_warning_on_duplicated_options.stderr
@@ -0,0 +1,67 @@
+warning: `message` is ignored due to previous definition of `message`
+  --> $DIR/report_warning_on_duplicated_options.rs:9:5
+   |
+LL |     message = "first message",
+   |     ------------------------- `message` is first declared here
+...
+LL |     message = "second message",
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^ `message` is already declared here
+   |
+   = note: `#[warn(unknown_or_malformed_diagnostic_attributes)]` on by default
+
+warning: `label` is ignored due to previous definition of `label`
+  --> $DIR/report_warning_on_duplicated_options.rs:12:5
+   |
+LL |     label = "first label",
+   |     --------------------- `label` is first declared here
+...
+LL |     label = "second label",
+   |     ^^^^^^^^^^^^^^^^^^^^^^ `label` is already declared here
+
+warning: `message` is ignored due to previous definition of `message`
+  --> $DIR/report_warning_on_duplicated_options.rs:9:5
+   |
+LL |     message = "first message",
+   |     ------------------------- `message` is first declared here
+...
+LL |     message = "second message",
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^ `message` is already declared here
+   |
+   = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
+
+warning: `label` is ignored due to previous definition of `label`
+  --> $DIR/report_warning_on_duplicated_options.rs:12:5
+   |
+LL |     label = "first label",
+   |     --------------------- `label` is first declared here
+...
+LL |     label = "second label",
+   |     ^^^^^^^^^^^^^^^^^^^^^^ `label` is already declared here
+   |
+   = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
+
+error[E0277]: first message
+  --> $DIR/report_warning_on_duplicated_options.rs:23:15
+   |
+LL |     takes_foo(());
+   |     --------- ^^ first label
+   |     |
+   |     required by a bound introduced by this call
+   |
+   = help: the trait `Foo` is not implemented for `()`
+   = note: custom note
+   = note: second note
+help: this trait has no implementations, consider adding one
+  --> $DIR/report_warning_on_duplicated_options.rs:17:1
+   |
+LL | trait Foo {}
+   | ^^^^^^^^^
+note: required by a bound in `takes_foo`
+  --> $DIR/report_warning_on_duplicated_options.rs:20:22
+   |
+LL | fn takes_foo(_: impl Foo) {}
+   |                      ^^^ required by this bound in `takes_foo`
+
+error: aborting due to previous error; 4 warnings emitted
+
+For more information about this error, try `rustc --explain E0277`.

Original file line number	Diff line number	Diff line change
`@@ -2729,7 +2729,7 @@ impl<'tcx> InferCtxtPrivExt<'tcx> for TypeErrCtxt<'_, 'tcx> {`
`2729`	`2729`	`Some(format!("{cannot_do_this} in const contexts"))`
`2730`	`2730`	`}`
`2731`	`2731`	`// overridden post message`
`2732`		`- (true, Some(AppendConstMessage::Custom(custom_msg))) => {`
	`2732`	`+ (true, Some(AppendConstMessage::Custom(custom_msg, _))) => {`
`2733`	`2733`	`Some(format!("{cannot_do_this}{custom_msg}"))`
`2734`	`2734`	`}`
`2735`	`2735`	`// fallback to generic message`