haskell-checkers / checkers

Check properties on standard classes and data structures
Other
79 stars 13 forks source link

Fix compilation with GHC-8.8 #38

Closed sjakobi closed 5 years ago

sjakobi commented 5 years ago

Sorry for the lack of explanation!

foldable already contains a check for the new foldMap' method introduced in base-4.13:

https://github.com/conal/checkers/blob/1fcf99edad49538f8723bbbd36c9e2ebd13959ba/src/Test/QuickCheck/Classes.hs#L783-L787

I had simply forgotten to add the Arbitrary constraint.

conal commented 5 years ago

I don't have GHC 8.8 to test. Do you know why the Arbitrary m constrain is redundant under GHC 8.6.4 (as I just tried) but is required (I assume, given your PR) for 8.8?

sjakobi commented 5 years ago

Do you know why the Arbitrary m constrain is redundant under GHC 8.6.4 (as I just tried) but is required (I assume, given your PR) for 8.8?

The constraint is only required for the foldMap'P property which is behind CPP so it doesn't compile with GHC 8.6.

conal commented 5 years ago

Got it. Thanks. Would you please add a similar CPP guard for that constraint, for warning-free compilation pre-8.4?

sjakobi commented 5 years ago

Would you please add a similar CPP guard for that constraint, for warning-free compilation pre-8.4?

Personally I don't think that's a good idea:

  1. IMHO CPP is too heavy a hammer for silencing this rather innocuous warning. I believe that the decrease in readability due to the CPP would outweigh the benefits in this case.
  2. In a few months, once most users use GHC 8.8 or greater, nobody is going to get the warning anyway.
  3. If we hide the constraint for GHC <= 8.6, some users might find that they have to update their tests once they start using GHC 8.8. On the other hand, if we leave the constraint unhidden, the API is consistent across all GHC versions.

Of course, I'll still add the CPP if you insist! :)

conal commented 5 years ago

Ah, interesting point. Thanks for sharing your reasoning.

If we hide the constraint for GHC <= 8.6, some users might find that they have to update their tests once they start using GHC 8.8. On the other hand, if we leave the constraint unhidden, the API is consistent across all GHC versions.

Isn't your PR a counterexample to API stability? When someone adds another test, the constraint part of the API may change again. Moreover, if we do add this constraint, people who intentionally or accidentally pull in the new checkers with GHC < 8.8 may have to update their tests as well.

Hm. I imagine this issue would arise repeatedly, but I've not encountered it before. I wonder how others deal with it.

My current inclination is to add the new constraint and silence the warning message with {-# OPTIONS_GHC -Wno-redundant-constraints #-} in Test.QuickCheck.Classes or an equivalent project-wide setting in the .cabal file. Still, I'm interested in your thoughts on the above.

newhoggy commented 5 years ago

ghc-8.8.1 has been released, so it would be good to have this fixed so some downstream projects can build.

Anything can be done to push this forward?

sjakobi commented 5 years ago

Sorry for letting this languish! Let's try to wrap up the discussion and get this merged! :)

Isn't your PR a counterexample to API stability? When someone adds another test, the constraint part of the API may change again. Moreover, if we do add this constraint, people who intentionally or accidentally pull in the new checkers with GHC < 8.8 may have to update their tests as well.

Good point! I usually assume library users to be diligent library maintainers themselves, who eagerly try to support to each new GHC version, and therefore have to satisfy the constraint anyway. The reality is probably a bit different!

My current inclination is to add the new constraint and silence the warning message with {-# OPTIONS_GHC -Wno-redundant-constraints #-} in Test.QuickCheck.Classes or an equivalent project-wide setting in the .cabal file. Still, I'm interested in your thoughts on the above.

When I see disabled warnings I often worry that they might hide more than intended. I'd rather add a comment on the constraint explaining the warning. But it's your call, @conal! :)

newhoggy commented 5 years ago

Needed by this:

newhoggy commented 5 years ago

My current inclination is to add the new constraint and silence the warning message with {-# OPTIONS_GHC -Wno-redundant-constraints #-} in Test.QuickCheck.Classes

I prefer this option because constraints are the same across all GHC versions for any given version of the library, preventing the need for users of the library to use CPP if they choose their bounds properly.

conal commented 5 years ago

Released to Hackage. Thanks for the input!

conal commented 5 years ago

Oops! The Travis build for this commit ran into trouble with GHC 7.10.3:

src/Test/QuickCheck/Classes.hs:8:16:
    unknown flag in  {-# OPTIONS_GHC #-} pragma: -Wno-redundant-constraints

Apparently GHC added this option in 8.0. I can CPP-protect the option:

#if MIN_VERSION_base(4,9,0)
{-# OPTIONS_GHC -Wno-redundant-constraints #-}
#endif

I needed a similar hack for Test.QuickCheck.Instances.Array, which requires an Ix a constraint for GHC 7.10.3, but such a constraint is redundant for later versions.

To fix breaking pre-GHC-8 builds, I pushed these changes. This time I waited for Travis builds to succeed, and then released to Hackage.

Several redundant import warnings arise in 7.10.3 as well, but I think I'll leave them be, presuming relatively few users of 7.10.3 (reasoning as @sjakobi above).

I'm not at all confident that I've chosen the cleanest way to keep builds working, reduce warning noise, and maintain simple code. Comments welcome.

sjakobi commented 5 years ago

Sorry for the trouble, @conal! Tricky stuff…

I'm not at all confident that I've chosen the cleanest way to keep builds working, reduce warning noise, and maintain simple code. Comments welcome.

As a library consumer, I don't care much whether a library builds with warnings, and particularly not when it's only with old GHCs. I'd trust the maintainer that they have seen the warning and don't perceive it as dangerous. I also usually see these warnings at most once: When I install the dependency.

So in my own code I try to keep it warnings-free with the latest GHC version, but I don't mind if there are warnings with old GHC versions. I usually try to figure out the cause of the warning and might document it in the code as a reminder. But I don't unconditionally disable warnings and try to avoid CPP as that tends to cause (or hide) more problems.

conal commented 5 years ago

Thanks for the perspective, @sjakobi. These goals and non-goals sound good to me.

For this set of changes, I don't know how to simultaneously (a) avoid unconditionally disabling warnings, (b) not use CPP, and (c) keep the compilation warning-free in the latest GHC. Note that some constraints have become redundant, and so the redundancy warning crops up with more recent GHCs. Complicating matters, the flag to suppress this particular warning didn't appear until GHC 8.0. Thoughts?

sjakobi commented 5 years ago

Here's one proposal to handle the warnings: https://github.com/conal/checkers/pull/39.

gwils commented 5 years ago

I've made a metadata revision on hackage to the version that did not build with 7.10 It will not try to build with that compiler any more.