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

confusing error message around trait object bound #54779

Closed
nikomatsakis opened this issue Oct 3, 2018 · 9 comments · Fixed by #96615
Closed

confusing error message around trait object bound #54779

nikomatsakis opened this issue Oct 3, 2018 · 9 comments · Fixed by #96615
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: Lifetimes / regions A-NLL Area: Non-lexical lifetimes (NLL) A-trait-system Area: Trait system C-enhancement Category: An issue proposing an enhancement or a PR with one. D-confusing Diagnostics: Confusing error or lint that should be reworked. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. NLL-diagnostics Working towards the "diagnostic parity" goal P-medium Medium priority requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

This example:

#![feature(in_band_lifetimes)]
#![feature(nll)]

trait DebugWith<Cx: ?Sized> {
    fn debug_with(&'me self, cx: &'me Cx) -> DebugCxPair<'me, Self, Cx> {
        DebugCxPair { value: self, cx }
    }
    
    fn fmt_with(&self, cx: &Cx, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result;
}

struct DebugCxPair<'me, Value: ?Sized, Cx: ?Sized>
where
    Value: DebugWith<Cx>,
{
    value: &'me Value,
    cx: &'me Cx,
}

trait DebugContext { }

struct Foo {
    bar: Bar
}

impl DebugWith<dyn DebugContext> for Foo {
    fn fmt_with(&self, cx: &dyn DebugContext, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        let Foo { bar } = self;
        bar.debug_with(cx);
        Ok(())
    }
}

struct Bar { }

impl DebugWith<dyn DebugContext> for Bar {
    fn fmt_with(&self, cx: &dyn DebugContext, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        Ok(())
    }
}

fn main() { }

gives this error:

error: unsatisfied lifetime constraints
  --> src/main.rs:29:9
   |
27 |     fn fmt_with(&self, cx: &dyn DebugContext, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
   |                 -          - let's call the lifetime of this reference `'1`
   |                 |
   |                 let's call the lifetime of this reference `'2`
28 |         let Foo { bar } = self;
29 |         bar.debug_with(cx);
   |         ^^^^^^^^^^^^^^^^^^ argument requires that `'1` must outlive `'2`

It took me some time to puzzle out what was happening here:

  • cx: &dyn DebugContext expands to &'a dyn (DebugContext + 'a)
  • the default from the impl however is dyn (DebugContext + 'static)
  • the impl method is accepted because it is more general than the trait definition
  • but it fails to recursively invoke

The error message without NLL, of course, is also not very illuminating.

@nikomatsakis nikomatsakis added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-NLL Area: Non-lexical lifetimes (NLL) labels Oct 3, 2018
@matthewjasper matthewjasper self-assigned this Oct 3, 2018
@matthewjasper
Copy link
Contributor

Assigning myself to bookmark.

@estebank
Copy link
Contributor

estebank commented Oct 3, 2018

the default from the impl however is dyn (DebugContext + 'static)

This seems to be a persistent ergonomics wart where traits get evaluated to 'static (happens as well with returning &(impl Trait) and others I can't remember now) where it isn't expected. It is a bit sad, and we can improve the diagnostic errors, but I'm worried we'll be unable to preempt all the cases from being caught by accurate diagnostics, pushing people away from deeply nested structures. Maybe that is for the best, though.

@pnkfelix pnkfelix added the NLL-diagnostics Working towards the "diagnostic parity" goal label Oct 9, 2018
@pnkfelix
Copy link
Member

pnkfelix commented Oct 9, 2018

I just want to note that the use of #![feature(in_band_lifetimes)] is, I think, a red herring in terms of the complaint about the diagnostic here.

That is, I get the exact same diagnostic even if I add an explicit <'me> binder to the fn debug_with method, and then one can remove the #![feature(in_band_lifetimes)].

This is relevant because it means that if that feature is a red herring, then this bug may be relevant to the 2018 edition.

@pnkfelix
Copy link
Member

pnkfelix commented Oct 9, 2018

discussed with @nikomatsakis . We agreed this is not a blocker for 2018 edition. Tagging with NLL-deferred so it does not clog up our triage process.

@pnkfelix
Copy link
Member

I forgot to actual test how migration-mode (enabled for the 2018 edition) compares to #![feature(nll)].

Current (nightly) 2018 edition output (play):

error[E0495]: cannot infer an appropriate lifetime due to conflicting requirements
  --> src/main.rs:26:24
   |
26 |         bar.debug_with(cx);
   |                        ^^
   |
note: first, the lifetime cannot outlive the anonymous lifetime #2 defined on the method body at 24:5...
  --> src/main.rs:24:5
   |
24 | /     fn fmt_with(&self, cx: &dyn DebugContext, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
25 | |         let Foo { bar } = self;
26 | |         bar.debug_with(cx);
27 | |         Ok(())
28 | |     }
   | |_____^
note: ...so that the declared lifetime parameter bounds are satisfied
  --> src/main.rs:26:24
   |
26 |         bar.debug_with(cx);
   |                        ^^
   = note: but, the lifetime must be valid for the static lifetime...
   = note: ...so that the expression is assignable:
           expected &(dyn DebugContext + 'static)
              found &dyn DebugContext

error: aborting due to previous error

Current (nightly) output with #![feature(nll)] (play):

error: lifetime may not live long enough
  --> src/main.rs:28:24
   |
26 |     fn fmt_with(&self, cx: &dyn DebugContext, _fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
   |                            - let's call the lifetime of this reference `'1`
27 |         let Foo { bar } = self;
28 |         bar.debug_with(cx);
   |                        ^^ cast requires that `'1` must outlive `'static`

@pnkfelix
Copy link
Member

As for this week's triage, tagging this as P-medium priority.

@pnkfelix pnkfelix added the P-medium Medium priority label Mar 20, 2019
@crlf0710 crlf0710 added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jun 11, 2020
@estebank estebank added A-lifetimes Area: Lifetimes / regions A-trait-system Area: Trait system labels Jul 15, 2020
@estebank
Copy link
Contributor

I recently landed some changes (#72543 and #72804) that are tangentially related to this but a similar strategy to them could be applied to this case.

@estebank estebank added the D-confusing Diagnostics: Confusing error or lint that should be reworked. label Jul 15, 2020
@Aaron1011 Aaron1011 added the requires-nightly This issue requires a nightly compiler in some way. label Oct 2, 2021
@marmeladema
Copy link
Contributor

Since in_band_lifetimes feature has been removed, can this issue be closed as obsolete?

@jackh726
Copy link
Member

jackh726 commented Apr 5, 2022

The example without in_band_lifetimes: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=d0d998218da2b4376859b07acc331644

The error message isn't the same as migrate mode, but is on-par in terms of information (pointing out that the anonymous lifetime doesn't live as long as 'static). This issue can be closed by adding the above as a test.

@jackh726 jackh726 added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Apr 5, 2022
JohnTitor added a commit to JohnTitor/rust that referenced this issue May 1, 2022
bors added a commit to rust-lang-ci/rust that referenced this issue May 13, 2022
…askrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#96154 (Expand core::hint::unreachable_unchecked() docs)
 - rust-lang#96615 (Add a regression test for rust-lang#54779)
 - rust-lang#96982 (fix clippy expect_fun_call)
 - rust-lang#97003 (Remove some unnecessary `rustc_allow_const_fn_unstable` attributes.)
 - rust-lang#97011 (Add regression test for rust-lang#28935)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors closed this as completed in 281be09 May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: Lifetimes / regions A-NLL Area: Non-lexical lifetimes (NLL) A-trait-system Area: Trait system C-enhancement Category: An issue proposing an enhancement or a PR with one. D-confusing Diagnostics: Confusing error or lint that should be reworked. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. NLL-diagnostics Working towards the "diagnostic parity" goal P-medium Medium priority requires-nightly This issue requires a nightly compiler in some way. 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.

8 participants