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

Aggregate the various effect types (&mut, Lazy, Cloned) via new trait (StateTransformer) #30

Closed
GregoryConrad opened this issue Jan 7, 2024 · 8 comments · Fixed by #35
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@GregoryConrad
Copy link
Owner

GregoryConrad commented Jan 7, 2024

I started noticing a ton of redundancy when writing regular side effects, lazy side effects, and cloneable side effects. Thus, we can make one generic side effect of each type (state, reducer, value, etc.) that takes in a generic input in order to produce different results. We can call these generic inputs StateTransformers or something similar. A trait for this could look something like:

pub trait StateTransformer: Send + 'static {
    type TransformedOutput<'a>;
    fn transform(&mut self) -> Self::TransformedOutput<'_>;
}

I’m thinking we can ship MutRef, Cloned, LazyMutRef, LazyCloned, Ref, and LazyRef state transformers out of the box. MutRef will just return the data with &mut, LazyMutRef will do the same but initializes with a oncecell Lazy, Cloned returns a clone of the value, etc. it’d be even cooler if we can compose the state transformers; i.e., make lazy a transformer that takes another transformer as an input, so we can write:

effects::state(Lazy(Cloned(|| 1234)))

or maybe:

effects::state(Cloned(Lazy(|| 1234)))

@GregoryConrad GregoryConrad added the enhancement New feature or request label Jan 7, 2024
@GregoryConrad
Copy link
Owner Author

I think an issue with Cloned(Lazy(T)) is that we would need a counterpart NonLazy because we can’t make Cloned work for both T and StateTransformer because negative trait impls don’t exist. Thus, we would need to do Lazy(Cloned(T)), which I think looks better anyways. However, I also don’t know if that’s possible, because the clone would be for cloning the function passed into Cloned and not the value itself…

Thus, we probably would need to expose an api like:

  • Ref(T)
  • Ref::lazy(T)
  • Same for MutRef and Cloned

or, what will probably happen, is just expose LazyFooBar et al.

@GregoryConrad
Copy link
Owner Author

I think I'm running into a compiler bug, because this looks correct to me:

trait StateTransformer: Send + 'static {
    type TransformedOutput<'a>;
    fn transform(&mut self) -> Self::TransformedOutput<'_>;
}

fn dumbed_down_raw<ST: StateTransformer>(
    st: ST,
) -> impl for<'a> SideEffect<Api<'a> = ST::TransformedOutput<'a>> {
    EffectLifetimeFixer0::new(move |register: SideEffectRegistrar| {
        let (transformer, _, _) = register.raw(st);
        transformer.transform()
    })
}

// Not needed to reproduce bug, but helpful for understanding:
struct Ref<T>(pub T);
impl<T: Send + 'static> StateTransformer for Ref<T> {
    type TransformedOutput<'a> = &'a T;
    fn transform(&mut self) -> Self::TransformedOutput<'_> {
        &self.0
    }
}

But produces this compilation error:

error[E0308]: mismatched types
  --> rearch-effects/src/lib.rs:91:9
   |
91 |         transformer.transform()
   |         ^^^^^^^^^^^^^^^^^^^^^^^ expected `&mut _`, found associated type
   |
   = note: expected mutable reference `&mut _`
                found associated type `<ST as StateTransformer>::TransformedOutput<'_>`
   = help: consider constraining the associated type `<ST as StateTransformer>::TransformedOutput<'_>` to `&mut _`
   = note: for more information, visit https://doc.rust-lang.org/book/ch19-03-advanced-traits.html
help: try removing the method call
   |
91 -         transformer.transform()
91 +         transformer
   |

error[E0271]: type mismatch resolving `<EffectLifetimeFixer0<{[email protected]:89:31}> as SideEffect>::Api<'a> == <ST as StateTransformer>::TransformedOutput<'a>`
  --> rearch-effects/src/lib.rs:88:6
   |
88 | ) -> impl for<'a> SideEffect<Api<'a> = ST::TransformedOutput<'a>...
   |      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `&mut _`, found associated type
   |
   = note: expected mutable reference `&mut _`
                found associated type `<ST as StateTransformer>::TransformedOutput<'_>`
help: consider constraining the associated type `<ST as StateTransformer>::TransformedOutput<'_>` to `&mut _`
   |
86 | pub fn ez_raw<ST: StateTransformer<TransformedOutput<'_> = &mut _>>(
   |                                   ++++++++++++++++++++++++++++++++

Some errors have detailed explanations: E0271, E0308.

@GregoryConrad GregoryConrad added the help wanted Extra attention is needed label Jan 10, 2024
@GregoryConrad GregoryConrad changed the title Add “state transformers” instead of all the different effect types Aggregate the various effect types (&mut, Lazy, Cloned) via new trait (StateTransformer) Jan 10, 2024
@GregoryConrad
Copy link
Owner Author

