Normalize possibly un-normalized GAT projections #93361

compiler-errors · 2022-01-27T05:51:14Z

Add some normalization during confirmation/obligation fulfillment because we have some difficulties normalizing GAT projection types (see note in rustc_trait_selection::traits::project).

This is kinda a hack, and perhaps we want to work towards a better solution about failing to normalize GAT projection types cc: @jackh726, I'd like to be involved if brainstorming around that is happening!

Fixes #93341
Fixes #93342

rust-highfive · 2022-01-27T05:51:18Z

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

compiler-errors · 2022-01-27T05:51:38Z

thanks highfive, you r?'ed the right person lol

compiler/rustc_trait_selection/src/traits/select/confirmation.rs

compiler-errors · 2022-01-27T06:00:51Z

@rustbot author

compiler-errors · 2022-01-27T06:26:22Z

@rustbot ready

Looks like one test got fixed (?), and another lost one of its errors. I don't think this is a bad thing though, but I'm not really sure.

rust-log-analyzer · 2022-01-27T07:07:25Z

The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)


running 165 tests
...................................i...........................

command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/i686-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/mir-opt" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/mir-opt" "--stage-id" "stage2-i686-unknown-linux-gnu" "--suite" "mir-opt" "--mode" "mir-opt" "--target" "i686-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-12/bin/FileCheck" "--nodejs" "/usr/bin/node" "--linker" "cc" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0  -Lnative=/checkout/obj/build/i686-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python3" "--lldb-python" "/usr/bin/python3" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "12.0.0" "--llvm-components" "aarch64 aarch64asmparser aarch64codegen aarch64desc aarch64disassembler aarch64info aarch64utils aggressiveinstcombine all all-targets amdgpu amdgpuasmparser amdgpucodegen amdgpudesc amdgpudisassembler amdgpuinfo amdgpuutils analysis arm armasmparser armcodegen armdesc armdisassembler arminfo armutils asmparser asmprinter avr avrasmparser avrcodegen avrdesc avrdisassembler avrinfo binaryformat bitreader bitstreamreader bitwriter bpf bpfasmparser bpfcodegen bpfdesc bpfdisassembler bpfinfo cfguard codegen core coroutines coverage debuginfocodeview debuginfodwarf debuginfogsym debuginfomsf debuginfopdb demangle dlltooldriver dwarflinker engine executionengine extensions filecheck frontendopenacc frontendopenmp fuzzmutate globalisel hellonew hexagon hexagonasmparser hexagoncodegen hexagondesc hexagondisassembler hexagoninfo instcombine instrumentation interfacestub interpreter ipo irreader jitlink lanai lanaiasmparser lanaicodegen lanaidesc lanaidisassembler lanaiinfo libdriver lineeditor linker lto mc mca mcdisassembler mcjit mcparser mips mipsasmparser mipscodegen mipsdesc mipsdisassembler mipsinfo mirparser msp430 msp430asmparser msp430codegen msp430desc msp430disassembler msp430info native nativecodegen nvptx nvptxcodegen nvptxdesc nvptxinfo objcarcopts object objectyaml option orcjit orcshared orctargetprocess passes perfjitevents powerpc powerpcasmparser powerpccodegen powerpcdesc powerpcdisassembler powerpcinfo profiledata remarks riscv riscvasmparser riscvcodegen riscvdesc riscvdisassembler riscvinfo runtimedyld scalaropts selectiondag sparc sparcasmparser sparccodegen sparcdesc sparcdisassembler sparcinfo support symbolize systemz systemzasmparser systemzcodegen systemzdesc systemzdisassembler systemzinfo tablegen target textapi transformutils vectorize webassembly webassemblyasmparser webassemblycodegen webassemblydesc webassemblydisassembler webassemblyinfo windowsmanifest x86 x86asmparser x86codegen x86desc x86disassembler x86info xcore xcorecodegen xcoredesc xcoredisassembler xcoreinfo xray" "--cc" "" "--cxx" "" "--cflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--channel" "nightly" "--color" "always"
expected success, got: signal: 11 (core dumped)

Build completed unsuccessfully in 0:01:49

compiler-errors · 2022-01-27T07:16:38Z

Pretty sure that CI test failure is unrelated since it also failed on #93358 lol

