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

Fix #54822 and associated faulty tests #59083

Merged
merged 2 commits into from
Mar 13, 2019
Merged

Fix #54822 and associated faulty tests #59083

merged 2 commits into from
Mar 13, 2019

Conversation

kyren
Copy link
Contributor

@kyren kyren commented Mar 11, 2019

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 #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

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @varkor (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 11, 2019
@kyren
Copy link
Contributor Author

kyren commented Mar 11, 2019

This PR is highly likely to be wrong, there's an enormous amount of subtlety in type checking that I don't yet understand, but it was suggested that I start with a PR so it could be discussed.

I haven't checked yet, but if this ever previously worked, it probably stopped working around #42189.

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 #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
@varkor
Copy link
Member

varkor commented Mar 12, 2019

This change looks completely sensible to me. In fact, I'm confused at the second test case (i.e. FailStruct2 for trait-associated-constant.rs, which seems like it should never have been included in the test (or otherwise, it should have had an associated FIXME).

If you could just change the struct name in the test to make it less confusing, everything looks good!

@kyren
Copy link
Contributor Author

kyren commented Mar 12, 2019

How's that, would you like me to collapse it into a single commit?

@varkor
Copy link
Member

varkor commented Mar 12, 2019

That's great — thank you for looking into this issue!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 12, 2019

📌 Commit aa9bd68 has been approved by varkor

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 12, 2019
Centril added a commit to Centril/rust that referenced this pull request 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 pull request 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
@bors bors merged commit aa9bd68 into rust-lang:master Mar 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Erroneous compilation failure with associated constant
4 participants