Skip to content

Fix regionck failure when converting Index to IndexMut#74960

Merged
bors merged 2 commits intorust-lang:masterfrom
nbdd0121:typeck
Aug 12, 2020
Merged

Fix regionck failure when converting Index to IndexMut#74960
bors merged 2 commits intorust-lang:masterfrom
nbdd0121:typeck

Conversation

@nbdd0121
Copy link
Member

@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

nikomatsakis commented Jul 30, 2020

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 Outdated
Copy link
Member Author

@nbdd0121 nbdd0121 Jul 31, 2020

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
Member Author

@nbdd0121 nbdd0121 Jul 31, 2020

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.

estebank
estebank reviewed Jul 31, 2020
View reviewed changes
@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
Member 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
Collaborator

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
Collaborator

bors commented Aug 11, 2020

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

@bors
Copy link
Collaborator

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

Mark-Simulacrum commented Aug 11, 2020

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

@spastorino
Copy link
Member

spastorino commented Aug 11, 2020

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