Skip to content

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Jun 19, 2024

Follow-up to #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 #126658.

@rustbot
Copy link
Collaborator

rustbot commented Jun 19, 2024

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
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

@rustbot 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. A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) labels Jun 19, 2024
@Zalathar Zalathar force-pushed the test-coverage-attr branch from 4870b7d to 7b31692 Compare June 19, 2024 10:50
@Zalathar Zalathar force-pushed the test-coverage-attr branch from 7b31692 to 71fc321 Compare June 20, 2024 00:31
@Zalathar Zalathar force-pushed the test-coverage-attr branch from 71fc321 to 425cded Compare June 20, 2024 07:05
@Zalathar Zalathar force-pushed the test-coverage-attr branch from 425cded to ebb3aa0 Compare June 20, 2024 07:12
@cjgillot
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Jun 20, 2024

📌 Commit ebb3aa0 has been approved by cjgillot

It is now in the queue for this repository.

@bors 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 Jun 20, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 20, 2024
…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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 20, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#126380 (Add std Xtensa targets support)
 - rust-lang#126636 (Resolve Clippy `f16` and `f128` `unimplemented!`/`FIXME`s )
 - rust-lang#126659 (More status-quo tests for the `#[coverage(..)]` attribute)
 - rust-lang#126711 (Make Option::as_[mut_]slice const)
 - rust-lang#126717 (Clean up some comments near `use` declarations)
 - rust-lang#126719 (Fix assertion failure for some `Expect` diagnostics.)
 - rust-lang#126730 (Add opaque type corner case test)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9cbfbda into rust-lang:master Jun 20, 2024
@rustbot rustbot added this to the 1.81.0 milestone Jun 20, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 20, 2024
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.
@Zalathar Zalathar deleted the test-coverage-attr branch June 20, 2024 23:14
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 24, 2024
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
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 24, 2024
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
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants