Update `#[coverage(..)]` attribute error messages to match the current implementation by Zalathar · Pull Request #134750 · rust-lang/rust

Zalathar · 2024-12-25T05:22:18Z

The allowed positions for #[coverage(..)] attributes were expanded by #126721, but the corresponding error messages were never updated to reflect the new behaviour.

Part of #134749.

Some of these cases are also implicitly checked by other tests, but it's helpful to also explicitly list them in the main test.

rustbot · 2024-12-25T05:22:26Z

r? @compiler-errors

rustbot has assigned @compiler-errors.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Zalathar · 2024-12-25T05:26:03Z

compiler/rustc_passes/src/errors.rs

+/// "coverage attribute not allowed here"
 #[derive(Diagnostic)]
-#[diag(passes_coverage_not_fn_or_closure, code = E0788)]
-pub(crate) struct CoverageNotFnOrClosure {
+#[diag(passes_coverage_attribute_not_allowed, code = E0788)]
+pub(crate) struct CoverageAttributeNotAllowed {
    #[primary_span]
    pub attr_span: Span,
-    #[label]
-    pub defn_span: Span,
+    /// "not a function, impl block, or module"
+    #[label(passes_not_fn_impl_mod)]
+    pub not_fn_impl_mod: Option<Span>,
+    /// "function has no body"
+    #[label(passes_no_body)]
+    pub no_body: Option<Span>,
+    /// "coverage attribute can be applied to a function (with body), impl block, or module"
+    #[help]
+    pub help: (),
 }


Aside: #[derive(Diagnostic)] is one of the most egregiously unpleasant compiler-internal APIs I've encountered. It's awful.

cf. #132181

jieyouxu

Thanks, I have a few questions, but these error messages do indeed look much more accurate than it was previously.

jieyouxu · 2024-12-25T06:42:13Z

compiler/rustc_passes/messages.ftl

+    .not_fn_impl_mod = not a function, impl block, or module
+    .no_body = function has no body
+    .help = coverage attribute can be applied to a function (with body), impl block, or module


Question: does this specific wording account for this particular case:

// In situations where attributes can already be applied to expressions, // the coverage attribute is allowed on closure expressions. let _closure_tail_expr = { #[coverage(off)] || () };

My intention here is to make the error message less verbose by implicitly treating closure expressions as a kind of “function”, whereas the error code documentation has the luxury of listing closures separately.

Is this a good idea? I'm not sure; I don't have much experience with user-facing error messages.

I see what you mean. Should we perhaps say something like

help: coverage attribute can be applied to a function with body, impl block, module, and a few other kinds of attachees; see error code docs for more details

But yeah, I agree that it's not very helpful to exhaustively list them either.

I don't think this needs to be perfect (in this PR anyway).

jieyouxu · 2024-12-25T06:44:22Z

compiler/rustc_error_codes/src/error_codes/E0788.md

+The coverage attribute can be applied to:
+- Function and method declarations that have a body, including trait methods
+  that have a default implementation.
+- Closure expressions, in situations where attributes can be applied to
+  expressions.
+- `impl` blocks (inherent or trait), and modules.


Question: I know this is very verbose for an error message, but cf. previous commit's error messages like "not a function, impl block, or module" or "coverage attribute can be applied to a function (with body), impl block, or module", the docs here doesn't quite correspond to those messages, right?

Anyway, not a huge deal.

jieyouxu · 2024-12-25T06:45:08Z

compiler/rustc_error_codes/src/error_codes/E0788.md

-If you wish to apply this attribute to all methods in an impl or module,
-manually annotate each method; it is not possible to annotate the entire impl
-with a `#[coverage]` attribute.
+The coverage attribute is a hint to the `-C instrument-coverage` flag to


Nit: maybe also emphasis on "hint"? I.e. hint to the compiler.

Zalathar · 2024-12-25T08:33:41Z

Tweaked the wording of the error code docs (diff).

I think the other stuff is good enough to merge as a net improvement over the current docs; if someone has strong editorial opinions they can raise them in a follow-up thread or PR.

@rustbot ready

jieyouxu

Thanks! The wording can always be adjusted in a follow-up, but the improvements are already nice.

jieyouxu · 2024-12-25T08:37:30Z

tests/ui/coverage-attr/allowed-positions.rs

@@ -0,0 +1,116 @@
+//! Tests where the `#[coverage(..)]` attribute can and cannot be used.
+
+//@ reference: attributes.coverage.allowed-positions


cc @rust-lang/spec: should compiler reviewers ping you if a reference annotated test has expanded test coverage or modified test coverage?

Actually don't answer that (sorry), I'll add that as a question for the T-spec testing RFC.

jieyouxu · 2024-12-25T08:37:55Z

compiler/rustc_passes/messages.ftl

+    .not_fn_impl_mod = not a function, impl block, or module
+    .no_body = function has no body
+    .help = coverage attribute can be applied to a function (with body), impl block, or module


I don't think this needs to be perfect (in this PR anyway).

jieyouxu · 2024-12-25T08:39:14Z

@bors r+ rollup

bors · 2024-12-25T08:39:17Z

📌 Commit e48fc62 has been approved by jieyouxu

It is now in the queue for this repository.

Rollup of 3 pull requests Successful merges: - rust-lang#134743 (Default to short backtraces for dev builds of rustc itself) - rust-lang#134750 (Update `#[coverage(..)]` attribute error messages to match the current implementation) - rust-lang#134751 (Enable LSX feature for LoongArch OpenHarmony target) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#134750 - Zalathar:coverage-attr-errors, r=jieyouxu Update `#[coverage(..)]` attribute error messages to match the current implementation The allowed positions for `#[coverage(..)]` attributes were expanded by rust-lang#126721, but the corresponding error messages were never updated to reflect the new behaviour. Part of rust-lang#134749.

Zalathar added 4 commits December 25, 2024 11:40

Rename tests/ui/coverage-attr/no-coverage.rs to allowed-positions.rs

d4005b6

Fully redact the [E0788] error message in tests, to make changes easier

5e98118

Expand the main test for where the coverage attribute is allowed

9124662

Some of these cases are also implicitly checked by other tests, but it's helpful to also explicitly list them in the main test.

Overhaul error messages for disallowed coverage attributes

3996209

Zalathar added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Dec 25, 2024

rustbot assigned compiler-errors Dec 25, 2024

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 Dec 25, 2024

Zalathar commented Dec 25, 2024

View reviewed changes

Zalathar mentioned this pull request Dec 25, 2024

Tracking issue for stabilizing the #[coverage(..)] attribute #134749

Open

6 tasks

jieyouxu reviewed Dec 25, 2024

View reviewed changes

jieyouxu assigned jieyouxu and unassigned compiler-errors Dec 25, 2024

jieyouxu 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 Dec 25, 2024

Zalathar added 2 commits December 25, 2024 19:23

Rewrite the error-code docs for coverage attributes [E0788]

db02b1d

Un-redact one occurrence of "coverage attribute not allowed here"

e48fc62

Zalathar force-pushed the coverage-attr-errors branch from 94fa5b2 to e48fc62 Compare December 25, 2024 08:24

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 Dec 25, 2024

jieyouxu approved these changes Dec 25, 2024

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 Dec 25, 2024

jieyouxu mentioned this pull request Dec 25, 2024

Rollup of 3 pull requests #134756

Merged

bors merged commit db3404a into rust-lang:master Dec 25, 2024
6 checks passed

rustbot added this to the 1.85.0 milestone Dec 25, 2024

Zalathar deleted the coverage-attr-errors branch December 25, 2024 23:50

		@@ -0,0 +1,116 @@
		//! Tests where the `#[coverage(..)]` attribute can and cannot be used.

		//@ reference: attributes.coverage.allowed-positions

Uh oh!

Conversation

Zalathar commented Dec 25, 2024

Uh oh!

rustbot commented Dec 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zalathar commented Dec 25, 2024

Uh oh!

jieyouxu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu commented Dec 25, 2024

Uh oh!

bors commented Dec 25, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jieyouxu left a comment •

edited

Loading