Skip to content
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

Disjointness based on associated types. #1672

Closed

Conversation

withoutboats
Copy link
Contributor

@withoutboats withoutboats commented Jul 11, 2016

@withoutboats
Copy link
Contributor Author

cc @sgrif you may have had use cases for this in Deisel?

cc @ticki if you want, "proofs that two type variables are disjoint" seems like a good opportunity for writing up some formal judgments

@sgrif
Copy link
Contributor

sgrif commented Jul 11, 2016

Yeah, I've definitely needed this in Diesel. I don't recall the specific place that I'm working around it, but it was likely of the form

impl<T> Something for T where T: Expression<SqlType=VarChar> {
    // ...
}

impl<T> Something for T where T: Expression<SqlType=Text> {
    // ...
}

Honestly, I had figured that this was a bug and not an explicit decision.

1. If they are both concrete types, and they are not the same type (this rule
already exists).
2. If they are both bound by the same trait, and both specify the same
associated type for that trait, and the types they specify are disjoint.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps add a rule that instantiations of generic types with disjoint parameters are disjoint. i.e.

impl<T: Trait1<Item=X>> Trait2 for Foo<T> { ... }
impl<T: Trait1<Item=Y>> Trait2 for Foo<T> { ... }

or even

impl<T: Trait1<Item=X>, U: Trait2<Item=Foo<T>>> Trait3 for U { ... }
impl<T: Trait1<Item=Y>, U: Trait2<Item=Foo<T>>> Trait3 for U { ... }

Copy link
Contributor Author

@withoutboats withoutboats Jul 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. We don't have good, consistent language about this, but when I wrote "two type variables" I meant any type variables (really I meant any type, whether an abstract variable or a concrete type, since the first rule only applies to concrete types), whether they be the receiver of the trait or not (I realize the summary is less broad than this, oops).

This is exactly what I had in mind when I said this rule was recursive.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any case where T is disjoint but Foo<T> is not disjoint today? If not it seems redundant to re-specify it here.

Copy link
Contributor Author

@withoutboats withoutboats Jul 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@comex was talking about the transitivity of the disjunction rule, but perhaps we should note that if T is disjoint with U and X is a type constructor of the kind type -> type, X<T> is disjoint with X<U>. No reason this couldn't be a roundup to establish and document some of the basic disjunction rules.

EDIT: Actually, this may impact how that rule is implemented. Currently I think all disjunction is based on inequality of concrete types (e.g. we can just see that Foo<Bar<Baz<i32>> is not Foo<Bar<Baz<bool>>), but now that rule also needs to take into account type variables that are bound exclusively.

@withoutboats
Copy link
Contributor Author

Honestly, I had figured that this was a bug and not an explicit decision.

I think the best word for this is 'omission' - the current rules are correct so its not really a bug, but no one ever decided this rule shouldn't be added as well. This kind of disjointness just hasn't been evaluated for inclusion yet.

@seanmonstar
Copy link
Contributor

Perhaps the motivation section should be updated to not be in first person, per #61?

@durka
Copy link
Contributor

durka commented Jul 12, 2016

What about an impl where you don't specify the associated type at all, but use it in the implementation?

trait Tr { type A; }

impl<T: Iterator> Tr for T {
    type A = T::Item;
}

@withoutboats
Copy link
Contributor Author

@durka What type do you think T might be disjoint with?

@durka
Copy link
Contributor

durka commented Jul 12, 2016

None, I guess.

@withoutboats
Copy link
Contributor Author

I agree, none of the disjointness rules seem to make T there disjoint with any other types (except for local reasoning shenanigans)

@nrc nrc added the T-lang Relevant to the language team, which will review and decide on the RFC. label Jul 12, 2016
@cristicbz
Copy link

cristicbz commented Jul 12, 2016

There are a bunch of issues about this, including one I filed early last year: rust-lang/rust#23341, rust-lang/rust#20400, rust-lang/rust#30191

(thought it would be useful to link to them)

@withoutboats
Copy link
Contributor Author

withoutboats commented Jul 12, 2016

Looking at each of those:

See also #1148 & #1658

@WildCryptoFox
Copy link

