Inline `evaluate_obligation` instead of going through the query system #81150

jyn514 · 2021-01-18T02:07:27Z

There are several reasons I changed this:

evaluate_obligation is only used in one place in
rustc_trait_selection, and in fact warns you against using it
anywhere else.
The implementation in rustc_traits was considerably more complicated
than necessary, because it didn't have the context available in
rustc_trait_selection. I think I just didn't understand how it worked.
It allows moving OverflowError into rustc_trait_selection, making
Move reporting recursion limit errors outside of the trait system #81091 simpler (in particular,
it allows holding an Obligation in OverflowError, which is only
defined in rustc_infer and not available in rustc_middle).

The only reason to keep the previous behavior is if the cache from the
query system made it significantly faster.

rust-highfive · 2021-01-18T02:07:31Z

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

There are several reasons I changed this: - `evaluate_obligation` is only used in one place in `rustc_trait_selection`, and in fact warns you against using it anywhere else. - The implementation in `rustc_traits` was considerably more complicated than necessary, because it didn't have the context available in `rustc_trait_selection`. - It allows moving OverflowError into rustc_trait_selection, making rust-lang#81091 simpler (in particular, it allows holding an `Obligation` in OverflowError, which is only defined in `rustc_infer` and not available in rustc_middle). The only reason to keep the previous behavior is if the cache from the query system made it significantly faster.

jyn514 · 2021-01-18T02:21:23Z

Hmm, I'm not sure why removing caching causes this test to fail:

failures:

---- [ui] ui/traits/cycle-cache-err-60010.rs stdout ----

error: ui test compiled successfully!

rust/src/test/ui/traits/cycle-cache-err-60010.rs

Lines 67 to 70 in 4253153

    
           // we used to fail to report an error here because we got the 
        
           // caching wrong. 
        
           SourceDatabase::parse(db); 
        
           //~^ ERROR overflow

rust-log-analyzer · 2021-01-18T02:22:58Z

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

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

.................................................................................................... 9100/11266
.................................................................................................... 9200/11266
...............................................................i......i............................. 9300/11266
.................................................................................................... 9400/11266
...iiiiiiiiiiii.i................................................................................... 9500/11266
.................................................................................................... 9700/11266
.................................................................................................... 9800/11266
.................................................................................................... 9900/11266
.................................................................................................... 10000/11266
---

---- [ui] ui/generator/auto-trait-regions.rs stdout ----
diff of stderr:

1 error: implementation of `Foo` is not general enough
-   --> $DIR/auto-trait-regions.rs:31:5
-    |
- LL | auto trait Foo {}
-    | ----------------- trait `Foo` defined here
- ...
- LL |     assert_foo(gen);
-    |     ^^^^^^^^^^ implementation of `Foo` is not general enough
-    |
-    = note: `Foo` would have to be implemented for the type `&'0 OnlyFooIfStaticRef`, for any lifetime `'0`...
-    = note: ...but `Foo` is actually implemented for the type `&'1 OnlyFooIfStaticRef`, for some specific lifetime `'1`
- 
- error: implementation of `Foo` is not general enough
-   --> $DIR/auto-trait-regions.rs:31:5
-    |
- LL | auto trait Foo {}
-    | ----------------- trait `Foo` defined here
- ...
- LL |     assert_foo(gen);
-    |     ^^^^^^^^^^ implementation of `Foo` is not general enough
-    |
-    = note: `Foo` would have to be implemented for the type `&'0 OnlyFooIfStaticRef`, for any lifetime `'0`...
-    = note: ...but `Foo` is actually implemented for the type `&'1 OnlyFooIfStaticRef`, for some specific lifetime `'1`
- 
- error: implementation of `Foo` is not general enough
27    |
27    |
28 LL | auto trait Foo {}

46    = note: `Foo` would have to be implemented for the type `A<'0, '1>`, for any two lifetimes `'0` and `'1`...
47    = note: ...but `Foo` is actually implemented for the type `A<'_, '2>`, for some specific lifetime `'2`
- error: aborting due to 4 previous errors
+ error: aborting due to 2 previous errors
50 
51 
51 


The actual stderr differed from the expected stderr.
Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/generator/auto-trait-regions/auto-trait-regions.stderr
To update references, rerun the tests and pass the `--bless` flag
Some tests failed in compiletest suite=ui mode=ui host=x86_64-unknown-linux-gnu target=x86_64-unknown-linux-gnu
To only update this specific test, also pass `--test-args generator/auto-trait-regions.rs`
error: 1 errors occurred comparing output.
status: exit code: 1
status: exit code: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/generator/auto-trait-regions.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zemit-future-incompat-report" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/generator/auto-trait-regions" "-A" "unused" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/generator/auto-trait-regions/auxiliary"
------------------------------------------

------------------------------------------
stderr:
stderr:
------------------------------------------
error: implementation of `Foo` is not general enough
   |
   |
LL | auto trait Foo {}
   | ----------------- trait `Foo` defined here
...
LL |     assert_foo(gen);
   |     ^^^^^^^^^^ implementation of `Foo` is not general enough
   |
   = note: `Foo` would have to be implemented for the type `A<'0, '1>`, for any two lifetimes `'0` and `'1`...
   = note: ...but `Foo` is actually implemented for the type `A<'_, '2>`, for some specific lifetime `'2`

error: implementation of `Foo` is not general enough
   |
   |
LL | auto trait Foo {}
   | ----------------- trait `Foo` defined here
...
LL |     assert_foo(gen);
   |     ^^^^^^^^^^ implementation of `Foo` is not general enough
   |
   = note: `Foo` would have to be implemented for the type `A<'0, '1>`, for any two lifetimes `'0` and `'1`...
   = note: ...but `Foo` is actually implemented for the type `A<'_, '2>`, for some specific lifetime `'2`
error: aborting due to 2 previous errors


------------------------------------------
------------------------------------------


---- [ui] ui/impl-trait/auto-trait-leak.rs stdout ----
diff of stderr:

34    |
35 LL | fn cycle1() -> impl Clone {
36    | ^^^^^^^^^^^^^^^^^^^^^^^^^
-    = note: ...which requires evaluating trait selection obligation `impl std::clone::Clone: std::marker::Send`...
38 note: ...which requires computing type of `cycle2::{opaque#0}`...
40    |

70    |
70    |
71 LL | fn cycle2() -> impl Clone {
72    | ^^^^^^^^^^^^^^^^^^^^^^^^^
-    = note: ...which requires evaluating trait selection obligation `impl std::clone::Clone: std::marker::Send`...
74    = note: ...which again requires computing type of `cycle1::{opaque#0}`, completing the cycle
75 note: cycle used when checking item types in top-level module


The actual stderr differed from the expected stderr.
The actual stderr differed from the expected stderr.
Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/impl-trait/auto-trait-leak/auto-trait-leak.stderr
To update references, rerun the tests and pass the `--bless` flag
To only update this specific test, also pass `--test-args impl-trait/auto-trait-leak.rs`
error: 1 errors occurred comparing output.
status: exit code: 1
status: exit code: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/impl-trait/auto-trait-leak.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zemit-future-incompat-report" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/impl-trait/auto-trait-leak" "-A" "unused" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/impl-trait/auto-trait-leak/auxiliary"
------------------------------------------

------------------------------------------
stderr:
stderr:
------------------------------------------
error[E0391]: cycle detected when computing type of `cycle1::{opaque#0}`
   |
   |
LL | fn cycle1() -> impl Clone {
   |
   |
note: ...which requires borrow-checking `cycle1`...
   |
   |
LL | fn cycle1() -> impl Clone {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...which requires processing `cycle1`...
   |
   |
LL | fn cycle1() -> impl Clone {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...which requires processing MIR for `cycle1`...
   |
   |
LL | fn cycle1() -> impl Clone {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...which requires unsafety-checking `cycle1`...
   |
   |
LL | fn cycle1() -> impl Clone {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...which requires building MIR for `cycle1`...
   |
   |
LL | fn cycle1() -> impl Clone {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...which requires type-checking `cycle1`...
   |
   |
LL | fn cycle1() -> impl Clone {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...which requires computing type of `cycle2::{opaque#0}`...
   |
   |
LL | fn cycle2() -> impl Clone {
   |                ^^^^^^^^^^
note: ...which requires borrow-checking `cycle2`...
   |
   |
LL | fn cycle2() -> impl Clone {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...which requires processing `cycle2`...
   |
   |
LL | fn cycle2() -> impl Clone {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...which requires processing MIR for `cycle2`...
   |
   |
LL | fn cycle2() -> impl Clone {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...which requires unsafety-checking `cycle2`...
   |
   |
LL | fn cycle2() -> impl Clone {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...which requires building MIR for `cycle2`...
   |
   |
LL | fn cycle2() -> impl Clone {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...which requires type-checking `cycle2`...
   |
   |
LL | fn cycle2() -> impl Clone {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: ...which again requires computing type of `cycle1::{opaque#0}`, completing the cycle
note: cycle used when checking item types in top-level module
   |
   |
LL | / use std::cell::Cell;
LL | | use std::rc::Rc;
LL | |
LL | | fn send<T: Send>(_: T) {}
...  |
LL | |     Rc::new(String::from("foo"))
LL | | }


error[E0277]: `Rc<String>` cannot be sent between threads safely
   |
   |
LL | fn send<T: Send>(_: T) {}
   |            ---- required by this bound in `send`
...
LL |     send(cycle2().clone());
   |     ^^^^ `Rc<String>` cannot be sent between threads safely
...
LL | fn cycle2() -> impl Clone {
   |                ---------- within this `impl Clone`
   |
   = help: within `impl Clone`, the trait `Send` is not implemented for `Rc<String>`
   = note: required because it appears within the type `impl Clone`
error: aborting due to 2 previous errors

Some errors have detailed explanations: E0277, E0391.
For more information about an error, try `rustc --explain E0277`.
For more information about an error, try `rustc --explain E0277`.

------------------------------------------


---- [ui] ui/traits/cycle-cache-err-60010.rs stdout ----

error: ui test compiled successfully!
status: exit code: 0
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/traits/cycle-cache-err-60010.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zemit-future-incompat-report" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/traits/cycle-cache-err-60010" "-A" "unused" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/traits/cycle-cache-err-60010/auxiliary"
------------------------------------------

------------------------------------------
stderr:
---
test result: FAILED. 11176 passed; 3 failed; 87 ignored; 0 measured; 0 filtered out; finished in 111.66s



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/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--suite" "ui" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-9/bin/FileCheck" "--nodejs" "/usr/bin/node" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python3" "--lldb-python" "/usr/bin/python3" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "9.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 codegen core coroutines coverage debuginfocodeview debuginfodwarf debuginfogsym debuginfomsf debuginfopdb demangle dlltooldriver engine executionengine fuzzmutate globalisel hexagon hexagonasmparser hexagoncodegen hexagondesc hexagondisassembler hexagoninfo instcombine instrumentation 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 passes perfjitevents powerpc powerpcasmparser powerpccodegen powerpcdesc powerpcdisassembler powerpcinfo profiledata remarks riscv riscvasmparser riscvcodegen riscvdesc riscvdisassembler riscvinfo riscvutils 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 x86utils xcore xcorecodegen xcoredesc xcoredisassembler xcoreinfo xray" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"


failed to run: /checkout/obj/build/bootstrap/debug/bootstrap --stage 2 test --exclude src/tools/tidy
Build completed unsuccessfully in 0:12:38

Aaron1011 · 2021-01-18T02:43:35Z

I suspect that the test failure us due to the removed query canonicalization.

jyn514 · 2021-01-18T02:46:12Z

I suspect that the test failure us due to the removed query canonicalization.

Yup, I changed it to have the old code and the test passes now. I'm still confused why the cycle test is now failing, though.

rust-log-analyzer · 2021-01-18T02:57:32Z

The job mingw-check failed! Check out the build log: (web) (plain)

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

configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
Checking which error codes lack tests...
Found 435 error codes
Found 0 error codes with no tests
Done!
tidy error: /checkout/compiler/rustc_trait_selection/src/traits/query/evaluate_obligation.rs:81: trailing whitespace
tidy error: /checkout/compiler/rustc_trait_selection/src/traits/query/evaluate_obligation.rs:84: trailing whitespace
some tidy checks failed

command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "/checkout/obj/build"
expected success, got: exit code: 1

lcnr · 2021-01-18T10:38:48Z

i would expect that caching this query is quite useful, so I am hesitant on landing this.

Once CI passes I would start by running perf

bors · 2021-02-02T15:02:16Z

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

jyn514 · 2021-02-11T22:35:30Z

I don't know how to fix the test suite and it sounds like this won't be very useful.

jyn514 added A-traits Area: Trait system T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 18, 2021

rust-highfive assigned lcnr Jan 18, 2021

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

jyn514 force-pushed the evaluate-obligation branch from 8d69194 to 1548e0f Compare January 18, 2021 02:07

Add back canonicalization

81ea1b9

jyn514 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 23, 2021

jyn514 closed this Feb 11, 2021

jyn514 deleted the evaluate-obligation branch February 11, 2021 22:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inline `evaluate_obligation` instead of going through the query system #81150

Inline `evaluate_obligation` instead of going through the query system #81150

jyn514 commented Jan 18, 2021 •

edited

Loading

rust-highfive commented Jan 18, 2021

jyn514 commented Jan 18, 2021

rust-log-analyzer commented Jan 18, 2021

Aaron1011 commented Jan 18, 2021

jyn514 commented Jan 18, 2021

rust-log-analyzer commented Jan 18, 2021

lcnr commented Jan 18, 2021

bors commented Feb 2, 2021

jyn514 commented Feb 11, 2021

Inline evaluate_obligation instead of going through the query system #81150

Inline evaluate_obligation instead of going through the query system #81150

Conversation

jyn514 commented Jan 18, 2021 • edited Loading

rust-highfive commented Jan 18, 2021

jyn514 commented Jan 18, 2021

rust-log-analyzer commented Jan 18, 2021

Aaron1011 commented Jan 18, 2021

jyn514 commented Jan 18, 2021

rust-log-analyzer commented Jan 18, 2021

lcnr commented Jan 18, 2021

bors commented Feb 2, 2021

jyn514 commented Feb 11, 2021

Inline `evaluate_obligation` instead of going through the query system #81150

Inline `evaluate_obligation` instead of going through the query system #81150

jyn514 commented Jan 18, 2021 •

edited

Loading