Skip to content

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
Contributor

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
Collaborator

bors commented Dec 10, 2021

📌 Commit 85558ad has been approved by dtolnay

@bors
Copy link
Collaborator

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