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

const checking: do not do value-based reasoning for interior mutability #121786

Closed

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Feb 29, 2024

This basically means a nicer error for #121610, see the new test in tests/ui/consts/refs-to-cell-in-final.rs.

Currently we have a mismatch between the dynamic semantics of & (things might be mutable when the pointee type is !Freeze, no matter the pointee value) and const-qualif (doing value-based reasoning). This removes that mismatch. I don't think this can break anything that builds on current nightly; any such code would already later run into the error that the interner complains about the mutable pointer (that's exactly #121610). So a crater run makes little sense.

Overall this peanuts compared to the actual problem with value-based reasoning for interior mutability in promotion (rust-lang/unsafe-code-guidelines#493). I have no idea how to resolve that. But at least now the use of the problematic qualif is down to 1...

The first commit is independent, I just got worried about promotion of unions and was happy to notice that at least there we don't do value-based reasoning.

r? @oli-obk

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 29, 2024
@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member Author

RalfJung commented Feb 29, 2024

Lol, so much for this not being a breaking change. It breaks bootstraping.^^

error[E0492]: constants cannot refer to interior mutable data
  --> /home/r/.cargo/registry/src/index.crates.io-6f17d22bba15001f/litemap-0.7.2/src/store/slice_impl.rs:15:33
   |
15 |     const EMPTY: &'a [(K, V)] = &[];
   |                                 ^^^ this borrow of an interior mutable value may end up in the final value

Why does that not get promoted, though...?

@RalfJung
Copy link
Member Author

RalfJung commented Feb 29, 2024

Surrounding code can be seen here.

This does not get promoted because the type of the slice is not known to be 'static. Interesting, I didn't realize such lifetime constraints factor into promotion decisions...

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member Author

RalfJung commented Mar 2, 2024

Very strange, this breaks the following code:

const NONE: &'static Option<AtomicUsize> = &None;

This gets promoted, but then there are reborrows in the const itself. But reborrows should be ignored by const-checking...

@RalfJung
Copy link
Member Author

RalfJung commented Mar 2, 2024

Oh wait is const-checking happening before promotion?

@RalfJung
Copy link
Member Author

RalfJung commented Mar 2, 2024

Hm yes that may be the case. In that case I am very surprised that this does not lead to more errors... now I wonder what crater would say.
@bors try

@bors
Copy link
Contributor

bors commented Mar 2, 2024

⌛ Trying commit ac1ed22 with merge ca7d067...

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 2, 2024
…g, r=<try>

const checking: do not do value-based reasoning for interior mutability

This basically means a nicer error for rust-lang#121610, see the new test in `tests/ui/consts/refs-to-cell-in-final.rs`.

Currently we have a mismatch between the dynamic semantics of `&` (things might be mutable when the pointee type is `!Freeze`, no matter the pointee value) and const-qualif (doing value-based reasoning). This removes that mismatch. I don't think this can break anything that builds on current nightly; any such code would already later run into the error that the interner complains about the mutable pointer (that's exactly rust-lang#121610). So a crater run makes little sense.

Overall this peanuts compared to the *actual* problem with value-based reasoning for interior mutability in promotion (rust-lang/unsafe-code-guidelines#493). I have no idea how to resolve that. But at least now the use of the problematic qualif is down to 1...

The first commit is independent, I just got worried about promotion of unions and was happy to notice that at least there we don't do value-based reasoning.

r? `@oli-obk`
@RalfJung
Copy link
Member Author

RalfJung commented Mar 2, 2024

Ah wait this doesn't entirely remove value-based reasoning from promotion, it only makes that not work inside const any more. Never mind.

It is probably futile to use different checks in promotion and const-checking here...

When it comes to promotion, even rustc itself relies on value-based reasoning:

pub(crate) fn ctor(self) -> &'p Constructor<Cx> {
match self {
PatOrWild::Wild => &Wildcard,
PatOrWild::Pat(pat) => pat.ctor(),
}
}

Wildcard here is of type Constructor<Cx> which involves some associated types of Cx which we do not know to be Freeze at this point.

@rustbot
Copy link
Collaborator

rustbot commented Mar 2, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-16 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
#12 writing image sha256:297e8dfe892dd1c08dbbf1d4b9bc5c25bff8b8b29a8e84fa658ba844013243ef done
#12 naming to docker.io/library/rust-ci done
#12 DONE 10.2s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-16]
##[group]Clock drift check
  local time: Sat Mar  2 09:50:48 UTC 2024
  network time: Sat, 02 Mar 2024 09:50:48 GMT
  network time: Sat, 02 Mar 2024 09:50:48 GMT
##[endgroup]
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-16', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'build.optimized-compiler-builtins', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-16/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---
error[E0515]: cannot return value referencing temporary value
   --> compiler/rustc_pattern_analysis/src/pat.rs:229:9
    |
229 | /         match self {
230 | |             PatOrWild::Wild => &Wildcard,
    | |                                 -------- temporary value created here
231 | |             PatOrWild::Pat(pat) => pat.ctor(),
232 | |         }
    | |_________^ returns a value referencing data owned by the current function
For more information about this error, try `rustc --explain E0515`.
error: could not compile `rustc_pattern_analysis` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...
Build completed unsuccessfully in 0:07:39

@RalfJung
Copy link
Member Author

RalfJung commented Mar 2, 2024

I don't think there's anything we can do here, unless and until we decide to properly tackle value-based reasoning for interior mutability...

@RalfJung RalfJung closed this Mar 2, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 11, 2024
… r=oli-obk

Add tests (and a bit of cleanup) for interior mut handling in promotion and const-checking

Basically these are the parts of rust-lang#121786 that can be salvaged.

r? `@oli-obk`
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 11, 2024
… r=oli-obk

Add tests (and a bit of cleanup) for interior mut handling in promotion and const-checking

Basically these are the parts of rust-lang#121786 that can be salvaged.

r? ``@oli-obk``
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 11, 2024
Rollup merge of rust-lang#121893 - RalfJung:const-interior-mut-tests, r=oli-obk

Add tests (and a bit of cleanup) for interior mut handling in promotion and const-checking

Basically these are the parts of rust-lang#121786 that can be salvaged.

r? ``@oli-obk``
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Mar 12, 2024
Add tests (and a bit of cleanup) for interior mut handling in promotion and const-checking

Basically these are the parts of rust-lang/rust#121786 that can be salvaged.

r? ``@oli-obk``
@RalfJung
Copy link
Member Author

Oh wait is const-checking happening before promotion?

For the record -- yes it does: mir_promoted first calls mir_const_qualif, which does const-checking, and then it runs the promote_consts pass.

@RalfJung RalfJung deleted the interior-mut-value-reasoning branch August 2, 2024 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants