Enable more warnings in purescript.cabal#4336
Conversation
This enables -Wincomplete-uni-patterns and -Wincomplete-record-updates
by default. They are both good warnings to have on, as they help
identify situations where code is likely to result in an error at
runtime.
-Wincomplete-uni-patterns causes a warning to be emitted for things like
this:
head = \(x:_) -> x
which would blow up at runtime if given an empty list.
-Wincomplete-record-updates causes a warning to be emitted for code like
this:
data Foo = Foo { x :: Int }
| Bar
f :: Foo -> Foo
f foo = foo { x = 6 }
which will blow up if you try to call `f Bar`, since only the `Foo`
constructor has the `x` field. (This one isn't triggered by any of our
code since we generally avoid incomplete record updates anyway).
The reason I'm sending this now is that as of version 9.2.1, GHC
includes these two warnings as part of -Wall by default, which means
that in order to upgrade, we'd need to either rework our code to stop
emitting them, or explicitly turn them off. The former seems more
appealing to me.
Another nice consequence of this is that some functions become more
efficient, as we remove unnecessary extra traversals of lists in a
couple of cases.
| -- tails1 (fromList [1]) == fromList [fromList [1]] | ||
| -- tails1 (fromList [1,2]) == fromList [fromList [1,2], fromList [2]] | ||
| -- tails1 (fromList [1,2,3]) == fromList [fromList [1,2,3], fromList [2,3], fromList [3]] | ||
| tails1 :: NonEmpty a -> NonEmpty (NonEmpty a) |
There was a problem hiding this comment.
I opened haskell/core-libraries-committee#67 to see if we can get this into base.
| ImportRecord (Qualified (Just mnNew) _) mnOrig _ _ -> | ||
| return (mnNew, mnOrig) | ||
| _ -> | ||
| internalError "checkImportConflicts: ImportRecord should be qualified" |
There was a problem hiding this comment.
This probably won't ever happen, but if it does, we'll get a much more useful error.
| let (x, a : y) = splitAt i l | ||
| in x ++ [t a] ++ y | ||
| updateAtFirstOrPrepend predicate update def xs = | ||
| case break predicate xs of |
There was a problem hiding this comment.
This has the exact same behaviour as before. Using break here removes an unnecessary list traversal.
| go jss | not (any isReturn jss) = jss | ||
| | otherwise = let (body, ret : _) = break isReturn jss in body ++ [ret] | ||
| go jss = | ||
| case break isReturn jss of |
There was a problem hiding this comment.
This has the exact same behaviour as before; not (any isReturn xs) is just as expensive to compute as break isReturn xs, so we might as well do the latter since that also gives us access to the information we need here in case there is a return statement.
|
Just in case you aren't aware, I removed myself from the core team on GitHub so I'm no longer able to merge things by myself, even with approvals - please merge this for me when you're ready. |
This enables -Wincomplete-uni-patterns and -Wincomplete-record-updates
by default. They are both good warnings to have on, as they help
identify situations where code is likely to result in an error at
runtime.
-Wincomplete-uni-patterns causes a warning to be emitted for things like
this:
which would blow up at runtime if given an empty list.
-Wincomplete-record-updates causes a warning to be emitted for code like
this:
which will blow up if you try to call
f Bar, since only theFooconstructor has the
xfield. (This one isn't triggered by any of ourcode at the moment, thankfully).
The reason I'm sending this now is that as of version 9.2.1, GHC
includes these two warnings as part of -Wall by default, which means
that in order to upgrade, we'd need to either rework our code to stop
emitting them, or explicitly turn them off. The former seems more
appealing to me.
Another nice consequence of this is that some functions become more
efficient, as we remove unnecessary extra traversals of lists in a
couple of cases.
Checklist: