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

feat: signal traits should take associated types instead of generics #1578

Merged
merged 1 commit into from
Aug 25, 2023

Conversation

gbj
Copy link
Collaborator

@gbj gbj commented Aug 24, 2023

I initially began this as part of #1577, before realizing that that had a trait conflict with Fn() in any case and needed to use the concrete types instead. It was not possible to do something like

impl<T, U> IntoAttribute for T where
  T: SignalGet<U>,
  U: IntoAttribute { /* */ }

because the compiler says U is unused. I guess because it's only used as the generic of the constraint on T.

This led me down a (short) path re: the difference between generics on traits and associated types. I think it's probably correct for these to be associated types rather than generics, because there truly only is one value, and it's provided by the implementor of the trait, not the caller: i.e., any T: SignalGet will always have some T::Value which is the value it returns. It doesn't make sense to have SignalGet<U>: the SignalGet trait itself isn't generic, i.e., we can't specify different T here and get anything else out of our .get() call.

@jquesada2016 this was your proposal originally of course. Does making this change make sense to you? I think it's probably more correct. No longer necessary for #1577 but if we're going to do it, we should do it now before 0.5 as it's breaking.

@jquesada2016
Copy link
Contributor

@gbj I didn't completely follow the intention here. Could you please ellaborate a bit?

I understood that you wanted to add an associated type to the IntoAttribute trait? Only this trait, or a few others as well?

Anyways, yeah, your example above wouldn't work because U is never used in the trait or the receiver.

@gbj
Copy link
Collaborator Author

gbj commented Aug 24, 2023

@gbj I didn't completely follow the intention here. Could you please ellaborate a bit?

I understood that you wanted to add an associated type to the IntoAttribute trait? Only this trait, or a few others as well?

Oh, sorry. No, I'm talking about removing the generic on the SignalGet, SignalWith, etc. traits, and replacing them with an associated type.

Before:

trait SignalGet<T> {
  fn get(&self) -> T /* etc. */

After:

trait SignalGet {
  type Value;
  
  fn get(&self) -> Self::Value /* etc. */

Anyways, yeah, your example above wouldn't work because U is never used in the trait or the receiver.

Right, whereas changing to associated types would allow

impl<T, U> IntoAttribute for T where
  T: SignalGet<Value = U>,
  U: IntoAttribute { /* */ }

which is what started me thinking about this.

Note that this actual example doesn't work, because it conflicts with the implementation for Fn() -> T anyway, even on stable.

Basically: traits that allow multiple generic types (like AsRef<T>) should take a generic. Traits that only really allow one type (like Iterator) should use an associated type.

@jquesada2016
Copy link
Contributor

@gbj hmmm...I hadn't thought about this, but this totally makes sense! I'm on board.

@gbj gbj merged commit c322ef3 into main Aug 25, 2023
@gbj gbj deleted the signal-trait-assoc-types branch August 25, 2023 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants