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

Failure to automatically derive classes with operators #26

Closed
berdario opened this issue Oct 31, 2016 · 5 comments · Fixed by #27
Closed

Failure to automatically derive classes with operators #26

berdario opened this issue Oct 31, 2016 · 5 comments · Fixed by #27

Comments

@berdario
Copy link

If you try to

mkFixture "FixtureInst" [
      ''Alternative
    ]

You'll get the following error:

/home/dario/Projects/jira2sheet/test/Spec.hs:34:1:
    Illegal variable name: ‘_<|>’
    When splicing a TH declaration:
      data FixtureInst m_0
    = FixtureInst {_getWith :: (forall . forall (a_1 :: *) . Data.Aeson.Types.Class.FromJSON a_1 =>
                                                             Network.Wreq.Internal.Types.Options ->
                                                             GHC.Base.String ->
                                                             m_0 (Network.HTTP.Client.Types.Response a_1)),
                   _throwM :: (forall . forall (e_2 :: *) (a_3 :: *) . GHC.Exception.Exception e_2 =>
                                                                       e_2 -> m_0 a_3),
                   _logDebug :: (forall . Data.Text.Internal.Text -> m_0 ()),
                   _logInfo :: (forall . Data.Text.Internal.Text -> m_0 ()),
                   _mzero :: (forall . forall (a_4 :: *) . m_0 a_4),
                   _mplus :: (forall . forall (a_5 :: *) . m_0 a_5 ->
                                                           m_0 a_5 -> m_0 a_5),
                   _empty :: (forall . forall (a_6 :: *) . m_0 a_6),
                   _<|> :: (forall . forall (a_7 :: *) . m_0 a_7 ->
                                                         m_0 a_7 -> m_0 a_7),
                   _some :: (forall . forall (a_8 :: *) . m_0 a_8 -> m_0 ([a_8])),
                   _many :: (forall . forall (a_9 :: *) . m_0 a_9 -> m_0 ([a_9]))}

This error is quite obvious, and in my case I needed an Alternative instance, because I wanted my TestFixture to be an instance of MonadPlus

In turn, I wanted to use MonadPlus instead of MaybeT, to avoid having a concrete transformer stack to juggle.

I got the MaybeT in my code from Haskeline, and I solved this issue like this:

import           System.Console.Haskeline   (InputT, MonadException (..),
                                             RunIO (..), defaultSettings,
                                             runInputT)
import qualified System.Console.Haskeline   as Haskeline


class (Monad m) => MonadInput m where
    getInputLine :: String -> m String
    getPassword :: Maybe Char -> String -> m String

runInput :: (MonadException m, MonadPlus m) => InputT m (Maybe a) -> m a
runInput = maybe mzero return <=< runInputT defaultSettings

instance MonadInput IO where
    getInputLine = runInput . Haskeline.getInputLine
    getPassword c = runInput . Haskeline.getPassword c

Just like in the other issue I opened, it seems a common pattern that sometimes trying to directly reuse/implementing an existing class might turn out to be cumbersome, and the best way to encode effects and selectively define testfixture instances for them would be to instead define my own classes, and add thus a layer of indirection. Do you think this is truly the best approach?

Obviously I could also try to define my own instance of Alternative for TestFixture, but it feels icky to define an instance for a type that is not even defined in the source code (when using TH)... and I thought that I was going down the wrong path.

OTOH now I'm thinking if I should reconsider this choice, since I now got a need to use mzero as the result of a TestFIxture instance function, and this would obviously imply having a MonadPlus

@lexi-lambda
Copy link
Contributor

I admit I don’t totally grok what you’re trying to do here. It doesn’t make much sense to me as to why you would want to stub out <|>, so supplying Alternative seems odd. In general, you’re right that the _-prepending strategy won’t work for infix operators, though, so we’d need to come up with a different name mangling scheme for operators.

But yeah, the list of classes you supply to mkFixture is supposed to be a set of typeclasses you want to stub out. Why would you want to stub out Alternative or MonadPlus, given that those aren’t really stateful or effectful in any way?

@berdario
Copy link
Author

berdario commented Oct 31, 2016

Well, I only need MonadPlus for mzero, so if mkFixture might've saved me the effort of writing a bunch of error ":<|> not implemented" instances I thought of (ab)using it :)

I just got attracted in that direction since writing something like

mkFixture "FixtureInst" [
    ''MonadHTTPGet
  , ''MonadThrow
  , ''Log
  , ''MonadInput]

instance Alternative (TestFixture fixture log state) where
    empty = error "`empty` not implemented"
    (<|>) = error "`<|>` not implemented"
    some  = error "`some` not implemented"
    many  = error "`many` not implemented"

instance MonadPlus (TestFixture fixture log state) where
    mzero = error "`mzero` not implemented"
    mplus = error "`mplus` not implemented"

Seemed wrong (even though it compiles just fine)

Actually, I'm not even sure if I should define instances on FixtureInst or on TestFixtureT, the former seems a dead end, since it doesn't have a parameter upon which I can add constraints (which could be useful to implement MonadPlus)

Thank you again for your work on this library and the quick replies :)

@lexi-lambda
Copy link
Contributor

So, yes, if you want to define your own instances, defining them on TestFixture FixtureInst log state is the way to do it, or Monad m => TestFixtureT FixtureInst log state m if you want the a more general implementation. However, I’d still like to understand a little bit better how you’re using test-fixture here, since I find it a little interesting that you seem to be giving your main monad transformer stack an Alternative instance, which is not something I would normally consider doing.

What does your monad transformer stack look like, exactly, and how are you defining those instances? Are you using the IO instance?

@berdario
Copy link
Author

berdario commented Nov 7, 2016

Sure, I actually realized that indeed I probably don't want a MonadPlus instance, since

execRWST over a RWST of Maybe a will yield a Maybe (s, w), while I actually want a (s, Maybe w) (I don't want the state to be lost only because the computation failed)

I used to define the MonadPlus instance like this:

instance (Monad m, Alternative m) => Alternative (TestFixtureT fixture log state m) where
    empty = lift empty
    (<|>) = (<|>)

instance (MonadPlus m) => MonadPlus (TestFixtureT fixture log state m)

but now I'm trying to understand why do I actually need a MonadPlus instance for TestFixtureT in this kind of code:

https://gist.github.com/berdario/394ca0be1ecbde9f057612e1c5dc9740

(again, this might be a trivial doubt... I just haven't had the time to think properly on it)

@lexi-lambda
Copy link
Contributor

Yeah, I’m sort of unclear why you’re using MonadPlus here instead of something like MonadError, since you seem to only be using mzero as a failure state. Would it be possible to just use MonadError and ExceptT instead of MonadPlus? That seems like it would make more sense for your use case, anyway.

lexi-lambda added a commit that referenced this issue Nov 10, 2016
Since infix operators cannot contain underscores, use a tilde instead
when generating fixture record names for infix operators.

fixes #26
@jxv jxv closed this as completed in #27 Nov 12, 2016
jxv pushed a commit that referenced this issue Nov 12, 2016
…on for classes with infix operators (#27)

* Add tests for the TH code on GHC 8 using th-to-exp and test-fixture

* Support fixture generation for classes with infix operators

Since infix operators cannot contain underscores, use a tilde instead
when generating fixture record names for infix operators.

fixes #26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants