-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Make inductive cycles always ambiguous #122791
Make inductive cycles always ambiguous #122791
Conversation
|
Apologies. I misread the crater report. The regressions are a combination of For the record, the regression was removed when a bunch of impls were reworked in: LNP-BP/client_side_validation#152. |
Regarding /// Used only internally for blank implementation on reference types.
#[doc(hidden)]
pub enum AsRef {}
impl<'a, T> CommitEncode for Holder<&'a T, AsRef>
where
T: CommitEncode,
{
fn commit_encode(&self, e: &mut impl io::Write) {
self.as_type().commit_encode(e);
}
}
impl<T> CommitStrategy for &T
where
T: CommitEncode,
{
type Strategy = AsRef;
}
impl<T> CommitEncode for T
where
T: CommitStrategy,
for<'a> Holder<&'a T, T::Strategy>: CommitEncode,
{
fn commit_encode(&self, e: &mut impl io::Write)
Holder::new(self).commit_encode(e)
}
} ... which I guess because we don't treat ambiguity as error, we end up actually evaluating to overflow. Removing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
"fatal overflow" i guess?
What is a test which is affected here. How does this handle <AssocTy as Id>::Assoc: Bound
? writing a test rn
@@ -219,52 +218,9 @@ pub enum EvaluationResult { | |||
/// variables. We are somewhat imprecise there, so we don't actually | |||
/// know the real result. | |||
/// | |||
/// This can't be trivially cached for the same reason as `EvaluatedToErrStackDependent`. | |||
/// This can't be trivially cached because the result depends on the | |||
/// stack results. | |||
EvaluatedToAmbigStackDependent, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated: isn't this just utterly broken if you have stack dependent ambiguity resulting in difference inference constraints, causing a later goal to error without stack dependence?
anyways, the old solver cannot be salvaged. This is an existing issue 😁
This already fails on nightly, so this PR seems good to me. I am a bit confused why though, we somehow keep the trait Id {
type This: ?Sized;
}
impl<T: ?Sized> Id for T {
type This = T;
}
trait Trait
where
<Self::Assoc as Id>::This: Copy,
{
type Assoc;
}
fn foo<T: Trait>(x: T::Assoc) -> (T::Assoc, T::Assoc) {
(x, x)
} edit: it's because that hasn't landed yet '^^ that is from a separate PR #120752 The following also already fails, which makes sense, but seems like a shortcoming of our approach to normalization #![feature(associated_type_bounds)]
trait Id {
type This: ?Sized;
}
impl<T: ?Sized> Id for T {
type This = T;
}
trait Trait {
type Assoc: Id<This: Copy>;
}
fn foo<T: Trait>(x: T::Assoc) -> (T::Assoc, T::Assoc) {
(x, x)
} however, why does the following compile? #![feature(associated_type_bounds)]
trait Id {
type This: ?Sized;
}
trait Trait {
type Assoc: Id<This: Copy>;
}
fn foo<T: Trait>(x: T::Assoc) -> (T::Assoc, T::Assoc)
where
T::Assoc: Id<This = T::Assoc>,
{
(x, x)
} please add these all as tests 😁 |
Ignoring the question about nested For more details, see the PR description by @compiler-errors. @rfcbot fcp merge |
Team member @lcnr has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
☔ The latest upstream changes (presumably #121123) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment was marked as outdated.
This comment was marked as outdated.
Actually, yeah, it's due to candidate shadowing, just for associated item bounds shadowing the global impl. |
7e0b48c
to
c594682
Compare
…und-tests, r=lcnr Add tests for shortcomings of associated type bounds Adds the test in rust-lang#122791 (comment) Turns out that rust-lang#121123 is what breaks `tests/ui/associated-type-bounds/cant-see-copy-bound-from-child-rigid.rs` (passes on nightly), but given that associated type bounds haven't landed anywhere yet, I'm happy with breaking it. This is unrelated to rust-lang#122791, which just needed that original commit e6b64c6 stacked on top of it so that it wouldn't have tests failing. r? lcnr
…und-tests, r=lcnr Add tests for shortcomings of associated type bounds Adds the test in rust-lang#122791 (comment) Turns out that rust-lang#121123 is what breaks `tests/ui/associated-type-bounds/cant-see-copy-bound-from-child-rigid.rs` (passes on nightly), but given that associated type bounds haven't landed anywhere yet, I'm happy with breaking it. This is unrelated to rust-lang#122791, which just needed that original commit e6b64c6 stacked on top of it so that it wouldn't have tests failing. r? lcnr
Rollup merge of rust-lang#122826 - compiler-errors:associated-type-bound-tests, r=lcnr Add tests for shortcomings of associated type bounds Adds the test in rust-lang#122791 (comment) Turns out that rust-lang#121123 is what breaks `tests/ui/associated-type-bounds/cant-see-copy-bound-from-child-rigid.rs` (passes on nightly), but given that associated type bounds haven't landed anywhere yet, I'm happy with breaking it. This is unrelated to rust-lang#122791, which just needed that original commit e6b64c6 stacked on top of it so that it wouldn't have tests failing. r? lcnr
🔔 This is now entering its final comment period, as per the review above. 🔔 |
@bors r+ rollup=never |
…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 `[email protected]` (edit: and `[email protected]`) - 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
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
@bors retry |
☀️ Test successful - checks-actions |
Finished benchmarking commit (40f743d): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression 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.
CyclesResultsThis 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.
Binary sizeResultsThis 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.
Bootstrap: 668.585s -> 668.957s (0.06%) |
Only compute `specializes` query if (min)specialization is enabled in the crate of the specializing impl Fixes (after backport) rust-lang#125197 ### What rust-lang#122791 makes it so that inductive cycles are no longer hard errors. That means that when we are testing, for example, whether these impls overlap: ```rust impl PartialEq<Self> for AnyId { fn eq(&self, _: &Self) -> bool { todo!() } } impl<T: Identifier> PartialEq<T> for AnyId { fn eq(&self, _: &T) -> bool { todo!() } } ``` ...given... ```rust pub trait Identifier: Display + 'static {} impl<T> Identifier for T where T: PartialEq + Display + 'static {} ``` Then we try to see if the second impl holds given `T = AnyId`. That requires `AnyId: Identifier`, which requires that `AnyId: PartialEq`, which is satisfied by these two impl candidates... The `PartialEq<T>` impl is a cycle, and we used to winnow it when we used to treat inductive cycles as errors. However, now that we don't winnow it, this means that we *now* try calling `candidate_should_be_dropped_in_favor_of`, which tries to check whether one of the impls specializes the other: the `specializes` query. In that query, we currently bail early if the impl is local: However, in a foreign crate, we try to compute if the two impls specialize each other by doing trait solving. This may itself lead to the same situation where we call `specializes`, which will lead to a query cycle. ### How does this fix the problem We now record whether specialization is enabled in foreign crates, and extend this early-return behavior to foreign impls too. This means that we can only encounter these cycles if we truly have a specializing impl from a crate with specialization enabled. ----- r? `@oli-obk` or `@lcnr`
Only compute `specializes` query if (min)specialization is enabled in the crate of the specializing impl Fixes (after backport) rust-lang#125197 ### What rust-lang#122791 makes it so that inductive cycles are no longer hard errors. That means that when we are testing, for example, whether these impls overlap: ```rust impl PartialEq<Self> for AnyId { fn eq(&self, _: &Self) -> bool { todo!() } } impl<T: Identifier> PartialEq<T> for AnyId { fn eq(&self, _: &T) -> bool { todo!() } } ``` ...given... ```rust pub trait Identifier: Display + 'static {} impl<T> Identifier for T where T: PartialEq + Display + 'static {} ``` Then we try to see if the second impl holds given `T = AnyId`. That requires `AnyId: Identifier`, which requires that `AnyId: PartialEq`, which is satisfied by these two impl candidates... The `PartialEq<T>` impl is a cycle, and we used to winnow it when we used to treat inductive cycles as errors. However, now that we don't winnow it, this means that we *now* try calling `candidate_should_be_dropped_in_favor_of`, which tries to check whether one of the impls specializes the other: the `specializes` query. In that query, we currently bail early if the impl is local. However, in a foreign crate, we try to compute if the two impls specialize each other by doing trait solving. This may itself lead to the same situation where we call `specializes`, which will lead to a query cycle. ### How does this fix the problem We now record whether specialization is enabled in foreign crates, and extend this early-return behavior to foreign impls too. This means that we can only encounter these cycles if we truly have a specializing impl from a crate with specialization enabled. ----- r? `@oli-obk` or `@lcnr`
Only compute `specializes` query if (min)specialization is enabled in the crate of the specializing impl Fixes (after backport) rust-lang/rust#125197 ### What rust-lang/rust#122791 makes it so that inductive cycles are no longer hard errors. That means that when we are testing, for example, whether these impls overlap: ```rust impl PartialEq<Self> for AnyId { fn eq(&self, _: &Self) -> bool { todo!() } } impl<T: Identifier> PartialEq<T> for AnyId { fn eq(&self, _: &T) -> bool { todo!() } } ``` ...given... ```rust pub trait Identifier: Display + 'static {} impl<T> Identifier for T where T: PartialEq + Display + 'static {} ``` Then we try to see if the second impl holds given `T = AnyId`. That requires `AnyId: Identifier`, which requires that `AnyId: PartialEq`, which is satisfied by these two impl candidates... The `PartialEq<T>` impl is a cycle, and we used to winnow it when we used to treat inductive cycles as errors. However, now that we don't winnow it, this means that we *now* try calling `candidate_should_be_dropped_in_favor_of`, which tries to check whether one of the impls specializes the other: the `specializes` query. In that query, we currently bail early if the impl is local. However, in a foreign crate, we try to compute if the two impls specialize each other by doing trait solving. This may itself lead to the same situation where we call `specializes`, which will lead to a query cycle. ### How does this fix the problem We now record whether specialization is enabled in foreign crates, and extend this early-return behavior to foreign impls too. This means that we can only encounter these cycles if we truly have a specializing impl from a crate with specialization enabled. ----- r? `@oli-obk` or `@lcnr`
Only compute `specializes` query if (min)specialization is enabled in the crate of the specializing impl Fixes (after backport) rust-lang/rust#125197 ### What rust-lang/rust#122791 makes it so that inductive cycles are no longer hard errors. That means that when we are testing, for example, whether these impls overlap: ```rust impl PartialEq<Self> for AnyId { fn eq(&self, _: &Self) -> bool { todo!() } } impl<T: Identifier> PartialEq<T> for AnyId { fn eq(&self, _: &T) -> bool { todo!() } } ``` ...given... ```rust pub trait Identifier: Display + 'static {} impl<T> Identifier for T where T: PartialEq + Display + 'static {} ``` Then we try to see if the second impl holds given `T = AnyId`. That requires `AnyId: Identifier`, which requires that `AnyId: PartialEq`, which is satisfied by these two impl candidates... The `PartialEq<T>` impl is a cycle, and we used to winnow it when we used to treat inductive cycles as errors. However, now that we don't winnow it, this means that we *now* try calling `candidate_should_be_dropped_in_favor_of`, which tries to check whether one of the impls specializes the other: the `specializes` query. In that query, we currently bail early if the impl is local. However, in a foreign crate, we try to compute if the two impls specialize each other by doing trait solving. This may itself lead to the same situation where we call `specializes`, which will lead to a query cycle. ### How does this fix the problem We now record whether specialization is enabled in foreign crates, and extend this early-return behavior to foreign impls too. This means that we can only encounter these cycles if we truly have a specializing impl from a crate with specialization enabled. ----- r? `@oli-obk` or `@lcnr`
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 #116494 (comment), which boils down to two regressions:
lu_packets
- This code should have never compiled in the first place. More below.[email protected]
(edit: and[email protected]
) - This actually seems to be fixed in version0.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 pickingbeta.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 to:
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 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 #121123.
r? lcnr for an initial review