Simplify memory categorization #66246

matthewjasper · 2019-11-09T10:51:57Z

With AST borrowck gone, mem_categorization can be simplified, a lot.

cmt_ is now called Place. Most local variable names have been updated to reflect this, but the cat_* methods retain their names.
MemCategorizationContext no longer needs a ScopeTree and always needs an InferCtxt.
Place now uses a similar representation to mir::Place with a Vec of projections.
Upvar places don't include the implicit environment and capture derefs. These are now handled by regionck when needed.
Various types, methods and variants only used by AST borrowck have been removed.
ExprUseVisitor now lives in rustc_typeck::expr_use_visitor.
MemCategorizationContext and Place live in rustc_typeck::mem_categorization.
Place is re-exported in rustc_typeck::expr_use_visitor so that Clippy can access it.

The loss of an error in issue-4335.rs is due to a change in capture inference in ill-formed programs. If any projection from a variable is moved from then we capture that variable by move, whether or not the place being moved from allows this.

Closes #66270

rust-highfive · 2019-11-09T10:52:01Z

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

matthewjasper · 2019-11-09T10:52:45Z

r? @nikomatsakis or @pnkfelix

src/librustc/middle/mem_categorization.rs

bors · 2019-11-11T17:13:12Z

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

bors · 2019-11-14T17:15:47Z

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

JohnCSimon · 2019-11-24T01:49:00Z

Ping from triage
@matthewjasper - can you please address the merge conflicts?
Thank you!

bors · 2019-11-25T04:33:52Z

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

pnkfelix · 2019-11-27T09:40:32Z

r? @pnkfelix

pnkfelix · 2019-11-27T10:06:25Z

src/librustc_typeck/check/regionck.rs

@@ -899,10 +896,6 @@ impl<'a, 'tcx> RegionCtxt<'a, 'tcx> {
            }

            cmt = self.with_mc(|mc| mc.cat_expr_adjusted(expr, cmt, &adjustment))?;
-
-            if let Categorization::Deref(_, mc::BorrowedPtr(_, r_ptr)) = cmt.cat {
-                self.mk_subregion_due_to_dereference(expr.span, expr_region, r_ptr);


do you know what this was originally here for? I just want a note in this PR about why we don't need it, no need to add comments to the source code or anything.

(is this a case where mir-borrowck (and/or nll/type-check) is imposing the same constraint and so we do not need to impose it here?)

That's the case for most of the constraints in this file. This check specifically is to ensure, in the language of NLL, that 'a is live at the point marked:

let x: &'a (i32, i32) = ...; let _ = x.0; //here

This is redundant because:

There's no longer any check that makes use of ReScopes (except for the leak check, but that's hopefully going away soon).

NLL does is own region liveness inference.

I think that we're already adding this relation because all of the regions in the unadjusted expression are already marked live.

src/librustc_typeck/check/regionck.rs

pnkfelix · 2019-11-27T10:39:04Z

This looks great! r=me once you have addressed the nits above.

matthewjasper · 2019-11-27T21:43:40Z

@bors r=pnkfelix

bors · 2019-11-27T21:43:42Z

📌 Commit 5bc1586 has been approved by pnkfelix

bors · 2019-11-28T06:59:08Z

⌛ Testing commit 5bc1586 with merge 223a873de4d7729c3869832b3189f8ddbd17b7c2...

rust-highfive · 2019-11-28T07:06:29Z

The job dist-powerpc64-linux of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

bors · 2019-11-28T07:06:35Z

💔 Test failed - checks-azure

matthewjasper · 2019-11-28T07:53:47Z

@bors retry

bors · 2019-11-28T10:37:03Z

⌛ Testing commit 5bc1586 with merge 4752c05...

Simplify memory categorization With AST borrowck gone, mem_categorization can be simplified, a lot. * `cmt_` is now called `Place`. Most local variable names have been updated to reflect this, but the `cat_*` methods retain their names. * `MemCategorizationContext` no longer needs a `ScopeTree` and always needs an `InferCtxt`. * `Place` now uses a similar representation to `mir::Place` with a `Vec` of projections. * `Upvar` places don't include the implicit environment and capture derefs. These are now handled by `regionck` when needed. * Various types, methods and variants only used by AST borrowck have been removed. * `ExprUseVisitor` now lives in `rustc_typeck::expr_use_visitor`. * `MemCategorizationContext` and `Place` live in `rustc_typeck::mem_categorization`. * `Place` is re-exported in `rustc_typeck::expr_use_visitor` so that Clippy can access it. The loss of an error in `issue-4335.rs` is due to a change in capture inference in ill-formed programs. If any projection from a variable is moved from then we capture that variable by move, whether or not the place being moved from allows this. Closes #66270

bors · 2019-11-28T14:22:40Z

☀️ Test successful - checks-azure
Approved by: pnkfelix
Pushing 4752c05 to master...

rust-highfive · 2019-11-28T14:24:16Z

📣 Toolstate changed by #66246!

Tested on commit 4752c05.
Direct link to PR: #66246

💔 clippy-driver on windows: test-pass → build-fail (cc @mcarton @oli-obk @Manishearth @flip1995 @yaahc @phansch @llogiq, @rust-lang/infra).
💔 clippy-driver on linux: test-pass → build-fail (cc @mcarton @oli-obk @Manishearth @flip1995 @yaahc @phansch @llogiq, @rust-lang/infra).
💔 rls on windows: test-pass → test-fail (cc @Xanewok, @rust-lang/infra).

@mcarton

Tested on commit rust-lang/rust@4752c05. Direct link to PR: <rust-lang/rust#66246> 💔 clippy-driver on windows: test-pass → build-fail (cc @mcarton @oli-obk @Manishearth @flip1995 @yaahc @phansch @llogiq, @rust-lang/infra). 💔 clippy-driver on linux: test-pass → build-fail (cc @mcarton @oli-obk @Manishearth @flip1995 @yaahc @phansch @llogiq, @rust-lang/infra). 💔 rls on windows: test-pass → test-fail (cc @Xanewok, @rust-lang/infra).

Manishearth · 2019-11-28T15:13:20Z

src/librustc_typeck/lib.rs

@@ -83,6 +86,7 @@ mod collect;
 mod constrained_generic_params;
 mod structured_errors;
 mod impl_wf_check;
+mod mem_categorization;


Uh, this needs to be public too, Clippy can't use EUV without memcat

In general we should be very careful about marking existing things as private

Oh, I see, you want us to use Place from EUV, and Categorization isn't a thing anymore

Rustup From rust-lang/rust#66246 changelog: none

@Centril

…-DPC Add regression tests for fixed ICEs Closes rust-lang#61747 (fixed from 1.41.0-nightly (4007d4e 2019-12-01)) Closes rust-lang#66205 (fixed from 1.41.0-nightly (4007d4e 2019-12-01)) Closes rust-lang#66270 (fixed by rust-lang#66246) Closes rust-lang#66868 (fixed by rust-lang#67071) Closes rust-lang#67424 (fixed by rust-lang#67160) Also picking a minor nit up from rust-lang#67071 with 101dd7b r? @Centril

@Centril

…-DPC Add regression tests for fixed ICEs Closes rust-lang#61747 (fixed from 1.41.0-nightly (4007d4e 2019-12-01)) Closes rust-lang#66205 (fixed from 1.41.0-nightly (4007d4e 2019-12-01)) Closes rust-lang#66270 (fixed by rust-lang#66246) Closes rust-lang#66868 (fixed by rust-lang#67071) Closes rust-lang#67424 (fixed by rust-lang#67160) Also picking a minor nit up from rust-lang#67071 with 101dd7b r? @Centril

@Centril

Add regression tests for fixed ICEs Closes rust-lang#61747 (fixed from 1.41.0-nightly (4007d4e 2019-12-01)) Closes rust-lang#66205 (fixed from 1.41.0-nightly (4007d4e 2019-12-01)) Closes rust-lang#66270 (fixed by rust-lang#66246) Closes rust-lang#67424 (fixed by rust-lang#67160) Also picking a minor nit up from rust-lang#67071 with 101dd7b r? @Centril

@Centril

Add regression tests for fixed ICEs Closes rust-lang#61747 (fixed from 1.41.0-nightly (4007d4e 2019-12-01)) Closes rust-lang#66205 (fixed from 1.41.0-nightly (4007d4e 2019-12-01)) Closes rust-lang#66270 (fixed by rust-lang#66246) Closes rust-lang#67424 (fixed by rust-lang#67160) Also picking a minor nit up from rust-lang#67071 with 101dd7b r? @Centril

rust-highfive assigned petrochenkov Nov 9, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 9, 2019

rust-highfive assigned nikomatsakis and unassigned petrochenkov Nov 9, 2019

tesuji reviewed Nov 9, 2019

View reviewed changes

src/librustc/middle/mem_categorization.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Centril reviewed Nov 10, 2019

View reviewed changes

src/librustc/middle/mem_categorization.rs Outdated Show resolved Hide resolved

src/librustc/middle/mem_categorization.rs Outdated Show resolved Hide resolved

matthewjasper force-pushed the simplify-mem-cat branch from 0974f81 to 0ce49a5 Compare November 10, 2019 18:27

Centril mentioned this pull request Nov 11, 2019

Destructuring of incorrectly defined struct produces ICE #66270

Closed

matthewjasper force-pushed the simplify-mem-cat branch from 0ce49a5 to 3496922 Compare November 11, 2019 21:38

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 24, 2019

matthewjasper force-pushed the simplify-mem-cat branch from 0de037c to 17b6cd0 Compare November 24, 2019 20:58

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

rust-highfive assigned pnkfelix and unassigned nikomatsakis Nov 27, 2019

pnkfelix reviewed Nov 27, 2019

View reviewed changes

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

pnkfelix reviewed Nov 27, 2019

View reviewed changes

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

pnkfelix 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 27, 2019

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 27, 2019

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 Nov 28, 2019

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 Nov 28, 2019

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 28, 2019

bors merged commit 5bc1586 into rust-lang:master Nov 28, 2019

rust-highfive mentioned this pull request Nov 28, 2019

clippy-driver no longer builds after rust-lang/rust#66246 #66840

Closed

Manishearth reviewed Nov 28, 2019

View reviewed changes

Manishearth mentioned this pull request Nov 28, 2019

Rustup rust-lang/rust-clippy#4857

Merged

bors added a commit to rust-lang/rust-clippy that referenced this pull request Nov 29, 2019

Auto merge of #4857 - rust-lang:rustup, r=matthewjasper,oli-obk

4780ac0

Rustup From rust-lang/rust#66246 changelog: none

JohnTitor mentioned this pull request Dec 22, 2019

Add regression tests for fixed ICEs #67543

Merged

matthewjasper deleted the simplify-mem-cat branch December 31, 2019 14:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify memory categorization #66246

Simplify memory categorization #66246

matthewjasper commented Nov 9, 2019 •

edited

Loading

rust-highfive commented Nov 9, 2019

matthewjasper commented Nov 9, 2019

This comment has been minimized.

bors commented Nov 11, 2019

bors commented Nov 14, 2019

JohnCSimon commented Nov 24, 2019

bors commented Nov 25, 2019

pnkfelix commented Nov 27, 2019

pnkfelix Nov 27, 2019 •

edited

Loading

pnkfelix Nov 27, 2019

matthewjasper Nov 27, 2019

pnkfelix commented Nov 27, 2019

matthewjasper commented Nov 27, 2019

bors commented Nov 27, 2019

bors commented Nov 28, 2019

rust-highfive commented Nov 28, 2019

bors commented Nov 28, 2019

matthewjasper commented Nov 28, 2019

bors commented Nov 28, 2019

bors commented Nov 28, 2019

rust-highfive commented Nov 28, 2019

Manishearth Nov 28, 2019

Manishearth Nov 28, 2019

Manishearth Nov 28, 2019

Simplify memory categorization #66246

Simplify memory categorization #66246

Conversation

matthewjasper commented Nov 9, 2019 • edited Loading

rust-highfive commented Nov 9, 2019

matthewjasper commented Nov 9, 2019

This comment has been minimized.

bors commented Nov 11, 2019

bors commented Nov 14, 2019

JohnCSimon commented Nov 24, 2019

bors commented Nov 25, 2019

pnkfelix commented Nov 27, 2019

pnkfelix Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

pnkfelix Nov 27, 2019

Choose a reason for hiding this comment

matthewjasper Nov 27, 2019

Choose a reason for hiding this comment

pnkfelix commented Nov 27, 2019

matthewjasper commented Nov 27, 2019

bors commented Nov 27, 2019

bors commented Nov 28, 2019

rust-highfive commented Nov 28, 2019

bors commented Nov 28, 2019

matthewjasper commented Nov 28, 2019

bors commented Nov 28, 2019

bors commented Nov 28, 2019

rust-highfive commented Nov 28, 2019

Manishearth Nov 28, 2019

Choose a reason for hiding this comment

Manishearth Nov 28, 2019

Choose a reason for hiding this comment

Manishearth Nov 28, 2019

Choose a reason for hiding this comment

matthewjasper commented Nov 9, 2019 •

edited

Loading

pnkfelix Nov 27, 2019 •

edited

Loading