Any progress on this issue?

@burdges
Copy link

burdges commented Oct 25, 2016

I've run into this with a toy branch where I wanted to replace some macros that ran over numeric types with type constraints. I believe the type constraint based version made more sense, but it failed and I never thought deeply enough to say anything. If this happens, then great. If this is not a good idea, then maybe comment could be added to the documentation?

@eddyb
Copy link
Member

eddyb commented Oct 25, 2016

@Mark-Simulacrum hit this in rust-lang/rust#37270 where they needed two impls for different iterator element types, but had to proxy to a trait dispatched on <Self as Iterator>::Item instead.

@withoutboats
Copy link
Contributor Author

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Nov 3, 2016

Team member @withoutboats has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once these reviewers reach consensus, 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.

@nikomatsakis
Copy link
Contributor

Hmm. I would like to see something like this proposal go forward, but I'm a bit wary here.

@rfcbot concern negative reasoning

This seems to be strongly connected to negative reasoning. Basically the argument here is that we can prove that two where clauses are mutually incompatible, which isn't how our coherence check works now. I do think it makes sense to go in this direction, but I want to approach it with a somewhat more formal approach.

Along those lines, I've been working on converting our trait system into a lambda-prolog-based logic in this repository. I'm not all that far along yet, but I hope that it can give us a basis for reasoning better about negative reasoning and its impact.

That said, I think that these changes likely fit fairly well with our trait system logic (modulo my "underspecified" concern below). Basically I think what we should be shooting for is something that relies only on minimal logic. Essentially, in prolog terms, somewhere we can define an "inconsistent" predicate. I think we would translate this RFC as follows:

inconsistent :- 
    <P[0] as Trait<P[1]...P[n]>>::Item = X,
    <P[0] as Trait<P[1]...P[n]>>::Item = Y,
    not_equal X Y. // note that this is a predicate we have to define =)

@rfcbot concern underspecified

Today we only have X = Y constraints in our system. This would introduce X != Y -- even if only limited to cherence -- and the RFC doesn't specify very clearly what this means. It gives some simple examples (u32 and i32 are unequal) but (for example) what about &'a u32 and &'b u32, or T and U?

Presumably the answer is that all variables would be considered potentially equal, and the only way to prove two types are disjoint is if they are unified with distinct nominal types (i.e., u32 vs i32, or Rc vs Arc), right?

In any case, more examples are certainly needed. But I do think we'll be able to specify this nicely.

@withoutboats
Copy link
Contributor Author

withoutboats commented Nov 11, 2016

About under-specification:

The reference case is interesting because it introduces subtyping, presumable &'a u32 != &'b u32 iff 'a has no overlap with 'b? I don't think there's a time when coherence has enough info to prove that and we can leave these as considered overlapping at least for now.

T != U not only if they are unified with distinct types but also if they are bound by disjoint traits (for example because those traits have incompatible associated types); that is, the rule this is aimed at introducing should apply recursively.

I wrote this RFC very quickly (as I write every RFC I actually publish 😉) and it could definitely use revision to be more formal.

@nikomatsakis
Copy link
Contributor

@rfcbot concern underspecified

Today we only have X = Y constraints in our system. This would introduce X != Y -- even if only limited to cherence -- and the RFC doesn't specify very clearly what this means. It gives some simple examples (u32 and i32 are unequal) but (for example) what about &'a u32 and &'b u32, or T and U?

Presumably the answer is that all variables would be considered potentially equal, and the only way to prove two types are disjoint is if they are unified with distinct nominal types (i.e., u32 vs i32, or Rc vs Arc), right?

In any case, more examples are certainly needed. But I do think we'll be able to specify this nicely.

@nikomatsakis
Copy link
Contributor

@withoutboats I'd be happy to work with you on it

@withoutboats
Copy link
Contributor Author

withoutboats commented Nov 22, 2016

I've realized this can express mutually exclusive traits and that it therefore carries all of the baggage associated with that feature.

// Two disjoint types
struct Left;
struct Right;

trait Exclusive {
    type Distinguisher;
}

