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

No more strategy #239

Closed
wants to merge 6 commits into from
Closed

No more strategy #239

wants to merge 6 commits into from

Conversation

isovector
Copy link
Member

@TheMatten and I are hanging out this weekend and cleaning up things. This PR gets rid of Strategy as a unique concept, though doesn't clean up the *S functions.

@KingoftheHomeless
Copy link
Collaborator

I see how this works. Clever!

It's a shame we need to keep runS/bindS etc. for backwards compatibility issues. I suppose we'll have to wait until the eventual v2.0 release before we can get rid of the *S methods.

A thing I recently realized is that it is possible to strengthen Strategy, by having
type WithStrategy m f n = '[Tactics m f n, Embed m, Final m]. This allows you to embed monadic effects anywhere within the Strategy environment, and not just the end.

This also allows you to define

type Strategic m n a = forall f. Functor f => Sem (WithStrategy m f n) (f a)

And use embed to embed monadic effects with Strategy instead of pure. However, that's not backwards compatible. If we want, we can keep Strategic the way it is (and only change WithStrategy to add the capability of having embedding monadic actions anywhere within the strategy). Your thoughts? I'll add a pull request soon to no-more-strategy to show what I mean.

@KingoftheHomeless
Copy link
Collaborator

Wait, this introduces ambiguity issues in previously-valid uses of Tactics methods (like what Travis reports). I assume that can be addressed with polysemy-plugin, but this essentially makes Tactics unusable for anyone who isn't using the plugin, and that is a massive breaking change.
I don't know if I can get behind that. I feel such a big breaking change is best saved for v2.0, and even then, this will force us to declare "if you want to use certain important features of this library, you need the plugin".

@KingoftheHomeless
Copy link
Collaborator

KingoftheHomeless commented Oct 5, 2019

One way to get rid of the ambiguity issues is to require Tactics to be the first element of the effect row, rather than using a Member constraint, like this:

getInitialStateT :: forall m n r f. Sem (Tactics m f n ': r) (f ())
getInitialStateT = send @(Tactics m _ n) GetInitialState

I think that's a much better idea, seeing how Tactics will always be the first element of the effect row when it's used.

@isovector
Copy link
Member Author

Yeah. I mean it's not great, but then again, neither is the current situation. Way more people are complaining about how complicated tactics/strategy is, than the number complaining about having to use a plugin. Since our performance is already tied to enabling the plugin, it doesn't seem like much of a burden --- though I could definitely be persuaded on this front.

@isovector
Copy link
Member Author

isovector commented Oct 5, 2019 via email

@KingoftheHomeless
Copy link
Collaborator

Yup.

@isovector
Copy link
Member Author

Nice. Thanks! I'll get this cleaned up then.

@KingoftheHomeless
Copy link
Collaborator

I'll make it part of my pull request; it's almost ready

@isovector
Copy link
Member Author

I beat you!

@KingoftheHomeless
Copy link
Collaborator

KingoftheHomeless commented Oct 5, 2019

dammit

You win this one, but I'll show you yet!

@KingoftheHomeless
Copy link
Collaborator

One thing I'm curious about is liftS. It's the only part of the Strategic interface that can't be done Tactics-generically if Strategic is strengthened, as it uses the Embed that is part of WithStrategy. Should we remove liftS, forcing people to use embed m >>= pureT instead of liftS m?

@isovector
Copy link
Member Author

isovector commented Oct 5, 2019

To date, nobody has used liftS anyway, so maybe let's chop it out until someone complains that it doesn't exist? I think I'm getting it confused with pureS

@KingoftheHomeless
Copy link
Collaborator

KingoftheHomeless commented Oct 5, 2019

Are we sure about removing runS, bindS, getInitialStateS and getInspectorS (and with #241 , pureS) before v2.0? That's a pretty big breaking change.

@isovector
Copy link
Member Author

Let's just make it 2.0 /shrug

Copy link
Collaborator

@KingoftheHomeless KingoftheHomeless left a comment

Choose a reason for hiding this comment

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

Oh. Alright then.

* Strengthen Strategy, fix tactics ambiguity issues

* Generalize pureT

* Fix compilation errors

* Revert unintended change to stack.yaml
=> Sem '[Strategy m f n] a
runStrategy :: forall m f n a
. (Functor f, Monad m)
=> Sem '[Tactics m f n, Embed m, Final m] a
Copy link
Member Author

Choose a reason for hiding this comment

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

@KingoftheHomeless, the suggestion is to replace runM with this thing, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, runM shouldn't have anything to do with tactics: the suggestion was to have runM like this:

runM :: Monad m => Sem '[Embed m, Final m] a -> m a
runM = usingSem $ \u -> case decomp u of
  Right (Weaving (Embed m) s _ ex _) ->
    (ex . (<$ s)) <$> m
  Left g -> case extract g of
    Weaving (WithWeavingToFinal wav) s wv ex ins ->
      ex <$> wav s (runM . wv) ins

If we do, then runStrategy can be implemented like this:

runStrategy sem = \s wv ins -> runM $ interpret
  (\case
    GetInitialState       -> pure s
    HoistInterpretation f -> pure $ \fa -> wv (f <$> fa)
    GetInspector          -> pure (Inspector ins)
  ) sem

@isovector isovector closed this Nov 26, 2020
@isovector isovector deleted the no-more-strategy branch October 19, 2021 05:02
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