jackh726 · 2022-01-27T15:06:41Z

@compiler-errors can you explain a bit about the problem that's occurring (at least in one of the added tests)?

jackh726 · 2022-01-27T15:12:09Z

compiler/rustc_trait_selection/src/traits/project.rs

-    let obligation_pred_ty = obligation.predicate.term.ty().unwrap();
+    // FIXME(compiler-errors): We need to normalize here until we properly handle
+    // GAT projection types that we can't normalize properly, see note in
+    // `rustc_trait_selection::traits::project`


This...is the note?

lmao sorry, copied this from another file and it's definitely ambiguous now.

I meant the note about trying to normalize GAT projections then treating them differently if they fail to normalize (not inserting a type variable because it would capture escaping bounds). I think I linked it in the PR description. I'll fix this comment when I get to my computer.

jackh726 · 2022-01-27T15:40:07Z

Any change those tests are also fixed by #90887? Does this fix #90729?

compiler-errors · 2022-01-27T15:43:37Z

Yeah probably, that PR looks more general than mine. Lemme check.

jackh726 · 2022-01-27T15:45:35Z

Cool, thanks. If you can't, I'll check at some point (busy week for me).

compiler-errors · 2022-01-27T16:08:51Z

@jackh726 to answer your questions, yup this PR seems totally fixed by #90887 instead, and nope this PR doesn't fix #90729. Assuming to get that fixed would require me to insert more normalization somewhere during trait selection, but my approach is kinda like a band-aid, compared to a more general solution like yours in #90887.

I'll just close this PR in favor of yours.

jackh726 · 2022-01-27T17:02:05Z

I think I'd like to keep this open for now - if only solely for the eventual discussion I'll have with @nikomatsakis

I don't particularly dislike this PR. If nothing else, it does point out that maybe with #90887 we can remove some normalize calls in confirmation.

rust-log-analyzer · 2022-01-27T17:36:21Z

The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)


running 165 tests
...................................i...........

command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/i686-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/mir-opt" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/mir-opt" "--stage-id" "stage2-i686-unknown-linux-gnu" "--suite" "mir-opt" "--mode" "mir-opt" "--target" "i686-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-12/bin/FileCheck" "--nodejs" "/usr/bin/node" "--linker" "cc" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0  -Lnative=/checkout/obj/build/i686-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python3" "--lldb-python" "/usr/bin/python3" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "12.0.0" "--llvm-components" "aarch64 aarch64asmparser aarch64codegen aarch64desc aarch64disassembler aarch64info aarch64utils aggressiveinstcombine all all-targets amdgpu amdgpuasmparser amdgpucodegen amdgpudesc amdgpudisassembler amdgpuinfo amdgpuutils analysis arm armasmparser armcodegen armdesc armdisassembler arminfo armutils asmparser asmprinter avr avrasmparser avrcodegen avrdesc avrdisassembler avrinfo binaryformat bitreader bitstreamreader bitwriter bpf bpfasmparser bpfcodegen bpfdesc bpfdisassembler bpfinfo cfguard codegen core coroutines coverage debuginfocodeview debuginfodwarf debuginfogsym debuginfomsf debuginfopdb demangle dlltooldriver dwarflinker engine executionengine extensions filecheck frontendopenacc frontendopenmp fuzzmutate globalisel hellonew hexagon hexagonasmparser hexagoncodegen hexagondesc hexagondisassembler hexagoninfo instcombine instrumentation interfacestub interpreter ipo irreader jitlink lanai lanaiasmparser lanaicodegen lanaidesc lanaidisassembler lanaiinfo libdriver lineeditor linker lto mc mca mcdisassembler mcjit mcparser mips mipsasmparser mipscodegen mipsdesc mipsdisassembler mipsinfo mirparser msp430 msp430asmparser msp430codegen msp430desc msp430disassembler msp430info native nativecodegen nvptx nvptxcodegen nvptxdesc nvptxinfo objcarcopts object objectyaml option orcjit orcshared orctargetprocess passes perfjitevents powerpc powerpcasmparser powerpccodegen powerpcdesc powerpcdisassembler powerpcinfo profiledata remarks riscv riscvasmparser riscvcodegen riscvdesc riscvdisassembler riscvinfo runtimedyld scalaropts selectiondag sparc sparcasmparser sparccodegen sparcdesc sparcdisassembler sparcinfo support symbolize systemz systemzasmparser systemzcodegen systemzdesc systemzdisassembler systemzinfo tablegen target textapi transformutils vectorize webassembly webassemblyasmparser webassemblycodegen webassemblydesc webassemblydisassembler webassemblyinfo windowsmanifest x86 x86asmparser x86codegen x86desc x86disassembler x86info xcore xcorecodegen xcoredesc xcoredisassembler xcoreinfo xray" "--cc" "" "--cxx" "" "--cflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--channel" "nightly" "--color" "always"
expected success, got: signal: 11 (core dumped)

