Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update #[coverage(..)] attribute error messages to match the current implementation #134750

Merged
merged 6 commits into from
Dec 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 17 additions & 19 deletions compiler/rustc_error_codes/src/error_codes/E0788.md
Original file line number Diff line number Diff line change
@@ -1,26 +1,24 @@
A `#[coverage]` attribute was applied to something which does not show up
in code coverage, or is too granular to be excluded from the coverage report.
A `#[coverage(off|on)]` attribute was found in a position where it is not
allowed.

For now, this attribute can only be applied to function, method, and closure
definitions. In the future, it may be added to statements, blocks, and
expressions, and for the time being, using this attribute in those places
will just emit an `unused_attributes` lint instead of this error.
Coverage attributes 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.

Example of erroneous code:

```compile_fail,E0788
#[coverage(off)]
struct Foo;

#[coverage(on)]
const FOO: Foo = Foo;
unsafe extern "C" {
#[coverage(off)]
fn foreign_fn();
}
```

`#[coverage(off)]` tells the compiler to not generate coverage instrumentation
for a piece of code when the `-C instrument-coverage` flag is passed. Things
like structs and consts are not coverable code, and thus cannot do anything
with this attribute.

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.
When using the `-C instrument-coverage` flag, coverage attributes act as a
hint to the compiler that it should instrument or not instrument the
corresponding function or enclosed functions. The precise effect of applying
a coverage attribute is not guaranteed and may change in future compiler
versions.
8 changes: 5 additions & 3 deletions compiler/rustc_passes/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,11 @@ passes_coroutine_on_non_closure =
attribute should be applied to closures
.label = not a closure
passes_coverage_not_fn_or_closure =
attribute should be applied to a function definition or closure
.label = not a function or closure
passes_coverage_attribute_not_allowed =
coverage attribute not allowed here
.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
Comment on lines +117 to +119
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)]
    || ()
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

passes_dead_codes =
{ $multiple ->
Expand Down
25 changes: 19 additions & 6 deletions compiler/rustc_passes/src/check_attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -432,21 +432,34 @@ impl<'tcx> CheckAttrVisitor<'tcx> {

/// Checks that `#[coverage(..)]` is applied to a function/closure/method,
/// or to an impl block or module.
fn check_coverage(&self, attr: &Attribute, span: Span, target: Target) {
fn check_coverage(&self, attr: &Attribute, target_span: Span, target: Target) {
let mut not_fn_impl_mod = None;
let mut no_body = None;

match target {
Target::Fn
| Target::Closure
| Target::Method(MethodKind::Trait { body: true } | MethodKind::Inherent)
| Target::Impl
| Target::Mod => {}
| Target::Mod => return,

// These are "functions", but they aren't allowed because they don't
// have a body, so the usual explanation would be confusing.
Target::Method(MethodKind::Trait { body: false }) | Target::ForeignFn => {
no_body = Some(target_span);
}

_ => {
self.dcx().emit_err(errors::CoverageNotFnOrClosure {
attr_span: attr.span,
defn_span: span,
});
not_fn_impl_mod = Some(target_span);
}
}

self.dcx().emit_err(errors::CoverageAttributeNotAllowed {
attr_span: attr.span,
not_fn_impl_mod,
no_body,
help: (),
});
}

/// Checks that `#[optimize(..)]` is applied to a function/closure/method,
Expand Down
16 changes: 12 additions & 4 deletions compiler/rustc_passes/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,21 @@ pub(crate) struct InlineNotFnOrClosure {
pub defn_span: Span,
}

/// "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: (),
}
Comment on lines +74 to 89
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cf. #132181


#[derive(Diagnostic)]
Expand Down
116 changes: 116 additions & 0 deletions tests/ui/coverage-attr/allowed-positions.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
//! Tests where the `#[coverage(..)]` attribute can and cannot be used.

//@ reference: attributes.coverage.allowed-positions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


#![feature(coverage_attribute)]
#![feature(extern_types)]
#![feature(impl_trait_in_assoc_type)]
#![warn(unused_attributes)]
#![coverage(off)]

#[coverage(off)]
mod submod {}

#[coverage(off)] //~ ERROR coverage attribute not allowed here [E0788]
type MyTypeAlias = ();

#[coverage(off)] //~ ERROR [E0788]
trait MyTrait {
#[coverage(off)] //~ ERROR [E0788]
const TRAIT_ASSOC_CONST: u32;

#[coverage(off)] //~ ERROR [E0788]
type TraitAssocType;

#[coverage(off)] //~ ERROR [E0788]
fn trait_method(&self);

#[coverage(off)]
fn trait_method_with_default(&self) {}

#[coverage(off)] //~ ERROR [E0788]
fn trait_assoc_fn();
}

#[coverage(off)]
impl MyTrait for () {
const TRAIT_ASSOC_CONST: u32 = 0;

#[coverage(off)] //~ ERROR [E0788]
type TraitAssocType = Self;

#[coverage(off)]
fn trait_method(&self) {}
#[coverage(off)]
fn trait_method_with_default(&self) {}
#[coverage(off)]
fn trait_assoc_fn() {}
}

trait HasAssocType {
type T;
fn constrain_assoc_type() -> Self::T;
}

impl HasAssocType for () {
#[coverage(off)] //~ ERROR [E0788]
type T = impl Copy;
fn constrain_assoc_type() -> Self::T {}
}

#[coverage(off)] //~ ERROR [E0788]
struct MyStruct {
#[coverage(off)] //~ ERROR [E0788]
field: u32,
}

#[coverage(off)]
impl MyStruct {
#[coverage(off)]
fn method(&self) {}
#[coverage(off)]
fn assoc_fn() {}
}

extern "C" {
#[coverage(off)] //~ ERROR [E0788]
static X: u32;

#[coverage(off)] //~ ERROR [E0788]
type T;

#[coverage(off)] //~ ERROR [E0788]
fn foreign_fn();
}

#[coverage(off)]
fn main() {
#[coverage(off)] //~ ERROR [E0788]
let _ = ();

// Currently not allowed on let statements, even if they bind to a closure.
// It might be nice to support this as a special case someday, but trying
// to define the precise boundaries of that special case might be tricky.
#[coverage(off)] //~ ERROR [E0788]
let _let_closure = || ();

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

// Applying attributes to arbitrary expressions requires an unstable
// feature, but if that feature were enabled then this would be allowed.
let _closure_expr = #[coverage(off)] || ();
//~^ ERROR attributes on expressions are experimental [E0658]

match () {
#[coverage(off)] //~ ERROR [E0788]
() => (),
}

#[coverage(off)] //~ ERROR [E0788]
return ();
}
Loading
Loading