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 regionck failure when converting Index to IndexMut #74960

Merged
merged 2 commits into from
Aug 12, 2020
Merged

Fix regionck failure when converting Index to IndexMut #74960

merged 2 commits into from
Aug 12, 2020

Conversation

nbdd0121
Copy link
Contributor

@nbdd0121 nbdd0121 commented Jul 30, 2020 •
edited
Loading

Fixes #74933

Consider an overloaded index expression base[index]. Without knowing whether it will be mutated, this will initially be desugared into <U as Index<T>>::index(&base, index) for some U and T. Let V be the expr_ty_adjusted of index.

If this expression ends up being used in any mutable context (or used in a function call with &mut self receiver before #72280), we will need to fix it up. The current code will rewrite it to <U as IndexMut<V>>::index_mut(&mut base, index). In most cases this is fine as V will be equal to T, however this is not always true when V/T are references, as they may have different region.

This issue is quite subtle before #72280 as this code path is only used to fixup function receivers, but after #72280 we've made this a common path.

The solution is basically just rewrite it to <U as IndexMut<T>>::index_mut(&mut base, index). T can retrieved in the fixup path using node_substs.

@nbdd0121

This comment has been minimized.

Sign in to view
@nikomatsakis
Copy link
Contributor

r? @nikomatsakis I suppose, will try to check in on this soon

nbdd0121
nbdd0121 commented Jul 31, 2020
View reviewed changes
src/librustc_typeck/check/place_op.rs
@@ -246,15 +246,18 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

match expr.kind {
hir::ExprKind::Index(ref base_expr, ref index_expr) => {
// We need to get the final type in case dereferences were needed for the trait
// to apply (#72002).
let index_expr_ty = self.typeck_results.borrow().expr_ty_adjusted(index_expr);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I narrowed down the issue to this line. This will cause the actual type (and lifetime) of index expression to be used, rather than a supertype of 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 am not sure if there is a way to retrieve the input_ty in try_index_step. If we could we could just use it as index_expr_ty.

@nbdd0121 nbdd0121 changed the title [WIP] Fix regionck failure when converting Index to IndexMut Fix regionck failure when converting Index to IndexMut Jul 31, 2020
@nbdd0121 nbdd0121 marked this pull request as ready for review July 31, 2020 02:03
@JohnTitor JohnTitor added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 31, 2020
@nbdd0121 nbdd0121 mentioned this pull request Jul 31, 2020
Closed
@nbdd0121
Copy link
Contributor Author

nbdd0121 commented Aug 7, 2020

@nikomatsakis have you had time to go through this? The issue it fixes is P-critical and regression-from-stable-to-beta, so we'd need to address it asap and backport to beta.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Aug 11, 2020 •
edited
Loading

@bors r+

sorry. I don't 100% understand what went wrong, but this fix seems good.

@bors
Copy link
Contributor

bors commented Aug 11, 2020

📌 Commit 000f5cd has been approved by nikomatsakis

@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 Aug 11, 2020
@bors
Copy link
Contributor

bors commented Aug 11, 2020

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Aug 11, 2020

📌 Commit 000f5cd has been approved by nikomatsakis

@Mark-Simulacrum Mark-Simulacrum added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 11, 2020
@Mark-Simulacrum
Copy link
Member

beta-nominating as a fix for for a stable-to-beta regression.

@spastorino
Copy link
Member

Adding T-compiler label so this can be picked up by our automated prioritization tool and added to the following T-compiler weekly meeting agenda.

@spastorino spastorino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 11, 2020
@JohnTitor JohnTitor mentioned this pull request Aug 12, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 12, 2020
@bors
Auto merge of rust-lang#75436 - JohnTitor:rollup-ss0lxds, r=JohnTitor
c94ed5c 
Rollup of 9 pull requests

Successful merges:

 - rust-lang#74521 (older toolchains not valid anymore)
 - rust-lang#74960 (Fix regionck failure when converting Index to IndexMut)
 - rust-lang#75234 (Update asm! documentation in unstable book)
 - rust-lang#75368 (Move to doc links inside the prelude)
 - rust-lang#75371 (Move to doc links inside std/time.rs)
 - rust-lang#75394 (Add a function to `TyCtxt` for computing an `Allocation` for a `static` item's initializer)
 - rust-lang#75395 (Switch to intra-doc links in library/std/src/os/*/fs.rs)
 - rust-lang#75422 (Accept more safety comments)
 - rust-lang#75424 (fix wrong word in documentation)

Failed merges:

r? @ghost
@bors bors merged commit 43babed into rust-lang:master Aug 12, 2020
@spastorino
Copy link
Member

spastorino commented Aug 13, 2020 •
edited
Loading

discussed in T-compiler meeting.

@rustbot modify labels: beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Aug 13, 2020
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 19, 2020
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Aug 19, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 21, 2020
@bors
Auto merge of rust-lang#75722 - Mark-Simulacrum:beta-next, r=Mark-Sim…
cfbc6d4 
…ulacrum

[beta] backports

* Fix regionck failure when converting Index to IndexMut rust-lang#74960
* Update RELEASES.md for 1.46.0 rust-lang#74744
* allow escaping bound vars when normalizing `ty::Opaque` rust-lang#75443

r? @ghost
@nbdd0121 nbdd0121 deleted the typeck branch September 4, 2021 02:55
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estebank estebank estebank left review comments

@nikomatsakis nikomatsakis nikomatsakis approved these changes

Assignees

@nikomatsakis nikomatsakis

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. 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.47.0
Development

Successfully merging this pull request may close these issues.

Lifetime error when indexing with borrowed index in beta but not in stable
9 participants
@nbdd0121 @nikomatsakis @bors @Mark-Simulacrum @spastorino @estebank @cuviper @JohnTitor @rustbot