NLL: disallow creation of immediately unusable variables #54188

lqd · 2018-09-13T15:19:58Z

Fix #53695

Original description follows

This WIP PR is for discussing the impact of fixing #53695 by injecting a fake read in let patterns.

(Travis will fail, at least the mir-opt suite is failing in its current state)

rust-highfive · 2018-09-13T15:20:04Z

r? @zackmdavis

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

lqd · 2018-09-13T15:38:51Z

r? @nikomatsakis

nikomatsakis

What do you think @lqd -- up for slightly more invasive edits?

nikomatsakis · 2018-09-14T15:06:37Z

src/test/ui/generator/borrowing.nll.stderr

@@ -10,15 +10,16 @@ LL |     };
 error[E0597]: `a` does not live long enough
  --> $DIR/borrowing.rs:24:9
   |
+LL |       let _b = {
+   |           -- borrow later used here


So this message is generated here:

rust/src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs

Line 79 in dfabe4b

"borrow later used here"

Given that we have the location in the MIR in the location variable, it would be quite trivial for us to determine if this is a ReadForMatch statement. The only catch is that we use this same statement both for matches and for this. If we refactor ReadForMatch to be something like this:

FakeRead(FakeReadCause, Place<'tcx>),

with

enum FakeReadCause { ForMatch, ForLet, }

then we could match on this particular case and give a different message. Perhaps something like

-- borrowed value is then later stored into this variable

nikomatsakis

Left a minor nit, but this looks good!

nikomatsakis · 2018-09-14T19:29:17Z

src/librustc/mir/mod.rs

+#[derive(Copy, Clone, RustcEncodable, RustcDecodable, Debug)]
+pub enum FakeReadCause {
+    ForMatch,
+    ForLet,


Let's add some comments and links here.

I think the best option is to move the comments from the point of insertion to here, and then (in those spots) just refer to

// See comments on `FakeReadCause::ForLet

This is also good because we may insert fake read from multiple places, but this centralizes the comment in one clearly identifiable location.

nikomatsakis · 2018-09-14T19:29:44Z

src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs

+                        // Check if the location represents a `FakeRead`, and adapt the error
+                        // message to the `FakeReadCause` it is from: in particular,
+                        // the ones inserted in optimized `let var = <expr>` patterns.
+                        let is_fake_read_for_let = match self.mir.basic_blocks()[location.block]


extract to a helper function...somewhere? lots of random code for this.

nikomatsakis · 2018-09-14T19:29:58Z

src/librustc_mir/build/matches/mod.rs

+                // but in some cases it can affect the borrow checker, as in #53695.
+                // Therefore, we insert a "fake read" here to ensure that we get
+                // appropriate errors.
+                //


this is the comment I would move

nikomatsakis · 2018-09-14T19:30:41Z

src/librustc_mir/build/matches/mod.rs

@@ -157,7 +157,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                            *pre_binding_block,
                            Statement {
                                source_info: pattern_source_info,
-                                kind: StatementKind::ReadForMatch(borrow_temp.clone()),
+                                kind: StatementKind::FakeRead(FakeReadCause::ForMatch, borrow_temp.clone()),


presumably this has a decent-ish comment attached to it, too

lqd · 2018-09-14T20:28:11Z

I've pushed the updated comments, and helper function.

Note: I haven't updated the mir-opt yet so Travis will still fail.

nikomatsakis

Looks good so far!

lqd · 2018-09-15T10:26:44Z

(for anyone possibly not on zulip and following at home: mir-opt test suite updated, should be ready)

nikomatsakis · 2018-09-17T15:38:51Z

@bors r+

bors · 2018-09-17T15:38:55Z

📌 Commit c7587c0aa117894910e9a3cd3c39e9c926ae2a68 has been approved by nikomatsakis

bors · 2018-09-18T11:46:31Z

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

pnkfelix · 2018-09-20T10:30:02Z

cc @michaelwoerister : the above travis failure seems to be a linking problem connected to codegen-units; any advice about how to address this?

Update: further investigation indicates that this bug may indeed be due to a combination of the change added by this PR, plus an unfortunate codegen-unit partitioning inhibiting LLVM's ability to optimize the uses of C away? Further discussion of those details should perhaps be put on #54388.

pnkfelix · 2018-09-20T10:31:55Z

also nominating to discuss in compiler team meeting how best to address such codegen-units+linker problems when they arise (e.g., how to help contributors deal with such problems, either by mentoring them through the problems, or guiding them in how to delegate to specialists in the area)

Some of my questions:

Is it reasonable, in attempting to locally reproduce a bug like this, to manually increase codegen-units to e.g. 99 or 999 units? Or is that just as likely to mask issues?
How deterministic is the codegen-unit partitioning system? E.g. would it be a waste of bors-time to just respond to a bug like the above with a retry command?

pnkfelix · 2018-09-20T12:55:22Z

(also, I/we are confused about the expected behavior of running rustc on the extern-const.fixed file... as far as I can tell, there is no external definition of the global static C for it to link to in another file...)

Update: Filed separate issue regarding this question to #54388.

pnkfelix · 2018-09-20T15:11:13Z

(removing I-nominated tag; will raise question of how to deal with these kinds of test failures with compiler team asynchronously.)

pnkfelix · 2018-09-20T18:57:32Z

@lqd okay, based on the discussion regarding #54388, I think you should comment out the // run-rustfix, replacing it with a fixme that points to #54388.

After that, I'll just make a PR that fixes the test to make its .fixed output consistent across our targets, and as part of that, I'll put the // run-rustfix back in.

`src/test/ui/extern/extern-const.rs` seems to have an inconsistent behaviour across build configurations, possibly non-deterministically. This is tracked in issue 54388. For this PR, disable running rustfix on it because it failed on CI, until that issue is fixed.

lqd · 2018-09-20T23:06:41Z

I've pushed a commit which "unrustfixes" (rustbreaks?) this test

pnkfelix · 2018-09-21T10:34:33Z

@bors r+

bors · 2018-09-21T10:34:34Z

📌 Commit 3bdba74 has been approved by pnkfelix

pnkfelix · 2018-09-21T10:34:49Z

@bors r=nikomatsakis

bors · 2018-09-21T10:34:49Z

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

There's another pull request that is currently being tested, blocking this pull request: std: Check for overflow in str::repeat #54399

bors · 2018-09-21T10:34:50Z

📌 Commit 3bdba74 has been approved by nikomatsakis

bors · 2018-09-22T20:38:27Z

⌛ Testing commit 3bdba74 with merge 4591a24...

NLL: disallow creation of immediately unusable variables Fix #53695 Original description follows ---- This WIP PR is for discussing the impact of fixing #53695 by injecting a fake read in let patterns. (Travis will fail, at least the `mir-opt` suite is failing in its current state)

bors · 2018-09-22T23:07:57Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 4591a24 to master...

rust-highfive assigned zackmdavis Sep 13, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 13, 2018

memoryruins added the A-NLL Area: Non-lexical lifetimes (NLL) label Sep 13, 2018

rust-highfive assigned nikomatsakis and unassigned zackmdavis Sep 13, 2018

This comment has been minimized.

Sign in to view

nikomatsakis requested changes Sep 14, 2018

View reviewed changes

lqd force-pushed the fallout-53695 branch from af73dea to 7dc76ce Compare September 14, 2018 16:21

This comment has been minimized.

Sign in to view

lqd force-pushed the fallout-53695 branch 2 times, most recently from 08f419b to 9f76655 Compare September 14, 2018 19:27

nikomatsakis requested changes Sep 14, 2018

View reviewed changes

nikomatsakis changed the title ~~[WIP] Investigate fallout from NLL allowing creating unusable variables~~ Investigate fallout from NLL allowing creating unusable variables Sep 14, 2018

This comment has been minimized.

Sign in to view

nikomatsakis approved these changes Sep 14, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

lqd force-pushed the fallout-53695 branch from 8661de0 to df5c964 Compare September 15, 2018 01:02

matthewjasper mentioned this pull request Sep 15, 2018

[NLL] Be more permissive when checking access due to Match #53438

Merged

lqd force-pushed the fallout-53695 branch from df5c964 to c7587c0 Compare September 17, 2018 08:25

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 Sep 17, 2018

bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 18, 2018

bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 20, 2018

This comment has been minimized.

Sign in to view

pnkfelix added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-nominated labels Sep 20, 2018

kennytm 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 Sep 20, 2018

pnkfelix mentioned this pull request Sep 20, 2018

What should we guarantee regarding "sort-of unused" extern statics #54388

Closed

pnkfelix removed the I-nominated label Sep 20, 2018

lqd 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 Sep 21, 2018

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 Sep 21, 2018

bors merged commit 3bdba74 into rust-lang:master Sep 22, 2018

bors mentioned this pull request Sep 22, 2018

[nll] borrows that must be valid for a free lifetime should explain why #54229

Merged

lqd deleted the fallout-53695 branch September 23, 2018 12:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NLL: disallow creation of immediately unusable variables #54188

NLL: disallow creation of immediately unusable variables #54188

lqd commented Sep 13, 2018 •

edited by pnkfelix

Loading

rust-highfive commented Sep 13, 2018

lqd commented Sep 13, 2018

This comment has been minimized.

nikomatsakis left a comment

nikomatsakis Sep 14, 2018

This comment has been minimized.

nikomatsakis left a comment

nikomatsakis Sep 14, 2018

nikomatsakis Sep 14, 2018

nikomatsakis Sep 14, 2018

nikomatsakis Sep 14, 2018

This comment has been minimized.

lqd commented Sep 14, 2018

nikomatsakis left a comment

This comment has been minimized.

This comment has been minimized.

lqd commented Sep 15, 2018

nikomatsakis commented Sep 17, 2018

bors commented Sep 17, 2018

bors commented Sep 18, 2018

This comment has been minimized.

pnkfelix commented Sep 20, 2018 •

edited

Loading

pnkfelix commented Sep 20, 2018 •

edited

Loading

pnkfelix commented Sep 20, 2018 •

edited

Loading

pnkfelix commented Sep 20, 2018

pnkfelix commented Sep 20, 2018

lqd commented Sep 20, 2018

pnkfelix commented Sep 21, 2018

bors commented Sep 21, 2018

pnkfelix commented Sep 21, 2018

bors commented Sep 21, 2018

bors commented Sep 21, 2018

bors commented Sep 22, 2018

bors commented Sep 22, 2018

NLL: disallow creation of immediately unusable variables #54188

NLL: disallow creation of immediately unusable variables #54188

Conversation

lqd commented Sep 13, 2018 • edited by pnkfelix Loading

rust-highfive commented Sep 13, 2018

lqd commented Sep 13, 2018

This comment has been minimized.

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Sep 14, 2018

Choose a reason for hiding this comment

This comment has been minimized.

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Sep 14, 2018

Choose a reason for hiding this comment

nikomatsakis Sep 14, 2018

Choose a reason for hiding this comment

nikomatsakis Sep 14, 2018

Choose a reason for hiding this comment

nikomatsakis Sep 14, 2018

Choose a reason for hiding this comment

This comment has been minimized.

lqd commented Sep 14, 2018

nikomatsakis left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

lqd commented Sep 15, 2018

nikomatsakis commented Sep 17, 2018

bors commented Sep 17, 2018

bors commented Sep 18, 2018

This comment has been minimized.

pnkfelix commented Sep 20, 2018 • edited Loading

pnkfelix commented Sep 20, 2018 • edited Loading

pnkfelix commented Sep 20, 2018 • edited Loading

pnkfelix commented Sep 20, 2018

pnkfelix commented Sep 20, 2018

lqd commented Sep 20, 2018

pnkfelix commented Sep 21, 2018

bors commented Sep 21, 2018

pnkfelix commented Sep 21, 2018

bors commented Sep 21, 2018

bors commented Sep 21, 2018

bors commented Sep 22, 2018

bors commented Sep 22, 2018

lqd commented Sep 13, 2018 •

edited by pnkfelix

Loading

pnkfelix commented Sep 20, 2018 •

edited

Loading

pnkfelix commented Sep 20, 2018 •

edited

Loading

pnkfelix commented Sep 20, 2018 •

edited

Loading