Build completed unsuccessfully in 0:01:33

compiler-errors · 2022-01-27T19:14:47Z

Sounds good to me @jackh726.

@compiler-errors can you explain a bit about the problem that's occurring (at least in one of the added tests)?

In that case, do you still want an explanation for why the normalization I added fixes those issues? I'd be happy to write up what I found if you'd like.

I don't particularly dislike this PR. If nothing else, it does point out that maybe with #90887 we can remove some normalize calls in confirmation.

Yeah, I was thinking about #90887 this morning after you pointed the PR out. If you're normalizing predicates during the fulfillment loop, maybe we can rip out many of the normalize_with_depth_to calls in confirmation, which might offset the perf regression in #90887. It seems "cleaner" to consolidate all of the normalization to one (or a few) place(s), instead of peppering it all around the confirmation code.

I think I'd like to keep this open for now - if only solely for the eventual discussion I'll have with @nikomatsakis

Not sure where these discussions with @nikomatsakis are happening, but if they're public or documented anywhere, I'd love to listen in. I've fixed a couple of GAT issues in the past, and getting that feature polished seems like really cool stuff if there's any space to get involved.

jackh726 · 2022-01-27T19:27:38Z

In that case, do you still want an explanation for why the normalization I added fixes those issues? I'd be happy to write up what I found if you'd like.

That would be awesome!

Yeah, I was thinking about #90887 this morning after you pointed the PR out. If you're normalizing predicates during the fulfillment loop, maybe we can rip out many of the normalize_with_depth_to calls in confirmation, which might offset the perf regression in #90887. It seems "cleaner" to consolidate all of the normalization to one (or a few) place(s), instead of peppering it all around the confirmation code.

This makes sense. It would also make it easily in the future to "just" remove those for lazy norm.

Not sure where these discussions with @nikomatsakis are happening, but if they're public or documented anywhere, I'd love to listen in. I've fixed a couple of GAT issues in the past, and getting that feature polished seems like really cool stuff if there's any space to get involved.

Niko and I meet roughly weekly, on Monday mornings (EST). They are technically GAT meetings (and that's what we usually talk about). It's probably cool if you join; message me on Zulip.

nikomatsakis · 2022-01-31T14:47:41Z

(yes, np if you want to join those @compiler-errors)

compiler-errors · 2022-01-31T21:46:55Z

I think I owed @jackh726 a high-level description for why this PR fixes the issues mentioned above.

For issue-93342, I think what's happening is that when we call BinaryExpression::new, we instantiate the bound for<'a, 'b> fn(&'a str, 'b str) -> bool {str_contains}: Fn(<_#0t as Scalar>::RefType<'_>, <_#1t as Scalar>::RefType<'_>) -> _#2t with fresh inference variables (0, 1, 2) corresponding to the generics A, B, and O, normalize that obligation (which fails to normalize, so it falls back to being un-normalized), and only later unify these inference variables in the function signature with its substitutions (which sets _#0t = _#1t = String, and _#2t = bool in this case).
Then when we try to check that obligation, we try to relate the Fn trait with the fn type. When relating the input parameters on both of these, we try to relate <String as Scalar>::RefType<'placeholder> and &'placeholder str, but since relating types doesn't normalize any projections inside of it, this fails. That's why normalizing the trait refs in confirm_poly_trait_refs works, so then the parameter types we pass to infcx.sup are both &'placeholder str.

For issue-93341, a similar thing happens, but for the fn output type. We instantiate the projection obligation for<'a> <_#0t as FnOnce(Id<'a>)>::Output == <_#1t as HasLifetime>::AtLifetime<'a> where _#0t corresponds to the F argument and _#1t corresponds to the S type variables on ExistentialLifetime::new.
Then while processing the obligations (specifically during project_and_unify_type), we end up trying to relate the normalized output type of the input parameter F (namely ExampleS<'placeholder>) with the "expected" output type from the obligation: <ExampleMarker as HasLifetime>::AtLifetime<'placeholder>, which was not properly normalized when the projection obligation was instantiated in the first place... which is why adding that normalization to project_and_unify_type does the job.

I think both of these cases result from us instantiating obligations for method calls before we have substituted all of the generic substs for that the method call. Without GATs, having a not-yet-solved projection type is fine, since we just instantiate an inference variable and unify it later once it can be solved. But with GATs, we leave the un-normalized projection type inside of the obligation untouched if it fails to normalize. Then various parts of rustc_trait_selection that call InferCtxt::{eq, sup, sub} to relate types don't account for un-normalized projection types (and instead expect there to have been an inference type inserted instead).

Sorry this explanation is kind of a mess!

compiler-errors · 2022-01-31T21:48:19Z

... now, all of this would be solved with lazy normalization, but since I'm still pretty new to working on rustc, I don't know where that effort's currently at.

I also have been toying with the idea of inserting inference variables for GATs (introducing some concept of "bound inference variables"), and representing them as having captured their inner late bound regions as substs (to avoid the whole "inference variables shouldn't capture placeholders" problem)... I tried working out a PR for what this might look like, but got tied up in the query canonicalization code, and some inference var freshening code...
but I think this is the opposite direction we want to move if if we're eventually heading towards lazy normalization.

jackh726 · 2022-01-31T22:30:27Z

@compiler-errors that's actually a fantastic explanation, and I can definitely follow what the cause is (I've seen very similar similar issues from the same root problem).

We should definitely chat (with Niko too) about how to solve this problem "correctly" - because it definitely seems as though you're interested in taking a stab at it. We can talk about it the next time we meet together, maybe.

compiler-errors · 2022-01-31T22:38:55Z

We should definitely chat (with Niko too) about how to solve this problem "correctly" - because it definitely seems as though you're interested in taking a stab at it. We can talk about it the next time we meet together, maybe.

Absolutely! Sounds good to me.

bors · 2022-02-02T02:32:25Z

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

jackh726 · 2022-02-24T15:44:09Z

Gonna mark this as experimental - really just an alternative implementation that is open just for discussion.

nikomatsakis · 2022-03-07T15:37:04Z

compiler/rustc_trait_selection/src/traits/select/confirmation.rs


-        debug!(?trait_ref, ?obligations, "generator candidate obligations");
+        let nested = self.confirm_poly_trait_refs(obligation, trait_ref)?;


Does this represent cleanup or a change in behavior? I guess we are now normalizing the obligation trait-ref as well? What are we doing that, exactly?

I believe it was previously an invariant that the obligation trait-ref was always normalized, and it seems like jack's PR also helps ensure that.

We actually landed this specific part of the PR in #94108. So this is change in behavior technically (and kinda cleanup, but mostly the former). @jackh726 and I thought that this (#94108) would offset the perf regression in #90887, but then we came to the conclusion that the perf regression in #90887 is probably noise.

#94108 probably doesn't need to be around after that PR lands. I can put up a revert once it lands, to see if we see perf regressions.

Normalize possibly un-normalized GAT projections

573a4da

rust-highfive assigned jackh726 Jan 27, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 27, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 27, 2022

compiler-errors commented Jan 27, 2022

View reviewed changes

compiler/rustc_trait_selection/src/traits/select/confirmation.rs Outdated Show resolved Hide resolved

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 27, 2022

compiler-errors added 2 commits January 26, 2022 22:15

Fix bug (eq type to itself), add tests for GATs in Fn trait bounds

55466d6

bless tests changed due to more projection normalization

968012d

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 27, 2022

jackh726 reviewed Jan 27, 2022

View reviewed changes

compiler-errors closed this Jan 27, 2022

jackh726 reopened this Jan 27, 2022

klensy mentioned this pull request Jan 27, 2022

compiletest crashes on CI with: expected success, got: signal: 11 (core dumped) #93384

Closed

compiler-errors mentioned this pull request Feb 10, 2022

#92917 breaks type inference on GATs #93874

Closed

jackh726 added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 24, 2022

nikomatsakis reviewed Mar 7, 2022

View reviewed changes

compiler-errors mentioned this pull request Mar 7, 2022

Normalize obligation and expected trait_refs in confirm_poly_trait_refs #94108

Merged

compiler-errors closed this Mar 8, 2022

compiler-errors deleted the unnormalized-gat-projections-in-predicates branch April 7, 2022 04:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normalize possibly un-normalized GAT projections #93361

Normalize possibly un-normalized GAT projections #93361

compiler-errors commented Jan 27, 2022

rust-highfive commented Jan 27, 2022

compiler-errors commented Jan 27, 2022

compiler-errors commented Jan 27, 2022

compiler-errors commented Jan 27, 2022

rust-log-analyzer commented Jan 27, 2022

compiler-errors commented Jan 27, 2022

jackh726 commented Jan 27, 2022

jackh726 Jan 27, 2022

compiler-errors Jan 27, 2022

jackh726 commented Jan 27, 2022

compiler-errors commented Jan 27, 2022 •

edited

Loading

jackh726 commented Jan 27, 2022

compiler-errors commented Jan 27, 2022 •

edited

Loading

jackh726 commented Jan 27, 2022 •

edited

Loading

rust-log-analyzer commented Jan 27, 2022

compiler-errors commented Jan 27, 2022

jackh726 commented Jan 27, 2022

nikomatsakis commented Jan 31, 2022

compiler-errors commented Jan 31, 2022 •

edited

Loading

compiler-errors commented Jan 31, 2022 •

edited

Loading

jackh726 commented Jan 31, 2022

compiler-errors commented Jan 31, 2022

bors commented Feb 2, 2022

jackh726 commented Feb 24, 2022

nikomatsakis Mar 7, 2022

nikomatsakis Mar 7, 2022

compiler-errors Mar 7, 2022


		debug!(?trait_ref, ?obligations, "generator candidate obligations");
		let nested = self.confirm_poly_trait_refs(obligation, trait_ref)?;

Normalize possibly un-normalized GAT projections #93361

Normalize possibly un-normalized GAT projections #93361

Conversation

compiler-errors commented Jan 27, 2022

rust-highfive commented Jan 27, 2022

compiler-errors commented Jan 27, 2022

compiler-errors commented Jan 27, 2022

compiler-errors commented Jan 27, 2022

rust-log-analyzer commented Jan 27, 2022

compiler-errors commented Jan 27, 2022

jackh726 commented Jan 27, 2022

jackh726 Jan 27, 2022

Choose a reason for hiding this comment

compiler-errors Jan 27, 2022

Choose a reason for hiding this comment

jackh726 commented Jan 27, 2022

compiler-errors commented Jan 27, 2022 • edited Loading

jackh726 commented Jan 27, 2022

compiler-errors commented Jan 27, 2022 • edited Loading

jackh726 commented Jan 27, 2022 • edited Loading

rust-log-analyzer commented Jan 27, 2022

compiler-errors commented Jan 27, 2022

jackh726 commented Jan 27, 2022

nikomatsakis commented Jan 31, 2022

compiler-errors commented Jan 31, 2022 • edited Loading

compiler-errors commented Jan 31, 2022 • edited Loading

jackh726 commented Jan 31, 2022

compiler-errors commented Jan 31, 2022

bors commented Feb 2, 2022

jackh726 commented Feb 24, 2022

nikomatsakis Mar 7, 2022

Choose a reason for hiding this comment

nikomatsakis Mar 7, 2022

Choose a reason for hiding this comment

compiler-errors Mar 7, 2022

Choose a reason for hiding this comment

compiler-errors commented Jan 27, 2022 •

edited

Loading

compiler-errors commented Jan 27, 2022 •

edited

Loading

jackh726 commented Jan 27, 2022 •

edited

Loading

compiler-errors commented Jan 31, 2022 •

edited

Loading

compiler-errors commented Jan 31, 2022 •

edited

Loading