Dont suggest const when NamedFieldPuns are used #1521
Conversation
src/Hint/Match.hs
Outdated
| @@ -201,7 +201,7 @@ checkSide x bind = maybe True bool x | |||
| isType "Compare" x = True -- Just a hint for proof stuff | |||
| isType "Atom" x = isAtom x | |||
| isType "WHNF" x = isWHNF x | |||
| isType "Wildcard" x = any isFieldPun (universeBi x) || any hasFieldsDotDot (universeBi x) | |||
| isType "Wildcard" x = any isFieldPun (universeBi x) || any hasFieldsDotDot (universeBi x) || any isFieldPunUpdate (universeBi x) | |||
There was a problem hiding this comment.
Nice attempt! But I don't think either any isFieldPun (universeBi x) or any isFieldPunUpdate (universeBi x) is the right thing to do.
The goal is to avoid suggesting "Use const" for either \a -> Foo{a} or \a -> x{a}. The first one is field pun, and the second one is field pun update. But there's no need to exclude all field puns or field pun updates. We should indeed suggest "Use const" for \a -> Foo{b} and \a -> x{b}.
I think the correct approach is to reject \a -> Foo{a} and \a -> x{a} on the basis of introducing free variables: if we turn \a -> Foo{a} into const Foo{a}, then we'd introduce a new free variable a, and that's why we shouldn't do it. The check is here.
This check does the right thing for field puns, but not for field pun updates, because the freeVar computation for field pun updates is incorrect. When hfbPun==True, instead of returning freeVars x, it should return the OccName contained in the LHS (the argument before x) as the free variable. Hope this helps!
There was a problem hiding this comment.
Thanks for directions! Hope I got it right.
Nice attempt! But I don't think either any isFieldPun (universeBi x) or any isFieldPunUpdate (universeBi x) is the right thing to do.
Should I take the opportunity then and get rid of any isFieldPun (universeBi x) from Wildcard? It doesn't break any tests, but I don't really understand what Wildcard is supposed to check sweat_smile
There was a problem hiding this comment.
IMO isWildCard should just look for RecordWildCards, which is what hasFieldsDotDot is.
tests/hint.test
Outdated
| @@ -51,6 +51,29 @@ Perhaps: | |||
|
|
|||
| 1 hint | |||
|
|
|||
| --------------------------------------------------------------------- | |||
| RUN tests/use-const.hs | |||
There was a problem hiding this comment.
These tests can be written directly in the <TEST> section in hlint.yaml. The .test files are for tests that need custom hint files, which can't be written there.
There was a problem hiding this comment.
Thanks! I didn't notice <TEST> section in hlint.yaml 😅
| @@ -174,6 +174,7 @@ instance FreeVars (LocatedA (HsFieldBind (LocatedAn NoEpAnns (FieldOcc GhcPs)) ( | |||
| freeVars o@(L _ (HsFieldBind _ _ x _)) = freeVars x | |||
|
|
|||
| instance FreeVars (LocatedA (HsFieldBind (LocatedAn NoEpAnns (AmbiguousFieldOcc GhcPs)) (LocatedA (HsExpr GhcPs)))) where | |||
| freeVars (L _ (HsFieldBind _ x _ True)) = Set.singleton $ rdrNameOcc $ rdrNameAmbiguousFieldOcc $ unLoc x -- a pun | |||
There was a problem hiding this comment.
Nice, I didn't know about rdrNameAmbiguousFieldOcc!
src/Hint/Match.hs
Outdated
| @@ -201,7 +201,7 @@ checkSide x bind = maybe True bool x | |||
| isType "Compare" x = True -- Just a hint for proof stuff | |||
| isType "Atom" x = isAtom x | |||
| isType "WHNF" x = isWHNF x | |||
| isType "Wildcard" x = any isFieldPun (universeBi x) || any hasFieldsDotDot (universeBi x) | |||
| isType "Wildcard" x = any isFieldPun (universeBi x) || any hasFieldsDotDot (universeBi x) || any isFieldPunUpdate (universeBi x) | |||
There was a problem hiding this comment.
IMO isWildCard should just look for RecordWildCards, which is what hasFieldsDotDot is.
|
Thanks! |
Closes #1453