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

Support Result<T, E> across FFI when niche optimization can be used #122253

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Support Result<T, E> across FFI when niche optimization can be used #122253

wants to merge 9 commits into from
+826 −73

Conversation

MasterAwesome
Copy link
Contributor

@MasterAwesome MasterAwesome commented Mar 9, 2024 •
edited

Allow allow enums like Result<T, E> to be used across FFI if the T/E can be niche optimized and the non-niche-optimized type is FFI safe.

Implementation of rust-lang/rfcs#3391
Tracking issue: #110503

Additional ABI and codegen tests were added in #115372

@rustbot
Copy link
Collaborator

rustbot commented Mar 9, 2024

r? @cjgillot

rustbot has assigned @cjgillot.
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. labels Mar 9, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@MasterAwesome MasterAwesome marked this pull request as ready for review March 9, 2024 22:36
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cjgillot
Copy link
Contributor

r? compiler

@rustbot rustbot assigned petrochenkov and unassigned cjgillot Mar 10, 2024
@petrochenkov
Copy link
Contributor

Hmmm.
r? @RalfJung

@rustbot rustbot assigned RalfJung and unassigned petrochenkov Mar 12, 2024
@RalfJung
Copy link
Member

Sorry, I can't review these lint implementations.
Maybe r? @davidtwco ; I see you've worked on the improper-ctypes lint in the past.

@rustbot rustbot assigned davidtwco and unassigned RalfJung Mar 12, 2024
@RalfJung RalfJung mentioned this pull request Mar 12, 2024
Open
3 tasks
davidtwco
davidtwco requested changes Mar 18, 2024
View reviewed changes
compiler/rustc_lint/src/types.rs Outdated Show resolved Hide resolved
compiler/rustc_lint/src/types.rs Outdated Show resolved Hide resolved
tests/ui/lint/lint-ctypes-enum.rs Show resolved Hide resolved
compiler/rustc_lint/src/types.rs Show resolved Hide resolved
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 18, 2024
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 19, 2024
@bors
Copy link
Contributor

bors commented Apr 23, 2024

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

davidtwco
davidtwco approved these changes Apr 23, 2024
View reviewed changes
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

r=me after rebasing

@MasterAwesome
Support Result<T, E> across FFI when niche optimization can be used
fc37633 
Allow allow enums like `Result<T, E>` to be used across FFI if the
T/E can be niche optimized and the non-niche-optimized type is FFI safe.
@MasterAwesome
We don't need to check for non-exhaustive fields
b3e6d52 
Fields are disallowed so checking the top attribute is enough.
@MasterAwesome
Don't lint niche optimized variants in enums
08b85a1
@MasterAwesome
Add tests
223d5eb 
Tests both `T` and `E` for niche variant optimization lints
@MasterAwesome
Disallow single-variant enums
014ddac 
Couldn't find documentation supporting that single-variant
`#[repr(Rust)]` enums with RHS assigned work as expected with this
change.

```rust
enum Variants {
  A = 17,
} // Would this be zero sized optimized guaranteed?
```
@MasterAwesome
Add a test against Result<(), ()>
b3f6511
@MasterAwesome
Reword is_niche_optimization_candidate doc
764f64f
@MasterAwesome
Remove comment out of RFC's scope
437ca26
@MasterAwesome
Put the RFC behind a feature gate result_ffi_guarantees
ed532cc
@MasterAwesome
Copy link
Contributor Author

@bors r=davidtwco

@bors
Copy link
Contributor

bors commented Apr 24, 2024

@MasterAwesome: 🔑 Insufficient privileges: Not in reviewers

@MasterAwesome
Copy link
Contributor Author

I guess I don't have permissions @davidtwco, can you queue this in?

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

@davidtwco davidtwco davidtwco approved these changes

Assignees

@davidtwco davidtwco

Labels
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@MasterAwesome @rustbot @rust-log-analyzer @cjgillot @petrochenkov @RalfJung @bors @davidtwco