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.

Sign up for GitHub

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

Jump to bottom

stop promoting union field accesses in 'const' #77526

Merged
merged 1 commit into from
Oct 25, 2020
Merged

stop promoting union field accesses in 'const' #77526

merged 1 commit into from
Oct 25, 2020

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Oct 4, 2020 •
edited
Loading

Turns out that promotion of union field accesses is the only difference between "promotion in const/static bodies" and "explicit promotion". So if we can remove this, we have finally achieved what I thought to already be the case -- that the bodies of const/static initializers behave the same as explicit promotion contexts.

The reason we do not want to promote union field accesses is that they can introduce UB, i.e., they can go wrong. We want to minimize the ways promoteds can fail to evaluate. Also this change makes things more consistent overall, removing a special case that was added without much consideration (as far as I can tell).

Cc @rust-lang/wg-const-eval

@rust-highfive
Copy link
Collaborator

r? @lcnr

(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 Oct 4, 2020
@RalfJung
Copy link
Member Author

RalfJung commented Oct 4, 2020

This will need a crater run.

@bors try

@bors
Copy link
Contributor

bors commented Oct 4, 2020

⌛ Trying commit d727f64 with merge f47dd4da3ae8c32c9e65d307bfe640b143e674df...

@RalfJung RalfJung mentioned this pull request Oct 4, 2020
Closed
@bors
Copy link
Contributor

bors commented Oct 4, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: f47dd4da3ae8c32c9e65d307bfe640b143e674df (f47dd4da3ae8c32c9e65d307bfe640b143e674df)

@RalfJung
Copy link
Member Author

RalfJung commented Oct 4, 2020

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-77526 created and queued.
🤖 Automatically detected try build f47dd4da3ae8c32c9e65d307bfe640b143e674df
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 4, 2020
@craterbot
Copy link
Collaborator

🚧 Experiment pr-77526 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-77526 is completed!
📊 8 regressed and 12 fixed (124727 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Oct 10, 2020
@RalfJung
Copy link
Member Author

Not a single one of these looks like a true regression to me. :)

@joshtriplett joshtriplett added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Oct 12, 2020
@joshtriplett
Copy link
Member

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Oct 12, 2020 •
edited
Loading

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 12, 2020
@nikomatsakis
Copy link
Contributor

@rfcbot fcp reviewed

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Oct 12, 2020
@rfcbot
Copy link

rfcbot commented Oct 12, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Oct 22, 2020
@rfcbot
Copy link

rfcbot commented Oct 22, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 22, 2020
@bors
Copy link
Contributor

bors commented Oct 24, 2020

⌛ Testing commit d727f64 with merge 01aa42f71e23887973836b478d9600907ee3e01a...

@bors
Copy link
Contributor

bors commented Oct 24, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 24, 2020
@lcnr
Copy link
Contributor

lcnr commented Oct 24, 2020

looks spurious

@bors retry

@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 Oct 24, 2020
@bors
Copy link
Contributor

bors commented Oct 24, 2020

⌛ Testing commit d727f64 with merge d080ec54500edb05029a3d5dc3471e7ac34779bb...

@bors
Copy link
Contributor

bors commented Oct 24, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 24, 2020
@lcnr
Copy link
Contributor

lcnr commented Oct 24, 2020 


gzip: stdin: not in gzip format
tar: Child returned status 1
tar: Error is not recoverable: exiting now
The command '/bin/sh -c curl http://cdimage.ubuntu.com/ubuntu-base/releases/16.04/release/ubuntu-base-16.04-core-armhf.tar.gz |       tar xzf - -C rootfs/ubuntu &&       cd rootfs && mkdir proc sys dev etc etc/init.d' returned a non-zero code: 2
The command has failed after 5 attempts.

looks spurious, but it's the second time in a row, so cc @rust-lang/infra

@RalfJung
Copy link
Member Author

We can still try to
@bors retry

@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 Oct 24, 2020
@pietroalbini
Copy link
Member

That is fixed by #78316.

@bors
Copy link
Contributor

bors commented Oct 25, 2020

⌛ Testing commit d727f64 with merge 36a7494...

@bors
Copy link
Contributor

bors commented Oct 25, 2020

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 36a7494 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 25, 2020
@bors bors merged commit 36a7494 into rust-lang:master Oct 25, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 25, 2020
@RalfJung RalfJung deleted the dont-promote-unions branch October 26, 2020 11:44
@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Oct 29, 2020
@RalfJung RalfJung mentioned this pull request Dec 20, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 25, 2020
@bors
Auto merge of rust-lang#80235 - RalfJung:validate-promoteds, r=oli-obk
bb17823 
validate promoteds

Turn on const-value validation for promoteds. This is made possible now that rust-lang#67534 is resolved.

I don't think this is a breaking change. We don't promote any unsafe operation any more (since rust-lang#77526 landed). We *do* promote `const fn` calls under some circumstances (in `const`/`static` initializers), but union field access and similar operations are not allowed in `const fn`. So now is a perfect time to add this check. :D

r? `@oli-obk`
Fixes rust-lang#67465
@RalfJung RalfJung mentioned this pull request Jan 2, 2021
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcnr lcnr lcnr approved these changes

Assignees

@lcnr lcnr

Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.49.0
Development

Successfully merging this pull request may close these issues.