Repository navigation
#1637, add a typeApplications option for restricted functions - #1699
Conversation
|
Gentle ping. How would I go about getting this merged? |
|
CC @ndmitchell I'm not sure who to ping. |
zliu41
left a comment
There was a problem hiding this comment.
A name can appear in different places like declarations, patterns, type signatures etc. Need to test that the hint is only triggered when it should - the current tests aren't sufficient.
{name: fromIntegral, typeApplications: 2}
Does this allow fromIntegerl @_ @_? Either way, need test coverage.
| - {name: Just, typeApplications: required} | ||
| - {name: Left, typeApplications: forbidden} | ||
| - {name: Data.Map.fromList, typeApplications: required} | ||
| - {name: Data.Set.fromList, typeApplications: forbidden} |
There was a problem hiding this comment.
What happens if one of the two fromList is unqualified?
There was a problem hiding this comment.
Both rules matched and the merge decided it, so declaration order had no effect at all. Same root cause as your Semigroup comment, so it's fixed the same way: the last-declared rule now wins.
With Data.Map.fromList: required then fromList: forbidden, an unqualified fromList from Data.Map ends up forbidden from carrying one; swap the order and it's required. Both directions are pinned in tests/type_applications.test, along with the qualified-vs-qualified pair.
| TypeAppRequired a <> TypeAppRequired b = TypeAppRequired (max a b) | ||
| TypeAppForbidden <> TypeAppForbidden = TypeAppForbidden | ||
| -- If a function is both required and forbidden to carry a type application | ||
| -- (e.g. via overlapping rules), requiring one wins. |
There was a problem hiding this comment.
This is quite arbitrary. How about whichever rule declared later wins?
There was a problem hiding this comment.
Agreed, and done.
It needed a little more than swapping the operator: restrictions builds a Map and findFunction sconcats Map.elems, so by merge time the declaration order was gone entirely. Rules now carry their position in the settings and the payload is Maybe (Max (Arg Int RestrictTypeApp)), so the latest wins across both the same-key union and the cross-module sconcat. The ad-hoc Semigroup RestrictTypeApp is gone. Tested in both declaration orders.
6acab78 to
cbd3c6a
Compare
|
Thanks, both were real. Non-call positions. You were right that the tests weren't sufficient. It was firing on top-level type signatures, class method signatures, record field declarations and argument binders, and on a further sweep also on infix operators, sections, infix constructor patterns and record-syntax constructor patterns. None of those can carry a type application, so the advice couldn't be followed. The check now works off a positive set of sites — expression heads and prefix constructor patterns — instead of every One case I left firing and documented rather than fixed: a locally bound name shadowing a restricted one, since HLint doesn't resolve local binders. That matches the existing
|
|
@tomjaguarpaw this is the implementation of the haskell/core-libraries-committee#314; which you asked to be CC-ed on. |
|
Gentle ping |
|
Please resolve conflicts |
172e096 to
1601ad0
Compare
Reviving ndmitchell#1667 (closed stale). A function restriction may now set 'typeApplications' to require or forbid visible type applications at use sites, so a silent change to an inferred type cannot change behaviour unnoticed. - functions: - {name: show, typeApplications: required} # >= 1 type argument - {name: fromIntegral, typeApplications: 2} # >= N type arguments - {name: id, typeApplications: forbidden} # none 'required' demands at least one type argument, an integer N demands at least N (so fromIntegral, which needs both type variables fixed, can require two), and 'forbidden' demands none. It also applies to data constructors in patterns. Only positions that can carry a visible type application are checked: the head of an expression and a constructor in a prefix pattern. A name in a type signature, a class method signature, a record field declaration or a binder is never flagged, and neither is an operator used infix or in a section, or a constructor pattern in record or infix form, since the advice cannot be followed there. Parenthesised, as in (<+>), the operator is back in prefix position and is still checked. A '@_' argument does not count towards 'required', since it leaves the type just as inferred as writing no type argument at all, so 'fromIntegral @_ @_' is flagged under 'typeApplications: 2'. It does count towards 'forbidden', which asks that no visible type application is written at all. When several rules restrict one name's type applications, the last-declared one wins. Merging them would be arbitrary: a name cannot sensibly both require and forbid a type application.
1601ad0 to
1ddb17c
Compare
|
❤️ |
Revives #1667 (closed stale), addressing #1637.
Function restrictions gain a
typeApplicationsfield, so a silent change to an inferred type can't change behaviour unnoticed:Also applies to constructors in prefix patterns (
Just @Int x).Semantics
@_doesn't count towardsrequired, since it leaves the type as inferred as writing nothing —fromIntegral @_ @_is still flagged. It does count towardsforbidden.Testing
hlint --testpasses.tests/type_applications.testcovers required/forbidden/count, constructor patterns, each non-call position above, wildcards in both modes, and conflicting rules in both declaration orders.Changes since review
Per @zliu41's review:
@_no longer counts towardsrequired, sofromIntegral @_ @_is flaggedfromListcase in both declaration ordersWritten by Claude, reviewed and signed-off on by @NorfairKing