-
Notifications
You must be signed in to change notification settings - Fork 498
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
we reserve the right to reduce our amount of UB #1397
Conversation
@rustbot label: +I-lang-nominated I want to discuss this at the @rust-lang/lang design team meeting. In particular, on the associated zulip thread where I asked why someone would want the prior interpretation of this text, a couple interesting cases were described:
|
src/behavior-considered-undefined.md
Outdated
sure is undefined behavior. Please read the [Rustonomicon] before writing unsafe | ||
code. | ||
more behavior considered unsafe. We also reserve the right to make some of the | ||
behavior in that list defined in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
behavior in that list defined in the future. | |
behavior in that list defined in the future, or conversely, to guarantee that it will always be undefined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused, this now says "we reserve the right to [...] guarantee that it will always be undefined"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Josh is trying to capture the point that Niko raised below: We may want to promise that certain operations will always be UB, so that unsafe code can rely on that. edit: I don't personally think it's necessary to say this, but perhaps it would be good to say, ideally in another sentence to reduce confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the point of saying "in the future we may want to promise that something will always be UB". On its own, how is that statement useful for anyone? I would understand the point of marking particular items in the UB list as "this will definitely always be UB", but we don't currently have any such items, or do we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RalfJung The wording could absolutely change. The wording currently in the PR says that we could define it in the future. The point Niko made, which I wanted to capture, was that we could also commit to not defining something (or for that matter commit to not defining it in particular ways), which is something that some crates may need us to do.
I agree that that's already implicit, but at the same time, someone reading this might want to rely on a guarantee and know we won't define the behavior unexpectedly in a way that breaks their code, so they could ask for it to be guaranteed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I still don't understand whom this comment is serving/helping. People will be asking for us to commit to something like that no matter what we write here. But sure I can try to put in a phrase along those lines. I just worry it will cause a lot more confusion than it will help people. It would certainly confuse the heck out of me without a multi-paragraph explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried my hand at it, but honestly I don't like it. I think the text would be semantically equivalent but less confusing without that new sentence.
I think we should make this change, but I just want to raise the point I always raise, that simply "removing UB" isn't "always" a fine change to make, because it can make a sound API become unsound. My classic example is that rayon (and other libraries) assume that destructors will run for their local variables before the stack frame is popped. I think we eventually will want to be clear about those things we are guaranteeing (which implies a certain amount of operations that will always be UB) versus things that are UB now but which we may change. @pnkfelix points out this is similar to the "assume |
@rfcbot fcp merge I think we should make this change! |
In particular, @chorman0773 's example was a specific "the given UB list means that 'expression A is UB implies expression B is also UB', so if my method precondition includes 'expression B is well-defined', then my code can assume A is likewise well-defined." (i.e. an instance of using the contrapositive reasoning, but it relies on the given UB list remaining fixed!). In the meeting, I waffled on whether @nikomatsakis 's example is similar or not. I think I'm still waffling now, but I'm coming back around to thinking that it indeed is such an example. |
@rfcbot fcp merge I think we should make this change! |
(does rfcbot not monitor rust-lang/reference ...?) |
I agree in general, yes, if APIs are defined by referring to UB in negative position, like Connor's example. The rayon case wouldn't just remove UB though, that would significantly alter the lowering from surface Rust to MIR/MiniRust. (So I don't think I see how that's an example of this issue.) And if APIs refer to "my precondition is that
It checks in once an hour. |
Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
🔔 This is now entering its final comment period, as per the review above. 🔔 psst @nikomatsakis, I wasn't able to add the |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. psst @nikomatsakis, I wasn't able to add the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Update books ## rust-embedded/book 1 commits in 99ad2847b865e96d8ae7b333d3ee96963557e621..eac173690b8cc99094e1d88bd49dd61127fbd285 2023-09-12 07:34:44 UTC to 2023-09-12 07:34:44 UTC - USB connector-type correction (rust-embedded/book#360) ## rust-lang/nomicon 1 commits in e3f3af69dce71cd37a785bccb7e58449197d940c..ddfa4214487686e91b21aa29afb972c08a8f0d5b 2023-09-22 17:04:10 UTC to 2023-09-22 17:04:10 UTC - Fill "Beneath `std`" (rust-lang/nomicon#413) ## rust-lang/reference 1 commits in ee7c676fd6e287459cb407337652412c990686c0..5262e1c3b43a2c489df8f6717683a44c7a2260fd 2023-09-18 18:28:31 UTC to 2023-09-18 18:28:31 UTC - we reserve the right to reduce our amount of UB (rust-lang/reference#1397) ## rust-lang/rustc-dev-guide 8 commits in 08bb147..a13b7c2 2023-09-25 05:14:41 UTC to 2023-09-11 21:29:18 UTC - Clarify all the `{AP,RP}IT{,IT}` impl trait types (rust-lang/rustc-dev-guide#1798) - Modify build instructions for optimized build (rust-lang/rustc-dev-guide#1795) - Remove outdated references to coverage debug code (rust-lang/rustc-dev-guide#1797) - Add deep dive document about early/late bound parameters interacting with turbofish (rust-lang/rustc-dev-guide#1794) - explain the MIR const vs TY const situation (rust-lang/rustc-dev-guide#1793) - fix type name (rust-lang/rustc-dev-guide#1792) - Clarify that `run-coverage` only runs in some of the CI jobs (rust-lang/rustc-dev-guide#1791) - Document the `coverage-map` and `run-coverage` test suites (rust-lang/rustc-dev-guide#1790)
Update books ## rust-embedded/book 1 commits in 99ad2847b865e96d8ae7b333d3ee96963557e621..eac173690b8cc99094e1d88bd49dd61127fbd285 2023-09-12 07:34:44 UTC to 2023-09-12 07:34:44 UTC - USB connector-type correction (rust-embedded/book#360) ## rust-lang/nomicon 1 commits in e3f3af69dce71cd37a785bccb7e58449197d940c..ddfa4214487686e91b21aa29afb972c08a8f0d5b 2023-09-22 17:04:10 UTC to 2023-09-22 17:04:10 UTC - Fill "Beneath `std`" (rust-lang/nomicon#413) ## rust-lang/reference 1 commits in ee7c676fd6e287459cb407337652412c990686c0..5262e1c3b43a2c489df8f6717683a44c7a2260fd 2023-09-18 18:28:31 UTC to 2023-09-18 18:28:31 UTC - we reserve the right to reduce our amount of UB (rust-lang/reference#1397) ## rust-lang/rustc-dev-guide 8 commits in 08bb147..a13b7c2 2023-09-25 05:14:41 UTC to 2023-09-11 21:29:18 UTC - Clarify all the `{AP,RP}IT{,IT}` impl trait types (rust-lang/rustc-dev-guide#1798) - Modify build instructions for optimized build (rust-lang/rustc-dev-guide#1795) - Remove outdated references to coverage debug code (rust-lang/rustc-dev-guide#1797) - Add deep dive document about early/late bound parameters interacting with turbofish (rust-lang/rustc-dev-guide#1794) - explain the MIR const vs TY const situation (rust-lang/rustc-dev-guide#1793) - fix type name (rust-lang/rustc-dev-guide#1792) - Clarify that `run-coverage` only runs in some of the CI jobs (rust-lang/rustc-dev-guide#1791) - Document the `coverage-map` and `run-coverage` test suites (rust-lang/rustc-dev-guide#1790)
Rollup merge of rust-lang#116153 - rustbot:docs-update, r=ehuss Update books ## rust-embedded/book 1 commits in 99ad2847b865e96d8ae7b333d3ee96963557e621..eac173690b8cc99094e1d88bd49dd61127fbd285 2023-09-12 07:34:44 UTC to 2023-09-12 07:34:44 UTC - USB connector-type correction (rust-embedded/book#360) ## rust-lang/nomicon 1 commits in e3f3af69dce71cd37a785bccb7e58449197d940c..ddfa4214487686e91b21aa29afb972c08a8f0d5b 2023-09-22 17:04:10 UTC to 2023-09-22 17:04:10 UTC - Fill "Beneath `std`" (rust-lang/nomicon#413) ## rust-lang/reference 1 commits in ee7c676fd6e287459cb407337652412c990686c0..5262e1c3b43a2c489df8f6717683a44c7a2260fd 2023-09-18 18:28:31 UTC to 2023-09-18 18:28:31 UTC - we reserve the right to reduce our amount of UB (rust-lang/reference#1397) ## rust-lang/rustc-dev-guide 8 commits in 08bb147..a13b7c2 2023-09-25 05:14:41 UTC to 2023-09-11 21:29:18 UTC - Clarify all the `{AP,RP}IT{,IT}` impl trait types (rust-lang/rustc-dev-guide#1798) - Modify build instructions for optimized build (rust-lang/rustc-dev-guide#1795) - Remove outdated references to coverage debug code (rust-lang/rustc-dev-guide#1797) - Add deep dive document about early/late bound parameters interacting with turbofish (rust-lang/rustc-dev-guide#1794) - explain the MIR const vs TY const situation (rust-lang/rustc-dev-guide#1793) - fix type name (rust-lang/rustc-dev-guide#1792) - Clarify that `run-coverage` only runs in some of the CI jobs (rust-lang/rustc-dev-guide#1791) - Document the `coverage-map` and `run-coverage` test suites (rust-lang/rustc-dev-guide#1790)
It was pointed out to me that the docs currently say that the things on the UB list are definitely UB. That's not true, we do reserve the right to have less UB in the future. (Exactly that is happening e.g. in #1387.)