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

SuspenseStore refactor to allow caching of any HasBackend instance members #35

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

megamaddu
Copy link
Member

@megamaddu megamaddu commented Jul 23, 2020

The old store required the key and value types of the cache to be defined at cache creation. This refactor is to allow any type implementing HasBackend to be stored in the cache in a (hopefully) type-safe way.

The main bits I'd like feedback on:

  • does the HasBackend class behave correctly, i.e. do the functional dependencies correctly prevent invalid instances?
    • an instance is invalid if a given key, k, can be used in multiple instances with a different s or v
    • storing the same entity type using different key types is fine, but it should be impossible to cause overlapping s values across different choices for v.. does that make sense?
    • put another way, s needs to be unique for each choice of v
  • the HasBackend implementations are carried into the cache using a few internal classes, ToKey, ToAffUnsafe, and HasOpaque, using Storable -- do these make sense?
  • any better ways to accomplish this?

@megamaddu megamaddu self-assigned this Jul 23, 2020

class IsSymbol s <= HasBackend k v (s :: Symbol) | k -> s, k -> v where
fromKey :: k -> String
backend :: k -> Aff v
Copy link
Member Author

Choose a reason for hiding this comment

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

Implementing HasBackend and calling the getters above is the external API. The classes and types below should contain the unsafe bits.

Choose a reason for hiding this comment

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

I'm not sure this will do what you want it to: with this code, I think I can define instances

HasBackend String Int "1"
HasBackend Int Int "1"

since we are effectively only saying that there can only be one HasBackend instance for each key type k; we aren't preventing you from defining two different instances with different k types but the same s type. A functional dependency s -> k would prevent you from doing this, but that probably isn't workable because you'll fall foul of orphan instance restrictions there - since in this case s determines everything, then the instance can only be defined in the same module as the HasBackend class. Normally you'd also be able to define the instance in the same module as the type s, but in this case s is built in to the compiler so you can't put it there. See https://liamgoodacre.github.io/purescript/type/class/instance/orphan/functional/dependencies/2017/01/22/purescript-orphan-instance-detection.html for more information on how functional dependencies affect orphan instance checking.

I think you'd ideally need something like Haskell's Data.Typeable to do this safely, but PureScript doesn't have that. I'm not sure there's a good way of doing this without Data.Typeable, unfortunately.

@megamaddu megamaddu force-pushed the suspense-store-refactor branch from 1316f9f to cb14a90 Compare July 23, 2020 19:16
mkStorable :: forall k v s. HasBackend k v s => k -> StoreKey
mkStorable k = StoreKey \f -> f k

class Storable k where

Choose a reason for hiding this comment

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

Is the type class doing much for us here? In general I think defining a type class and not exporting it is a bit suspect; would normal functions work?

Choose a reason for hiding this comment

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

Same goes for HasOpaque

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, you're right, thanks!


class IsSymbol s <= HasBackend k v (s :: Symbol) | k -> s, k -> v where
fromKey :: k -> String
backend :: k -> Aff v

Choose a reason for hiding this comment

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

I'm not sure this will do what you want it to: with this code, I think I can define instances

HasBackend String Int "1"
HasBackend Int Int "1"

since we are effectively only saying that there can only be one HasBackend instance for each key type k; we aren't preventing you from defining two different instances with different k types but the same s type. A functional dependency s -> k would prevent you from doing this, but that probably isn't workable because you'll fall foul of orphan instance restrictions there - since in this case s determines everything, then the instance can only be defined in the same module as the HasBackend class. Normally you'd also be able to define the instance in the same module as the type s, but in this case s is built in to the compiler so you can't put it there. See https://liamgoodacre.github.io/purescript/type/class/instance/orphan/functional/dependencies/2017/01/22/purescript-orphan-instance-detection.html for more information on how functional dependencies affect orphan instance checking.

I think you'd ideally need something like Haskell's Data.Typeable to do this safely, but PureScript doesn't have that. I'm not sure there's a good way of doing this without Data.Typeable, unfortunately.

@i-am-the-slime
Copy link
Member

The suspense on this one is killing me!

@megamaddu
Copy link
Member Author

@i-am-the-slime tbh I'm not sure when I'll get to this. My new day job isn't using PS and I'm a bit burnt out on the after hours programming. Maybe someday inspiration will strike, but in the meantime I'll do my best to review and merge any work others want to contribute to this.

@i-am-the-slime
Copy link
Member

@spicydonuts
It's quite sad that your new job does not use PS. All good, please take all the time you need for yourself and only do this when you really feel like it! Your physical and mental health(s?) will thank you for it.

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.

3 participants