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

Silence private_bounds lint in KnownLayout derive #2182

Merged
merged 1 commit into from
Dec 23, 2024

Conversation

jswrenn
Copy link
Collaborator

@jswrenn jswrenn commented Dec 23, 2024

For reasons not-yet-known, the private_bounds lint flags the output of derive(KnownLayout) on repr(C) structs generated by macros. This is likely a rustc bug, but to preserve a good experience for our users, we nonetheless, allow(private_bounds).

Fixes #2177

@codecov-commenter
Copy link

codecov-commenter commented Dec 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.42%. Comparing base (2c8ef74) to head (83e68d3).

Additional details and impacted files
@@           Coverage Diff           @@
##           v0.8.x    #2182   +/-   ##
=======================================
  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.

For reasons not-yet-known, the `private_bounds` lint flags the
output of `derive(KnownLayout)` on `repr(C)` structs generated
by macros. This is likely a rustc bug, but to preserve a good
experience for our users, we nonetheless, `allow(private_bounds)`.

Fixes #2177
@jswrenn jswrenn force-pushed the allow-private-bounds branch from 6c3825e to 83e68d3 Compare December 23, 2024 15:48
@joshlf
Copy link
Member

joshlf commented Dec 23, 2024

Can we add a test for this?

@jswrenn
Copy link
Collaborator Author

jswrenn commented Dec 23, 2024

Hm. It's not straightforward to add it as a trybuild test, because trybuild expects compilation failures (which we avoid by allowing the lint in derive(KnownLayout)). It's not straightforward to add it as a regular integration test, since those tests don't fail on warnings.

And we can't meaningfully pull shenanigans with forbid(private_bounds), because rustc errors out early as soon as it sees the allow(private_bounds), rather than upon the actual triggering of the lint. force-warn might do the trick, but it doesn't exist in attribute form, and trybuild doesn't have a ui-test-style way of specifying per-file rustflags.

My preference would be to merge this PR and cut a release sooner, rather than later, then put it on our roadmap to seek a general solution (or, at least, policy) for the problem of "our consumers might deny lints that trigger in certain invocations of our derives`.

Copy link
Member

@joshlf joshlf left a comment

Choose a reason for hiding this comment

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

Approved, but let's file an issue to track either fixing the rustc bug or adding a test to prevent regressions.

@jswrenn jswrenn added this pull request to the merge queue Dec 23, 2024
Merged via the queue into v0.8.x with commit 0fa779e Dec 23, 2024
87 checks passed
@jswrenn jswrenn deleted the allow-private-bounds branch December 23, 2024 20:18
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.

3 participants