Revise never type fallback algorithm #88804

Mark-Simulacrum · 2021-09-10T00:28:36Z

This is a rebase of #84573, but dropping the stabilization of never type (and the accompanying large test diff).

Each commit builds & has tests updated alongside it, and could be reviewed in a more or less standalone fashion. But it may make more sense to review the PR as a whole, I'm not sure. It should be noted that tests being updated isn't really a good indicator of final behavior -- never_type_fallback is not enabled by default in this PR, so we can't really see the full effects of the commits here.

This combines the work by Niko, which is documented in this gist, with some additional rules largely derived to target specific known patterns that regress with the algorithm solely derived by Niko. We build these from an intuition that:

In general, fallback to () is sound in all cases
But, in general, we prefer fallback to ! as it accepts more code, particularly that written to intentionally use ! (e.g., Result's with a Infallible/! variant).

When evaluating Niko's proposed algorithm, we find that there are certain cases where fallback to ! leads to compilation failures in real-world code, and fallback to () fixes those errors. In order to allow for stabilization, we need to fix a good portion of these patterns.

The final rule set this PR proposes is that, by default, we fallback from ?T to !, with the following exceptions:

?T: Foo and Bar::Baz = ?T and (): Foo, then fallback to ()
Per Niko's algorithm, the "live" ?T also fallback to ().

The first rule is necessary to address a fairly common pattern which boils down to something like the snippet below. Without rule 1, we do not see the closure's return type as needing a () fallback, which leads to compilation failure.

#![feature(never_type_fallback)]

trait Bar { }
impl Bar for () {  }
impl Bar for u32 {  }

fn foo<R: Bar>(_: impl Fn() -> R) {}

fn main() {
    foo(|| panic!());
}

r? @jackh726

Mark-Simulacrum · 2021-09-10T00:39:55Z

@jackh726 The opening comment is hopefully helpful, but in terms of review I feel like the best bet is for you to take a look at the PR as a whole at a high level, calling out notable deficiencies or questions before we dive in to each commit in detail. But really up to you.

At a high level, this PR has the first several commits which add Niko's coercion graph and detect when we need to fallback to (), and then the one commit from me atop that which includes the special case and code necessary to handle it. The coercion graph is fairly self explanatory in some sense in terms of the code, though is likely not very intuitive in terms of the mapping to effect on actual Rust code. I'm not sure I fully grasp it at a low level - mostly because coercion and subtyping predicates aren't (to me) obviously visible in Rust source code.

My special casing instruments predicate registration to capture the appropriate projection and trait predicates and stores away the inference variables. I'm not confident in this code being placed appropriately -- happy to move it around. My main goal was to avoid a dependency on select_obligations_where_possible not being called "before" us and eliminating the obligations we need to successfully determine which direction we should fall back. That said, it's possible I got some things wrong there.

There's also a few commented sections and FIXMEs, some of which likely could use a more experienced hand to help with -- but I don't think they represent major roadblocks, more like polish, so we can iterate on that after the high level direction I suspect.

FWIW, I believe this PR shouldn't affect end-user visible behavior modulo new ICEs or something (not that I'm aware of any). So it can be merged once we deem it ready, and then we'd follow up with a stabilization PR and testing. My impression based on discussions with @nikomatsakis is that never type is sufficiently unstable that iterating like this on nightly is OK -- we will want to poll T-lang for opinions on the algorithm as part of a stabilization report, of course, but there is no need to surface before then. Niko's version of the algorithm was already discussed in T-lang as well, and my addition is a pretty small delta anyway.

Mark-Simulacrum · 2021-09-12T21:10:05Z

(Rebased to drop first commit, now that the PR has been merged).

jackh726

A few comments. Most of the commits look good. I have some reservations about the last commit, as discussed on zulip. But, I've reviewed based on "the logic here is what we want, at least for now" and "there might be another way to do this, but this is straightforward enough"

jackh726 · 2021-09-11T19:56:26Z

src/test/ui/never_type/diverging-fallback-no-leak.rs

