Fix warnings in hls-graph, enable pedantic in CI #4047
Conversation
| @@ -5,7 +5,65 @@ | |||
| {-# LANGUAGE RecordWildCards #-} | |||
| {-# LANGUAGE ViewPatterns #-} | |||
|
|
|||
| module Development.IDE.Graph.Internal.Types where | |||
| module Development.IDE.Graph.Internal.Types | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I don't have a strong opinion, although it is a rather long export list.
There was a problem hiding this comment.
I could try to make it shorter by splitting off KeyMap and KeySet into separate modules.. How does that sound?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok, moved Key + KeyMap + KeySet stuff (because they depend on the internals) to separate module and removed explicit export list from Types module.
.github/workflows/flags.yml
Outdated
| @@ -70,7 +70,7 @@ jobs: | |||
| os: ${{ runner.os }} | |||
|
|
|||
| - name: Build `hls-graph` with flags | |||
| run: cabal v2-build hls-graph --flags="embed-files stm-stats" | |||
| run: cabal v2-build hls-graph --flags="embed-files stm-stats pedantic" | |||
There was a problem hiding this comment.
I think this should be implied by the later build with pedantic?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
| @@ -5,7 +5,65 @@ | |||
| {-# LANGUAGE RecordWildCards #-} | |||
| {-# LANGUAGE ViewPatterns #-} | |||
|
|
|||
| module Development.IDE.Graph.Internal.Types where | |||
| module Development.IDE.Graph.Internal.Types | |||
There was a problem hiding this comment.
I don't have a strong opinion, although it is a rather long export list.
jhrcek
force-pushed
the
jhrcek/pedantic-hls-graph
branch
from
f91edec to
756b537
Compare
| run: cabal v2-build hls-graph --flags="embed-files stm-stats" | ||
|
|
||
| - name: Build `ghcide` with flags | ||
| run: cabal v2-build ghcide --flags="ghc-patched-unboxed-bytecode test-exe executable bench-exe ekg" |
There was a problem hiding this comment.
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
force-pushed
the
jhrcek/pedantic-hls-graph
branch
from
c212e98 to
36cb7d1
Compare
| {-# LANGUAGE ViewPatterns #-} | ||
|
|
||
| module Development.IDE.Graph.Internal.Key | ||
| ( Key -- Opaque - don't expose constructor, use newKey to create |
There was a problem hiding this comment.
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.
|
@michaelpj I did the changes you suggested
so please re-review. |
No description provided.