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

adjust const_eval_select documentation #91325

Merged
merged 2 commits into from
Dec 11, 2021

Conversation

RalfJung
Copy link
Member

"The Rust compiler assumes" indicates that this is language UB, but I don't think that is a good idea. This UB would be very hard to test for and looks like a way-too-big footgun. @oli-obk suggested this is meant to be more like "library UB", so I tried to adjust the docs accordingly.

I also removed all references to "referential transparency". That is a rather vague concept used to mean many different things, and I honestly have no idea what exactly is meant by it in this specific instance. But I assume @fee1-dead had in their mind a property that all const fn code upholds, so by demanding that the runtime code and the const-time code are observably equivalent, whatever that property is would also be enforced here.

Cc @rust-lang/wg-const-eval

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 28, 2021
@RalfJung
Copy link
Member Author

I started adjusting the const_eval_select safety comments accordingly, which lead me to realize that align_offset uses this operation. (Was wg-const-eval Cc'd for this? I was not aware of this, but it is possible I missed notifications.)

This is a problem, because align_offset is safe, but now it allows safe code to implement a "are we at compile-time" check! If we want any kind of assumption that compile-time and run-time code are equivalent (such as what is demonstrated by the example I added to the doc comment), then we can not have such an operation.

Put differently, we basically have to pick one of the two options:

  • We have to say that it is okay for safe const fn to behave differently at compiletime and runtime (basically removing the safety requirement from const_eval_select, and saying that the bug in the example is in crate B).
  • We have to make all functions that can implement a "are we at compile-time" unsafe -- in particular, we cannot use const_eval_select in align_offset.

@RalfJung
Copy link
Member Author

Here's an example of how to cause UB by assuming (just in a library, not as the compiler) that a const fn behaves consistently at compiletime and runtime:

#![feature(const_align_offset)]
use std::hint::unreachable_unchecked;

// Crate A
pub const fn inconsistent() -> usize {
    (&13 as *const i32).align_offset(2)
}

// Crate B
pub fn main() {
  const X: usize = inconsistent();
  let x = inconsistent();
  if x != X { unsafe { unreachable_unchecked(); }}
}

@fee1-dead
Copy link
Member

I mostly copied the documentation from a previous PR. (#64683)

@apiraino apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Dec 1, 2021
@dtolnay
Copy link
Member

dtolnay commented Dec 10, 2021

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 10, 2021

📌 Commit 85558ad has been approved by dtolnay

@bors
Copy link
Contributor

bors commented Dec 10, 2021

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 10, 2021
@dtolnay dtolnay assigned dtolnay and unassigned joshtriplett Dec 10, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 10, 2021
adjust const_eval_select documentation

"The Rust compiler assumes" indicates that this is language UB, but [I don't think that is a good idea](https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/const_eval_select.20assumptions). This UB would be very hard to test for and looks like a way-too-big footgun. `@oli-obk` suggested this is meant to be more like "library UB", so I tried to adjust the docs accordingly.

I also removed all references to "referential transparency". That is a rather vague concept used to mean many different things, and I honestly have no idea what exactly is meant by it in this specific instance. But I assume `@fee1-dead` had in their mind a property that all `const fn` code upholds, so by demanding that the runtime code and the const-time code are *observably equivalent*, whatever that property is would also be enforced here.

Cc `@rust-lang/wg-const-eval`
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 10, 2021
…askrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#90407 (Document all public items in `rustc_incremental`)
 - rust-lang#90897 (Fix incorrect stability attributes)
 - rust-lang#91105 (Fix method name reference in stream documentation)
 - rust-lang#91325 (adjust const_eval_select documentation)
 - rust-lang#91470 (code-cov: generate dead functions with private/default linkage)
 - rust-lang#91482 (Update documentation to use `from()` to initialize `HashMap`s and `BTreeMap`s)
 - rust-lang#91524 (Fix Vec::extend_from_slice docs)
 - rust-lang#91575 (Fix ICE on format string of macro with secondary-label)
 - rust-lang#91625 (Remove redundant [..]s)
 - rust-lang#91646 (Fix documentation for `core::ready::Ready`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d317da4 into rust-lang:master Dec 11, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 11, 2021
@RalfJung RalfJung deleted the const_eval_select branch December 11, 2021 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants