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

Fix promotion in a const when projections are present #65728

Merged
merged 3 commits into from
Nov 6, 2019
Merged

Fix promotion in a const when projections are present #65728

merged 3 commits into from
Nov 6, 2019

Conversation

ecstatic-morse
Copy link
Contributor

Resolves #65727.

This marks the entire local as "needs promotion" when only a projection of that local appears in a promotable context. This should only affect promotion in a const or static, not in a fn or const fn, which is handled in promote_consts.rs.

r? @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 23, 2019
eddyb
eddyb reviewed Oct 23, 2019
View reviewed changes
src/librustc_mir/transform/qualify_consts.rs Outdated
};

match promoted_place.base {
PlaceBase::Local(local) if !promoted_place.is_indirect() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "indirect" thing is irrelevant AFAIK, all you care about is the base.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we never promote the result of a deref projection, so that guard will never take effect. I could make this a bug or just remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made this an assert for now so it will fail loudly on the test suite if my assumptions are wrong. Probably want to make this a debug_assert or remove it entirely before merging.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just remove it IMO. We really need to stop using "delete StorageDead" for faux promotion, I dislike it more by the minute.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertion has been removed in the latest force push.

@ecstatic-morse ecstatic-morse force-pushed the promotion-const-proj branch 3 times, most recently from 3c4e28d to 4eb2936 Compare October 23, 2019 23:35
@bors
Copy link
Contributor

bors commented Oct 25, 2019

☔ The latest upstream changes (presumably #65804) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Oct 26, 2019

☔ The latest upstream changes (presumably #63812) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 2, 2019
@JohnCSimon
Copy link
Member

Ping from triage - the PR has sat idle for a week -
@ecstatic-morse can you please address the merge conflict ?
Also: once the that's resolved @eddyb can you please review this PR?

thanks.

@eddyb
Copy link
Member

eddyb commented Nov 6, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Nov 6, 2019

📌 Commit 420457e has been approved by eddyb

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 6, 2019
@bors
Copy link
Contributor

bors commented Nov 6, 2019

⌛ Testing commit 420457e with merge 3804876...

bors added a commit that referenced this pull request Nov 6, 2019
@bors
Auto merge of #65728 - ecstatic-morse:promotion-const-proj, r=eddyb
3804876 
Fix promotion in a `const` when projections are present

Resolves #65727.

This marks the entire local as "needs promotion" when only a projection of that local appears in a promotable context. This should only affect promotion in a `const` or `static`, not in a `fn` or `const fn`, which is handled in `promote_consts.rs`.

r? @eddyb
@bors
Copy link
Contributor

bors commented Nov 6, 2019

☀️ Test successful - checks-azure
Approved by: eddyb
Pushing 3804876 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 6, 2019
@bors bors merged commit 420457e into rust-lang:master Nov 6, 2019
@bors bors mentioned this pull request Nov 6, 2019
Merged
@SimonSapin SimonSapin mentioned this pull request Nov 12, 2019
Closed
@ecstatic-morse ecstatic-morse deleted the promotion-const-proj branch October 6, 2020 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddyb eddyb eddyb approved these changes

Assignees

@eddyb eddyb

Labels
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrect promotion for projection in a const
5 participants
@ecstatic-morse @bors @JohnCSimon @eddyb @rust-highfive