Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include bounds from promoted constants in NLL #57202

Merged
merged 3 commits into from
Mar 2, 2019

Conversation

matthewjasper
Copy link
Contributor

@matthewjasper matthewjasper commented Dec 29, 2018

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

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

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
Copy link
Contributor

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

@matthewjasper
Copy link
Contributor Author

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

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

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

@pnkfelix
Copy link
Member

NLL triage. Assigning to self for review.

@pnkfelix pnkfelix assigned pnkfelix and unassigned nikomatsakis Feb 20, 2019
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: promted

#![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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uh.... uh oh?

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bors r+

@bors
Copy link
Contributor

bors commented Feb 27, 2019

📌 Commit 78f9515b2c6de76dd1f51c36199bca590bd93cd7 has been approved by pnkfelix

@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 Feb 27, 2019
@pnkfelix
Copy link
Member

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

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

@matthewjasper
Copy link
Contributor Author

@bors r=pnkfelix

@bors
Copy link
Contributor

bors commented Feb 27, 2019

📌 Commit b45e5bac88971bbb7e81d4ea613318d829068ddb has been approved by pnkfelix

@rust-highfive

This comment has been minimized.

@pnkfelix
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 28, 2019

📌 Commit 255d36208a22a420776ec3ca43a63b2e3c634d7a has been approved by pnkfelix

@pnkfelix
Copy link
Member

pnkfelix commented Feb 28, 2019

@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
Copy link
Member

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
Copy link
Contributor Author

@bors r=pnkfelix

@bors
Copy link
Contributor

bors commented Feb 28, 2019

📌 Commit 4b1b357568cdf9f1c0bf964309c2e1d4088d72c6 has been approved by pnkfelix

@bors
Copy link
Contributor

bors commented Mar 1, 2019

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

@bors 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
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
Copy link
Contributor Author

@bors r=pnkfelix

@bors
Copy link
Contributor

bors commented Mar 1, 2019

📌 Commit 3b93d71 has been approved by pnkfelix

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 1, 2019
@bors
Copy link
Contributor

bors commented Mar 2, 2019

⌛ Testing commit 3b93d71 with merge a21d824...

bors added a commit that referenced this pull request Mar 2, 2019
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
Copy link
Contributor

bors commented Mar 2, 2019

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 2, 2019
@bors bors merged commit 3b93d71 into rust-lang:master Mar 2, 2019
@pnkfelix
Copy link
Member

pnkfelix commented Mar 7, 2019

discussed at T-compiler meeting. beta-accepting.

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Mar 7, 2019
@pietroalbini pietroalbini removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 16, 2019
bors added a commit that referenced this pull request Mar 16, 2019
[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
@matthewjasper matthewjasper deleted the nll-typeck-promoteds branch April 4, 2019 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non-lexical lifetimes (NLL) beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. 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
Development

Successfully merging this pull request may close these issues.

6 participants