// Foo and Bar are mutually exclusive because their associated types are
// disjoint.
trait Foo: Exclusive<Distinguisher = Left> { }
trait Bar: Exclusive<Distinguisher = Right> { }

trait Baz { }

// These impls are recognized as non-overlapping because the type parameters
// implement two exclusive forms of the same trait
impl<T: Foo> Baz for T { }
impl<T: Bar> Baz for T { }

If this RFC were accepted, it would resolve the most pressing use for general explicit mutual exclusion in my code, because the traits I want to make exclusive already have a supertrait parameterized by a dummy type that ought to be an associated type, but for lack of this feature.

@burdges
Copy link

burdges commented Nov 22, 2016

Just ran into this again when trying to be generic over built in numeric types. I could work around the first times I ran into this if all the core numeric types functionality, like leading_zeros(), MAX, etc. lived in traits, but this time I was trying to do something more akin to specialization, so even that won't help.

@burdges
Copy link

burdges commented Nov 22, 2016

Appears there are some situations where you can circumvent needing this using a parameterized helper trait to make the associated type behave exactly like a type parameter.

trait Helper<N> { ... }
impl<F> Helper<u16> for F where ... { ... }
impl<F> Helper<u32> for F where ... { ... }
impl<F> Helper<u64> for F where ... { ... }

trait Stuff {
    type N;
}

impl<T,NN> Stuff for T where T: Helper<NN> {
    type N = NN;
}

I'd expect logic to tends towards landing in Helper though, which gets ugly.

@rfcbot
Copy link
Collaborator

rfcbot commented Mar 15, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Mar 15, 2017
@rfcbot
Copy link
Collaborator

rfcbot commented Mar 25, 2017

The final comment period is now complete.

@withoutboats
Copy link
Contributor Author

Closing as postponed.

@RustyYato
Copy link

Given that this feature can be emulated on stable Rust, I think that this is not a large change to the language. So maybe the postponement can be reconsidered. This feature would be a really nice improvement over the current state, where a helper trait is needed to emulate this behavior.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=cdb3434f8b8f7cbe64feaa5d09da50af

pub trait Assoc {
    type Type;
}

// Foo and Bar are mutually exclusive, but can we use that
// to provide two blanket impls for a different trait?
// one for Foo and one for Bar?
pub trait Foo: Assoc<Type = i32> {}
pub trait Bar: Assoc<Type = String> {}

// some basic impls for types to test this out
impl Foo for i32 {}
impl Assoc for i32 {
    type Type = i32;
}

impl Bar for String {}
impl Assoc for String {
    type Type = String;
}

impl Assoc for f32 {
    type Type = i32;
}

// the other trait that we'll try to implement a blanket impl for
pub trait Yak {
    fn run(&self);
}

mod private {
    use super::*;
    
    // This delegates to a private helper trait which we can specialize on in stable rust
    impl<T: Assoc + YakHelper<T::Type>> Yak for T {
        fn run(&self) {
            YakHelper::run_imp(self)
        }
    }
    
    // Since impls with distinct parameters are considered disjoint
    // we can write multiple blanket impls for YakHelper given different paremeters
    trait YakHelper<Type> {
        fn run_imp(&self);
    }
    
    // blanket impl 1
    impl<T: Foo> YakHelper<i32> for T {
        fn run_imp(&self) {
            println!("impl Foo")
        }
    }
    
    // blanket impl 2
    impl<T: Bar> YakHelper<String> for T {
        fn run_imp(&self) {
            println!("impl Bar")
        }
    }
    
    // we can add as many blanket impls as we want. 
}

// test out the blanket impls
// we expect to see the output
// ```
// impl Foo
// impl Bar
// ```
fn main() {
    0i32.run();
    "0i32".to_string().run();
    // 0f32.run();
    
    // ^^ comment this line to see the following error message, which is pretty unhelpful.
    // it would be nice if it saw `YakHelper<i32>` requires `T: Foo`, but it doesn't
    
    //     error[E0599]: the method `run` exists for type `f32`, but its trait bounds were not satisfied
    //   --> src/main.rs:73:10
    //   |
    // 73 |     0f32.run();
    //   |          ^^^
    //   |
    // note: trait bound `f32: YakHelper<i32>` was not satisfied
    //   --> src/main.rs:35:21
    //   |
    // 35 |     impl<T: Assoc + YakHelper<T::Type>> Yak for T {
    //   |                     ^^^^^^^^^^^^^^^^^^  ---     -
    //   |                     |
    //   |                     unsatisfied trait bound introduced here

}

