Include bounds from promoted constants in NLL #57202

matthewjasper · 2018-12-29T17:52:37Z

Previously a promoted function wouldn't have its bound propagated out to
the main function body.

When we visit a promoted, we now type check the MIR of the promoted
and transfer any lifetime constraints to back to the main function's MIR.

Fixes #57170

r? @nikomatsakis

matthewjasper · 2018-12-30T00:00:34Z

I have an alternative approach that does the location adjustment in the BorrowCheckContext which can be seen here: https://github.com/matthewjasper/rust/tree/nll-typeck-promoteds-alternative. This feels a bit more principled, and ends up being a smaller diff.

nikomatsakis · 2019-01-02T18:37:51Z

@matthewjasper would you like me to compare the two branches, then?

matthewjasper · 2019-01-02T18:56:06Z

Yes, or suggest a third approach if you feel neither is satisfactory.

matthewjasper · 2019-01-17T20:10:07Z

I've pushed (yet) another version that avoids changing other parts of typeck.

pnkfelix · 2019-02-20T21:20:52Z

NLL triage. Assigning to self for review.

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

pnkfelix · 2019-02-27T15:00:57Z

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

+        let mut constraints = Default::default();
+        let mut closure_bounds = Default::default();
+        if let Some(ref mut bcx) = self.cx.borrowck_context {
+            // Don't try to add borrow_region facts for the promted MIR


typo: promted

pnkfelix · 2019-02-27T15:05:25Z

src/test/ui/nll/issue-48697.rs

 #![feature(nll)]

 fn foo(x: &i32) -> &i32 {
    let z = 4;
    let f = &|y| y;
    let k = f(&z);
-    f(x)
+    f(x) //~ cannot return value referencing local variable


uh.... uh oh?

Is this because we aren't inferring a sufficiently general type for the closure?

but I guess the "good" news is that AST-borrowck also rejects this code, so the impact of this "regression" will be limited... maybe...?

pnkfelix · 2019-02-27T15:09:45Z

@bors r+

bors · 2019-02-27T15:09:47Z

📌 Commit 78f9515b2c6de76dd1f51c36199bca590bd93cd7 has been approved by pnkfelix

pnkfelix · 2019-02-27T15:15:00Z

I'm r+'ing this but I'm also nominating it for discussion at the NLL meeting. In particular, I want us to make sure we're all okay with the change from accepting the test from #48697 to rejecting it. (My current hypothesis is that its being rejected because of known issues with inferring higher-ranked lifetime for closures; see also discussion on #58052.)

pnkfelix · 2019-02-27T15:16:06Z

oh and I'm also beta nominating, since this is fixing a soundness issue and is relatively straight-forward.

matthewjasper · 2019-02-27T19:00:51Z

@bors r=pnkfelix

bors · 2019-02-27T19:00:52Z

📌 Commit b45e5bac88971bbb7e81d4ea613318d829068ddb has been approved by pnkfelix

pnkfelix · 2019-02-28T09:56:56Z

@bors r+

bors · 2019-02-28T09:56:57Z

📌 Commit 255d36208a22a420776ec3ca43a63b2e3c634d7a has been approved by pnkfelix

pnkfelix · 2019-02-28T09:57:21Z

@matthewjasper if you get a chance it might be nice to make a unit test for whatever problem was uncovered by the ICE during bootstrap?

pnkfelix · 2019-02-28T15:39:09Z

discussed at T-compiler meeting. We'll revisit the question of whether or not to backport this to beta next week. (@nikomatsakis wants to review the approach.)

matthewjasper · 2019-02-28T20:00:35Z

@bors r=pnkfelix

bors · 2019-02-28T20:00:36Z

📌 Commit 4b1b357568cdf9f1c0bf964309c2e1d4088d72c6 has been approved by pnkfelix

bors · 2019-03-01T13:33:07Z

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

Previously, a promoted that contains a function item wouldn't have the function items bounds propagated to the main function body.

Type annotations are shared between the MIR of a function and the promoted constants for that function, so keep them in the type checker when we check the promoted MIR.

matthewjasper · 2019-03-01T19:51:28Z

@bors r=pnkfelix

bors · 2019-03-01T19:51:29Z

📌 Commit 3b93d71 has been approved by pnkfelix

bors · 2019-03-02T04:39:09Z

⌛ Testing commit 3b93d71 with merge a21d824...

@nikomatsakis

Include bounds from promoted constants in NLL Previously a promoted function wouldn't have its bound propagated out to the main function body. When we visit a promoted, we now type check the MIR of the promoted and transfer any lifetime constraints to back to the main function's MIR. Fixes #57170 r? @nikomatsakis

bors · 2019-03-02T07:33:50Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: pnkfelix
Pushing a21d824 to master...

pnkfelix · 2019-03-07T15:17:11Z

discussed at T-compiler meeting. beta-accepting.

@ghost

[beta] Rollup backports Cherry-picked: * Include bounds from promoted constants in NLL #57202 * Warning period for detecting nested impl trait #58608 * Don't promote function calls to nonpromotable things #58784 * Make migrate mode work at item level granularity #58788 * Expand where negative supertrait specific error is shown #58861 * Expand where negative supertrait specific error is shown #58861 Rolled up: * [BETA] Update cargo #59217 r? @ghost

rust-highfive assigned nikomatsakis Dec 29, 2018

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

matthewjasper force-pushed the nll-typeck-promoteds branch 2 times, most recently from ca095d9 to 78f9515 Compare January 17, 2019 20:04

pnkfelix mentioned this pull request Jan 21, 2019

NLL: turn on borrowck=migrate by default on 2015 edition #57804

Closed

pnkfelix assigned pnkfelix and unassigned nikomatsakis Feb 20, 2019

pnkfelix reviewed Feb 27, 2019

View reviewed changes

src/librustc_mir/borrow_check/nll/type_check/mod.rs Outdated Show resolved Hide resolved

pnkfelix reviewed Feb 27, 2019

View reviewed changes

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

pnkfelix added A-NLL Area: Non Lexical Lifetimes (NLL) I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. beta-nominated Nominated for backporting to the compiler in the beta channel. labels Feb 27, 2019

pnkfelix mentioned this pull request Feb 27, 2019

Borrow checker incorrectly accepts code when calling function with complex lifetime bounds through a fn pointer #57170

Closed

matthewjasper force-pushed the nll-typeck-promoteds branch from 78f9515 to b45e5ba Compare February 27, 2019 19:00

This comment has been minimized.

Sign in to view

pnkfelix removed the I-nominated label Feb 28, 2019

matthewjasper force-pushed the nll-typeck-promoteds branch from 255d362 to 4b1b357 Compare February 28, 2019 19:58

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 1, 2019

matthewjasper added 3 commits March 1, 2019 18:53

Include bounds from promoted constants in NLL

60eeed3

Previously, a promoted that contains a function item wouldn't have the function items bounds propagated to the main function body.

Remove unnecessary parameter

848c252

Handle type annotations in promoted MIR correctly

3b93d71

Type annotations are shared between the MIR of a function and the promoted constants for that function, so keep them in the type checker when we check the promoted MIR.

matthewjasper force-pushed the nll-typeck-promoteds branch from 4b1b357 to 3b93d71 Compare March 1, 2019 19:50

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 Mar 1, 2019

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 2, 2019

bors merged commit 3b93d71 into rust-lang:master Mar 2, 2019

pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Mar 7, 2019

pietroalbini mentioned this pull request Mar 16, 2019

[beta] Rollup backports #59235

Merged

pietroalbini removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 16, 2019

matthewjasper deleted the nll-typeck-promoteds branch April 4, 2019 21:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include bounds from promoted constants in NLL #57202

Include bounds from promoted constants in NLL #57202

matthewjasper commented Dec 29, 2018 •

edited

Loading

matthewjasper commented Dec 30, 2018

nikomatsakis commented Jan 2, 2019

matthewjasper commented Jan 2, 2019

matthewjasper commented Jan 17, 2019

pnkfelix commented Feb 20, 2019

pnkfelix Feb 27, 2019

pnkfelix Feb 27, 2019

pnkfelix Feb 27, 2019

pnkfelix commented Feb 27, 2019

bors commented Feb 27, 2019

pnkfelix commented Feb 27, 2019

pnkfelix commented Feb 27, 2019

matthewjasper commented Feb 27, 2019

bors commented Feb 27, 2019

This comment has been minimized.

pnkfelix commented Feb 28, 2019

bors commented Feb 28, 2019

pnkfelix commented Feb 28, 2019 •

edited

Loading

pnkfelix commented Feb 28, 2019

matthewjasper commented Feb 28, 2019

bors commented Feb 28, 2019

bors commented Mar 1, 2019

matthewjasper commented Mar 1, 2019

bors commented Mar 1, 2019

bors commented Mar 2, 2019

bors commented Mar 2, 2019

pnkfelix commented Mar 7, 2019

Include bounds from promoted constants in NLL #57202

Include bounds from promoted constants in NLL #57202

Conversation

matthewjasper commented Dec 29, 2018 • edited Loading

matthewjasper commented Dec 30, 2018

nikomatsakis commented Jan 2, 2019

matthewjasper commented Jan 2, 2019

matthewjasper commented Jan 17, 2019

pnkfelix commented Feb 20, 2019

pnkfelix Feb 27, 2019

Choose a reason for hiding this comment

pnkfelix Feb 27, 2019

Choose a reason for hiding this comment

pnkfelix Feb 27, 2019

Choose a reason for hiding this comment

pnkfelix commented Feb 27, 2019

bors commented Feb 27, 2019

pnkfelix commented Feb 27, 2019

pnkfelix commented Feb 27, 2019

matthewjasper commented Feb 27, 2019

bors commented Feb 27, 2019

This comment has been minimized.

pnkfelix commented Feb 28, 2019

bors commented Feb 28, 2019

pnkfelix commented Feb 28, 2019 • edited Loading

pnkfelix commented Feb 28, 2019

matthewjasper commented Feb 28, 2019

bors commented Feb 28, 2019

bors commented Mar 1, 2019

matthewjasper commented Mar 1, 2019

bors commented Mar 1, 2019

bors commented Mar 2, 2019

bors commented Mar 2, 2019

pnkfelix commented Mar 7, 2019

matthewjasper commented Dec 29, 2018 •

edited

Loading

pnkfelix commented Feb 28, 2019 •

edited

Loading