GregoryConrad commented Jan 10, 2024

Just tried this, which would enable something like effects::state<Cloned<_>>(1234) (which I think I like more, since we can maybe then use default generics to specify MutRef as the default):

pub trait StateTransformer: Send + 'static {
    type Input;
    fn from_input(input: Self::Input) -> Self;
    type Output<'a>;
    fn as_output(&mut self) -> Self::Output<'_>;
}

pub fn new_raw<ST: StateTransformer /* TODO = MutRef */>(
    initial: ST::Input,
) -> impl for<'a> SideEffect<Api<'a> = <ST as StateTransformer>::Output<'a>> {
    EffectLifetimeFixer0::new(move |register: SideEffectRegistrar| {
        let (transformer, _, _) = register.raw(ST::from_input(initial));
        transformer.as_output()
    })
}

Same set of compilation errors as before.

@GregoryConrad
Copy link
Owner Author

GregoryConrad commented Jan 11, 2024

This seems to be some weird error where the compiler for some reason thinks the first part of the API should always be getting a &mut T or T: 'static, but never anything else (like a &T or StateTransformer::Output<'a>). This is definitely some sort of compiler bug but I have too much going on right now to look into it further. Edit: I just didn't remember that the bounds in the EffectLifetimeFixer required &mut; see #30 (comment)

@0e4ef622
Copy link

I think that's just because the lifetime fixer wants &mut as a return value. It can compile with another fixer.

pub trait StateTransformer: Send + 'static {
    type Input;
    fn from_input(input: Self::Input) -> Self;
    type Output<'a>;
    fn as_output(&mut self) -> Self::Output<'_>;
}

struct LifetimeFixer<F, ST>(F, PhantomData<ST>);
impl<F, ST: StateTransformer> SideEffect for LifetimeFixer<F, ST>
where
    F: FnOnce(SideEffectRegistrar<'_>) -> ST::Output<'_>,
{
    type Api<'a> = ST::Output<'a>;
    fn build<'a>(self, registrar: SideEffectRegistrar<'a>) -> Self::Api<'a> {
        self.0(registrar)
    }
}

impl<F, ST> LifetimeFixer<F, ST>
where
    F: FnOnce(SideEffectRegistrar<'_>) -> ST::Output<'_>,
    ST: StateTransformer,
{
    fn new(f: F) -> Self {
        LifetimeFixer(f, PhantomData)
    }
}

pub fn new_raw<ST: StateTransformer /* TODO = MutRef */>(
    initial: ST::Input,
) -> impl for<'a> SideEffect<Api<'a> = ST::Output<'a>> {
    LifetimeFixer::<_, ST>::new(move |register: SideEffectRegistrar| {
        let (transformer, _, _) = register.raw(ST::from_input(initial));
        transformer.as_output()
    })
}

@GregoryConrad
Copy link
Owner Author

because the lifetime fixer wants &mut as a return value

Duh, can’t believe I didn’t think of that 😂

Thanks, I’ll take another crack at this today, or at least within the next couple of days

@GregoryConrad
Copy link
Owner Author

GregoryConrad commented Jan 14, 2024

I would want the API to look like this I think:

effects::raw::<_, Cloned>(1234)

But that requires non_lifetime_binders, which, based on the little I know, looks a hell of lot like higher-kinded types. The implementation would look something like:

trait A<T> {}
fn b<R, S: for<T> A<T>>() {}

So then I tried:

trait A<T> {}
fn b<T, S: A<T>>(t: T) {}

Which yields an API like:

b::<_, B<_>>(0);

// based on:
struct B<T>(T);
impl<T> A<T> for B<T> {}

Issue is I would want a default generic, probably MutRef. But that isn't possible because of rust-lang/rust#36887

Granted, the alternative API:

effects::raw(MutRef::from(1234))

Would never have a default either. So I just really need to decide between:

effects::raw::<_, MutRef<_>>(123)
// and:
effects::raw(MutRef::from(123))

Leaning more toward the first option at the moment, despite the few extra keystrokes, in the hope non_lifetime_binders eventually lands.

@GregoryConrad
Copy link
Owner Author

Oh, I think I was really overcomplicating it. We don't need a T type since we have StateTransformer::Input. So the API is one of:

effects::raw::<Ref<_>>(123)
// or
effects::raw(Ref::new(123))

I much prefer the first option. It's possible some state transformers could be like Ref(123) instead of Ref::new(123), which would save keystrokes, but not all of them would support that, and I'd rather keep API consistency.

All in all, I think the generic approach is better because it more clearly just defines the behavior of what is going on rather than the input data itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants