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.

Sign up for GitHub

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

Jump to bottom

Inconsistent handling of #[coverage(..)] attributes that are malformed or misplaced #126658

Closed
Zalathar opened this issue Jun 19, 2024 · 1 comment · Fixed by #126682
Closed

Inconsistent handling of #[coverage(..)] attributes that are malformed or misplaced #126658

Zalathar opened this issue Jun 19, 2024 · 1 comment · Fixed by #126682
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. D-confusing Diagnostics: Confusing error or lint that should be reworked. D-inconsistent Diagnostics: Inconsistency in formatting, grammar or style between diagnostic messages. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Zalathar
Copy link
Contributor

Zalathar commented Jun 19, 2024 •
edited
Loading

(Note: The coverage attribute is currently unstable; see #84605.)

While writing tests for #[coverage(..)] for #84605, I encountered various inconsistencies in diagnostics for using the attribute with incorrect syntax or on inappropriate items.

For example:

  • Bare #[coverage] is silently permitted in various situations, even though it should never be legal.
    • E.g. on modules, on impl blocks, on trait, and on impl Trait.
  • Using name-value syntax #[coverage = "off"] results in different error messages than other kinds of incorrect syntax, and sometimes results in multiple errors for the same attribute.
    • The error message also claims that bare #[coverage] is permitted, which is incorrect.
  • Using #[coverage(off)] or #[coverage(on)] on things other than functions/methods/closures sometimes results in an error, and sometimes only a warning.
    • It seems to be an error on inappropriate items, but only a warning on (non-closure) expressions, even though it would have no effect in either case.
  • There is no diagnostic for attaching multiple otherwise legal coverage annotations to a single item, even though it's unclear which one would prevail.
@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 19, 2024
Zalathar added a commit to Zalathar/rust that referenced this issue Jun 19, 2024
@Zalathar
More status-quo tests for the #[coverage(..)] attribute
a0d4c4d 
These tests reveal some inconsistencies that are tracked by
<rust-lang#126658>.
Zalathar added a commit to Zalathar/rust that referenced this issue Jun 19, 2024
@Zalathar
More status-quo tests for the #[coverage(..)] attribute
bd3323d 
These tests reveal some inconsistencies that are tracked by
<rust-lang#126658>.
@Zalathar Zalathar mentioned this issue Jun 19, 2024
Open
Zalathar added a commit to Zalathar/rust that referenced this issue Jun 19, 2024
@Zalathar
More status-quo tests for the #[coverage(..)] attribute
9851445 
These tests reveal some inconsistencies that are tracked by
<rust-lang#126658>.
@Zalathar Zalathar mentioned this issue Jun 19, 2024
Merged
@veera-sivarajan
Copy link
Contributor

@rustbot label -needs-triage +A-diagnostics +D-inconsistent +D-confusing

@rustbot rustbot added A-diagnostics Area: Messages for errors, warnings, and lints D-confusing Diagnostics: Confusing error or lint that should be reworked. D-inconsistent Diagnostics: Inconsistency in formatting, grammar or style between diagnostic messages. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jun 19, 2024
Zalathar added a commit to Zalathar/rust that referenced this issue Jun 20, 2024
@Zalathar
More status-quo tests for the #[coverage(..)] attribute
7cc3cf9 
These tests reveal some inconsistencies that are tracked by
<rust-lang#126658>.
Zalathar added a commit to Zalathar/rust that referenced this issue Jun 20, 2024
@Zalathar
More status-quo tests for the #[coverage(..)] attribute
8d766c2 
These tests reveal some inconsistencies that are tracked by
<rust-lang#126658>.
Zalathar added a commit to Zalathar/rust that referenced this issue Jun 20, 2024
@Zalathar
More status-quo tests for the #[coverage(..)] attribute
388aea4 
These tests reveal some inconsistencies that are tracked by
<rust-lang#126658>.
@Zalathar Zalathar mentioned this issue Jun 20, 2024
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 20, 2024
@matthiaskrgr
Rollup merge of rust-lang#126659 - Zalathar:test-coverage-attr, r=cjg…
f5ba4d5 
…illot

More status-quo tests for the `#[coverage(..)]` attribute

Follow-up to rust-lang#126621, after I found even more weird corner-cases in the handling of the coverage attribute.

These tests reveal some inconsistencies that are tracked by rust-lang#126658.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 20, 2024
@matthiaskrgr
Rollup merge of rust-lang#126659 - Zalathar:test-coverage-attr, r=cjg…
9cbfbda 
…illot

More status-quo tests for the `#[coverage(..)]` attribute

Follow-up to rust-lang#126621, after I found even more weird corner-cases in the handling of the coverage attribute.

These tests reveal some inconsistencies that are tracked by rust-lang#126658.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jun 20, 2024
@rust-timer
Unrolled build for rust-lang#126659
8ddba56 
Rollup merge of rust-lang#126659 - Zalathar:test-coverage-attr, r=cjgillot

More status-quo tests for the `#[coverage(..)]` attribute

Follow-up to rust-lang#126621, after I found even more weird corner-cases in the handling of the coverage attribute.

These tests reveal some inconsistencies that are tracked by rust-lang#126658.
@bors bors closed this as completed in 9ce2a07 Jun 24, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jun 24, 2024
@rust-timer
Unrolled build for rust-lang#126682
aafafa9 
Rollup merge of rust-lang#126682 - Zalathar:coverage-attr, r=lcnr

coverage: Overhaul validation of the `#[coverage(..)]` attribute

This PR makes sweeping changes to how the (currently-unstable) coverage attribute is validated:
- Multiple coverage attributes on the same item/expression are now treated as an error.
- The attribute must always be `#[coverage(off)]` or `#[coverage(on)]`, and the error messages for this are more consistent.
  -  A trailing comma is still allowed after off/on, since that's part of the normal attribute syntax.
- Some places that silently ignored a coverage attribute now produce an error instead.
  - These cases were all clearly bugs.
- Some places that ignored a coverage attribute (with a warning) now produce an error instead.
  - These were originally added as lints, but I don't think it makes much sense to knowingly allow new attributes to be used in meaningless places.
  - Some of these errors might soon disappear, if it's easy to extend recursive coverage attributes to things like modules and impl blocks.

---

One of the goals of this PR is to lay a more solid foundation for making the coverage attribute recursive, so that it applies to all nested functions/closures instead of just the one it is directly attached to.

Fixes rust-lang#126658.

This PR incorporates rust-lang#126659, which adds more tests for validation of the coverage attribute.

`@rustbot` label +A-code-coverage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. D-confusing Diagnostics: Confusing error or lint that should be reworked. D-inconsistent Diagnostics: Inconsistency in formatting, grammar or style between diagnostic messages. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

coverage: Overhaul validation of the #[coverage(..)] attribute Zalathar/rust
3 participants
@Zalathar @veera-sivarajan @rustbot