@Perksey
Copy link

Perksey commented Sep 4, 2023

I'm not sure I agree with the ability to emulate a feature is a good substitute for the feature itself. As someone who's had to do this for a crate I'm working on, the user experience really isn't great for the workaround.

The fact that there is a way to express this already should be more reason to implement the user-friendly path (as it is proven as possible), not postpone it and defer everyone to the non-user-friendly path because it exists.

@burdges

This comment was marked as resolved.

@Perksey
Copy link

Perksey commented Sep 5, 2023

Oh yeah I know, I was just reaffirming that :)

@RustyYato
Copy link

I'm proposing we postpone this and #1658 for a later date, when chalk is up and running, and we can put forward a unified proposal for how to expand the negative reasoning performed by our coherence system.

Given that chalk is not going to be merged used as the next trait solver, maybe this RFC can be revived? This feels like a missing feature of Rust, not a new feature.
I recently found someone else who could have benefited from this missing feature: https://users.rust-lang.org/t/two-blanket-implementations-for-different-classes-of-objects/100173/3


I also would like to address the two concerns brought up during the FCP

I think the concerns around negative reasoning is unfounded because it's literally the same reasoning used to determine if two impls are disjoint today, for example the following is allowed

  • we can have both impl<T> Foo<i32> for T and impl<T> Foo<bool> for T at the same time
  • we can have both impl<T: Bar> Foo for T and impl<T> Foo for Option<T> at the same time if Option<T>: !Bar

For the concern around underspecified, I think we could say that associated types should in impls should behave as if there were extra generic parameters on the trait, and each is filled by an associated type

for example,

trait MyTrait {
    type MyAssoc;
}

trait OtherTrait {
    fn get(&self) -> &str;
}

impl<T: MyTrait<MyAssoc = i32>> OtherTrait for T {}
impl<T: MyTrait<MyAssoc = bool>> OtherTrait for T {}

will lower to something like

impl<T: MyTrait<MyAssoc = i32>> OtherTrait</*MyTrait::MyAssoc = */ i32> for T {}
impl<T: MyTrait<MyAssoc = bool>> OtherTrait</*MyTrait::MyAssoc = */ bool> for T {}

Then these impls are disjoint because i32 != bool.

However these impls are not disjoint because it's possible that 'a = 'static

impl<T: MyTrait<MyAssoc = &'a i32>> OtherTrait for T {}
impl<T: MyTrait<MyAssoc = &'static i32>> OtherTrait for T {}

And so on, the rules for disjointness are already in place for generic parameters, so it's not underspecified.

@vigna
Copy link

vigna commented Oct 5, 2023

We are rewriting in Rust part of the framework developed at the LAW for large-scale data and graph storage and we're hitting constantly this problem. The workaround is great but it makes the code, which is already rather complicated, even more complicated (see, e.g., handling zero-copy vs. non-zero-copy types in ε-serde; or, as of now, having different implementations for atomic types vs. non-atomic types in sux-rs).

One vote for having this in stable.

@nikomatsakis
Copy link
Contributor

Chalk is really a shorthand here for "next gen trait solver", which is being actively developed, with coherence as a first milestone. I agree we could re-open the RFC and investigate how we would model / implement this there, I'd probably want to start by looking into it in the context of a-mir-formality.

@vigna
Copy link

vigna commented Mar 21, 2024

Are there by any chance any news on this issue? :)

@mversic
Copy link

mversic commented Jun 17, 2024

Are there by any chance any news on this issue? :)

@vigna check out this crate. It might just do what you need

@vigna
Copy link

vigna commented Jun 18, 2024

We are already using @RustyYato solution (which I guess your crate automates). It would be really nice to have it working directly in the compiler, because it's really natural.

github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this pull request Jul 19, 2024
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 <>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet