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

Erroneous compilation failure with associated constant #54822

Closed
joshlf opened this issue Oct 4, 2018 · 6 comments · Fixed by #59083 or #59151
Closed

Erroneous compilation failure with associated constant #54822

joshlf opened this issue Oct 4, 2018 · 6 comments · Fixed by #59083 or #59151
Labels
A-associated-items Area: Associated items such as associated types and consts. A-traits Area: Trait system T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@joshlf
Copy link
Contributor

joshlf commented Oct 4, 2018

EDIT: To anyone coming to this from the This Week in Rust CFP: This is currently an unsolved issue, so the first step to tackling this is to figure out where the bug is! That alone would be a huge help. If you want to implement a fix, that'd be great too :)

=====

The following program should compile on stable, but doesn't:

trait ConstDefault {
    const Default: Self;
}

trait Foo: Sized {}

trait FooExt: Foo {
    type T: ConstDefault;
}

trait Bar<F: FooExt> {
    const T: F::T;
}

impl<F: FooExt> Bar<F> for () {
    const T: F::T = <F::T as ConstDefault>::Default;
}

It fails to compile with the following error:

error[E0277]: the trait bound `F: FooExt` is not satisfied
  --> src/lib.rs:16:5
   |
16 |     const T: F::T = <F::T as ConstDefault>::Default;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `FooExt` is not implemented for `F`
   |
   = help: consider adding a `where F: FooExt` bound

Note that if we amend as either of the following, it works (credit to @ezrosent for figuring this out):

trait Bar<T: ConstDefault, F: FooExt<T=T>> {
    const C: T;
}

impl<T: ConstDefault, F: FooExt<T=T>> Bar<T, F> for () {
    const C: T = F::T::Default;
    const C: T = T::Default;
}
@Havvy Havvy added A-traits Area: Trait system A-associated-items Area: Associated items such as associated types and consts. labels Oct 5, 2018
@pnkfelix pnkfelix added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 5, 2018
@kyren
Copy link
Contributor

kyren commented Mar 10, 2019

Okay, so I came here from the TWIR CFP post, and I thought I would take a stab at this.

This is the first time I have ever dug deeply into the internals of rustc itself, so take everything I'm about to say with a grain of salt. I don't have a real understanding of most of the rustc internals yet, so I approached this bug purely experimentally. It may be that what I'm saying is very obvious to somebody who already understands how typechecking and trait resolution work, so I apologize if I'm wasting everyone's time.

So I started by playing with the failing example, and there are lots of ways here to get some obviously wrong output from rustc:

trait ConstDefault {
    const DEFAULT: Self;
}

trait Foo {
    type T;
}

trait Bar<F: Foo> {
    const T: F::T;
}

impl<T: ConstDefault, F: Foo<T = T>> Bar<F> for () {
    const T: T = T::DEFAULT;
}

fn main() {}
error[E0277]: the trait bound `F: Foo` is not satisfied                                                                                                       |    tcx.infer_ctxt().enter(|infcx| {
  --> ./test.rs:14:5                                                                                                                                          |        // let param_env = ty::ParamEnv::empty();
   |                                                                                                                                                          |        let param_env = tcx.param_env(impl_c.def_id);
14 |     const T: T = T::DEFAULT;                                                                                                                             |
   |     ^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Foo` is not implemented for `F`                                                                                  |        let inh = Inherited::new(infcx, impl_c.def_id);
   |                                                                                                                                                          |        let infcx = &inh.infcx;
   = help: consider adding a `where F: Foo` bound                                                                                                             |
                                                                                                                                                              |        // The below is for the most part highly similar to the procedure
error: aborting due to previous error                                                                                                                         |        // for methods above. It is simpler in many respects, especially
                                                                                                                                                              |        // because we shouldn't really have to deal with lifetimes or
For more information about this error, try `rustc --explain E0277`.                                                                                           |        // predicates. In fact some of this should probably be put into

So I started by looking into why in the world the compiler would output the trait Foo is not implemented for F when checking the associated constant when it's obviously in the where clause for the trait impl. After a lot of logging and staring at the code in librustc/traits/, I figured out where the trait resolver is supposed to get this information: each obligation's param_env field. I found that this param_env during trait resolution for <F as Foo>::T was empty! so clearly it made sense that the compiler couldn't find an implementation of Foo for F. From there, I figured out that this ParamEnv instance ultimately comes from compare_method.rs:920 where it is purposefully constructed as empty. Quickly checking the history of compare_method.rs I saw that the original implementation of associated constants did not use an empty ParamEnv, and I'm not sure why an empty ParamEnv is constructed now (though I'm not really sure of anything, I barely understand what's going on here).

After some experimentation, I was able to make all of the examples here compile by changing:

 let param_env = ty::ParamEnv::empty();

to

let param_env = tcx.param_env(impl_c.def_id);

to put the where clause for F: Foo in scope. However, this causes two tests to fail:

ui/associated-const/associated-const-generic-obligations.rs and ui/nll/trait-associated-constant.rs.

Through experimentation I'm able to fix ui/nll/trait-associated-constant.rs by limiting where the non-empty ParamEnv instance is passed to only here and passing in an empty ParamEnv in other places, but I'm unable to fix ui/associated-const/associated-const-generic-obligations.rs without again causing this bug.

So I don't think I'm actually fixing the bug here, because I don't understand things well enough yet to know what the right thing to do is, and there's a huge chance I have a lot of this wrong. I'm including all of this information simply in case it helps somebody more knowledgable actually fix it.

I hope this is helpful, if it's not helpful then feel free to ignore.

@vickenty
Copy link
Contributor

I'm no expert either, but just from the test failures it seems that @kyren's fix is actually correct.

Test failures after applying the fix: https://gist.github.com/vickenty/1ea9ea56c97e5e4902f6158a5f2410f1

ui/associated-const/associated-const-generic-obligations.rs has an expected failure, but checked for specific error message, that this issue is about. After the fix the code is still (correctly) rejected, but the error message is more specific to the issue in the code.

In ui/nll/trait-associated-constant.rs the compiler now accepts code that was rejected before. This is no longer an error:

trait Anything<'a: 'b, 'b> {
    const AC: Option<&'b str>;
}
struct FailStruct2 { }

impl<'a: 'b, 'b> Anything<'a, 'b> for FailStruct2 {
    const AC: Option<&'a str> = None;
    //~^ ERROR: mismatched types
}

The error message appears similar to the one on this issue: the lifetime 'a [..] .does not necessarily outlive the lifetime 'b while pointing right at a place in the code that says otherwise.

Since impls can use more generic lifetimes (as was recently discussed), it would make sense if this code was allowed as well.

@kyren
Copy link
Contributor

kyren commented Mar 10, 2019

Oh, I should have paid more attention to what the tests actually did, thank you for looking at that!

@joshlf
Copy link
Contributor Author

joshlf commented Mar 10, 2019

Wow, fantastic work @kyren ! I'm not familiar enough with rustc to be able to say anything more than @vickenty has, but I guess just throw up a PR and we'll see what the reviewers say!

Centril added a commit to Centril/rust that referenced this issue Mar 13, 2019
Fix rust-lang#54822 and associated faulty tests

Type checking associated constants can require trait bounds, but an empty
parameter environment was provided to the trait solver.  Providing an
appropriate parameter environment seems to fix rust-lang#54822 and also make one of the
cases in src/test/ui/nll/trait-associated-constant.rs that should compile
successfully do so.  It also (slightly) improves the error message in
src/test/ui/associated-const/associated-const-generic-obligations.rs
bors added a commit that referenced this issue Mar 13, 2019
Rollup of 16 pull requests

Successful merges:

 - #58829 (librustc_interface: Update scoped-tls to 1.0)
 - #58876 (Parse lifetimes that start with a number and give specific error)
 - #58908 (Update rand version)
 - #58998 (Fix documentation of from_ne_bytes and from_le_bytes)
 - #59056 (Use lifetime contravariance to elide more lifetimes in core+alloc+std)
 - #59057 (Standardize `Range*` documentation)
 - #59080 (Fix incorrect links in librustc_codegen_llvm documentation)
 - #59083 (Fix #54822 and associated faulty tests)
 - #59093 (Remove precompute_in_scope_traits_hashes)
 - #59101 (Reduces Code Repetitions like `!n >> amt`)
 - #59121 (impl FromIterator for Result: Use assert_eq! instead of assert!)
 - #59124 (Replace assert with assert_eq)
 - #59129 (Visit impl Trait for dead_code lint)
 - #59130 (Note that NonNull does not launder shared references for mutation)
 - #59132 (ignore higher-ranked object bound conditions created by WF)
 - #59138 (Simplify Iterator::{min, max})

Failed merges:

r? @ghost
@joshlf
Copy link
Contributor Author

joshlf commented Mar 13, 2019

@kyren I saw your PR got merged; thanks again for your work digging into this and for the fix!

@kyren
Copy link
Contributor

kyren commented Mar 13, 2019

No problem, glad to help 👍

It was more straightforward than I thought it was going to be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-associated-items Area: Associated items such as associated types and consts. A-traits Area: Trait system 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