Fix up autoderef when reborrowing #72280

nbdd0121 · 2020-05-16T18:45:15Z

Currently (f)() and f.call_mut() behaves differently if expression f contains autoderef in it. This causes a weird error in #72225.

When f is type checked, Deref is used (this is expected as we can't yet determine if we should use Fn or FnMut). When subsequently we determine the actual trait to be used, when using the f.call_mut() syntax the Deref is patched to DerefMut, while for the (f)() syntax case it is not.

This PR replicates the fixup for the first case.

Fixes #72225
Fixes #68590

rust-highfive · 2020-05-16T18:45:18Z

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

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

nbdd0121 · 2020-05-17T17:21:35Z

It ended up that #68590 is caused by a similar issue. When a implicit mutable reborrow happens, it is equivalent to &mut *expr, thus expr is ought to be typechecked with needs = Needs::MutPlace. The typechecker does not know this beforehand, so what we should do is to fix it up later with a similar method.

bors · 2020-05-19T15:29:06Z

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

eddyb · 2020-05-23T17:10:13Z

r? @nikomatsakis

nikomatsakis

Gah, ran out of time for reviews today, but here is one tiny nit =)

src/librustc_typeck/check/coercion.rs

nbdd0121 · 2020-05-27T00:37:35Z

Gah, ran out of time for reviews today, but here is one tiny nit =)

BTW it is recommended to review commit by commit, as I've intentionally separated the PR into individually meaningful commits.

nikomatsakis

What do you think of this refactoring idea I describe below?

src/librustc_typeck/check/reconciliation.rs

src/librustc_typeck/check/coercion.rs

src/librustc_typeck/check/callee.rs

nikomatsakis · 2020-06-12T21:30:12Z

@nbdd0121 Apologies for the slow review. Took me a bit to grok this PR. I'll try to be snappier from now on (I've gotten on top of my backlog)

nbdd0121 · 2020-06-14T02:17:50Z

I checked all callers of apply_adjustments and nothing seems problematic to me if we move the logic to apply_adjustments.

I also checked all construction of Adjust::Borrow:

check/method/confirm.rs, check/callee.rs, check/coercion perform mutable reborrows that require fixups, so good.
check/op.rs (LHS of op assign), check/expr.rs (unary deref), check/mod.rs (index step) can perform mutable reborrows, but the places are type checked with Needs::MutPlace already so we would be doing wasteful work trying to fix them again (will do be some further method resolutions that ends up as a no-op).

To avoid wasteful call into convert_place_derefs_to_mutable, we could just add additional argument to apply_adjustments to indicate if such a check is needed.

Alternatively, we can modify these three places to use Needs::None instead of Needs::MutPlace to save a little bit time when they do the the first pass, and have apply_adjustments fix them up to use mutable variants. Doing so have the benefit that all mutable reborrows are now handled in one single place, and we wouldn't have to worry about related bugs because apply_adjustments just magically have all Deref/Index mutability issues handled.

What do you think? @nikomatsakis

This can live inside FnCtxt rather than ConfirmContext, and would be useful for other operations as well.

nbdd0121 · 2020-06-15T15:32:56Z

Refactoring idea:
We might as well just remove all Needs, and typecheck all expressions with Needs::None. If an expression is used in assignment/mutable binding/(auto) borrow then use the fixup logic. This has an advantage that only one set of logic will lead to DerefMut/IndexMut and we don't need to pass Needs around anymore.

nikomatsakis · 2020-06-15T15:51:51Z

@nbdd0121 remind me, the Needs logic is the "top-down" logic that lets us first check things with mutable mode when it is "syntactically obvious"? (e.g., &mut foo.bar)

If so, I have indeed contemplating removing it and relying solely on the "fixup" path that we need to have anyway.

nikomatsakis · 2020-06-15T15:56:39Z

Regarding your other comments, I would prefer to not have an additional argument indicating whether the fixup is needed unless we have evidence that it's a performance hazard, just because I think it's better to keep things simple. If we can refactor away Needs altogether, so much the better!

nbdd0121 · 2020-06-15T16:02:50Z

@nbdd0121 remind me, the Needs logic is the "top-down" logic that lets us first check things with mutable mode when it is "syntactically obvious"? (e.g., &mut foo.bar)

If so, I have indeed contemplating removing it and relying solely on the "fixup" path that we need to have anyway.

Yes, Needs is top down. I am working on the refactoring right now, and we can do a try build and rust-timer queue to make sure nothing breaks and performance does not regress.

bors · 2020-06-16T10:43:29Z

⌛ Trying commit 5cedf5d with merge d6a8b4706fb7d366174f875dc50d1b591c21d310...

nikomatsakis

Gave it a quick skim, seems like a massive simplification, but I want to re-review when considering the full PR -- I feel like I'm missing something, maybe it's almost too easy now. ;) Anyway let's look at the perf results.

src/librustc_typeck/check/place_op.rs

bors · 2020-06-16T13:34:56Z

☀️ Try build successful - checks-azure
Build commit: d6a8b4706fb7d366174f875dc50d1b591c21d310 (d6a8b4706fb7d366174f875dc50d1b591c21d310)

rust-timer · 2020-06-16T13:34:58Z

Queued d6a8b4706fb7d366174f875dc50d1b591c21d310 with parent c8a9c34, future comparison URL.

rust-timer · 2020-06-16T18:18:16Z

Finished benchmarking try commit (d6a8b4706fb7d366174f875dc50d1b591c21d310): comparison url.

nikomatsakis · 2020-06-16T18:48:21Z

OK, perf looks neutral, that's good.

nikomatsakis

This looks great! I left a few comment nits (here, and in my previous review), but r=me once those are satisfied!

src/librustc_typeck/check/mod.rs

nbdd0121 · 2020-06-16T21:57:07Z

@bors r=nikomatsakis

bors · 2020-06-16T21:57:08Z

@nbdd0121: 🔑 Insufficient privileges: Not in reviewers

nbdd0121 · 2020-06-16T21:59:08Z

r? @nikomatsakis

nikomatsakis · 2020-06-17T13:29:58Z

@bors r+

bors · 2020-06-17T13:29:59Z

📌 Commit 2b7d858 has been approved by nikomatsakis

@ghost

Rollup of 10 pull requests Successful merges: - rust-lang#72280 (Fix up autoderef when reborrowing) - rust-lang#72785 (linker: MSVC supports linking static libraries as a whole archive) - rust-lang#73011 (first stage of implementing LLVM code coverage) - rust-lang#73044 (compiletest: Add directives to detect sanitizer support) - rust-lang#73054 (memory access sanity checks: abort instead of panic) - rust-lang#73136 (Change how compiler-builtins gets many CGUs) - rust-lang#73280 (Add E0763) - rust-lang#73317 (bootstrap: read config from $RUST_BOOTSTRAP_CONFIG) - rust-lang#73350 (bootstrap/install.rs: support a nonexistent `prefix` in `x.py install`) - rust-lang#73352 (Speed up bootstrap a little.) Failed merges: r? @ghost

Add UI test for issue 73592 It happens that rust-lang#72280 accidentally fixed a bug which is later discovered in rust-lang#73592. This PR adds a UI test to prevent future regression. Closes rust-lang#73592

Fix regionck failure when converting Index to IndexMut Fixes rust-lang#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 rust-lang#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 rust-lang#72280 as this code path is only used to fixup function receivers, but after rust-lang#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`.

rust-highfive assigned eddyb May 16, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 16, 2020

nbdd0121 force-pushed the typeck branch from d3cefc2 to 4f7134f Compare May 16, 2020 19:48

nbdd0121 force-pushed the typeck branch from d486e04 to 422b4c0 Compare May 19, 2020 16:54

nbdd0121 changed the title ~~Fix up autoderef when calling a FnMut~~ Fix up autoderef when reborrowing May 23, 2020

rust-highfive assigned nikomatsakis and unassigned eddyb May 23, 2020

nikomatsakis reviewed May 26, 2020

View reviewed changes

src/librustc_typeck/check/coercion.rs Outdated Show resolved Hide resolved

Elinvynia added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 3, 2020

nikomatsakis requested changes Jun 12, 2020

View reviewed changes

src/librustc_typeck/check/reconciliation.rs Outdated Show resolved Hide resolved

src/librustc_typeck/check/coercion.rs Outdated Show resolved Hide resolved

src/librustc_typeck/check/callee.rs Show resolved Hide resolved

nbdd0121 added 4 commits June 15, 2020 00:56

Move convert_place_derefs_to_mutable out from check/method/confirm.rs

5393a29

This can live inside FnCtxt rather than ConfirmContext, and would be useful for other operations as well.

Add some comments to librustc_typeck/check/callee.rs

fb0793c

Fix up autoderef when performing mutable auto borrow

8121d2e

Add ui tests for issue 68590 and 72225

4710f85

