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

Add check that live_region is live in sanitize_promoted #88698

Merged
merged 1 commit into from
Oct 14, 2021
Merged

Add check that live_region is live in sanitize_promoted #88698

merged 1 commit into from
Oct 14, 2021

Conversation

Noble-Mushtak
Copy link
Contributor

@Noble-Mushtak Noble-Mushtak commented Sep 6, 2021 •
edited
Loading

This pull request fixes #88434 by adding a check in sanitize_promoted to ensure that only regions which are actually live are added to the liveness_constraints of the BorrowCheckContext.

To implement this change, I needed to add a method to LivenessValues which gets the elements contained by a region:

/// Returns an iterator of all the elements contained by the region `r`
crate fn get_elements(&self, row: N) -> impl Iterator<Item = Location> + '_

Then, inside sanitize_promoted, we check whether the iterator returned by this method is non-empty to ensure that the region is actually live at at least one location before adding that region to the liveness_constraints of the BorrowCheckContext.

This is my first pull request to the Rust repo, so any feedback on how I can improve this pull request or if there is a better way to fix this issue would be very appreciated.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @wesleywiser (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

  • These commits modify submodules.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 6, 2021
@Noble-Mushtak
Copy link
Contributor Author

Apologies, I think I accidentally modified some submodules in my commit somehow, but I reverted and then reapplied my actual changes, so hopefully this should be fixed now.

@camelid
Copy link
Member

camelid commented Sep 6, 2021

Apologies, I think I accidentally modified some submodules in my commit somehow, but I reverted and then reapplied my actual changes, so hopefully this should be fixed now.

Yeah, it's easy to do that. Running git submodule update may make it so that you can't git add them by mistake (though sometimes that doesn't fix all of them). I usually run something like git add compiler/ to avoid committing submodules by mistake.

camelid
camelid reviewed Sep 6, 2021
View reviewed changes
src/test/ui/borrowck/issue-88434-removal-index-should-be-less.stderr Outdated
Comment on lines 8 to 17
= help: add `#![feature(const_fn_trait_bound)]` to the crate attributes to enable

error[E0658]: panicking in constant functions is unstable
--> $DIR/issue-88434-removal-index-should-be-less.rs:9:5
|
LL | panic!()
| ^^^^^^^^
|
= note: see issue #51999 <https://github.com/rust-lang/rust/issues/51999> for more information
= help: add `#![feature(const_panic)]` to the crate attributes to enable
Copy link
Member

Choose a reason for hiding this comment

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

Might be good to add these feature flags to the test to get any "real" errors.

@camelid
Copy link
Member

camelid commented Sep 6, 2021

@Noble-Mushtak If you change

This pull request fixes issue #88434

to

This pull request fixes #88434

then GitHub will automatically close the issue when this PR is merged.

@bors

This comment has been minimized.

Sign in to view
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 13, 2021
@wesleywiser
Copy link
Member

wesleywiser commented Sep 21, 2021 •
edited
Loading

r? @jackh726 or feel free to re-assign to someone more familiar with borrowck🙂

@jackh726
Copy link
Member

jackh726 commented Oct 6, 2021

r? @nikomatsakis

nikomatsakis
nikomatsakis reviewed Oct 6, 2021
View reviewed changes
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Gave this a first read. I'd like @oli-obk to take a look.

compiler/rustc_borrowck/src/type_check/mod.rs Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@oli-obk oli-obk self-assigned this Oct 13, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Oct 13, 2021 •
edited
Loading

@bors r=nikomatsakis,oli-obk

I want to dig into some (preexisting) things I saw while reviewing, but nothing that should block this change

@bors
Copy link
Contributor

bors commented Oct 13, 2021

📌 Commit 8fc329f has been approved by nikomatsakis,oli-obk

@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 13, 2021
@bors
Copy link
Contributor

bors commented Oct 14, 2021

⌛ Testing commit 8fc329f with merge 2044f12e4c5620666baf28d15401c194cedd02a0...

@bors
Copy link
Contributor

bors commented Oct 14, 2021

💥 Test timed out

@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 14, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Oct 14, 2021

@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 14, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Oct 14, 2021

⌛ Testing commit 8fc329f with merge 0a56eb1...

@bors
Copy link
Contributor

bors commented Oct 14, 2021

☀️ Test successful - checks-actions
Approved by: nikomatsakis,oli-obk
Pushing 0a56eb1 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 14, 2021
@bors bors merged commit 0a56eb1 into rust-lang:master Oct 14, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 14, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0a56eb1): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

This was referenced Oct 15, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikomatsakis nikomatsakis nikomatsakis left review comments

@camelid camelid camelid left review comments

Assignees

@nikomatsakis nikomatsakis

@oli-obk oli-obk

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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.57.0
Development

Successfully merging this pull request may close these issues.

ICE: removal index (is 0) should be < len (is 0) / glacier fixed/81899.rs