PatternWildCard hint

[stephenjudkins]

This scratches an itch for our particular codebase, but I think it's a general rule that might be pretty useful to apply in the right places.

By raising this pull request you confirm you are licensing your contribution under all licenses that apply to this project (see LICENSE) and that you have no patents covering your contribution. ✅

If you care, my PR preferences are at https://github.com/ndmitchell/neil#contributions, but they're all guidelines, and I'm not too fussy - you don't have to read them. ✅

[ndmitchell]

Thanks for the patch. What's the harm of using a wildcard in a pattern match? What do you suggest people do instead?

[stephenjudkins]

Well, in general it's not a problem so I chose to default it to "ignore". But we have a specific case where virutally every instance of discarding part of a data constructor has been a bug. I can imagine there are other contexts where this is the case.

I had planned to write a bespoke script to lint our codebase for just this, but a coworker suggested upstreaming it to hlint instead. If you don't think this is generally applicable enough for that to be appropriate, I apologize for the trouble.

(I can't tell what the CI issue is; the tests appeared to pass locally. Is there anything I can do to address this or is it unrelated to my change?)

[m5]

Hi Neil! I work with Stephen at Mercury, and wanted to take a moment to talk about the particular use case we have in mind.

There's this pattern in Yesod, where you write an isAuthorized function that takes a route as an argument, and returns whether the current user is allowed to access the route.

So we end up with case statements along the lines of

case route of
  CreatePostR blogId -> blogBelongsToCurrentUser blogId
  EditPostR blogId articleId -> blogBelongsToCurrentUser blogId && articleBelongsToBlog blogId articleId
  ...

These routes end up chock-full of object ids that each really need to be validated.

If we ever see an entry pop up in those case statements with a hole, eg

  DeletePostR blogId _ -> blogBelongsToCurrentUser blogId

we can bet that's going to be a security vulnerability. Somebody forgot to check if the article belongs to the blog, and an attacker is going to be able to delete posts on any blog so long as they know the article id.

So, we've been manually enforcing a policy that we don't allow holes in these case statements. All path parameters should be authorized inside these case statements. If a path parameter really doesn't need to be validated, we ask people to give it a meaningful name, like _status or whatever, and leave a comment.

Stephen's written this PR up with the hope that we can use hlint to help remind people to follow that convention. I wish I had a couple other use cases in mind to help sell you on the feature, but it will at minimum be super valuable for us for this particular use case.

[9999years]

This PR doesn't compile on GHC 9.6 yet. Here's a patch to enable support:

--- a/src/Hint/PatternWildCard.hs
+++ b/src/Hint/PatternWildCard.hs
@@ -1,3 +1,4 @@
+{-# LANGUAGE CPP #-}
 {-
     Warn against wildcards in pattern
 
@@ -22,7 +23,11 @@ patternWildCardHint :: DeclHint
 patternWildCardHint _ _ code = concatMap inspectCode $ childrenBi code
 
 inspectCode :: LHsExpr GhcPs -> [Idea]
+#if __GLASGOW_HASKELL__ >= 906
+inspectCode (L _ ((HsCase _ _ (MG _ (L _ cases))))) = concatMap inspectCase cases
+#else
 inspectCode (L _ ((HsCase _ _ (MG _ (L _ cases) _)))) = concatMap inspectCase cases
+#endif
 inspectCode o = concatMap inspectCode $ children o
 
 inspectCase :: LMatch GhcPs (LHsExpr GhcPs) -> [Idea]

[stephenjudkins]

Thanks @9999years!

[parsonsmatt]

Is there anything we can do to help get this merged?