nbdd0121 force-pushed the typeck branch from 422b4c0 to 4710f85 Compare June 15, 2020 00:10

Use expr_ty_adjusted in convert_place_op_to_mutable

e390acd

nikomatsakis approved these changes Jun 16, 2020

View reviewed changes

src/librustc_typeck/check/place_op.rs Show resolved Hide resolved

src/librustc_typeck/check/place_op.rs Show resolved Hide resolved

nikomatsakis added S-waiting-on-perf Status: Waiting on a perf run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 16, 2020

nikomatsakis requested changes Jun 16, 2020

View reviewed changes

src/librustc_typeck/check/mod.rs Outdated Show resolved Hide resolved

Add some comments related to place op typeck

2b7d858

nikomatsakis approved these changes Jun 17, 2020

View reviewed changes

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jun 17, 2020

RalfJung mentioned this pull request Jun 19, 2020

Rollup of 10 pull requests #73504

Merged

bors merged commit 70622db into rust-lang:master Jun 19, 2020

LeSeulArtichaut mentioned this pull request Jun 21, 2020

Incorrect unused mut warning in 1.46.0-nightly (f455e46ea 2020-06-20) #73592

Closed

nbdd0121 mentioned this pull request Jun 22, 2020

Add UI test for issue 73592 #73634

Merged

Aaron1011 mentioned this pull request Jul 10, 2020

Fix hygiene issues in declare_program! and declare_loader! solana-labs/solana#10905

Merged

spastorino mentioned this pull request Jul 30, 2020

Lifetime error when indexing with borrowed index in beta but not in stable #74933

Closed

nbdd0121 mentioned this pull request Jul 31, 2020

Fix regionck failure when converting Index to IndexMut #74960

Merged

jonas-schievink mentioned this pull request Aug 18, 2020

Index chosen over IndexMut incorrectly, with a confusing error message. #75680

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix up autoderef when reborrowing #72280

Fix up autoderef when reborrowing #72280

nbdd0121 commented May 16, 2020 •

edited

Loading

rust-highfive commented May 16, 2020

nbdd0121 commented May 17, 2020 •

edited

Loading

bors commented May 19, 2020

eddyb commented May 23, 2020

nikomatsakis left a comment

nbdd0121 commented May 27, 2020

nikomatsakis left a comment

nikomatsakis commented Jun 12, 2020

nbdd0121 commented Jun 14, 2020

nbdd0121 commented Jun 15, 2020

nikomatsakis commented Jun 15, 2020

nikomatsakis commented Jun 15, 2020

nbdd0121 commented Jun 15, 2020

bors commented Jun 16, 2020

nikomatsakis left a comment

bors commented Jun 16, 2020

rust-timer commented Jun 16, 2020

rust-timer commented Jun 16, 2020

nikomatsakis commented Jun 16, 2020

nikomatsakis left a comment •

edited

Loading

nbdd0121 commented Jun 16, 2020

bors commented Jun 16, 2020

nbdd0121 commented Jun 16, 2020

nikomatsakis commented Jun 17, 2020

bors commented Jun 17, 2020

Fix up autoderef when reborrowing #72280

Fix up autoderef when reborrowing #72280

Conversation

nbdd0121 commented May 16, 2020 • edited Loading

rust-highfive commented May 16, 2020

nbdd0121 commented May 17, 2020 • edited Loading

bors commented May 19, 2020

eddyb commented May 23, 2020

nikomatsakis left a comment

Choose a reason for hiding this comment

nbdd0121 commented May 27, 2020

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis commented Jun 12, 2020

nbdd0121 commented Jun 14, 2020

nbdd0121 commented Jun 15, 2020

nikomatsakis commented Jun 15, 2020

nikomatsakis commented Jun 15, 2020

nbdd0121 commented Jun 15, 2020

bors commented Jun 16, 2020

nikomatsakis left a comment

Choose a reason for hiding this comment

bors commented Jun 16, 2020

rust-timer commented Jun 16, 2020

rust-timer commented Jun 16, 2020

nikomatsakis commented Jun 16, 2020

nikomatsakis left a comment • edited Loading

Choose a reason for hiding this comment

nbdd0121 commented Jun 16, 2020

bors commented Jun 16, 2020

nbdd0121 commented Jun 16, 2020

nikomatsakis commented Jun 17, 2020

bors commented Jun 17, 2020

nbdd0121 commented May 16, 2020 •

edited

Loading

nbdd0121 commented May 17, 2020 •

edited

Loading

nikomatsakis left a comment •

edited

Loading