Fix warnings in hls-graph, enable pedantic in CI

[jhrcek]

No description provided.

[jhrcek]

Why am I adding explicit export list to this module?
I'm aware it's internal. But it exposes UnsafeMkKey + also things like newKey which use global Map within IORef to issue new Keys.
If we can make the Key newtype opaque, it would be safe to add {-# COMPLETE Key #-} pragma to get rid of "incomplete pattern match" warnings, because the only way to create new Key values are through newKey smart constructor.

[michaelpj]

I don't have a strong opinion, although it is a rather long export list.

[jhrcek]

I could try to make it shorter by splitting off KeyMap and KeySet into separate modules.. How does that sound?

[michaelpj]

Or Key I guess, if that's the thing you want to be opaque. I don't want to derail you too much from your warning-fixing quest, but both of those sound like sensible refactorings to me.

[jhrcek]

Ok, moved Key + KeyMap + KeySet stuff (because they depend on the internals) to separate module and removed explicit export list from Types module.

[michaelpj]

I think this should be implied by the later build with pedantic?

[jhrcek]

Indeed, that's what I expected too (that when you change flags cabal rebuilds all the libraries affected).

But then how do you explain the fact that this flags job passes on master with all those warnings present?

Running these 3 cabal builds locally (after cabal clean) I see that modules from hls-graph are compiled in the first two and the third (cabal v2-build --flags="pedantic") seems to be using hls-graph compiled in the first 2 runs:

$ cabal v2-build --flags="pedantic"
...
In order, the following will be built (use -v for more details):
 - ghcide-2.6.0.0 (exe:ghcide-test-preprocessor) (configuration changed)
 - hls-graph-2.6.0.0 (lib) (configuration changed)
 - shake-bench-0.2.0.0 (lib) (first run)
...
Preprocessing executable 'ghcide-test-preprocessor' for ghcide-2.6.0.0..
Building executable 'ghcide-test-preprocessor' for ghcide-2.6.0.0..
Preprocessing library for hls-graph-2.6.0.0..
Building library for hls-graph-2.6.0.0..                                   <<-- the output suggests the individual modules are no longer compiled (it's probably using artifacts from previous 2 builds???
Preprocessing library for shake-bench-0.2.0.0..
Building library for shake-bench-0.2.0.0..

With the additional pedantic in this PR it breaks on master, whereas it passes after the warning fixes in this PR.

[michaelpj]

Weird. I wonder if we could simplify this by setting up a cabal.project.local as for the test options and then just doing a simple cabal build all?

[michaelpj]

I don't have a strong opinion, although it is a rather long export list.

[jhrcek]

ghc-patched-unboxed-bytecode and bench-exe flags seem to be some historic leftovers - they don't exist in ghcide.cabal anymore, so I just removed them from here.

The idea behind using cabal configure is that we do only single cabal build with pedantic flags enabled for everything - and don't skip pedantic when building hls-graph and ghcide.

Also I guessed what is the purpose of this flags job and added some comments - shout if you don't agree with any of this.

[jhrcek]

Moved all the stuff that needs Key internals into this module with explicit import list, so that we don't expose the UnsafeMkKey constructor and all the unsafePerformIO stuff is hidden as impl. detail of this module.
I ultimately did this so that I can mark Key pattern as COMPLETE and fix bunch of "incomplete pattern match" warnings.

[jhrcek]

@michaelpj I did the changes you suggested

1. use cabal configure to turn on pedantic flag for everything and only do single cabal build
2. moved Keyrelated internals to separate module with explicit import list

• did minimal changes to make the build pass with the newly enabled pedantic flags

so please re-review.

[michaelpj]

Looks great! just one nit