[crater only] Always make inductive cycles as ambig during typeck #116494
Conversation
rustbot
added
S-waiting-on-review
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
…mbig, r=<try> [crater only] Always make inductive cycles as ambig during typeck r? `@ghost`
This comment has been minimized.
This comment has been minimized.
|
Lol, didn't mean to invoke rust-timer. Wanted to crater this 😆 |
|
☀️ Try build successful - checks-actions |
1 similar comment
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
Finished benchmarking commit (919137c): comparison URL. Overall result: no relevant changes - no action neededBenchmarking 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 may lead to changes in compiler perf. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 623.951s -> 623.316s (-0.10%) |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
…mbig, r=<try> [crater only] Always make inductive cycles as ambig during typeck r? `@ghost`
This comment has been minimized.
This comment has been minimized.
|
☀️ Try build successful - checks-actions |
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
compiler-errors
force-pushed
the
typeck-coinductive-ambig
branch
from
35865b3
to
9adee3e
Compare
|
This PR has served its purpose |
…ays, r=lcnr Make inductive cycles always ambiguous This makes inductive cycles always result in ambiguity rather than be treated like a stack-dependent error. This has some interactions with specialization, and so breaks a few UI tests that I don't agree should've ever worked in the first place, and also breaks a handful of crates in a way that I don't believe is a problem. On the bright side, it puts us in a better spot when it comes to eventually enabling coinduction everywhere. ## Results This was cratered in rust-lang#116494 (comment), which boils down to two regressions: * `lu_packets` - This code should have never compiled in the first place. More below. * **ALL** other regressions are due to `commit_verify@0.11.0-beta.1` (edit: and `commit_verify@0.10.x`) - This actually seems to be fixed in version `0.11.0-beta.5`, which is the *most* up to date version, but it's still prerelease on crates.io so I don't think cargo ends up picking `beta.5` when building dependent crates. ### `lu_packets` Firstly, this crate uses specialization, so I think it's automatically worth breaking. However, I've minimized [the regression](https://crater-reports.s3.amazonaws.com/pr-116494-3/try%23d614ed876e31a5f3ad1d0fbf848fcdab3a29d1d8/gh/lcdr.lu_packets/log.txt) to: ```rust // Upstream crate pub trait Serialize {} impl Serialize for &() {} impl<S> Serialize for &[S] where for<'a> &'a S: Serialize {} // ----------------------------------------------------------------------- // // Downstream crate #![feature(specialization)] #![allow(incomplete_features, unused)] use upstream::Serialize; trait Replica { fn serialize(); } impl<T> Replica for T { default fn serialize() {} } impl<T> Replica for Option<T> where for<'a> &'a T: Serialize, { fn serialize() {} } ``` Specifically this fails when computing the specialization graph for the `downstream` crate. The code ends up cycling on `&[?0]: Serialize` when we equate `&?0 = &[?1]` during impl matching, which ends up needing to prove `&[?1]: Serialize`, which since cycles are treated like ambiguity, ends up in a **fatal overflow**. For some reason this requires two crates, squashing them into one crate doesn't work. Side-note: This code is subtly order dependent. When minimizing, I ended up having the code start failing on `nightly` very easily after removing and reordering impls. This seems to me all the more reason to remove this behavior altogether. ## Side-note: Item Bounds (edit: this was fixed independently in rust-lang#121123) Due to the changes in rust-lang#120584 where we now consider an alias's item bounds *and* all the item bounds of the alias's nested self type aliases, I've had to add e6b64c6 which is a hack to make sure we're not eagerly normalizing bounds that have nothing to do with the predicate we're trying to solve, and which result in. This is fixed in a more principled way in rust-lang#121123. --- r? lcnr for an initial review
…ays, r=lcnr Make inductive cycles always ambiguous This makes inductive cycles always result in ambiguity rather than be treated like a stack-dependent error. This has some interactions with specialization, and so breaks a few UI tests that I don't agree should've ever worked in the first place, and also breaks a handful of crates in a way that I don't believe is a problem. On the bright side, it puts us in a better spot when it comes to eventually enabling coinduction everywhere. ## Results This was cratered in rust-lang#116494 (comment), which boils down to two regressions: * `lu_packets` - This code should have never compiled in the first place. More below. * **ALL** other regressions are due to `commit_verify@0.11.0-beta.1` (edit: and `commit_verify@0.10.x`) - This actually seems to be fixed in version `0.11.0-beta.5`, which is the *most* up to date version, but it's still prerelease on crates.io so I don't think cargo ends up picking `beta.5` when building dependent crates. ### `lu_packets` Firstly, this crate uses specialization, so I think it's automatically worth breaking. However, I've minimized [the regression](https://crater-reports.s3.amazonaws.com/pr-116494-3/try%23d614ed876e31a5f3ad1d0fbf848fcdab3a29d1d8/gh/lcdr.lu_packets/log.txt) to: ```rust // Upstream crate pub trait Serialize {} impl Serialize for &() {} impl<S> Serialize for &[S] where for<'a> &'a S: Serialize {} // ----------------------------------------------------------------------- // // Downstream crate #![feature(specialization)] #![allow(incomplete_features, unused)] use upstream::Serialize; trait Replica { fn serialize(); } impl<T> Replica for T { default fn serialize() {} } impl<T> Replica for Option<T> where for<'a> &'a T: Serialize, { fn serialize() {} } ``` Specifically this fails when computing the specialization graph for the `downstream` crate. The code ends up cycling on `&[?0]: Serialize` when we equate `&?0 = &[?1]` during impl matching, which ends up needing to prove `&[?1]: Serialize`, which since cycles are treated like ambiguity, ends up in a **fatal overflow**. For some reason this requires two crates, squashing them into one crate doesn't work. Side-note: This code is subtly order dependent. When minimizing, I ended up having the code start failing on `nightly` very easily after removing and reordering impls. This seems to me all the more reason to remove this behavior altogether. ## Side-note: Item Bounds (edit: this was fixed independently in rust-lang#121123) Due to the changes in rust-lang#120584 where we now consider an alias's item bounds *and* all the item bounds of the alias's nested self type aliases, I've had to add e6b64c6 which is a hack to make sure we're not eagerly normalizing bounds that have nothing to do with the predicate we're trying to solve, and which result in. This is fixed in a more principled way in rust-lang#121123. --- r? lcnr for an initial review
Make inductive cycles always ambiguous This makes inductive cycles always result in ambiguity rather than be treated like a stack-dependent error. This has some interactions with specialization, and so breaks a few UI tests that I don't agree should've ever worked in the first place, and also breaks a handful of crates in a way that I don't believe is a problem. On the bright side, it puts us in a better spot when it comes to eventually enabling coinduction everywhere. ## Results This was cratered in rust-lang/rust#116494 (comment), which boils down to two regressions: * `lu_packets` - This code should have never compiled in the first place. More below. * **ALL** other regressions are due to `commit_verify@0.11.0-beta.1` (edit: and `commit_verify@0.10.x`) - This actually seems to be fixed in version `0.11.0-beta.5`, which is the *most* up to date version, but it's still prerelease on crates.io so I don't think cargo ends up picking `beta.5` when building dependent crates. ### `lu_packets` Firstly, this crate uses specialization, so I think it's automatically worth breaking. However, I've minimized [the regression](https://crater-reports.s3.amazonaws.com/pr-116494-3/try%23d614ed876e31a5f3ad1d0fbf848fcdab3a29d1d8/gh/lcdr.lu_packets/log.txt) to: ```rust // Upstream crate pub trait Serialize {} impl Serialize for &() {} impl<S> Serialize for &[S] where for<'a> &'a S: Serialize {} // ----------------------------------------------------------------------- // // Downstream crate #![feature(specialization)] #![allow(incomplete_features, unused)] use upstream::Serialize; trait Replica { fn serialize(); } impl<T> Replica for T { default fn serialize() {} } impl<T> Replica for Option<T> where for<'a> &'a T: Serialize, { fn serialize() {} } ``` Specifically this fails when computing the specialization graph for the `downstream` crate. The code ends up cycling on `&[?0]: Serialize` when we equate `&?0 = &[?1]` during impl matching, which ends up needing to prove `&[?1]: Serialize`, which since cycles are treated like ambiguity, ends up in a **fatal overflow**. For some reason this requires two crates, squashing them into one crate doesn't work. Side-note: This code is subtly order dependent. When minimizing, I ended up having the code start failing on `nightly` very easily after removing and reordering impls. This seems to me all the more reason to remove this behavior altogether. ## Side-note: Item Bounds (edit: this was fixed independently in #121123) Due to the changes in #120584 where we now consider an alias's item bounds *and* all the item bounds of the alias's nested self type aliases, I've had to add e6b64c61941120f734657106ae2479d05b463197 which is a hack to make sure we're not eagerly normalizing bounds that have nothing to do with the predicate we're trying to solve, and which result in. This is fixed in a more principled way in #121123. --- r? lcnr for an initial review
r? @ghost