Skip to content

WIP: Fix unexpected E0110 when using GATs #51589

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

Conversation

LukasKalbertodt
Copy link
Member

@LukasKalbertodt LukasKalbertodt commented Jun 16, 2018

This PR fixes what @nikomatsakis describes in this comment. Prior to this change, parameters to a path segment were completely prohibited in two places. This resulted in incorrect errors E0110 ("lifetime parameter not allowed on this type") and E0109 ("type parameter not allowed"). I changed this to check the number of allowed/required lifetime and type parameters instead.

This PR can't be merged yet for two reasons:

  1. I left a TODO comment on a line which prohibits type parameters for all but the second two segments of a path. I don't understand why it is there and I'd say it can be removed. However, I'm not sure and I guess there might be a good reason for that, even with GATs?
  2. Some UI tests that have resulted in those errors now cause an ICE, because the compiler can progress further into later stages, where the ICE is triggered (I didn't introduce the ICE, it's this one: ICE: Region parameter out of range when substituting in region... #49362). I'm not sure what to do here. Should we not merge this PR before the ICE is fixed or can I somehow disable UI tests? Or...? (and I still need to change the output of all the other UI tests whose output has changed; but I wanted to wait until I know what to do with the ICE tests)

CC #44265 @eddyb @scalexm

In two places parameters were just prohibited. These were
replaced a check for the right number of lifetime and type
parameters.

Right now, a few UI tests cause an ICE, because the errors
that were created in an earlier stage are not created
anymore.
@rust-highfive
Copy link
Contributor

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 16, 2018
}
check_type_argument_count(tcx, span, ty_provided, ty_params);


let ty = self.projected_ty_from_poly_trait_ref(span, item.def_id, bound);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I see what's missing here - projected_ty_from_poly_trait_ref should not exist. create_substs_for_ast_path should be used, and it now needs to handle multiple segments containing parameters, which is unprecedenced.

@rust-highfive
Copy link
Contributor

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:05:00] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:05:00] tidy error: /checkout/src/librustc_typeck/astconv.rs:1071: TODO is deprecated; use FIXME
[00:05:02] some tidy checks failed
[00:05:02] 
[00:05:02] 
[00:05:02] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:05:02] 
[00:05:02] 
[00:05:02] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:02] Build completed unsuccessfully in 0:01:51
[00:05:02] Build completed unsuccessfully in 0:01:51
[00:05:02] Makefile:79: recipe for target 'tidy' failed
[00:05:02] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0417c9cf
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:15cdd790:start=1529138910188815571,finish=1529138910195677362,duration=6861791
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:011b7c94
$ head -30 ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
head: cannot open ‘./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers’ for reading: No such file or directory
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1a113920
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@eddyb
Copy link
Member

eddyb commented Jun 16, 2018

Progress in this area is blocked on #48149, which will make extending create_substs_for_ast_path to multi-segment paths easier. Eventually we'll have to clean up typeck path handling, it's not ideal.

@eddyb eddyb added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 16, 2018
@bors
Copy link
Collaborator

bors commented Jun 21, 2018

☔ The latest upstream changes (presumably #48149) made this pull request unmergeable. Please resolve the merge conflicts.

@LukasKalbertodt
Copy link
Member Author

I will just close this PR.

This was just an attempt to start something GAT related on my own. But as I have learned, these changes should be part of a bigger cleanup of several modules, including astconv. So I don't think this small PR is useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Blocked on something else such as an RFC or other implementation work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants