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

Define proper Applicative instances for Node, Tree and GenT #173

Merged
merged 6 commits into from Apr 14, 2019

Conversation

Projects
None yet
6 participants
@sjakobi
Copy link
Contributor

sjakobi commented Mar 18, 2018

Also canonicalize Monad instances.

This reduces allocations in a test of mine by ~40%.

@sjakobi

This comment has been minimized.

Copy link
Contributor Author

sjakobi commented Mar 18, 2018

Please check the instances carefully. I may have made a mistake.

Tree . pure . pure
(<*>) (Tree mab) (Tree ma) =
Tree $
(\nab na -> nab <*> na) <$> mab <*> ma

This comment has been minimized.

Copy link
@gwils

gwils Mar 20, 2018

Contributor

I don't like the eta expansion of (<*>)
I would change this to liftA2 (<*>) mab ma

This comment has been minimized.

Copy link
@sjakobi

sjakobi Mar 20, 2018

Author Contributor

Done! :)

@jystic

This comment has been minimized.

Copy link
Member

jystic commented Mar 21, 2018

Please check the instances carefully. I may have made a mistake.

Indeed, these are a bit tricky to get right and the consequences of them being wrong is pretty catastrophic 😆

Perhaps we can test them against the Monad instance with a property?

@sjakobi

This comment has been minimized.

Copy link
Contributor Author

sjakobi commented Mar 21, 2018

There was indeed a problem with my Applicative instance for Tree.

I have looked into writing the same tests for GenT but that looks quite a bit more complex than the rest.

@sjakobi

This comment has been minimized.

Copy link
Contributor Author

sjakobi commented Mar 21, 2018

Here are my test results for GenT:

GenT:
  applicative: identity:      *** Failed! Falsifiable (after 1 test): 
GenT { unGen = <function> }
Size 0
Seed 0 1
  applicative: composition:   *** Failed! Falsifiable (after 1 test): 
GenT { unGen = <function> }
GenT { unGen = <function> }
GenT { unGen = <function> }
Size 0
Seed 0 1
  applicative: homomorphism:  +++ OK, passed 500 tests.
  applicative: interchange:   *** Failed! Falsifiable (after 1 test): 
GenT { unGen = <function> }
False
Size 0
Seed 0 3
  applicative: functor:       *** Failed! Falsifiable (after 1 test): 
<function>
GenT { unGen = <function> }
Size 0
Seed (-1) (-1)
  monad laws: left  identity: *** Failed! Falsifiable (after 1 test): 
<function>
False
Size 0
Seed 1 3
  monad laws: right identity: *** Failed! Falsifiable (after 1 test): 
GenT { unGen = <function> }
Size 0
Seed 0 3
  monad laws: associativity:  *** Failed! Falsifiable (after 1 test): 
GenT { unGen = <function> }
<function>
<function>
Size 0
Seed (-1) 1
  monad applicative: pure:    +++ OK, passed 500 tests.
  monad applicative: ap:      *** Failed! Falsifiable (after 1 test): 
GenT { unGen = <function> }
GenT { unGen = <function> }
Size 0
Seed 1 1

The monad instance is broken too. Not sure if that's expected!? Maybe I should check QuickCheck's Gen to see if they got a lawful monad instance…

@jystic

This comment has been minimized.

Copy link
Member

jystic commented Mar 22, 2018

The monad instance is broken too. Not sure if that's expected!? Maybe I should check QuickCheck's Gen to see if they got a lawful monad instance…

QuickCheck doesn't have a lawful monad instance either, the reason is that both QuickCheck and Hedgehog use a splittable random number generator underneath so that they can generate infinite sequences. In Hedgehog, I think generating infinite sequences is no longer possible since we moved to the monad transformer. As such we could potentially move to using a StateT to store the random seed, and it would then be a valid monad. I'm not sure what other ill-effects this might have though.

@sjakobi

This comment has been minimized.

Copy link
Contributor Author

sjakobi commented Mar 22, 2018

Admittedly, I'm not too concerned about the lack of lawful instances for GenT. My original motivation was to find a way out of my OOM situation.

@gwils

This comment has been minimized.

Copy link
Contributor

gwils commented Mar 22, 2018

Require base >= 4.9

I would prefer that hedgehog continue to support GHC 7.10
To fix the Eq instance, just some CPP is needed, which I'm happy to do :)

If the other laws are already broken, then it doesn't particularly bother me that ap == (<*>) will now be broken too. The question of whether hedgehog should move to a different implementation to get back the laws, and what the consequences of this might be, is an interesting one! Perhaps it is worth discussing in its own issue? Either way, my preference is for this PR to go ahead in the meantime (preferably once I've written some CPP to fix 7.10)

@sjakobi

This comment has been minimized.

Copy link
Contributor Author

sjakobi commented Mar 22, 2018

Require base >= 4.9

I would prefer that hedgehog continue to support GHC 7.10

Note that the base bound applies only to the new testsuite.

To fix the Eq instance, just some CPP is needed, which I'm happy to do :)

The Eq instance is easy to fix but then GHC starts complaining about missing Show instances which I believe are defined via Show1 for GHC >= 8.0.

@sjakobi

This comment has been minimized.

Copy link
Contributor Author

sjakobi commented Mar 22, 2018

Another issue is that the testsuite succeeds with exit code 0 even when there are failing properties. I think one way way to fix that would be using hspec-checkers.

@gwils

This comment has been minimized.

Copy link
Contributor

gwils commented Mar 23, 2018

The Eq instance is easy to fix but then GHC starts complaining about missing Show instances which I believe are defined via Show1 for GHC >= 8.0

Thanks, I get you now. I've gone and looked at the code in Tree.hs. I don't like the kind of conditional API it has, where which instances you get are dependent on your GHC version, so I'd like the fix that. It's a hard problem though, so I don't know when I'll get around to it.

Go ahead with the PR. For now, you could change the .travis.yml to not run the tests on version 7.10 (it should still make sure the main library builds). That would fix the breakage.

@sjakobi

This comment has been minimized.

Copy link
Contributor Author

sjakobi commented Mar 26, 2018

I intend to work further on this later this week. This is what I have in mind:

  • Move Eq instances into library.

  • Use quickcheck-classes instead of checkers and make a proper testsuite with tasty-quickcheck.

    This way, test failures will actually make the test suite fail.

@sjakobi sjakobi force-pushed the sjakobi:ap branch from 827da17 to e7066ca Mar 28, 2018

@sjakobi

This comment has been minimized.

Copy link
Contributor Author

sjakobi commented Mar 28, 2018

Use quickcheck-classes instead of checkers

I had to stick with checkers as quickcheck-classes doesn't (easily) support types without Eq/Eq1 instances.

@sjakobi

This comment has been minimized.

Copy link
Contributor Author

sjakobi commented Mar 28, 2018

I have no idea why the Travis job with GHC-7.10.3 complains about eq1 not in scope. Builds fine on my machine.

@@ -538,13 +538,16 @@ instance Functor m => Functor (GenT m) where

instance Monad m => Applicative (GenT m) where

This comment has been minimized.

Copy link
@sjakobi

sjakobi Apr 7, 2018

Author Contributor

Should I document that the instance breaks the laws?

@sjakobi

This comment has been minimized.

Copy link
Contributor Author

sjakobi commented May 1, 2018

What remains to be done before this can be merged? I'd prefer not to end up fixing merge conflicts…

@thumphries

This comment has been minimized.

Copy link
Member

thumphries commented May 1, 2018

This looks good to me in the current state. I'm hesitant to merge it without approval from @jystic , since it touches a lot of critical code that only he truly understands. He is a little less active on GitHub these days, so it might be a little longer yet before this gets merged.

Thanks for doing all this work. Don't worry about merge conflicts, we will take care of them for you if they arise.

@gwils

This comment has been minimized.

Copy link
Contributor

gwils commented Sep 25, 2018

This PR is six months old now and I'd love to see it in hedgehog. @jystic, can you please review this?

import Test.QuickCheck
import Test.Tasty
import Test.Tasty.ExpectedFailure
import Test.Tasty.QuickCheck

This comment has been minimized.

Copy link
@moodmosaic

moodmosaic Sep 25, 2018

Member

I was just thinking that it may be possible to let Hedgehog itself run those tests instead of taking on tasty and friends. I like the simplicity of not having to take on any external dependencies:

{-# LANGUAGE OverloadedStrings #-}
module Main (
    main
  ) where

import           Control.Monad  (unless)
import           System.Exit    (exitFailure)

import           Hedgehog
import qualified Hedgehog.Gen   as Gen
import qualified Hedgehog.Range as Range

main :: IO ()
main = do
  results <- sequence [
      properties
    , regressionTests
    ]

  unless (and results)
    exitFailure

properties :: IO Bool
properties =
  checkParallel $ Group "Properties" [
        ("Property A"
        , property success)
      , ("Property B"
        , property discard)
      , ("Property C"
        , property failure)
    ]

regressionTests :: IO Bool
regressionTests =
  checkParallel $ Group "Regression" $
    do
    (a, b) <-
      [
        (1, 1),
        (2, 2),
        (3, 3)
      ]
    return
      ("Regression A"
      , withTests 1 . property $ a === b)
    ++

    do
    (a, b) <-
      [
        (1, 1),
        (2, 2),
        (3, 0) -- Uh-oh!
      ]
    return
      ("Regression B"
      , withTests 1 . property $ a === b)

It surely isn't perfect (not sure we can recheck a failing test), but I thought it'd be nice to share.


It also feels kind of weird having to use QC to test Hedgehog internals!

This comment has been minimized.

Copy link
@moodmosaic

moodmosaic Sep 25, 2018

Member

(This is just an idea, and shouldn't delay further the PR.)

This comment has been minimized.

Copy link
@sjakobi

sjakobi Sep 25, 2018

Author Contributor

I was just thinking that it may be possible to let Hedgehog itself run those tests instead of taking on tasty and friends. I like the simplicity of not having to take on any external dependencies:

Hmm, my intuition would be you shouldn't base tests for a test framework on the very same test framework. If the test framework has a bug, the internal tests might report false positives.

This comment has been minimized.

Copy link
@moodmosaic

moodmosaic Sep 25, 2018

Member

Well, not that I agree or disagree (and QC itself isn't perfect) but if they do it with compilers, it should be 'safe' doing for tests as well 😅

an existing compiler is used to build a “proto” compiler from the current source code. That “proto” compiler is then used to compile itself, producing a “final” compiler.
https://fsharp.github.io/2015/09/29/fsharp-compiler-guide.html#bootstrapping

, tasty
, tasty-expected-failure
, tasty-quickcheck
, transformers

This comment has been minimized.

Copy link
@moodmosaic

moodmosaic Oct 3, 2018

Member

I've been thinking about this for a while. Isn't it kind of oxymoron to use QC to test Hedgehog internals?

Perhaps, something like this allows us to mix and match properties as well as deterministic tests, by just using Hedgehog.


Feeling a bit worried about all the implicit QC arbitrary-magic. @jystic shows how a bug slipped in bos/text library by using default arbitraries. It's on this video at 11:50 (up to 13:09).

/cc @jystic @thumphries

This comment has been minimized.

Copy link
@charleso

charleso Oct 3, 2018

I'm definitely torn on this idea myself, but I took the plunge and self-tested in scala-hedgehog:

hedgehogqa/scala-hedgehog#16

I haven't worked out a way that will increase the level of confidence I have that the tests are actually running/working though.

This comment has been minimized.

Copy link
@moodmosaic

moodmosaic Oct 3, 2018

Member

I haven't worked out a way that will increase the level of confidence I have that the tests are actually running/working though.

That could very well be the case with any other testing tool we might want to use; its runner might not be picking up our tests, etc.

@jystic

This comment has been minimized.

Copy link
Member

jystic commented Mar 10, 2019

I'm afraid I refuse to depend on QuickCheck even in the tests. Just use hedgehog itself to do the tests please, I don't think there is an issue because trees are only to do with shrinking, not whether the test passes/fails. Other than that I'm happy as long as the comparison tests with Monad pass.

@moodmosaic

This comment has been minimized.

Copy link
Member

moodmosaic commented Mar 10, 2019

Just use hedgehog itself to do the tests please

👍

FWIW, in case it helps, one way of doing this is here: #173 (comment)

@sjakobi

This comment has been minimized.

Copy link
Contributor Author

sjakobi commented Mar 10, 2019

@jystic

I'm afraid I refuse to depend on QuickCheck even in the tests.

Would you mind pointing out why? I pulled in QuickCheck only to use the handy law properties from checkers.

Other than that I'm happy as long as the comparison tests with Monad pass.

Are you referring to the law ap f x ≡ f <*> x? In fact, the new Applicative GenT instance breaks that law. It's not obvious to me, why it needs to hold when most of the Applicative and Monad laws are broken too, though?

sjakobi added some commits Mar 17, 2018

Define proper Applicative instances for Node, Tree and GenT
* Also canonicalize a few Monad instances.

* Also add Eq instances for Tree and Node.

* As GenT's Monad and Applicative instances aren't lawful the associated
  tests are marked as ignored.

* The testsuite is unbuildable with GHC < 8.0 as a few required
   Show1/Show instances are only defined for base >= 4.9.

@jystic jystic force-pushed the sjakobi:ap branch from c2629e3 to c91752d Apr 14, 2019

@jystic

This comment has been minimized.

Copy link
Member

jystic commented Apr 14, 2019

I have made the necessary changes so that I'm to merge, just waiting for a green build.

Not sure about the namehedgehog-test-laws as the main point is that it depends on QuickCheck but we can always change it later as it's never published anywhere.

The idea would be to switch to using Hedgehog itself when we have some way to verify laws, which requires that we can generate functions.

Remove support for GHC 8.2.1, it introduced the getUnboxedSumName bug
Relevant issue: https://gitlab.haskell.org/ghc/ghc/issues/14051

The bug causes the following output when building `semigroupoids`:

```
ghc: panic! (the 'impossible' happened)
  (GHC version 8.2.1 for x86_64-unknown-linux):
	getUnboxedSumName
  513
  Call stack:
      CallStack (from HasCallStack):
        prettyCurrentCallStack, called at compiler/utils/Outputable.hs:1133:58 in ghc:Outputable
        callStackDoc, called at compiler/utils/Outputable.hs:1137:37 in ghc:Outputable
        pprPanic, called at compiler/prelude/KnownUniques.hs:104:5 in ghc:KnownUniques
```
@jystic

This comment has been minimized.

Copy link
Member

jystic commented Apr 14, 2019

@moodmosaic just pointed me to hedgehog-fn so we could resolve the QuickCheck dependency issue quicker than I anticipated.

@jystic jystic merged commit 1f026ea into hedgehogqa:master Apr 14, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sjakobi sjakobi deleted the sjakobi:ap branch Apr 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.