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

Fix incorrect clashing_extern_declarations warnings. #73990

Merged
merged 6 commits into from
Jul 30, 2020
Merged

Fix incorrect clashing_extern_declarations warnings. #73990

merged 6 commits into from
Jul 30, 2020

Conversation

jumbatm
Copy link
Contributor

@jumbatm jumbatm commented Jul 3, 2020 •
edited
Loading

Fixes #73735, fixes #73872.

Fix clashing_extern_declarations warning for #[repr(transparent)] structs and safely-FFI-convertible enums, and not warning for clashes of struct members of different types, but the same size.

r? @nagisa

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 3, 2020
@jumbatm jumbatm force-pushed the clashing-extern-decl branch 2 times, most recently from ee6f54b to b9efacf Compare July 3, 2020 12:43
teymour-aldridge
teymour-aldridge reviewed Jul 3, 2020
View reviewed changes
src/librustc_lint/builtin.rs Outdated Show resolved Hide resolved
src/librustc_lint/builtin.rs Outdated Show resolved Hide resolved
src/librustc_lint/types.rs Outdated Show resolved Hide resolved
src/librustc_lint/types.rs Outdated Show resolved Hide resolved
src/test/ui/lint/clashing-extern-fn.rs Outdated Show resolved Hide resolved
src/test/ui/lint/clashing-extern-fn.rs Outdated Show resolved Hide resolved
nagisa
nagisa reviewed Jul 4, 2020
View reviewed changes
src/librustc_lint/builtin.rs Outdated Show resolved Hide resolved
src/librustc_lint/builtin.rs Outdated Show resolved Hide resolved
src/librustc_lint/builtin.rs Outdated Show resolved Hide resolved
src/librustc_lint/builtin.rs Outdated Show resolved Hide resolved
src/librustc_lint/builtin.rs Outdated Show resolved Hide resolved
src/librustc_lint/builtin.rs Outdated Show resolved Hide resolved
src/librustc_lint/builtin.rs Outdated Show resolved Hide resolved
@jumbatm jumbatm force-pushed the clashing-extern-decl branch 3 times, most recently from d22ea6a to 7bba12b Compare July 4, 2020 07:32
nagisa
nagisa reviewed Jul 4, 2020
View reviewed changes
src/librustc_lint/types.rs Outdated Show resolved Hide resolved
nagisa
nagisa reviewed Jul 4, 2020
View reviewed changes
src/librustc_lint/types.rs Outdated Show resolved Hide resolved
@jumbatm
Copy link
Contributor Author

jumbatm commented Jul 5, 2020

I'm still working on getting is_repr_nullable_ptr returning the type that's FFI-safe to use, but in the meantime, I've got a working (but quite awkward) solution where the caller optionally passes a type to check against.

@jumbatm jumbatm force-pushed the clashing-extern-decl branch 2 times, most recently from 136fe20 to f84b951 Compare July 7, 2020 13:41
@jumbatm
Copy link
Contributor Author

jumbatm commented Jul 7, 2020

Alright, this good for re-review -- I have repr_nullable_ptr returning the corresponding FFI-safe type as I mentioned earlier.

nagisa
nagisa reviewed Jul 11, 2020
View reviewed changes
src/librustc_lint/types.rs Outdated Show resolved Hide resolved
nagisa
nagisa reviewed Jul 11, 2020
View reviewed changes
src/librustc_lint/types.rs Outdated Show resolved Hide resolved
@nagisa
Copy link
Member

nagisa commented Jul 11, 2020

Broadly lgtm, left 2 minor comments, r=me once resolved.

@jumbatm
Copy link
Contributor Author

jumbatm commented Jul 14, 2020 •
edited
Loading

The changes have been applied. This is good to go.

e: Oops, had some unstaged formatting changes. This will be good to go once CI passes.

@jumbatm
Copy link
Contributor Author

jumbatm commented Jul 14, 2020

Note that I've also added a few additional commits; they fix an assertion failure and false negative I noticed while working on this.

@bors
Copy link
Contributor

bors commented Jul 14, 2020

☔ The latest upstream changes (presumably #74342) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Jul 18, 2020

☔ The latest upstream changes (presumably #74468) made this pull request unmergeable. Please resolve the merge conflicts.

jumbatm and others added 6 commits July 30, 2020 21:59
@jumbatm
Add additional clashing_extern_decl cases.
4da72f5
@jumbatm
Don't emit clashing decl lint for FFI-safe enums.
3eaead7 
An example of an FFI-safe enum conversion is when converting
Option<NonZeroUsize> to usize. Because the Some value must be non-zero,
rustc can use 0 to represent the None variant, making this conversion is
safe. Furthermore, it can be relied on (and removing this optimisation
already would be a breaking change).
@jumbatm @teymour-aldridge
Apply suggested wording changes from code review.
5e52edc 
Co-authored-by: Teymour Aldridge <[email protected]>
@jumbatm
Address code review comments.
060666d 
- Make `is_repr_nullable_ptr` freestanding again to avoid usage of
ImproperCTypesVisitor in ClashingExternDeclarations (and don't
accidentally revert the ParamEnv::reveal_all() fix from a week earlier)
- Revise match condition for 1 Adt, 1 primitive
- Generalise check for non-null type so that it would also work for
ranges which exclude any single value (all bits set, for example)
- Make is_repr_nullable_ptr return the representable type instead of
just a boolean, to avoid adding an additional, independent "source of
truth" about the FFI-compatibility of Option-like enums. Also, rename to
`repr_nullable_ptr`.
@jumbatm
Handle structs with zst members.
30a6f57
@jumbatm
Fix missed same-sized member clash in ClashingExternDeclarations.
0bd292d
@jumbatm
Copy link
Contributor Author

jumbatm commented Jul 30, 2020

I've just done another rebase -- @nagisa, are you available to review and approve? I'd like to get this merged soon so that these fixes can be backported to beta, and so I can try a crater check run with this lint as deny-by-default.

@nagisa
Copy link
Member

nagisa commented Jul 30, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Jul 30, 2020

📌 Commit 0bd292d has been approved by nagisa

@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 Jul 30, 2020
@nagisa nagisa added beta-nominated Nominated for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 30, 2020
@bors
Copy link
Contributor

bors commented Jul 30, 2020

⌛ Testing commit 0bd292d with merge 6b09c37...

@pnkfelix
Copy link
Member

discussed at T-compiler meeting.

We decided the complexity of this PR, weighed against the bug it fixes, does not warrant a backport.

However, we agreed that a PR that just changed the lint on beta to be allow-by-default would be a reasonable thing to put on the beta branch.

@pnkfelix
Copy link
Member

@rustbot modify labels: -beta-nominated

@rustbot rustbot removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 30, 2020
@bors
Copy link
Contributor

bors commented Jul 30, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: nagisa
Pushing 6b09c37 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 30, 2020
@bors bors merged commit 6b09c37 into rust-lang:master Jul 30, 2020
@bors bors mentioned this pull request Jul 30, 2020
Merged
@jumbatm jumbatm mentioned this pull request Jul 31, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 9, 2020
@bors
Auto merge of rust-lang#74966 - jumbatm:allow-clashing-extern-decl, r…
32c481e 
…=Mark-Simulacrum

[beta] Make ClashingExternDeclarations Allow by default.

As per rust-lang#73990 (comment), this PR changes `clashing_extern_declarations` to allow-by-default to sidestep current false positives & negatives on the beta branch.

Note that the changes to fix the issue properly have been merged to master (see rust-lang#73990), but those changes will have to arrive on the next release train.
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Aug 24, 2020
Merged
@matthiaskrgr matthiaskrgr mentioned this pull request Sep 7, 2023
Closed
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
RalfJung
RalfJung reviewed Sep 21, 2024
View reviewed changes
src/librustc_lint/types.rs
/// can, return the the type that `ty` can be safely converted to, otherwise return `None`.
/// Currently restricted to function pointers, boxes, references, `core::num::NonZero*`,
/// `core::ptr::NonNull`, and `#[repr(transparent)]` newtypes.
/// FIXME: This duplicates code in codegen.
Copy link
Member

Choose a reason for hiding this comment

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

Does anyone involved still remember which code in codegen this duplicates? The comment as-is is unfortunately not very helpful since it is unclear where that duplication is.

Copy link
Member

Choose a reason for hiding this comment

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

Ah never mind, this comment predates this PR, it just got moved here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teymour-aldridge teymour-aldridge teymour-aldridge left review comments

@davidtwco davidtwco davidtwco left review comments

@nagisa nagisa nagisa left review comments

@RalfJung RalfJung RalfJung left review comments

Assignees

@nagisa nagisa

Labels
merged-by-bors This PR was explicitly merged by bors. 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
Milestone
1.47.0
10 participants
@jumbatm @nagisa @bors @pnkfelix @RalfJung @davidtwco @teymour-aldridge @cuviper @rust-highfive @rustbot