Allow constants to refer statics #66302
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
oli-obk
reviewed
Nov 11, 2019
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Centril
added
needs-rfc
|
I'm not sure why we are doing this change. If we are to do so, I think an RFC would be in order. For the time being, we have an RFC that explicitly states that this should not be allowed. Also, do we have any test which forbids the following? extern {
static FOO: u8;
}
const X: u8 = unsafe { FOO }; |
spastorino
force-pushed
the
constants-can-refer-statics
branch
from
723630e to
774445d
Compare
|
@Centril I've just added the test you mentioned, good point ;). |
|
Thanks! Since the main purpose of this work is to simplify compiler internals rather than as a new language feature, I would suggest moving the existing check to HIR. That shouldn't be too difficult I think -- could be its own pass or we could do it in typeck maybe. |
We have? I was under the assumption this is just a restriction left over from the old const evaluator. The miri-based const eval can handle this without any problems. Which RFC are you referring to? |
|
@Centril and I discussed this a bit via chat. Here's the summary:
cc @RalfJung |
|
☔ The latest upstream changes (presumably #66314) made this pull request unmergeable. Please resolve the merge conflicts. |
|
The crucial thing to make sure is that CTFE does not read from (let alone write do) mutable statics. We should make sure to have a few miri-unleash tests that cover this. Cnetril mentioned one, @oli-obk mentioned this one: const FOO: usize {
static FOO: AtomicUsize = AtomicUsize::new(0);
FOO.fetch_add(1, Ordering::Relaxed)
};and then there also is const FOO: usize {
static FOO: AtomicUsize = AtomicUsize::new(0);
*(&FOO as *const _ as *const usize)
};and static mut MUTABLE: u32 = 0;
const BAD: u32 = unsafe { MUTABLE };All of these should fail even with miri-unleashed, to make sure that our dynamic checks are covering all possibilities. What remains then is the question of whether we want to have static checks on top of the dynamic ones, and what those are good for. Doing only dynamic checks leads to monomorphization-time errors. But from how I understood @oli-obk, we either already have those or ruling those out also rules out way too many useful things, and hence monomorphization-time errors are the better option? I do not have a strong stanza on this, if @oli-obk and @Centril agree on that that's good enough for me. I just want to see the Miri engine do enough checks to rule out all these examples. :) (And I think so far it does not do enough checks, actually.) |
JohnCSimon
added
S-waiting-on-author
|
Ping from triage |
|
@JohnCSimon, I don't think it makes sense to rebase this until we don't figure out a way to make this idea work. |
spastorino
added
S-waiting-on-team
My stance is that should not be adding post-monomorphization errors and that |
|
I'm currently unable to come up with a non-postmonomorphization plan that would not end up making const qualif more complex or require new marker traits Let's close until we have a plan for working this out |
|
CTFE already causes post-monomorphization errors because some parts of CTFE can only happen after monomorphization and sometimes that can error (causing a panic during CTFE is the most obvious case). Not sure if I would consider "you read from a static" a * new* post-monomorphization error. |
|
writing to mutable statics by first taking a reference and then writing through that reference is also only detected post monomorphization |
Centril
added a commit
to Centril/rust
that referenced
this pull request
Dec 24, 2019
…t, r=RalfJung Ensure that evaluating or validating a constant never reads from a static r? @RalfJung as per rust-lang#66302 (comment) This does not yet address the fact that evaluation of a constant can read from a static (under unleash-miri)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
r? @oli-obk, now @eddyb will be happy ❤️