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

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.
2024-11-22 14:55:26 +00:00 · 2023-11-18 02:57:09 +00:00 · 2023-11-18 02:57:09 +00:00 · 547ace8051
commit 547ace8051
parent 1a740c3816 10538d4d2b
7 changed files with 219 additions and 17 deletions
--- 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]`
--- 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)
    }
--- 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
--- 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 {}
--- 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`.
--- 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
+}
--- 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`.