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: need to capture and present causes that led to region constraints #51188

Closed
pnkfelix opened this issue May 29, 2018 · 6 comments
Closed
Assignees
Labels
A-NLL Area: Non-lexical lifetimes (NLL) NLL-diagnostics Working towards the "diagnostic parity" goal
Milestone

Comments

@pnkfelix
Copy link
Member

Consider the test in-band-lifetimes/impl/dyn-trait.rs.

  • An aside: Normally pnkfelix would copy the source text inline, but in this case the source text is testing a totally different bug and will probably obscure the main point I am making here.
    • There is probably a much smaller test case that would also succeed in making the point here; I may try to come up with such a minimal test, and post it here if I do.

For AST-borrowck we produce this output dyn-trait.stderr:

error[E0495]: cannot infer an appropriate lifetime due to conflicting requirements
  --> $DIR/dyn-trait.rs:32:16
   |
LL |     static_val(x); //~ ERROR cannot infer
   |                ^
   |
note: first, the lifetime cannot outlive the lifetime 'a as defined on the function body at 31:1...
  --> $DIR/dyn-trait.rs:31:1
   |
LL | fn with_dyn_debug_static<'a>(x: Box<dyn Debug + 'a>) {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: ...so that the expression is assignable:
           expected std::boxed::Box<std::fmt::Debug>
              found std::boxed::Box<std::fmt::Debug + 'a>
   = note: but, the lifetime must be valid for the static lifetime...
   = note: ...so that the types are compatible:
           expected StaticTrait
              found StaticTrait

error: aborting due to previous error

but for NLL we currently produce this output dyn-trait.nll.stderr:

error: free region `'a` does not outlive free region `'static`
  --> $DIR/dyn-trait.rs:32:5
   |
LL |     static_val(x); //~ ERROR cannot infer
   |     ^^^^^^^^^^^^^

error: aborting due to previous error

One can debate how much utility the end user gets from the potential overload of lines that the AST-borrowck note generates, but it is certainly more useful than the output that NLL is currently generated.

At its heart, the issue here is that NLL currently builds up a set of constraints and tells us if it couldn't resolve them, but it fails to present the causes that led to that constraint set.

@estebank estebank added the NLL-diagnostics Working towards the "diagnostic parity" goal label May 29, 2018
@pnkfelix pnkfelix added A-NLL Area: Non-lexical lifetimes (NLL) WG-compiler-nll labels May 29, 2018
@nikomatsakis nikomatsakis added this to the Rust 2018 Preview 2 milestone Jun 29, 2018
@nikomatsakis
Copy link
Contributor

We have to get a handle on this. @davidtwco and I have been working on it.

@pnkfelix
Copy link
Member Author

Things have gotten a lot better here.

The only real diagnostic issue remaining with https://github.com/rust-lang/rust/blob/master/src/test/ui/in-band-lifetimes/impl/dyn-trait.nll.stderr is that it talks about "closures" when there is no closure in the source code from the view point of the end developer.

@davidtwco
Copy link
Member

@pnkfelix With #52648 merged, errors now correctly refer to "closure" or "function" where appropriate.

@Mark-Simulacrum
Copy link
Member

@rust-lang/wg-compiler-nll It looks like this issue might be done? Perhaps someone could take a look and update the current state.

@nikomatsakis
Copy link
Contributor

I'm going to close this issue now. I re-opened #51175 which seem relevant.

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) NLL-diagnostics Working towards the "diagnostic parity" goal
Projects
None yet
Development

No branches or pull requests

5 participants