+fn main() {
+    // Here the type variable falls back to `!`,
+    // and hence we get a type error:
+    unconstrained_arg(return); //~ ERROR trait bound `!: Test` is not satisfied


If they don't exist, I'd like to see identical tests without never_type_fallback enabled.

compiler/rustc_trait_selection/src/traits/relationships.rs

jackh726 · 2021-09-15T22:51:41Z

compiler/rustc_trait_selection/src/traits/relationships.rs

+                    .to_predicate(infcx.tcx),
+            );
+            // Don't report overflow errors. Otherwise equivalent to may_hold.
+            if let Ok(result) = infcx.probe(|_| infcx.evaluate_obligation(&o)) {


So, thinking about this. This essentially means that for every ?X: Foo predicate that gets registered, we also check if (): Foo...I'm going to be surprised if this doesn't hit perf.

I had the same thought. It seems like we should only do this if we observe that some potentially diverging variable is involved. Is there a reason that doesn't work? (Maybe this was covered on zulip?)

compiler/rustc_typeck/src/check/fallback.rs

jackh726 · 2021-09-15T23:00:44Z

compiler/rustc_typeck/src/check/fallback.rs

+                } else if let ty::PredicateKind::Subtype(ty::SubtypePredicate {
+                    a_is_expected: _,
+                    a,
+                    b,
+                }) = atom
+                {
+                    let a_vid = self.root_vid(a)?;
+                    let b_vid = self.root_vid(b)?;
+                    Some((a_vid, b_vid))


I think this needs a comment...

Is the idea that even though it's not a "coercion", it still counts because we effectively unify them?

compiler/rustc_typeck/src/check/fallback.rs

src/test/ui/never_type/fallback-closure-wrap.rs

compiler/rustc_typeck/src/check/fallback.rs

rust-timer · 2021-09-16T03:50:26Z

Finished benchmarking commit (b7c0a1f556d43c70af537ca4d9c44c9ac390fe0c): comparison url.

Summary: This change led to large relevant mixed results 🤷 in compiler performance.

Large improvement in instruction counts (up to -3.2% on full builds of inflate)
Large regression in instruction counts (up to 4.3% on full builds of diesel)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

jackh726 · 2021-09-16T05:01:58Z

Wow! Surprising...did I miss something.

We now fallback type variables using the following rules: * Construct a coercion graph `A -> B` where `A` and `B` are unresolved type variables or the `!` type. * Let D be those variables that are reachable from `!`. * Let N be those variables that are reachable from a variable not in D. * All variables in (D \ N) fallback to `!`. * All variables in (D & N) fallback to `()`.

The comment seems incorrect. Testing revealed that the examples in question still work (as well as some variants) even without the special casing here.

Instead, we now record those type variables that are the target of a `NeverToAny` adjustment and consider those to be the "diverging" type variables. This allows us to remove the special case logic that creates a type variable for `!` in coercion.

Instead, we now rely on the code that looks for a NeverToAny adjustment.

Extend the `DepthFirstSearch` iterator so that it can be re-used and extended with add'l start nodes. Then replace the FxHashSets of nodes we were using in the fallback analysis with a single iterator. This way we won't re-walk portions of the graph that are reached more than once, and we also do less allocation etc.

nikomatsakis · 2021-09-17T18:13:03Z

compiler/rustc_trait_selection/src/traits/relationships.rs

+                    .to_predicate(infcx.tcx),
+            );
+            // Don't report overflow errors. Otherwise equivalent to may_hold.
+            if let Ok(result) = infcx.probe(|_| infcx.evaluate_obligation(&o)) {


I had the same thought. It seems like we should only do this if we observe that some potentially diverging variable is involved. Is there a reason that doesn't work? (Maybe this was covered on zulip?)

nikomatsakis · 2021-09-17T18:13:59Z

compiler/rustc_middle/src/ty/mod.rs

+    ///  * `(): Foo` may be satisfied
+    pub self_in_trait: bool,
+    /// This is true if we identified that this Ty (`?T`) is found in a `<_ as
+    /// _>::AssocType = ?T`


We should document why these conditions are being used -- but it would be fine (imo) for this to be on a never-type-initiative repo (and linked from here).

jackh726 · 2021-09-19T03:17:03Z

r=me here. I would feel better if @nikomatsakis can take a look and give the final signoff, but given that this is gated behind a feature gate, it might be better to merge and iterate in-tree.

nikomatsakis · 2021-09-21T10:24:08Z

I am not sure that we will end up with "great" reasoning beyond the 'it fixes regressions'.

I think that is fine, I'd just like to comment on what kind of regression we are looking to fix.

Mark-Simulacrum · 2021-09-21T10:59:50Z

I am not sure that we will end up with "great" reasoning beyond the 'it fixes regressions'.

I think that is fine, I'd just like to comment on what kind of regression we are looking to fix.

There's now a comment in the fallback code pointing to the regression test the relationship code is fixing. Happy to edit it, or expand it - I think creating that repository for the alternatives documentation is a good idea - once I rebase the stabilization commits it'll be easier to off hand indicate which patterns break with particular algorithms without needing to refer to various hackmds :)

nikomatsakis · 2021-09-23T17:47:14Z

@bors r+

bors · 2021-09-23T17:47:15Z

📌 Commit c4c5fc8 has been approved by nikomatsakis

jackh726 · 2021-09-23T17:51:43Z

@bors r=nikomatsakis,jackh726

even though it might be procedural, I'd like to note that I did review this (particularly because Niko did originally author many of these commits)

bors · 2021-09-23T17:51:44Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: fix non_blanket_impls iteration order #89016

bors · 2021-09-23T17:51:45Z

📌 Commit c4c5fc8 has been approved by nikomatsakis,jackh726

bors · 2021-09-23T18:51:41Z

⌛ Testing commit c4c5fc8 with merge 665a996483054f10275697444bc0ddf6a2951118...

bors · 2021-09-23T19:38:23Z

💔 Test failed - checks-actions

rust-log-analyzer · 2021-09-23T19:38:29Z

The job i686-msvc-1 failed! Check out the build log: (web) (plain)

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

test [rustdoc] rustdoc\where-sized.rs ... ok
test [rustdoc] rustdoc\without-redirect.rs ... ok
test [rustdoc] rustdoc\wrapping.rs ... ok
test [rustdoc] rustdoc\where.rs ... ok
Some tests failed in compiletest suite=rustdoc mode=rustdoc host=i686-pc-windows-msvc target=i686-pc-windows-msvc
failures:

---- [rustdoc] rustdoc\macro-document-private-duplicate.rs stdout ----


error: htmldocck failed!
status: exit code: 1
command: "C:\\hostedtoolcache\\windows\\Python\\3.9.7\\x64\\python3.exe" "D:\\a\\rust\\rust\\src/etc/htmldocck.py" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\test\\rustdoc\\macro-document-private-duplicate" "D:\\a\\rust\\rust\\src/test\\rustdoc\\macro-document-private-duplicate.rs"
------------------------------------------

------------------------------------------
stderr:
stderr:
------------------------------------------
12: @has check failed
 `PATTERN` did not match
 // @has macro_document_private_duplicate/macro.a_macro.html 'Doc 1.'
19: @!has check failed
 `PATTERN` did not match
 // @!has macro_document_private_duplicate/macro.a_macro.html 'Doc 2.'
Encountered 2 errors

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

---
test result: FAILED. 462 passed; 1 failed; 7 ignored; 0 measured; 0 filtered out; finished in 87.18s



command did not execute successfully: "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage0-tools-bin\\compiletest.exe" "--compile-lib-path" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\bin" "--run-lib-path" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\lib\\rustlib\\i686-pc-windows-msvc\\lib" "--rustc-path" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\bin\\rustc.exe" "--rustdoc-path" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\bin\\rustdoc.exe" "--src-base" "D:\\a\\rust\\rust\\src/test\\rustdoc" "--build-base" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\test\\rustdoc" "--stage-id" "stage2-i686-pc-windows-msvc" "--suite" "rustdoc" "--mode" "rustdoc" "--target" "i686-pc-windows-msvc" "--host" "i686-pc-windows-msvc" "--llvm-filecheck" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\llvm\\build\\bin\\FileCheck.exe" "--nodejs" "C:\\Program Files\\nodejs\\node" "--npm" "C:\\Program Files\\nodejs\\npm" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0  -Lnative=D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\native\\rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0  -Lnative=D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\native\\rust-test-helpers" "--docck-python" "C:\\hostedtoolcache\\windows\\Python\\3.9.7\\x64\\python3.exe" "--lldb-python" "C:\\hostedtoolcache\\windows\\Python\\3.9.7\\x64\\python3.exe" "--gdb" "C:\\msys64\\usr\\bin\\gdb" "--llvm-version" "13.0.0-rust-1.57.0-nightly" "--llvm-components" "aarch64 aarch64asmparser aarch64codegen aarch64desc aarch64disassembler aarch64info aarch64utils aggressiveinstcombine all all-targets 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 dwp engine executionengine extensions filecheck frontendopenacc frontendopenmp fuzzmutate globalisel hexagon hexagonasmparser hexagoncodegen hexagondesc hexagondisassembler hexagoninfo instcombine instrumentation interfacestub interpreter ipo irreader jitlink libdriver lineeditor linker lto m68k m68kasmparser m68kcodegen m68kdesc m68kdisassembler m68kinfo 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 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 webassemblyutils windowsmanifest x86 x86asmparser x86codegen x86desc x86disassembler x86info xray" "--cc" "" "--cxx" "" "--cflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--channel" "nightly" "--color" "always"


Build completed unsuccessfully in 0:41:05
Build completed unsuccessfully in 0:41:05
make: *** [Makefile:72: ci-subset-1] Error 1

Mark-Simulacrum · 2021-09-23T19:42:47Z

@bors retry - hopefully spurious failure in rustdoc/macro-document-private-duplicate.rs, not clear how that could fail due to this PR. Seems to work on the linux builders.

bors · 2021-09-23T22:45:26Z

⌛ Testing commit c4c5fc8 with merge 900cf5e...

bors · 2021-09-24T01:47:57Z

☀️ Test successful - checks-actions
Approved by: nikomatsakis,jackh726
Pushing 900cf5e to master...

rust-timer · 2021-09-24T04:40:50Z

Finished benchmarking commit (900cf5e): comparison url.

Summary: This change led to large relevant regressions 😿 in compiler performance.

Large regression in instruction counts (up to 2.7% on full builds of keccak)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

rust-highfive assigned jackh726 Sep 10, 2021

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

Mark-Simulacrum force-pushed the never-algo-v2 branch from 0cf72db to 0dd4ba3 Compare September 10, 2021 00:30

Mark-Simulacrum force-pushed the never-algo-v2 branch from 0dd4ba3 to de88f22 Compare September 12, 2021 21:09

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 13, 2021

jackh726 requested changes Sep 15, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 15, 2021

This comment has been minimized.

Sign in to view

jackh726 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 Sep 15, 2021

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 16, 2021

jackh726 removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 17, 2021

nikomatsakis added 5 commits September 17, 2021 12:47

remove diverging type variables from fn check

ebb8ff9

The comment seems incorrect. Testing revealed that the examples in question still work (as well as some variants) even without the special casing here.

stop categorizing inference variables as diverging when created

bc2ece2

Instead, we now rely on the code that looks for a NeverToAny adjustment.

Mark-Simulacrum mentioned this pull request Sep 17, 2021

[WIP] Never type experiments #84573

Closed

nikomatsakis reviewed Sep 17, 2021

View reviewed changes

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 18, 2021

Mark-Simulacrum assigned nikomatsakis Sep 20, 2021

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

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 23, 2021

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

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 24, 2021

bors merged commit 900cf5e into rust-lang:master Sep 24, 2021

rustbot added this to the 1.57.0 milestone Sep 24, 2021

bors mentioned this pull request Sep 24, 2021

Use larger span for adjustment THIR expressions #89110

Merged

petrochenkov mentioned this pull request Sep 24, 2021

Test src\test\rustdoc\macro-document-private-duplicate.rs fails spuriously #89228

Open

Mark-Simulacrum deleted the never-algo-v2 branch September 27, 2021 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revise never type fallback algorithm #88804

Revise never type fallback algorithm #88804

Mark-Simulacrum commented Sep 10, 2021 •

edited

Loading

Mark-Simulacrum commented Sep 10, 2021

Mark-Simulacrum commented Sep 12, 2021

jackh726 left a comment

jackh726 Sep 11, 2021

jackh726 Sep 15, 2021

nikomatsakis Sep 17, 2021

jackh726 Sep 15, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rust-timer commented Sep 16, 2021

jackh726 commented Sep 16, 2021

nikomatsakis Sep 17, 2021

nikomatsakis Sep 17, 2021

jackh726 commented Sep 19, 2021

nikomatsakis commented Sep 21, 2021

Mark-Simulacrum commented Sep 21, 2021

nikomatsakis commented Sep 23, 2021

bors commented Sep 23, 2021

jackh726 commented Sep 23, 2021

bors commented Sep 23, 2021

bors commented Sep 23, 2021

bors commented Sep 23, 2021

bors commented Sep 23, 2021

rust-log-analyzer commented Sep 23, 2021

Mark-Simulacrum commented Sep 23, 2021

bors commented Sep 23, 2021

bors commented Sep 24, 2021

rust-timer commented Sep 24, 2021

Revise never type fallback algorithm #88804

Revise never type fallback algorithm #88804

Conversation

Mark-Simulacrum commented Sep 10, 2021 • edited Loading

Mark-Simulacrum commented Sep 10, 2021

Mark-Simulacrum commented Sep 12, 2021

jackh726 left a comment

Choose a reason for hiding this comment

jackh726 Sep 11, 2021

Choose a reason for hiding this comment

jackh726 Sep 15, 2021

Choose a reason for hiding this comment

nikomatsakis Sep 17, 2021

Choose a reason for hiding this comment

jackh726 Sep 15, 2021

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rust-timer commented Sep 16, 2021

jackh726 commented Sep 16, 2021

nikomatsakis Sep 17, 2021

Choose a reason for hiding this comment

nikomatsakis Sep 17, 2021

Choose a reason for hiding this comment

jackh726 commented Sep 19, 2021

nikomatsakis commented Sep 21, 2021

Mark-Simulacrum commented Sep 21, 2021

nikomatsakis commented Sep 23, 2021

bors commented Sep 23, 2021

jackh726 commented Sep 23, 2021

bors commented Sep 23, 2021

bors commented Sep 23, 2021

bors commented Sep 23, 2021

bors commented Sep 23, 2021

rust-log-analyzer commented Sep 23, 2021

Mark-Simulacrum commented Sep 23, 2021

bors commented Sep 23, 2021

bors commented Sep 24, 2021

rust-timer commented Sep 24, 2021

Mark-Simulacrum commented Sep 10, 2021 •

edited

Loading