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

we reserve the right to reduce our amount of UB #1397

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

RalfJung
Copy link
Member

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.)

@pnkfelix
Copy link
Member

@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:

  • I hypothesized, as a strawman, that compiler optimization authors want it so they don't have to update their code when the language spec changes
  • @chorman0773 pointed out that some API's might be written in a way where they just say "precondition: *x is a legal operation", and then treat the implications of that according to this list at the time when they are writing the API as something they can assume for all time.
  • @RalfJung pointed out their own motivating case, which is verification tools that want to similarly make assumptions in their reasoning based on the UB list.

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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Member Author

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"?

Copy link
Member

@tmandry tmandry Sep 6, 2023

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.

Copy link
Member Author

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?

Copy link
Member

@joshtriplett joshtriplett Sep 12, 2023

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Sep 5, 2023

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 *x is legal to deref" example that @chorman0773 raised.

@nikomatsakis
Copy link
Contributor

@rfcbot fcp merge

I think we should make this change!

@pnkfelix
Copy link
Member

pnkfelix commented Sep 5, 2023

@pnkfelix points out this is similar to the "assume *x is legal to deref" example that @chorman0773 raised.

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.

@pnkfelix pnkfelix added the T-lang Team: Lang label Sep 5, 2023
@pnkfelix
Copy link
Member

pnkfelix commented Sep 5, 2023

@rfcbot fcp merge

I think we should make this change!

@pnkfelix
Copy link
Member

pnkfelix commented Sep 5, 2023

(does rfcbot not monitor rust-lang/reference ...?)

@RalfJung
Copy link
Member Author

RalfJung commented Sep 5, 2023

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.

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 *x as defined in Rust 1.52 is not UB", then those should also be stable under having less UB.

(does rfcbot not monitor rust-lang/reference ...?)

It checks in once an hour.

@rfcbot
Copy link

rfcbot commented Sep 5, 2023

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.
See this document for info about what commands tagged team members can give me.

@rfcbot
Copy link

rfcbot commented Sep 7, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

psst @nikomatsakis, I wasn't able to add the final-comment-period label, please do so.

@rfcbot
Copy link

rfcbot commented Sep 17, 2023

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 finished-final-comment-period label, please do so.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks!

@ehuss ehuss added this pull request to the merge queue Sep 18, 2023
Merged via the queue into rust-lang:master with commit 5262e1c Sep 18, 2023
1 check passed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 26, 2023
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 08bb147d51e815b96e8db7ba4cf870f201c11ff8..a13b7c28ed705891c681ce5417b3d1cdb12cecd1
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)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 26, 2023
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 08bb147d51e815b96e8db7ba4cf870f201c11ff8..a13b7c28ed705891c681ce5417b3d1cdb12cecd1
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)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 26, 2023
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 08bb147d51e815b96e8db7ba4cf870f201c11ff8..a13b7c28ed705891c681ce5417b3d1cdb12cecd1
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)
@RalfJung RalfJung deleted the less-ub-in-the-future branch October 9, 2023 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants