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

[derive] Don't emit #[cfg(coverage_nightly)] #2123

Merged
merged 1 commit into from
Dec 3, 2024
Merged

[derive] Don't emit #[cfg(coverage_nightly)] #2123

merged 1 commit into from
Dec 3, 2024

Conversation

joshlf
Copy link
Member

@joshlf joshlf commented Dec 3, 2024

As of nightly-2024-11-20 - specifically [1] - this triggers an unexpected_cfgs lint even when emitted in derive-generated code.

[1] rust-lang/rust#132577

Fixes #2117

As of nightly-2024-11-20 - specifically [1] - this triggers an
`unexpected_cfgs` lint even when emitted in derive-generated code.

[1] rust-lang/rust#132577

Fixes #2117

gherrit-pr-id: I9d536f0ea08475afe0b65411b225aa55f4db449a
@joshlf joshlf requested a review from jswrenn December 3, 2024 15:35
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.42%. Comparing base (1cc24c0) to head (20f01c4).

Additional details and impacted files
@@           Coverage Diff           @@
##           v0.8.x    #2123   +/-   ##
=======================================
  Coverage   87.42%   87.42%           
=======================================
  Files          16       16           
  Lines        6115     6115           
=======================================
  Hits         5346     5346           
  Misses        769      769           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

joshlf added a commit that referenced this pull request Dec 3, 2024
As of nightly-2024-11-20 - specifically [1] - this triggers an
`unexpected_cfgs` lint even when emitted in derive-generated code.

[1] rust-lang/rust#132577

Fixes #2117

gherrit-pr-id: I9d536f0ea08475afe0b65411b225aa55f4db449a
@joshlf
Copy link
Member Author

joshlf commented Dec 3, 2024

Backporting to main in #2126

Merged via the queue into v0.8.x with commit 8211fbc Dec 3, 2024
87 checks passed
@joshlf joshlf deleted the fix-2117 branch December 3, 2024 16:06
github-merge-queue bot pushed a commit that referenced this pull request Dec 3, 2024
As of nightly-2024-11-20 - specifically [1] - this triggers an
`unexpected_cfgs` lint even when emitted in derive-generated code.

[1] rust-lang/rust#132577

Fixes #2117

gherrit-pr-id: I9d536f0ea08475afe0b65411b225aa55f4db449a
@taiki-e
Copy link

taiki-e commented Dec 6, 2024

AFAIK, this is the kind of cfg that is set globally on the coverage tool side, so the warning itself is not something we need to be concerned about except that it will break the build if we deny the warning.

Therefore, I wonder if it would be fine to simply allow unexpected_cfg in the generated code.
(See taiki-e/pin-project-lite#33 (comment) for why we believe that generated code should not be expected to be compatible with forbid except for certain lints.)

@joshlf
Copy link
Member Author

joshlf commented Dec 6, 2024

AFAIK, this is the kind of cfg that is set globally on the coverage tool side, so the warning itself is not something we need to be concerned about except that it will break the build if we deny the warning.

Therefore, I wonder if it would be fine to simply allow unexpected_cfg in the generated code. (See taiki-e/pin-project-lite#33 (comment) for why we believe that generated code should not be expected to be compatible with forbid except for certain lints.)

I tend to agree that this is bad CI hygiene. However, the benefit to us of using cfg(coverage_nightly) is nearly zero, so we decided to err on the side of unblocking users even if technically those users were assuming too much. But I do agree with your reasoning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants