-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Can't write non-overlapping blanket impls that involve associated type bindings #20400
Comments
I encountered this bug too, while trying to optimize pub enum Void {}
pub trait Display {
// ...
type AsStr = Void;
fn fmt_as_str(&self) -> &<Self as Display>::AsStr { unimplemented!() }
}
pub trait ToString {
fn to_string(&self) -> String;
}
impl<T: Display<AsStr=Void> + ?Sized> ToString for T {
fn to_string(&self) -> String {
format!("{}", self)
}
}
impl<T: Display<AsStr=str> + ?Sized> ToString for T {
fn to_string(&self) -> String {
String::from_str(self.fmt_as_str())
}
} This change could speed up |
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This is really causing problems for a crate I'm working on. |
Is this bug likely to get any love? I keep hitting this (see #23341) too. I had a look around by I can't really follow how overlapping impls are determined, I would have liked to have a go at it. |
Any chance of this getting triaged any time soon? Also feeling pains from this issue. |
This is part of a greater effort to clean up some of the messier bits of our type system. Unfortunately, we still can't implement something both for null and not null in a non-overlapping way most of the time, as we still will bump into rust-lang/rust#20400. Otherwise we could write something like: ```rust impl<T, ST> Expression for Something<T> where T: Expression<SqlType=ST>, ST: NotNull, { } impl<T, ST> Expression for Something<T> where T: Expression<SqlType=Nullable<ST>>, ST: NotNull, { } ``` While these clearly do not overlap, rust treats them as overlapping. Unfortunately this is just one more type that we need to implement for new SQL types added in the future. I can't just make `IntoNullable` be a requirement of `NativeSqlType`, as it would require specifying the nullable case when packing it into a trait object. `NotNull` can just be replaced with an OIBIT in the future.
I ran into this problem just now. I wanted to provide |
What's needed to advance this issue towards a fix? Are we dependent on Chalk? |
Looks like you can kind of "trick" the compiler by using type specialization. On stable, 2015 edition: |
I'd like to add myself to the list of people that this causes problems for :). |
@ZerothLaw could you explain your trick a bit? I think I'm running into this issue and am hoping for a workaround but I'm struggling to understand your example. If you could give some advice on how to apply that to the code in #58171 that would be great. |
We wanted |
Due to rust-lang#20400 the corresponding TrustedLen impls need a helper trait instead of directly adding `Item = &[T;N]` bounds. Since TrustedLen is a public trait this in turn means the helper trait needs to be public. Since it's just a workaround for a compiler deficit it's marked hidden, unstable and unsafe.
Payload Builder Refactor (Part 1) This MR refactors the way, the `BatchPayload` is being constructed. The idea is quite simple: I am introducing a new trait called `BatchPayloadSectionBuilder`. It is supposed to replace the traits `XNetPayloadBuilder`, `SelfValidatingPayloadBuilder` and `IngressSelector` (and also the not yet implemented `CanisterHttpPayloadBuilder`), as they all do the same things. Then inside consensus, there is wrapper called `BatchPayloadSectionAdapter`, that is responsible for mapping the sections into the `BatchPayload`. This boilerplate is unfortunately necessary, to eliminate the Generic in `BatchPayloadSectionBuilder`. My original design would use another trait, however, this approach was prevented because of a [long standing compiler bug](rust-lang/rust#20400). On the validation side, every validation call also returns its `byte_size`, so we can check, that the overall size is not exceeded either. This MR also contains the implementations of `BatchPayloadSectionBuilder` on top the existing traits. This way, We don't need to touch test code for now. We might want to remove the old traits at a later date. The payload builder itself just has a Vec<BatchPayloadSectionAdapter> that it calls in a rotating order on a default `BatchPayload`. **NOTE**: This MR does NOT contain the functional changes to `payload_builder.rs`. Those will be made in a separate MR, to keep the scope of this MR small. See merge request dfinity-lab/public/ic!3543
(Technically, this does let you call trait methods with nontrivial arguments, but it's not in a final/decent place.) This is an alternate version of unknown commit which tried to continue to defer materialization. However, as discussed in[] currently impossible. In particular, we cannot define a trait like this: ```rs impl<T: Ctor<Output=A>> MyTrait<T> for S {...} impl<T: Ctor<Output=B>> MyTrait<T> for S {...} ``` ... because Rust does not understand that these impls are disjoint: rust-lang/rust#20400 #### What's next? (Apologies if this is a bit unorganized, I've spent too much time in the trenches.) So this CL is just a first step: we *must* monomorphize the implementation. Rather than accepting any `T: Ctor`, accept an `RvalueReference` (a concrete type). After this CL, I think we have a slightly more open field than I thought. In particular, we should be able to regain the `Ctor` API, except using only *one* parameterized impl. So, instead of the broken impls above, we can have this impl: ```rs impl<'a> MyTrait<RvalueReference<'a, A>> for S {...} impl<'a> MyTrait<RvalueReference<'a, B>> for S {...} impl<U, CtorType> MyTrait<CtorType> for S where &C : for<'a> MyTrait<RvalueReference<'a, U>>, CtorType: Ctor<Output=U> {...} ``` Because this is only _one_ parameterized impl, there's no conflicts. It is implemented in terms of the concrete non-parameterized impls as generated by this change. However, I'm not yet 100% certain this will work, and it is actually not a small task to do, even on top of this CL. For example, there's a bunch of refactoring to let one generate a second blanket impl using knowledge about the trait function etc. from the concrete impl. ##### RvalueReference might need to get replaced. If we can use the `Ctor` approach described above... we can't use `RvalueReference`, actually, because Rust will then recurse infinitely. The `RvalueReference` type used for the `for<'a> MyTrait<RvalueReference<...>>` bound must be in the _same_ crate so that Rust knows that `RvalueReference` doesn't itself impl `Ctor`. And unfortunately, no, negative impls aren't good enough here, yet, apparently. At least, it didn't resolve it when I tried it! You can test this in a local two-crate setup. Crate 1: ```rs pub trait Ctor { type Output; } pub struct RvalueReference<'a, T>(&'a T); ``` Crate 2: ```rs use lib1::*; pub struct A1; pub struct A2; pub struct B; impl <'a> From<RvalueReference<'a, A1>> for B { fn from(_: RvalueReference<'a, A1>) -> Self { todo!(); } } impl <'a> From<RvalueReference<'a, A2>> for B { fn from(_: RvalueReference<'a, A2>) -> Self { todo!(); } } impl <T: Ctor> From<T> for B where B : for<'a> From<RvalueReference<'a, T::Output>> { fn from(_: T) -> Self { todo!(); } } ``` If you build crate 2, it will fail with the following error: ``` error[E0275]: overflow evaluating the requirement `for<'a> B: From<lib1::RvalueReference<'a, _>>` | = help: consider increasing the recursion limit by adding a `#![recursion_limit = "256"]` attribute to your crate (`lib2`) note: required because of the requirements on the impl of `for<'a> From<lib1::RvalueReference<'a, _>>` for `B` --> src/lib.rs:15:16 | 15 | impl <T: Ctor> From<T> for B | ^^^^^^^ ^ = note: 126 redundant requirements hidden = note: required because of the requirements on the impl of `for<'a> From<lib1::RvalueReference<'a, _>>` for `B` For more information about this error, try `rustc --explain E0275`. error: could not compile `lib2` due to previous error ``` But it will work fine if you move `RvalueReference` to another crate! ##### If all else fails, we'll force the caller to materialize the Ctor If even the approach outlined above doesn't work, well, we'll just have to force callers to materialize the `Ctor`: call `Trait::method(mov(emplace!(foo())))` instead of `Trait::method(foo())`. That's what I described in[] but I'm hoping we can work our way out after all! Either way, both approaches build on this change. Investigating the followups may take some time, so I'd rather not leave this change sitting around generating merge conflicts, if possible. :X PiperOrigin-RevId: 464613254
There is also an associated
shouldn't conflict. |
Interestingly, it still fails with |
This is possible on stable already, but not in a user friendly way. |
This crate enables you to write disjoint implementations in a straightforward way |
@mversic This looks great! Would it be possible for the same thing to work with functions as well? Example: pub trait RouterExt {
fn mount_route<E: ApiEndpoint<Auth = NoAuth>>(self, endpoint: E) -> Self;
fn mount_route<E: ApiEndpoint<Auth = SomeAuth>>(self, endpoint: E) -> Self;
} This would allow different functions to be called depending on the associated type of the endpoint. Is this possible? I went through the tests, but couldn't find a similar example |
no, you can't have two methods of the same name under one trait. I believe you should be able to do sth like this: use disjoint_impls::disjoint_impls;
trait ApiEndpointDispatch {
type Auth;
}
enum NoAuth {}
enum SomeAuth {}
struct Endpoint1 {}
impl ApiEndpointDispatch for Endpoint1 {
type Auth = NoAuth;
}
struct Endpoint2 {}
impl ApiEndpointDispatch for Endpoint2 {
type Auth = SomeAuth;
}
disjoint_impls! {
pub trait ApiEndpoint {}
impl<E: ApiEndpointDispatch<Auth = NoAuth>> ApiEndpoint for E {}
impl<E: ApiEndpointDispatch<Auth = SomeAuth>> ApiEndpoint for E {}
}
pub trait RouterExt {
fn mount_route<E: ApiEndpoint>(self, endpoint: E) -> Self;
} |
but due to limitation of rustc rust-lang/rust#20400 we can't have two new functions based on non-overlapping associated types
Fixes #4960 Configuring `FeeManager` enforces the boundary `Into<[u8; 32]>` for the `AccountId` type. Here is how it works currently: Configuration: ```rust type FeeManager = XcmFeeManagerFromComponents< IsChildSystemParachain<primitives::Id>, XcmFeeToAccount<Self::AssetTransactor, AccountId, TreasuryAccount>, >; ``` `XcmToFeeAccount` struct: ```rust /// A `HandleFee` implementation that simply deposits the fees into a specific on-chain /// `ReceiverAccount`. /// /// It reuses the `AssetTransactor` configured on the XCM executor to deposit fee assets. If /// the `AssetTransactor` returns an error while calling `deposit_asset`, then a warning will be /// logged and the fee burned. pub struct XcmFeeToAccount<AssetTransactor, AccountId, ReceiverAccount>( PhantomData<(AssetTransactor, AccountId, ReceiverAccount)>, ); impl< AssetTransactor: TransactAsset, AccountId: Clone + Into<[u8; 32]>, ReceiverAccount: Get<AccountId>, > HandleFee for XcmFeeToAccount<AssetTransactor, AccountId, ReceiverAccount> { fn handle_fee(fee: Assets, context: Option<&XcmContext>, _reason: FeeReason) -> Assets { deposit_or_burn_fee::<AssetTransactor, _>(fee, context, ReceiverAccount::get()); Assets::new() } } ``` `deposit_or_burn_fee()` function: ```rust /// Try to deposit the given fee in the specified account. /// Burns the fee in case of a failure. pub fn deposit_or_burn_fee<AssetTransactor: TransactAsset, AccountId: Clone + Into<[u8; 32]>>( fee: Assets, context: Option<&XcmContext>, receiver: AccountId, ) { let dest = AccountId32 { network: None, id: receiver.into() }.into(); for asset in fee.into_inner() { if let Err(e) = AssetTransactor::deposit_asset(&asset, &dest, context) { log::trace!( target: "xcm::fees", "`AssetTransactor::deposit_asset` returned error: {:?}. Burning fee: {:?}. \ They might be burned.", e, asset, ); } } } ``` --- In order to use **another** `AccountId` type (for example, 20 byte addresses for compatibility with Ethereum or Bitcoin), one has to duplicate the code as the following (roughly changing every `32` to `20`): ```rust /// A `HandleFee` implementation that simply deposits the fees into a specific on-chain /// `ReceiverAccount`. /// /// It reuses the `AssetTransactor` configured on the XCM executor to deposit fee assets. If /// the `AssetTransactor` returns an error while calling `deposit_asset`, then a warning will be /// logged and the fee burned. pub struct XcmFeeToAccount<AssetTransactor, AccountId, ReceiverAccount>( PhantomData<(AssetTransactor, AccountId, ReceiverAccount)>, ); impl< AssetTransactor: TransactAsset, AccountId: Clone + Into<[u8; 20]>, ReceiverAccount: Get<AccountId>, > HandleFee for XcmFeeToAccount<AssetTransactor, AccountId, ReceiverAccount> { fn handle_fee(fee: XcmAssets, context: Option<&XcmContext>, _reason: FeeReason) -> XcmAssets { deposit_or_burn_fee::<AssetTransactor, _>(fee, context, ReceiverAccount::get()); XcmAssets::new() } } pub fn deposit_or_burn_fee<AssetTransactor: TransactAsset, AccountId: Clone + Into<[u8; 20]>>( fee: XcmAssets, context: Option<&XcmContext>, receiver: AccountId, ) { let dest = AccountKey20 { network: None, key: receiver.into() }.into(); for asset in fee.into_inner() { if let Err(e) = AssetTransactor::deposit_asset(&asset, &dest, context) { log::trace!( target: "xcm::fees", "`AssetTransactor::deposit_asset` returned error: {:?}. Burning fee: {:?}. \ They might be burned.", e, asset, ); } } } ``` --- This results in code duplication, which can be avoided simply by relaxing the trait enforced by `XcmFeeToAccount`. In this PR, I propose to introduce a new trait called `IntoLocation` to be able to express both `Into<[u8; 32]>` and `Into<[u8; 20]>` should be accepted (and every other `AccountId` type as long as they implement this trait). Currently, `deposit_or_burn_fee()` function converts the `receiver: AccountId` to a location. I think converting an account to `Location` should not be the responsibility of `deposit_or_burn_fee()` function. This trait also decouples the conversion of `AccountId` to `Location`, from `deposit_or_burn_fee()` function. And exposes `IntoLocation` trait. Thus, allowing everyone to come up with their `AccountId` type and make it compatible for configuring `FeeManager`. --- Note 1: if there is a better file/location to put `IntoLocation`, I'm all ears Note 2: making `deposit_or_burn_fee` or `XcmToFeeAccount` generic was not possible from what I understood, due to Rust currently do not support a way to express the generic should implement either `trait A` or `trait B` (since the compiler cannot guarantee they won't overlap). In this case, they are `Into<[u8; 32]>` and `Into<[u8; 20]>`. See [this](rust-lang/rust#20400) and [this](rust-lang/rfcs#1672 (comment)). Note 3: I should also submit a PR to `frontier` that implements `IntoLocation` for `AccountId20` if this PR gets accepted. ### Summary this new trait: - decouples the conversion of `AccountId` to `Location`, from `deposit_or_burn_fee()` function - makes `XcmFeeToAccount` accept every possible `AccountId` type as long as they they implement `IntoLocation` - backwards compatible - keeps the API simple and clean while making it less restrictive @franciscoaguirre and @gupnik are already aware of the issue, so tagging them here for visibility. --------- Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com> Co-authored-by: Branislav Kontur <bkontur@gmail.com> Co-authored-by: Adrian Catangiu <adrian@parity.io> Co-authored-by: command-bot <>
Fixes paritytech#4960 Configuring `FeeManager` enforces the boundary `Into<[u8; 32]>` for the `AccountId` type. Here is how it works currently: Configuration: ```rust type FeeManager = XcmFeeManagerFromComponents< IsChildSystemParachain<primitives::Id>, XcmFeeToAccount<Self::AssetTransactor, AccountId, TreasuryAccount>, >; ``` `XcmToFeeAccount` struct: ```rust /// A `HandleFee` implementation that simply deposits the fees into a specific on-chain /// `ReceiverAccount`. /// /// It reuses the `AssetTransactor` configured on the XCM executor to deposit fee assets. If /// the `AssetTransactor` returns an error while calling `deposit_asset`, then a warning will be /// logged and the fee burned. pub struct XcmFeeToAccount<AssetTransactor, AccountId, ReceiverAccount>( PhantomData<(AssetTransactor, AccountId, ReceiverAccount)>, ); impl< AssetTransactor: TransactAsset, AccountId: Clone + Into<[u8; 32]>, ReceiverAccount: Get<AccountId>, > HandleFee for XcmFeeToAccount<AssetTransactor, AccountId, ReceiverAccount> { fn handle_fee(fee: Assets, context: Option<&XcmContext>, _reason: FeeReason) -> Assets { deposit_or_burn_fee::<AssetTransactor, _>(fee, context, ReceiverAccount::get()); Assets::new() } } ``` `deposit_or_burn_fee()` function: ```rust /// Try to deposit the given fee in the specified account. /// Burns the fee in case of a failure. pub fn deposit_or_burn_fee<AssetTransactor: TransactAsset, AccountId: Clone + Into<[u8; 32]>>( fee: Assets, context: Option<&XcmContext>, receiver: AccountId, ) { let dest = AccountId32 { network: None, id: receiver.into() }.into(); for asset in fee.into_inner() { if let Err(e) = AssetTransactor::deposit_asset(&asset, &dest, context) { log::trace!( target: "xcm::fees", "`AssetTransactor::deposit_asset` returned error: {:?}. Burning fee: {:?}. \ They might be burned.", e, asset, ); } } } ``` --- In order to use **another** `AccountId` type (for example, 20 byte addresses for compatibility with Ethereum or Bitcoin), one has to duplicate the code as the following (roughly changing every `32` to `20`): ```rust /// A `HandleFee` implementation that simply deposits the fees into a specific on-chain /// `ReceiverAccount`. /// /// It reuses the `AssetTransactor` configured on the XCM executor to deposit fee assets. If /// the `AssetTransactor` returns an error while calling `deposit_asset`, then a warning will be /// logged and the fee burned. pub struct XcmFeeToAccount<AssetTransactor, AccountId, ReceiverAccount>( PhantomData<(AssetTransactor, AccountId, ReceiverAccount)>, ); impl< AssetTransactor: TransactAsset, AccountId: Clone + Into<[u8; 20]>, ReceiverAccount: Get<AccountId>, > HandleFee for XcmFeeToAccount<AssetTransactor, AccountId, ReceiverAccount> { fn handle_fee(fee: XcmAssets, context: Option<&XcmContext>, _reason: FeeReason) -> XcmAssets { deposit_or_burn_fee::<AssetTransactor, _>(fee, context, ReceiverAccount::get()); XcmAssets::new() } } pub fn deposit_or_burn_fee<AssetTransactor: TransactAsset, AccountId: Clone + Into<[u8; 20]>>( fee: XcmAssets, context: Option<&XcmContext>, receiver: AccountId, ) { let dest = AccountKey20 { network: None, key: receiver.into() }.into(); for asset in fee.into_inner() { if let Err(e) = AssetTransactor::deposit_asset(&asset, &dest, context) { log::trace!( target: "xcm::fees", "`AssetTransactor::deposit_asset` returned error: {:?}. Burning fee: {:?}. \ They might be burned.", e, asset, ); } } } ``` --- This results in code duplication, which can be avoided simply by relaxing the trait enforced by `XcmFeeToAccount`. In this PR, I propose to introduce a new trait called `IntoLocation` to be able to express both `Into<[u8; 32]>` and `Into<[u8; 20]>` should be accepted (and every other `AccountId` type as long as they implement this trait). Currently, `deposit_or_burn_fee()` function converts the `receiver: AccountId` to a location. I think converting an account to `Location` should not be the responsibility of `deposit_or_burn_fee()` function. This trait also decouples the conversion of `AccountId` to `Location`, from `deposit_or_burn_fee()` function. And exposes `IntoLocation` trait. Thus, allowing everyone to come up with their `AccountId` type and make it compatible for configuring `FeeManager`. --- Note 1: if there is a better file/location to put `IntoLocation`, I'm all ears Note 2: making `deposit_or_burn_fee` or `XcmToFeeAccount` generic was not possible from what I understood, due to Rust currently do not support a way to express the generic should implement either `trait A` or `trait B` (since the compiler cannot guarantee they won't overlap). In this case, they are `Into<[u8; 32]>` and `Into<[u8; 20]>`. See [this](rust-lang/rust#20400) and [this](rust-lang/rfcs#1672 (comment)). Note 3: I should also submit a PR to `frontier` that implements `IntoLocation` for `AccountId20` if this PR gets accepted. ### Summary this new trait: - decouples the conversion of `AccountId` to `Location`, from `deposit_or_burn_fee()` function - makes `XcmFeeToAccount` accept every possible `AccountId` type as long as they they implement `IntoLocation` - backwards compatible - keeps the API simple and clean while making it less restrictive @franciscoaguirre and @gupnik are already aware of the issue, so tagging them here for visibility. --------- Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com> Co-authored-by: Branislav Kontur <bkontur@gmail.com> Co-authored-by: Adrian Catangiu <adrian@parity.io> Co-authored-by: command-bot <>
This seems to have been raised at the start of 2015 and here in October 2024 I get:
So the "bug" or "lack of feature" remains after nearly 10 years. Is it time to decide to "fix" or say "not part of the language"? |
For those expecting it to work with the new trait solver automatically, here's a reminder that it's blocked on an RFC as it unlocks mutually exclusive traits. |
STR
Example from libcore:
Version
7d4f487
No type can implement both
Iterator<Item=u8>
andIterator<Item=u16>
, therefore these blanket impls are non overlapping and should be accepted.cc @nikomatsakis
The text was updated successfully, but these errors were encountered: