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

The Drop flag is a footgun for #[repr(C)] #24585

Closed
pnkfelix opened this issue Apr 19, 2015 · 6 comments · Fixed by #24935
Closed

The Drop flag is a footgun for #[repr(C)] #24585

pnkfelix opened this issue Apr 19, 2015 · 6 comments · Fixed by #24935
Assignees
Labels
P-high High priority

Comments

@pnkfelix
Copy link
Member

(imported from improperly closed bug #18380)

As suggested by @huonw:

the improper_ctypes lint should check for repr(C) on the RHS of a Drop impl. "warning: implementing Drop changes the representation of the type, conflicting with repr(C)".

(It should presumably also check for #[unsafe_no_drop_flag] and not warn if it exists.)

Until #5016 is done, we really should lint for this scenario. People have run into it

@pnkfelix
Copy link
Member Author

triage: P-low

@rust-highfive rust-highfive added the P-low Low priority label Apr 19, 2015
@pnkfelix
Copy link
Member Author

oops.

triage: P-high

(but nonetheless not 1.0)

@rust-highfive rust-highfive added P-medium Medium priority and removed P-low Low priority labels Apr 19, 2015
@geofft
Copy link
Contributor

geofft commented Apr 27, 2015

I feel like this should be stronger than a medium-priority lint. I feel like rustc should reject programs that attempt to impl Drop on a #[repr(C)] struct, for as long as that changes the size of that struct.

In particular, not fixing this by 1.0 would foreclose the ability to change the drop flag after 1.0, since that would change the size / ABI of #[repr(C)] structs, right? Or is ABI stability not a guarantee of #[repr(C)]?

@pnkfelix
Copy link
Member Author

Yeah this label got auto renamed. I'm up'ing the priority

@pnkfelix
Copy link
Member Author

triage: P-high

I'll assign to self too

@rust-highfive rust-highfive added P-high High priority and removed P-medium Medium priority labels Apr 28, 2015
@pnkfelix
Copy link
Member Author

( @geofft I'm pretty confident we could fix this post 1.0, e.g. adding new warnings is allowed, and the representation issue would be "just a bug to be fixed." Nonetheless I I do I do want I do want to fix.)

@pnkfelix pnkfelix self-assigned this Apr 28, 2015
alexcrichton added a commit to alexcrichton/rust that referenced this issue Apr 29, 2015
Lint: warn when mixing `#[repr(C)]` with Drop

Fix rust-lang#24585
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-high High priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants