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

NLL error on closure, but not on equivalent function #55526

Closed
Avi-D-coder opened this issue Oct 31, 2018 · 26 comments
Closed

NLL error on closure, but not on equivalent function #55526

Avi-D-coder opened this issue Oct 31, 2018 · 26 comments
Assignees
Labels
A-closures Area: Closures (`|…| { … }`) A-inference Area: Type inference A-lifetimes Area: Lifetimes / regions A-NLL Area: Non-lexical lifetimes (NLL) NLL-complete Working towards the "valid code works" goal P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@Avi-D-coder
Copy link
Contributor

With the 2018 edition on nightly or #![feature(nll)]:

fn main() {
    let _v_lambda = |v: &Vec<String>| v.iter().chain(v.iter()).collect::<Vec<&String>>();
}

fn _v_fn(v: &Vec<String>) -> Vec<&String> {
    v.iter().chain(v.iter()).collect::<Vec<&String>>()
}

(Playground)

Errors:

   Compiling playground v0.0.1 (/playground)
warning: unsatisfied lifetime constraints
 --> src/main.rs:2:39
  |
2 |     let _v_lambda = |v: &Vec<String>| v.iter().chain(v.iter()).collect::<Vec<&String>>();
  |                         -           - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ returning this value requires that `'1` must outlive `'2`
  |                         |           |
  |                         |           return type of closure is std::vec::Vec<&'2 std::string::String>
  |                         let's call the lifetime of this reference `'1`
  |
  = warning: This error has been downgraded to a warning for backwards compatibility with previous releases.
          It represents potential unsoundness in your code.
          This warning will become a hard error in the future.

    Finished dev [unoptimized + debuginfo] target(s) in 0.55s
     Running `target/debug/playground`

@memoryruins
Copy link
Contributor

Using the above playground:

  • Stable / Nightly 2015
    • Builds without warning / error
  • Nightly 2015 #![feature(nll)]
    • error: unsatisfied lifetime constraints
  • Nightly 2018
    • warning: unsatisfied lifetime constraints
    • (downgraded due to migrate mode)

Attempting to then use _v_lambda on stable 2015 or nightly 2018 on a &Vec<String> creates the following error on both with or without NLL

error[E0495]: cannot infer an appropriate lifetime for lifetime parameter in function call due to conflicting requirements

_v_fn builds and runs on both.

@memoryruins memoryruins added A-lifetimes Area: Lifetimes / regions A-closures Area: Closures (`|…| { … }`) A-inference Area: Type inference labels Oct 31, 2018
@pnkfelix pnkfelix added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-NLL Area: Non-lexical lifetimes (NLL) labels Nov 2, 2018
@pnkfelix
Copy link
Member

pnkfelix commented Nov 2, 2018

I am curious whether the lifetime elision rules are part of the reason we see the discrepancy here.

@pnkfelix
Copy link
Member

pnkfelix commented Nov 2, 2018

(though my personal suspicion is that NLL is buggy and ignoring the constraint that the the input and the output types on the closure are supposed to use the same lifetime.)

@pnkfelix
Copy link
Member

pnkfelix commented Nov 2, 2018

e..g this does compile under NLL:

fn _v_closure<'a: 'b, 'b>(_: &'b (), _: &'a ()) {
    let _v_lambda = |v: &'a Vec<String>| -> Vec<&'b String> {
        v.iter().chain(v.iter()).collect::<Vec<&String>>()
    };
}

@pnkfelix
Copy link
Member

pnkfelix commented Nov 2, 2018

and, just in case someone wants a potentially more useful workaround, this also compiles today under NLL:

fn _v_closure<'a, 'b>(_: &'b (), _: &'a ())
                     -> impl Fn(&'a Vec<String>) -> Vec<&'b String>
    where 'a: 'b
{
    |v| { v.iter().chain(v.iter()).collect::<Vec<&String>>() }
}

@pnkfelix pnkfelix added the NLL-complete Working towards the "valid code works" goal label Nov 2, 2018
@pnkfelix
Copy link
Member

pnkfelix commented Nov 6, 2018

triage: P-high (because this looks annoying and perhaps very misleading/frustrating to end users), but not Release milestone.

I'm opting to not put it on the Release milestone because I do not think we would need to back port a hypothetical fix for this to the beta channel; this is an instance where the NLL migration mode saves us from that pain.

@pnkfelix pnkfelix added the P-high High priority label Nov 6, 2018
@pnkfelix pnkfelix self-assigned this Nov 8, 2018
@pnkfelix
Copy link
Member

pnkfelix commented Nov 8, 2018

triage: assigning to self to look at either Friday or Monday.

@pnkfelix
Copy link
Member

pnkfelix commented Nov 12, 2018

I didn't get a chance to look at this (before I go on leave for ~2 weeks tomorrow). Its still P-high, but it bears repeating that NLL migration gives us breathing room here (i.e. other P-high items should probably be P-higher)

@pnkfelix pnkfelix removed their assignment Nov 12, 2018
@nikomatsakis nikomatsakis self-assigned this Nov 15, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Nov 22, 2018

triage: no change since last week

@pnkfelix pnkfelix self-assigned this Nov 27, 2018
@pnkfelix
Copy link
Member

triage: no change since last week (its continues to be the case that NLL migration gives us breathing room here). I'm now back from PTO and have thus self-assigned this.

@pnkfelix
Copy link
Member

FYI the problem even arises with a trivialized example like this:

fn main() {
    let _v_lambda = |v: &str| -> &str { v };
}

fn _v_fn(v: &str) -> &str {
    v
}

@pnkfelix
Copy link
Member

pnkfelix commented Nov 30, 2018

Ah, its also worth noting that the following take-away from @memoryruins 's comment above: Even with AST-borrowck, you cannot use the _v_lambda closure from the original example. (Attempting to do so yields a compile-time region-inference failure.)

I'm in the process of teasing out what our region-inference is supposed to do in this case. What it seems we do is just plug a region-inference variable into the return type &str and then hope it all works out.

(I think this makes sense, since we cannot apply the same lifetime elision rules here that we do to fn; in particular, lambda expressions are allowed to return references derived from free variables in their expression environment, rather than solely based on input formal parameters to the closure. E.g.

fn _two_arg<'a, 'b>(a: &'a str, b: &'b str) -> &'b str {
    let _has_free = |_x: &str| b;
    _has_free(a)
}

)

@pnkfelix
Copy link
Member

pnkfelix commented Dec 3, 2018

Update: this comment and the two following it are based on a flawed testing methodology. Don't believe them.

Original comment follows


As an experiment, I decided to check if this case has always been failing for NLL.

It turns out that we used to accept the closure written here, back in rustc 1.27.0-nightly (7360d6d 2018-04-15). And then it breaks in rustc 1.27.0-nightly (65d201f 2018-04-18)

The changes there are captured by this series of bors merges:

% git log 7360d6d..65d201f --format=oneline --author=bors
65d201f Auto merge of #49981 - nox:fix-signed-niches, r=eddyb
f4bb956 Auto merge of #49349 - Zoxc:sync-errors, r=michaelwoerister
23561c6 Auto merge of #49972 - Mark-Simulacrum:remove-build, r=alexcrichton
dcb44ca Auto merge of #49969 - mark-i-m:allocator_fmt, r=estebank
b91e6a2 Auto merge of #49950 - Zoxc:default-span, r=estebank
9a59133 Auto merge of #50036 - nrc:update, r=alexcrichton
786e305 Auto merge of #49904 - michaelwoerister:no-debug-attr, r=alexcrichton
9379bcd Auto merge of #50033 - GuillaumeGomez:rollup, r=GuillaumeGomez
881a7cd Auto merge of #49836 - nikomatsakis:nll-facts-prep, r=pnkfelix
d703622 Auto merge of #49626 - fanzier:chalk-lowering, r=scalexm
8728c7a Auto merge of #49542 - GuillaumeGomez:intra-link-resolution-error, r=GuillaumeGomez
6b12d36 Auto merge of #49882 - Zoxc:sync-misc2, r=michaelwoerister
186db76 Auto merge of #49664 - alexcrichton:stable-simd, r=BurntSushi
94516c5 Auto merge of #50012 - Zoxc:msvc-fix, r=Mark-Simulacrum
3809bbf Auto merge of #49488 - alexcrichton:small-wasm-panic, r=sfackler
4a3ab8b Auto merge of #50003 - kennytm:rollup, r=kennytm
49317cd Auto merge of #49130 - smmalis37:range, r=alexcrichton
1ef1563 Auto merge of #48945 - clarcharr:iter_exhaust, r=Kimundi
d6a2dd9 Auto merge of #49433 - varkor:metadata-skip-mir-opt, r=michaelwoerister
532764c Auto merge of #49963 - llogiq:stabilize-13226, r=kennytm
3e70dfd Auto merge of #49956 - QuietMisdreavus:rustdoc-codegen, r=GuillaumeGomez
748c549 Auto merge of #49847 - sinkuu:save_analysis_implicit_extern, r=petrochenkov
d6ba1b9 Auto merge of #49719 - mark-i-m:no_sep, r=petrochenkov
8de5353 Auto merge of #49947 - oli-obk:turing_complete_const_eval, r=nagisa

@pnkfelix
Copy link
Member

pnkfelix commented Dec 3, 2018

Okay I have now confirmed that this bug was injected by PR #49836.

(Which is odd because I believe that was intended to be a pure refactoring, and indeed my informal review of its commits, at least based on their log messages, reinforces that belief...)

@pnkfelix
Copy link
Member

pnkfelix commented Dec 3, 2018

In fact the bug was specifically injected by commit 45d281d (!), which definitely sounded trivial to me.

This is actually great news, since it increases the likelihood that I'll be able to identify the source of the regression and find a smallish fix.

@pnkfelix
Copy link
Member

pnkfelix commented Dec 3, 2018

Ugh, I just realized: my testing methodology was flawed.

I was using -Z borrowck=mir as the way to opt into NLL for each build as I bisected above.

But PR #49836 specifically switched the way you opt into NLL. Prior to that PR, you needed to pass -Z nll in order to get non-lexical lifetimes; without it, -Z borrowck=mir used MIR as the basis for a lexical borrow-check.

So of course my (flawed) testing was claiming that PR #49836 injected the bug. 😞

(I have since confirmed that this bug appears to have always been present as part of NLL.)

@pnkfelix
Copy link
Member

pnkfelix commented Dec 4, 2018

@Avi-D-coder can you share information about the original code where you encountered this regression?

In particular, based on my investigation of what the AST-borrowck actually does for a case like this, it is not clear to me whether it is ever going to produce a case where you can actually call the _v_lambda closure.

(That is, even though it seems like _v_fn and _v_closure are equivalent at a superficial level, I do not think that is actually the case even under AST-borrowck once you start looking underneath the surface...)

@Avi-D-coder
Copy link
Contributor Author

Avi-D-coder commented Dec 4, 2018

@pnkfelix I didn't want to write an extremely long type signature for a locally scoped function, so I used a closure.

Since your work around exists, this is not the most important issue. When I came across it seemed extremely counter intuitive, especially given how trivial code giving rise to variants of this issue can be.

I still feel this is counter intuitive. Perhaps function inference rules could be applied where the closure does not capture. If not a specialized error would be useful.

@pnkfelix
Copy link
Member

pnkfelix commented Dec 5, 2018

@Avi-D-coder ah I see.

So if I understand you correctly, you were experimenting with the change to a closure rather than a locally-scoped fn (and were using NLL at the time), but you never had with a complete piece of example code that 1. included a call to the introduced closure and 2. compiled and run properly under AST-borrowck. Is that right?

I agree the case is counter-intuitive, and I would like to do better here (either in terms of providing better diagnostic feedback, or simply accepting the code as written, at least with no calls to the closure).

But I just wanted to know how dire the underlying problem might be, in terms of whether we might be breaking code where the closure in question is actually being called.

@pnkfelix
Copy link
Member

pnkfelix commented Dec 5, 2018

Discussed quite a bit with @nikomatsakis on zulip

The main takeaways we have currently:

  • Niko is surprised that let v_lambda = |v: &str| -> &str { v }; (with an explicit return type) does not work, as he would have expected lifetime elision rules to handle that case.
    • I am going to shift my focus to figuring out why that case isn't working
  • We decided that the fact that AST-borrowck is currently accepting let v_lambda = |v: &str| { v }; is probably a bug; given the structure of our type+region system, Niko would expect that to be rejected by both AST-borrowck and NLL.
  • I did find that latter conclusion surprising, until I saw this explanation:
    • in closures with no expected signature, if anything is missing (i.e. -> &str), it is equivalent to _
    • _ (in the closure signature) creates a type variable that is in the scope of the enclosing fn (not just the closure)
    • in this case, the _ would need to name a region that is in scope only for the closure
  • @nikomatsakis expects PR introduce "type must be valid for" into lexical region solver #55988 to cause AST-borrowck to start rejecting the code with no return type given, which would bring AST-borrowck into sync with NLL.

@pnkfelix
Copy link
Member

pnkfelix commented Dec 5, 2018

I filed issue #56537 to track the specific task of fixing the variant case where one provides the return type |...| -> &_ { ... } as part of the closure expression.

Fixing #56537 won't make the code here start compiling, but it provides a path towards a far more trivial workaround.

@pnkfelix
Copy link
Member

pnkfelix commented Dec 6, 2018

I'm going to demote this to P-medium. The short-term plan is not to make NLL accept this code with no return type annotation (i.e. let v_lambda = |v: &str| { v };), but rather to (hopefully?) change AST-borrowck to start rejecting such code. Other variations of the example either fall into that same bucket, or should filed as separate bugs, as I did with #56537

@pnkfelix pnkfelix added P-medium Medium priority and removed P-high High priority labels Dec 6, 2018
@pnkfelix
Copy link
Member

Update: I closed issue #56537 because it is not as trivially actionable as I had originally thought (see the comment thread there for an explanation why).

I now think the clearest path forward here is the following suggestion (taken from #56537 (comment)):

yet another option would be to sidestep trying to resolve this via elision, and instead focus attention on adding support for an expression form for <'r> CLOSURE so that we could do for <'r> |s: &'r _| -> &'r _ { ... }. (Which I think is issue #22340)

@pnkfelix
Copy link
Member

also, given that @nikomatsakis believes that the best way to synchronize behavior between AST-borrowck and NLL for the original example from the description on this issue (#55526) is to change AST-borrowck (see PR #55988), I think that means this is not actually an NLL-complete issue...

(That is, It is either an issue we should just close, perhaps as "wontfix"/"notabug", or at least tag as NLL-deferred.)

I'm going to change the label for NLL-deferred for now.

@pnkfelix pnkfelix added NLL-deferred and removed NLL-complete Working towards the "valid code works" goal labels Dec 12, 2018
@pnkfelix
Copy link
Member

triaging for #56754. leaving as P-medium. Looping in T-lang.

@pnkfelix pnkfelix added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed NLL-deferred labels Dec 21, 2018
@nikomatsakis nikomatsakis removed their assignment Jan 4, 2019
@nikomatsakis nikomatsakis added the NLL-complete Working towards the "valid code works" goal label Jan 23, 2019
@jonas-schievink
Copy link
Contributor

Triage: The AST borrow checker has been removed. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-closures Area: Closures (`|…| { … }`) A-inference Area: Type inference A-lifetimes Area: Lifetimes / regions A-NLL Area: Non-lexical lifetimes (NLL) NLL-complete Working towards the "valid code works" goal P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants