Add support for type class instances #126
Conversation
Also includes class declarations and (both regular and standalone) derived instances. Consequently, they show up in the output if unreachable.
Also tested on 9.4.5.
`OverloadedStringsNoSig` correctly gives no output. `Monads` incorrectly claims that the instances for `Identity'` (used by `bar`) are unreachable, while correctly giving no output on the instances for `Identity` (used by `foo`). `RangeEnum` incorrectly claims that `$fEnumColour` is unreachable.
Note that what all the incorrect outputs have in common are top-level type signatures. The type signature does not have to be immediately relevant: see `OverloadedStringsNoSig` - the type of `root` and `root'` is `Char`, but explicitly writing that breaks their evidence variables for `IsString`. When type signatures are omitted, evidence usages for syntax are present in `hie` files as expected.
In `Monads.hie`, this is the node corresponding to `bar`:
```
929 │ Node@test/Spec/Monads/Monads.hs:(36,1)-(38,13): Source: From source
930 │ {(annotations: {(FunBind, HsBindLR),
931 │ (Match, Match),
932 │ (XHsBindsLR, HsBindLR)}),
933 │ (types: [304]), (identifier info: {})}
```
and this is the node for `foo`:
```
837 │ Node@test/Spec/Monads/Monads.hs:(31,1)-(33,12): Source: From source
838 │ {(annotations: {(FunBind, HsBindLR),
839 │ (Match, Match),
840 │ (XHsBindsLR, HsBindLR)}),
841 │ (types: [302]),
842 │ (identifier info: {(name $dNum, Details: Just 3 {usage of evidence variable}),
843 │ (name $fMonadIdentity, Details: Just 167 {usage of evidence variable}),
844 │ (name $dMonad, Details: Just 167 {evidence variable bound by a let, depending on: [$fMonadIdentity]
845 │ with scope: LocalScope test/Spec/Monads/Monads.hs:(31,1)-(33,12)
846 │ bound at: test/Spec/Monads/Monads.hs:(31,1)-(33,12)}),
847 │ (name $dNum, Details: Just 3 {evidence variable bound by a let, depending on: [$dNum]
848 │ with scope: LocalScope test/Spec/Monads/Monads.hs:(31,1)-(33,12)
849 │ bound at: test/Spec/Monads/Monads.hs:(31,1)-(33,12)}),
850 │ (name $dNum, Details: Just 3 {evidence variable bound by a let, depending on: [$dNum]
851 │ with scope: LocalScope test/Spec/Monads/Monads.hs:(31,1)-(33,12)
852 │ bound at: test/Spec/Monads/Monads.hs:(31,1)-(33,12)})})}
```
The appearance of `foo` and `bar` in GHC's renamer AST does not differ in any meaningful way. In the typechecker AST, `bar` contains a (seemingly redundant) `EpAnn` annotation, which is missing in `foo`, but is otherwise the same everywhere else. For this reason I suppose the problem is not too deep within GHC and any required fixes are probably limited to modules directly handling `hie` files.
A `.failing` file should contain the current expected output of a failing test, overriding the `.stdout` file. This allows the test suite return a 0 exit code without having to disable failing tests. If the test happens to return exactly the correct output contained in the `.stdout` file, it fails with a `not expected:` failure. I think it would be worthwhile to make a small extension of `hspec` that allows marking tests as expected failures in a more polished way, assuming such an extension does not already exist.
OverloadedLabels already works perfectly, but ApplicativeDo has the same problem as Monad
|
Amazing work @ryndubei, I'm really excited getting this merged! I'm away for the weekend, but I'll try and prioritize a more thorough review in the week |
|
Made a minor change - the pretty-printed type of instances is now visible in the output as such: |
|
I wonder if there's any real benefit to printing the underlying name at all? What do you think about just printing |
|
I would say this has the benefit of distingushing the part which is matched against by if we really want to discourage using the |
|
The weed will now show up as |
|
I just ran this branch against my work code base and it was great! It found tons of weeds, and the reporting format was useful for figuring out what was going on. This is a feature I've long wanted Weeder to have. Thanks for adding it! Hopefully this PR can get merged and released sooner rather than later 😄 |
|
We're still working on the finishing touches, but it should be merged soon 😄 For disclosure, @ryndubei is working with me for Haskell Summer of Code, and we're catching up on Thursday's to chat next steps. |
|
Updated the code to make use of This does not change any user-facing behaviour, but it seems to improve performance (haskell-language-server: ~3s to ~2.4s) as This also allows us to skip analysing evidence uses when |
Co-authored-by: Ollie Charles <ollie@ocharles.org.uk>
|
Okay, I'll merge this now then. |
This allows Weeder to detect whether a type class instance declaration is used. This includes both regular type class instances as well as those in
derivingclauses (including standalone deriving).Two new optional fields are added to the config file that allow selecting type class instances to be marked as roots.
root-classesmarks all instances with parent classes that match the given pattern as roots.root-instancesmarks all instances with types matching the given pattern as roots. SeeInstanceTypeclass.toml,InstanceRootConstraint.tomlandRootClasses.tomlin the test suite for examples.Due to current limitations of
hiefiles, instance usages from syntax such as overloaded literals will in general not be detected. For this reason it is recommended to addIsString,IsListandEnumtoroot-classes, and potentially alsoNum,Fractional,Monad, andApplicative.OverloadedLabelsdoes not appear to have this problem.The test suite now also allows for marking tests as failing via
.failingfiles. A.failingfile should contain the current output of a failing test, overriding the.stdoutfile. This allows the test suite return a 0 exit code without having to remove the failing tests.If the failing test happens to return exactly the correct output contained in the
.stdoutfile, it fails with anot expected:failure.