Check exports

[garyb]

Closing #562 again, and also checks that when a class is exported so are all its members, as suggested by @jdegoes.

[garyb]

I'm being incredibly dense, but couldn't work out a better way of doing this... suggestions please?

[paf31]

Maybe I'm missing something, but what's wrong with this?

[garyb]

forM_ exps ... 3 times seemed a little silly. If it's fine though, that's great, it'd explain why I couldn't think of a way to avoid it! (I tried using >> to compose the checks at first, but then errors get lost, etc.)

[paf31]

Well, you could do

forM_ exps $ \e -> do
  checkTypesAreExported e
  checkClassMembersAreExported e
  checkClassesAreExported e

I don't think that's too bad. Lambdabot kindly offers the following:

forM_ exps $ flip mapM 
  [ checkTypesAreExported
  , checkClassMembersAreExported
  , checkClassesAreExported
  ] . flip id

which seems much worse.

[garyb]

Haha, yeah. First one looks good to me.

[paf31]

Maybe

forM_ exps check
  [ typesAreExported
  , classMembersAreExported
  , classesAreExported
  ]
  where

  check :: (Monad m) => [a -> m b] -> a -> m ()
  check fs = flip mapM_ fs . flip id

I think that's the maximally acceptable amount of golfing.

[coveralls]

Coverage decreased (-0.0%) when pulling 9f4cb6f on check-exports into 5a20c09 on master.

[coveralls]

Coverage increased (+0.0%) when pulling 4350be8 on check-exports into 5a20c09 on master.

[garyb]

Just went with the most basic option in the end... ok to merge when you're happy with the rest of it. :)

[paf31]

The code looks good, but I don't understand why we're doing this to be honest. Why should a class export force members to be exported? Maybe I want the exact opposite, because I'm exposing "smart members".

Edit: certainly the converse should be required, or the type signatures wouldn't make sense.

Edit: basically, I'm saying that I think checkClassMembersAreExported is too strong a condition.

[garyb]

Wouldn't partially exporting a class prevent it from ever having new instances added?

[paf31]

Yes, that's true. Is that a bad thing? (genuine question)

[jdegoes]

If it can't have new instances, why does it need to be a type class? It can be hidden entirely inside the module, right?

[paf31]

I suppose that's true. But in that case, the exports section is unnecessarily verbose. The members should just be exported whenever the type class is.

[paf31]

This is fine though. I say we merge this, and consider a less verbose syntax for type class exports later.

[jdegoes]

I originally asked for this feature due to the number of times I export a class but fail to export its methods. It compiles clean, all tests pass, and no one can use the damn thing!

It'd be nice if, like Haskell, we supported Foo(..) for type classes so you could export all members automatically. I think I did that a few times before downgrading to Foo once I got a compilation error.

[paf31]

Agreed, it would be nice to have the same syntax as Haskell. In imports too, obviously.

Edit: actually, I still think it makes sense to just import everything for a type class together, just by importing the type class name.

[garyb]

Maybe that's the way to go then, and we can remove this check again later. At the moment we have a slight syntactic difficulty with using T(..) and C(..), but will just have to add a step in Names or something to lookup whether things are types or classes and remove the ambiguity there.

[paf31]

My vote would be for import T, which would import a type class and all its members. Importing a class and only some members is disallowed, so should be syntactically invalid.

[paf31]

@garyb Thanks for fixing this!

[garyb]

No problem. 🍰