`#[diagnostic::on_unimplemented]` without filters #114452

weiznich · 2023-08-04T07:54:27Z

This commit adds support for a #[diagnostic::on_unimplemented] attribute with the following options:

message to customize the primary error message
note to add a customized note message to an error message
label to customize the label part of the error message

The relevant behavior is specified in RFC-3368

rustbot · 2023-08-04T07:54:36Z

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

compiler/rustc_trait_selection/messages.ftl

tests/ui/diagnostic_namespace/on_unimplemented/do_not_fail_parsing_on_invalid_options_1.rs

tests/ui/diagnostic_namespace/on_unimplemented/feature-gate-diagnostic_on_unimplemented.rs

petrochenkov · 2023-08-07T04:47:05Z

compiler/rustc_lint_defs/src/builtin.rs

+    /// be available on newer compiler versions
+    pub MALFORMED_DIAGNOSTIC_ATTRIBUTES,
+    Warn,
+    "unrecognized diagnostic attribute options"


There are too many new lints for this feature, IMO.

Is it ever necessary to e.g. allow malformed diagnostic attributes, but warn on unknown?
I don't think so, this lints generally exists to warn on diagnostic attributes that are both known and well-formed, just on some other version of Rust compiler, so the distinction is not significant.

Also the previous lint was introduced as stable, but it should probably use a feature gate @feature_gate = sym::feature_gate_name; for now.

👍 Will change that to use the other lint. We might consider renaming the lint then.

For the @feature_gate change: Is it ok to put that into this PR as well or should that be opened as a separate PR?

I've pushed 9f51111 to address these concerns.

bors · 2023-08-15T09:13:29Z

☔ The latest upstream changes (presumably #114023) made this pull request unmergeable. Please resolve the merge conflicts.

apiraino · 2023-08-18T10:39:47Z

r? compiler

bors · 2023-08-24T13:39:02Z

☔ The latest upstream changes (presumably #115094) made this pull request unmergeable. Please resolve the merge conflicts.

compiler-errors · 2023-09-02T19:24:10Z

compiler/rustc_ast/src/attr/mod.rs

+                        .segments
+                        .iter()
+                        .zip(name)
+                        .all(|(s, n)| s.ident == Ident::with_dummy_span(*n))


This equality is weird, and relies on the fact that idents are equal only if their symbols are equal and their span contexts are identical:
https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_span/symbol.rs.html#1795-1800

If you're just trying to compare the symbols against name, then try

Suggested change

.all(|(s, n)| s.ident == Ident::with_dummy_span(*n))

.all(|(s, n)| s.ident.name == *n)

compiler/rustc_ast/src/attr/mod.rs

compiler-errors · 2023-09-02T19:26:26Z

compiler/rustc_hir_analysis/src/check/check.rs

+    let def_kind = tcx.def_kind(id.owner_id);
+    if !matches!(def_kind, DefKind::Trait) {
+        let item_def_id = id.owner_id.to_def_id();
+        if let Some(attr) = tcx
+            .get_attrs_by_path(item_def_id, Box::new([sym::diagnostic, sym::on_unimplemented]))
+            .next()
+        {
+            tcx.emit_spanned_lint(
+                UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES,
+                tcx.hir().local_def_id_to_hir_id(item_def_id.expect_local()),
+                vec![attr.span],
+                DiagnosticOnUnimplementedOnlyForTraits,
+            );
+        }
+    }


I don't think this check should live here. Attr validation better lives in compiler/rustc_passes/src/check_attr.rs or something like that.

compiler-errors · 2023-09-02T19:27:03Z

compiler/rustc_middle/src/ty/mod.rs

+    pub fn get_attrs_by_path(
+        self,
+        did: impl Into<DefId>,
+        attr: Box<[Symbol]>,


Why does this take a Box? 😅

Please use lifetimes here and make it &'a [Symbol] -- the allocation here is really unnecessary.

compiler-errors · 2023-09-02T19:27:26Z

compiler/rustc_middle/src/ty/mod.rs

@@ -2410,6 +2410,23 @@ impl<'tcx> TyCtxt<'tcx> {
        }
    }

+    pub fn get_attrs_by_path(
+        self,
+        did: impl Into<DefId>,


Why does this take an Into<DefId>? It's really general -- please just take a DefId, and make the caller call into_def_id() if needed.

compiler-errors · 2023-09-02T19:29:19Z

compiler/rustc_hir_analysis/src/check/check.rs

+            tcx.emit_spanned_lint(
+                UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES,
+                tcx.hir().local_def_id_to_hir_id(item_def_id.expect_local()),
+                vec![attr.span],


Span is Into<MultiSpan>

Suggested change

vec![attr.span],

attr.span,

compiler/rustc_middle/src/ty/mod.rs

compiler-errors · 2023-09-02T19:36:13Z

compiler/rustc_trait_selection/src/traits/error_reporting/on_unimplemented.rs

@@ -401,15 +410,30 @@ impl<'tcx> OnUnimplementedDirective {
                && message.is_none()
                && label.is_none()
                && note.is_none()
+                && !is_diagnostic_namespace_variant
+            // disallow filters for now


Suggested change

// disallow filters for now

// FIXME(diagnostic_namespace): disallow filters for now

compiler-errors · 2023-09-02T19:36:22Z

compiler/rustc_trait_selection/src/traits/error_reporting/on_unimplemented.rs

+                        is_diagnostic_namespace_variant,
+                    ) {
+                        Ok(Some(subcommand)) => subcommands.push(subcommand),
+                        Ok(None) => unreachable!(


Suggested change

Ok(None) => unreachable!(

Ok(None) => bug!(

compiler-errors · 2023-09-02T19:37:04Z

@rustbot author

weiznich · 2023-09-07T06:02:14Z

@compiler-errors Thanks for the review. These comments are really helpful. ❤️
I've pushed f5c87ba to address the comments.

@rustbot reviewer

compiler/rustc_middle/src/ty/mod.rs

compiler/rustc_hir_analysis/src/check/check.rs

compiler/rustc_passes/src/check_attr.rs

compiler-errors · 2023-09-07T07:03:34Z

compiler/rustc_trait_selection/src/traits/error_reporting/on_unimplemented.rs

+                tcx.emit_spanned_lint(
+                    UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES,
+                    tcx.hir().local_def_id_to_hir_id(item_def_id.expect_local()),
+                    vec![attr.span],


Please fix this here too.

compiler-errors · 2023-09-07T07:03:39Z

compiler/rustc_trait_selection/src/traits/error_reporting/on_unimplemented.rs

+            tcx.emit_spanned_lint(
+                UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES,
+                tcx.hir().local_def_id_to_hir_id(item_def_id.expect_local()),
+                vec![attr.span],


compiler-errors · 2023-09-07T07:04:42Z

Also please squash all of these "apply suggestions", "address suggestion", "update" commits. It's not super needed to retain them in git history, and I don't really review them individually. Thanks!

compiler-errors · 2023-09-07T07:06:39Z

compiler/rustc_passes/src/check_attr.rs

+        attr_span: Span,
+        hir_id: HirId,
+        target: Target,
+    ) -> bool {


I also mentioned making this function not return bool.

compiler-errors · 2023-09-07T07:06:47Z

compiler/rustc_passes/src/check_attr.rs

@@ -284,6 +292,23 @@ impl CheckAttrVisitor<'_> {
        }
    }

+    /// Checks if `#[diagnostic::on_unimplemented[` is applied to a trait definition


Suggested change

/// Checks if `#[diagnostic::on_unimplemented[` is applied to a trait definition

/// Checks if `#[diagnostic::on_unimplemented]` is applied to a trait definition

compiler-errors · 2023-09-07T07:10:20Z

compiler/rustc_ast/src/attr/mod.rs

@@ -99,6 +99,22 @@ impl Attribute {
        }
    }

+    pub fn is_path(&self, name: &[Symbol]) -> bool {


The name here is a bit weird, since it's inconsistent with is_word, which only allows #[word] and not #[word()]. Maybe let's name this path_matches or something?

This commit adds support for a `#[diagnostic::on_unimplemented]` attribute with the following options: * `message` to customize the primary error message * `note` to add a customized note message to an error message * `label` to customize the label part of the error message Co-authored-by: León Orell Valerian Liehr <[email protected]> Co-authored-by: Michael Goulet <[email protected]>

weiznich · 2023-09-13T14:16:51Z

@compiler-errors This PR is still marked as waiting-on-review. Is there anything that I can to do move it forward?

compiler-errors · 2023-09-13T14:18:51Z

@weiznich: please be patient, it's been less than 24 hours since you last updated it, as far as I can tell. I haven't had the time to review the changes, and probably will not for a few days.

weiznich · 2023-09-13T14:38:51Z

I'm mainly asking because you did not remove the waiting-on-review flag after your last review. Its unclear for me whether you've finished that one or if I should expect more comments from that review round.

compiler-errors · 2023-09-16T19:45:45Z

@bors r+

bors · 2023-09-16T19:45:47Z

📌 Commit 5b8a7a0 has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2023-09-17T10:00:18Z

⌛ Testing commit 5b8a7a0 with merge 327e6cf...

bors · 2023-09-17T11:45:45Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 327e6cf to master...

rust-timer · 2023-09-17T13:06:32Z

Finished benchmarking commit (327e6cf): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.6%	[0.4%, 0.9%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.9%	[4.9%, 4.9%]	1
Improvements ✅ (primary)	-2.1%	[-2.1%, -2.1%]	1
Improvements ✅ (secondary)	-3.5%	[-6.2%, -1.0%]	5
All ❌✅ (primary)	-2.1%	[-2.1%, -2.1%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 636.485s -> 634.734s (-0.28%)
Artifact size: 318.24 MiB -> 318.52 MiB (0.09%)

rustbot assigned TaKO8Ki Aug 4, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 4, 2023

fmease requested changes Aug 4, 2023

View reviewed changes

compiler/rustc_trait_selection/messages.ftl Outdated Show resolved Hide resolved

tests/ui/diagnostic_namespace/on_unimplemented/do_not_fail_parsing_on_invalid_options_1.rs Outdated Show resolved Hide resolved

weiznich commented Aug 4, 2023

View reviewed changes

tests/ui/diagnostic_namespace/on_unimplemented/feature-gate-diagnostic_on_unimplemented.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

weiznich force-pushed the feature/diagnostic_on_unimplemented branch from 8a50b1a to 8ffd5f6 Compare August 4, 2023 12:04

petrochenkov reviewed Aug 7, 2023

View reviewed changes

weiznich mentioned this pull request Aug 8, 2023

Improve CI tokio-rs/axum#2162

Merged

weiznich force-pushed the feature/diagnostic_on_unimplemented branch from c59ef01 to 9f51111 Compare August 10, 2023 05:41

weiznich force-pushed the feature/diagnostic_on_unimplemented branch from 9f51111 to adcaef1 Compare August 18, 2023 09:05

rustbot assigned compiler-errors and unassigned TaKO8Ki Aug 18, 2023

weiznich force-pushed the feature/diagnostic_on_unimplemented branch from adcaef1 to fe719f9 Compare August 25, 2023 06:54

compiler-errors requested changes Sep 2, 2023

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 2, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 7, 2023

compiler-errors reviewed Sep 7, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

compiler-errors reviewed Sep 7, 2023

View reviewed changes

weiznich force-pushed the feature/diagnostic_on_unimplemented branch 3 times, most recently from e0d4bc0 to 3538130 Compare September 12, 2023 17:40

This comment has been minimized.

Sign in to view

weiznich force-pushed the feature/diagnostic_on_unimplemented branch from 3538130 to 5b8a7a0 Compare September 12, 2023 18:03

compiler-errors approved these changes Sep 16, 2023

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 16, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 17, 2023

bors merged commit 327e6cf into rust-lang:master Sep 17, 2023
11 checks passed

rustbot added this to the 1.74.0 milestone Sep 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`#[diagnostic::on_unimplemented]` without filters #114452

`#[diagnostic::on_unimplemented]` without filters #114452

weiznich commented Aug 4, 2023 •

edited by tmandry

Loading

rustbot commented Aug 4, 2023

This comment has been minimized.

petrochenkov Aug 7, 2023

petrochenkov Aug 7, 2023

weiznich Aug 7, 2023

weiznich Aug 10, 2023 •

edited

Loading

bors commented Aug 15, 2023

apiraino commented Aug 18, 2023

bors commented Aug 24, 2023

compiler-errors Sep 2, 2023

compiler-errors Sep 2, 2023

compiler-errors Sep 2, 2023

compiler-errors Sep 2, 2023

compiler-errors Sep 2, 2023

compiler-errors Sep 2, 2023

compiler-errors Sep 2, 2023

compiler-errors commented Sep 2, 2023

weiznich commented Sep 7, 2023

compiler-errors Sep 7, 2023

compiler-errors Sep 7, 2023

This comment has been minimized.

compiler-errors commented Sep 7, 2023

compiler-errors Sep 7, 2023

compiler-errors Sep 7, 2023

compiler-errors Sep 7, 2023 •

edited

Loading

This comment has been minimized.

weiznich commented Sep 13, 2023

compiler-errors commented Sep 13, 2023

weiznich commented Sep 13, 2023

compiler-errors commented Sep 16, 2023

bors commented Sep 16, 2023

bors commented Sep 17, 2023

bors commented Sep 17, 2023

rust-timer commented Sep 17, 2023

	.all(\|(s, n)\| s.ident == Ident::with_dummy_span(*n))
	.all(\|(s, n)\| s.ident.name == *n)

	// disallow filters for now
	// FIXME(diagnostic_namespace): disallow filters for now

	/// Checks if `#[diagnostic::on_unimplemented[` is applied to a trait definition
	/// Checks if `#[diagnostic::on_unimplemented]` is applied to a trait definition

#[diagnostic::on_unimplemented] without filters #114452

#[diagnostic::on_unimplemented] without filters #114452

Conversation

weiznich commented Aug 4, 2023 • edited by tmandry Loading

rustbot commented Aug 4, 2023

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiznich Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

bors commented Aug 15, 2023

apiraino commented Aug 18, 2023

bors commented Aug 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented Sep 2, 2023

weiznich commented Sep 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

compiler-errors commented Sep 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

weiznich commented Sep 13, 2023

compiler-errors commented Sep 13, 2023

weiznich commented Sep 13, 2023

compiler-errors commented Sep 16, 2023

bors commented Sep 16, 2023

bors commented Sep 17, 2023

bors commented Sep 17, 2023

rust-timer commented Sep 17, 2023

Overall result: ❌ regressions - no action needed

`#[diagnostic::on_unimplemented]` without filters #114452

`#[diagnostic::on_unimplemented]` without filters #114452

weiznich commented Aug 4, 2023 •

edited by tmandry

Loading

weiznich Aug 10, 2023 •

edited

Loading

compiler-errors Sep 7, 2023 •

edited

Loading