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

improper_ctypes lint allows all "Option-like" enums, but we only guarantee this specifically for Option #130652

Closed
RalfJung opened this issue Sep 21, 2024 · 16 comments
Labels
A-FFI Area: Foreign function interface (FFI) A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. L-improper_ctypes Lint: improper_ctypes T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

Ever since its introduction, the FFI safety lint has accepted MyOption<Box<T>> as FFI safe even if MyOption is not the "official" Option type from libcore. This was originally done because "being sound but slightly incomplete is acceptable". However, it doesn't quite reflect today's approaches any more, and IMO the lint should be adjusted to match the documented guarantees -- which AFAIK only cover specifically Option, and not all "Option-like" types.

Cc @rust-lang/opsem @workingjubilee @Noratrieb

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 21, 2024
@jieyouxu jieyouxu added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. L-improper_ctypes Lint: improper_ctypes C-bug Category: This is a bug. labels Sep 21, 2024
@workingjubilee
Copy link
Member

@RalfJung Frankly, the improper_ctypes lint has so many false positives that I cannot honestly countenance adding more BS to it.

@workingjubilee
Copy link
Member

It also cannot be modified because even the smallest change, to not eagerly warn on things that are not a problem, is deferred to a T-lang decision, e.g. #116863

@workingjubilee
Copy link
Member

workingjubilee commented Sep 21, 2024

I already actively recommend to people to simply silence the lint, in direct communication, when it proves to be the slightest problem. I will feel forced to more proactively recommend silencing the lint if this is done, in venues like "the documentation for FFI libraries used by many corporations".

Yyyes, it's kind of silly that we don't guarantee this quality for non-Option, non-Result enums that nonetheless "look like" Option and Result. But I already have enough trouble encouraging people to use the compiler correctly when things "don't cause problems in practice" (read: their code is actively UB but it hasn't visibly miscompiled yet). In this case, we actually do know it doesn't cause problems in practice.

If we fix literally every other problem with improper_ctypes first?

...well, that'll be a while, but then I'd endorse such an action: https://github.com/rust-lang/rust/issues?q=state%3Aopen%20label%3A%22L-improper_ctypes%22

Though, I'd probably file an RFC to extend the Option and Result guarantees first to all enums that we will give similar representations. Because I feel like we do want to guarantee this for them, and the main reason we haven't is that we aren't completely sure we want to close that door yet, or do something like give Option and Result their own repr annotation that would guarantee this representation.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 21, 2024 via email

@saethlin saethlin added A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. A-FFI Area: Foreign function interface (FFI) labels Sep 21, 2024
@lolbinarycat
Copy link
Contributor

I was under the impression that the null pointer optimization was guaranteed for all "Option-like" enums, but i can't find a normative reference for that

closest i can find is this:
https://rust-lang.github.io/unsafe-code-guidelines/layout/enums.html#discriminant-elision-on-option-like-enums

@RalfJung
Copy link
Member Author

Cc @chorman0773

@chorman0773
Copy link
Contributor

It is, in fact, only guaranteed for Option<T> and now Result<T,E>/Result<S,T> for a 1zst E or S.

@RalfJung
Copy link
Member Author

The FCP here seems to have established a guarantee for all Option-like types?

@chorman0773
Copy link
Contributor

Interesting. It's not been documented outside of the UCG from what I can tell.

@chorman0773
Copy link
Contributor

chorman0773 commented Sep 22, 2024

In relation to the lint itself, I would agree with Jubilee also - even if this is a false negative, I'd very much prefer we fix the inordinate false positives before addressing false negatives. The lint is IMO useless as is, because of its false positives, so making it lint on more things (even things it should be linting on) isn't going to make it any more useful (can't catch a bug if the lint has been blanket allowed due to FP).

@workingjubilee
Copy link
Member

Noting in passing this T-lang decision that specifically permits rustc_nonnull_optimization_guaranteed to include -1, for extra fun: #97122

@RalfJung
Copy link
Member Author

Interesting -- this was never put into any docs it seems, only the lint "knows" about this.

@the8472
Copy link
Member

the8472 commented Oct 1, 2024

The FCP here seems to have established a guarantee for all Option-like types?

That didn't update the layout chapter in the rust reference though. In fact the reference still doesn't mention niches at all, even the ones guaranteed in core.

If a guarantee is made in an FCP and it's nowhere to be read, does that constitute a spec?

🌲🔉

@RalfJung RalfJung added the I-lang-nominated Nominated for discussion during a lang team meeting. label Oct 1, 2024
@RalfJung
Copy link
Member Author

RalfJung commented Oct 1, 2024

@rust-lang/lang since this is a t-lang FCP (from five years ago), we'd be interested in your take on this. :)

Does the behavior of the improper_ctypes lint, where all option-like enums are accepted for the null pointer layout guarantee, constitute a stable guarantee that we should properly document somewhere, or not?

@traviscross traviscross removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 1, 2024
@traviscross
Copy link
Contributor

@rustbot labels -I-lang-nominated

We discussed this in the triage meeting today and agreed that we meant what was in the FCP in #60300, that we're still OK with it, that it is (and has been) a language guarantee, and that we're OK with documenting it.

Note that this guarantee will be subsumed by the one that we're making now via FCP in:

@rustbot rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Oct 9, 2024
@RalfJung
Copy link
Member Author

RalfJung commented Oct 9, 2024

Okay, I think that means we can close this issue -- thanks for clarifying!

@RalfJung RalfJung closed this as completed Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-FFI Area: Foreign function interface (FFI) A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. L-improper_ctypes Lint: improper_ctypes T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants