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

lifetime inference regression in closure #47524

Closed
cuviper opened this issue Jan 17, 2018 · 8 comments · Fixed by #59114
Closed

lifetime inference regression in closure #47524

cuviper opened this issue Jan 17, 2018 · 8 comments · Fixed by #59114
Assignees
Labels
A-borrow-checker Area: The borrow checker A-NLL Area: Non-lexical lifetimes (NLL) C-bug Category: This is a bug. NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@cuviper
Copy link
Member

cuviper commented Jan 17, 2018

The owning_ref crate has a couple test failures starting with Rust 1.23, which worked fine on 1.22. It is fixed by Kimundi/owning-ref-rs#43, but I wanted to see if this is a deliberate compiler change. The only compatibility note for 1.23 that sounds like it might be related is regarding #45852, but I'm not sure.

Reduced example, playground:

fn map<F, T, U>(x: &T, f: F) -> &U
where
    F: FnOnce(&T) -> &U,
{
    f(x)
}

fn borrow<'a>(a: &'a &[i32; 2]) -> &'a i32 {
    &a[0]
}

fn main() {
    let foo = [413, 612];
    let bar = &foo;

    map(&bar, borrow); // OK

    map(&bar, |a: &&[i32; 2]| &a[0]); // does not live long enough
}

This example works on 1.22, but fails on 1.23 through nightly:

error[E0597]: `a[..]` does not live long enough
  --> src/main.rs:18:32
   |
18 |     map(&bar, |a: &&[i32; 2]| &a[0]); // does not live long enough
   |                                ^^^^- borrowed value only lives until here
   |                                |
   |                                does not live long enough
   |
note: borrowed value must be valid for the anonymous lifetime #2 defined on the body at 18:15...
  --> src/main.rs:18:15
   |
18 |     map(&bar, |a: &&[i32; 2]| &a[0]); // does not live long enough
   |               ^^^^^^^^^^^^^^^^^^^^^

xref: https://bugzilla.redhat.com/show_bug.cgi?id=1535396

@sfackler sfackler added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 17, 2018
@nikomatsakis nikomatsakis self-assigned this Jan 18, 2018
@nikomatsakis
Copy link
Contributor

triage: P-high

Regressions BAD‼️

@rust-highfive rust-highfive added the P-high High priority label Jan 18, 2018
@nikomatsakis
Copy link
Contributor

I wonder if this could have been caused by #45072

@nikomatsakis
Copy link
Contributor

That PR was merged on Nov 5, 2017.

Rust 1.22 was released Nov 22.

So I guess not.

@cuviper
Copy link
Member Author

cuviper commented Jan 20, 2018

If you're comparing dates, don't forget the skew of the beta branch. Or you can click on the PR commit and github shows the tags containing it -- 666687a shows only 1.23.0.

@nikomatsakis
Copy link
Contributor

Worth mentioning: the problem is fixed by NLL

@nikomatsakis
Copy link
Contributor

@cuviper oh, really? Then that is probably the culprit. Gotta go, more later.

@nikomatsakis
Copy link
Contributor

OK, so, I think this is a bug and a genuine regression, but it's one that will kinda be a pain to fix in the existing system. The NLL system is newer and shinier and it can handle this scenario more gracefully. I'm tempted to say let's wait and fix this by moving to NLL, but then I do hate regressions. Have to think about it, there might be an easy fix in the existing, lexical system.

Detailed, jargon-full account of what's happening

OK, so, here's what's happening. The older system could be incoherent, but in this case, it would have ignored the expected type and assigned the closure a type that was equivalent to the type that the standalone function (in the fix patch) has. The newer system respects the expected type, and winds up with a type like for<'a> &'a &'b X -- note that 'b is not bound in the closure. It's some region from the surrounding function. The shortcomings in our lexical region inference wind up calling this an error, though due to the implied bounds we ought to accept it. It may be possible to fix that. In contrast, in the newer, shiny NLL system, those annotations that got inferred by typeck are ignored. The closure is assigned the most flexible regions it can get, and everything works out fine.

@nikomatsakis
Copy link
Contributor

triage: P-medium

Discussed in the @rust-lang/compiler meeting. The summary is:

  • This is a genuine bug and regression.
  • It is however fixed in NLL.
  • It's a pain in the neck to fix in the existing system and doesn't seem to be affecting many crates.

Therefore, we are going to downgrade to P-medium and mark this as blocked on NLL.

@rust-highfive rust-highfive added P-medium Medium priority and removed P-high High priority labels Feb 15, 2018
@nikomatsakis nikomatsakis added A-NLL Area: Non-lexical lifetimes (NLL) WG-compiler-nll and removed A-NLL Area: Non-lexical lifetimes (NLL) labels Feb 15, 2018
@nikomatsakis nikomatsakis added the NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. label Mar 14, 2018
@jkordish jkordish added A-borrow-checker Area: The borrow checker C-bug Category: This is a bug. labels Apr 17, 2018
@nikomatsakis nikomatsakis added A-NLL Area: Non-lexical lifetimes (NLL) and removed WG-compiler-nll labels Aug 27, 2018
bors added a commit that referenced this issue Mar 11, 2019
Enable NLL migrate mode on the 2015 edition

Blocked on #58739

## What is in this PR?

* Remove the `-Zborrowck=ast` flag option from rustc.
* The default in the 2015 edition is now `-Zborrowck=migrate`.
* The 2018 edition default is unchanged: it's still `-Zborrowck=migrate`.
* Enable the `-Ztwo-phase-borrows` flag on all editions.
* Remove most dead code that handled these options.
* Update tests for the above changes.

## What is *not* in this PR?

These are left for future PRs

* Use `-Zborrowck=mir` in NLL compare mode tests
* Remove the `-Zborrowck=compare` option
* Remove the `-Ztwo-phase-borrows` flag. It's kept so that perf.rlo has time to stop using it (cc @Mark-Simulacrum)
* Remove MIR typeck as its own MIR pass - it's now run by NLL.
* Enabling `-Zborrowck=mir` by default

Soundness issues that are fixed by NLL will stay open until full NLL is emitting hard errors. However, these diagnostics and completeness issues can now be closed:

Closes #18330
Closes #22323
Closes #23591
Closes #26736
Closes #27487
Closes #28092
Closes #28970
Closes #29733
Closes #30104
Closes #38915
Closes #39908
Closes #43407
Closes #47524
Closes #48540
Closes #49073
Closes #52614
Closes #55085
Closes #56093
Closes #56496
Closes #57804

cc #43234

r? @pnkfelix
cc @rust-lang/lang
cc @rust-lang/wg-compiler-nll
bors added a commit that referenced this issue Apr 22, 2019
Enable NLL migrate mode on the 2015 edition

## What is in this PR?

* Remove the `-Zborrowck=ast` flag option from rustc.
* The default in the 2015 edition is now `-Zborrowck=migrate`.
* The 2018 edition default is unchanged: it's still `-Zborrowck=migrate`.
* Enable two-phase borrows (currently toggled via the `-Ztwo-phase-borrows` flag) on all editions.
* Remove most dead code that handled these options.
* Update tests for the above changes.

## What is *not* in this PR?

These are left for future PRs

* Use `-Zborrowck=mir` in NLL compare mode tests (#56993)
* Remove the `-Zborrowck=compare` option (#59193)
* Remove the `-Ztwo-phase-borrows` flag. It's kept, as a flag that does nothing so that perf.rlo has time to stop using it (cc @Mark-Simulacrum)
* Remove MIR typeck as its own MIR pass - it's now run by NLL.
* Enabling `-Zborrowck=mir` by default (#58781)
* Replace `allow_bind_by_move_patterns_with_guards` and `check_for_mutation_in_guard_via_ast_walk` with just using the feature gate. (#59192)

Soundness issues that are fixed by NLL will stay open until full NLL is emitting hard errors. However, these diagnostics and completeness issues can now be closed:

Closes #18330
Closes #22323
Closes #23591
Closes #26736
Closes #27487
Closes #28092
Closes #28970
Closes #29733
Closes #30104
Closes #38915
Closes #39908
Closes #43407
Closes #47524
Closes #48540
Closes #49073
Closes #52614
Closes #55085
Closes #56093
Closes #56496
Closes #57804

cc #43234

r? @pnkfelix
cc @rust-lang/lang
cc @rust-lang/wg-compiler-nll
bors added a commit that referenced this issue Apr 22, 2019
Enable NLL migrate mode on the 2015 edition

## What is in this PR?

* Remove the `-Zborrowck=ast` flag option from rustc.
* The default in the 2015 edition is now `-Zborrowck=migrate`.
* The 2018 edition default is unchanged: it's still `-Zborrowck=migrate`.
* Enable two-phase borrows (currently toggled via the `-Ztwo-phase-borrows` flag) on all editions.
* Remove most dead code that handled these options.
* Update tests for the above changes.

## What is *not* in this PR?

These are left for future PRs

* Use `-Zborrowck=mir` in NLL compare mode tests (#56993)
* Remove the `-Zborrowck=compare` option (#59193)
* Remove the `-Ztwo-phase-borrows` flag. It's kept, as a flag that does nothing so that perf.rlo has time to stop using it (cc @Mark-Simulacrum)
* Remove MIR typeck as its own MIR pass - it's now run by NLL.
* Enabling `-Zborrowck=mir` by default (#58781)
* Replace `allow_bind_by_move_patterns_with_guards` and `check_for_mutation_in_guard_via_ast_walk` with just using the feature gate. (#59192)

Soundness issues that are fixed by NLL will stay open until full NLL is emitting hard errors. However, these diagnostics and completeness issues can now be closed:

Closes #18330
Closes #22323
Closes #23591
Closes #26736
Closes #27487
Closes #28092
Closes #28970
Closes #29733
Closes #30104
Closes #38915
Closes #39908
Closes #43407
Closes #47524
Closes #48540
Closes #49073
Closes #52614
Closes #55085
Closes #56093
Closes #56496
Closes #57804

cc #43234

r? @pnkfelix
cc @rust-lang/lang
cc @rust-lang/wg-compiler-nll
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-borrow-checker Area: The borrow checker A-NLL Area: Non-lexical lifetimes (NLL) C-bug Category: This is a bug. NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants