Uh oh!
There was an error while loading. Please reload this page.
Allow assignments in const contexts - #56070
Conversation
rust-highfive
commented
Nov 19, 2018
(rust_highfive has picked a reviewer for you, use r? to override) |
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
petrochenkov
commented
Nov 19, 2018
r? @RalfJung |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
RalfJung
commented
Nov 20, 2018
r=me with tidy happy: |
oli-obk
commented
Nov 20, 2018
@bors r=RalfJung |
bors
commented
Nov 20, 2018
📌 Commit 37fcd5c has been approved by |
| Place::Local(index) => break *index, | ||
| // projections are transparent for assignments | ||
| // we qualify the entire destination at once, even if just a field would have | ||
| // stricter qualification |
There was a problem hiding this comment.
This doesn't seem correct - if you assign something with less qualifications to a field of a value with more, you turn those bits off.
eddyb
commented
Nov 21, 2018
In the future, please cc me on @bors r- |
| // projections are transparent for assignments | ||
| // we qualify the entire destination at once, even if just a field would have | ||
| // stricter qualification | ||
| Place::Projection(proj) => dest = &proj.base, |
There was a problem hiding this comment.
Continuing with
the mindset when dealing with [const qualif] should be "how do I break this?"
Projections would include Deref, which isn't a problem because we can't have mutable references at all inside const contexts. Still something to think about.
Downcast also can't happen, because we'd need match for that.
Maybe we should just whitelist field projections for now.
There was a problem hiding this comment.
The problem is something like this:
constFOO:&(Cell<usize>,bool) = &{letmut foo = (Cell::new(0),false);
foo.1 = true;// resets `qualif(foo)` to `qualif(true)`
foo
};I suspect that will actually compile with this PR, although I'd like a confirmation.
We could add this as a testcase, but you might be able to come up with something simpler/more general.
There was a problem hiding this comment.
This indeed compiles successfully with this PR
There was a problem hiding this comment.
My suggestion is field (or projections in general) assignment should combine qualifications (i.e. |=) instead of replacing them.
There was a problem hiding this comment.
I now reuse the existing projection checks for reading values. These checks replace the qualification with NOT_CONST, so there's no real use in combining I think?
RalfJung
commented
Nov 21, 2018
I'll happily hand them over to you. ;) r? @eddyb |
rust-highfive
commented
Nov 21, 2018
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 |
Uh oh!
There was an error while loading. Please reload this page.
oli-obk
commented
Nov 21, 2018
@bors r=eddyb |
bors
commented
Nov 21, 2018
📌 Commit 6db8c6c has been approved by |
bors
commented
Nov 26, 2018
bors
commented
Nov 26, 2018
☀️ Test successful - status-appveyor, status-travis |
fixes#54098
fixes#51251
fixes#52613