category feedStatic analyzer wishlistWishlistseditdelete

This category is a work in progress






Collecting common mistakes in code, feature requests for static analyzers, etc.

edit description
or press Ctrl+Enter to savemarkdown supported
#
Dead code elimination
other
move item up move item down edit item info delete item
Summary edit summary

A common pattern when developing big Haskell applications is “library + executable” (with the library not being used anywhere other than the executable). Thus, it often happens that functions (and sometimes whole modules) are not used in the final executable, but GHC doesn't report them as unused because they are still exported from the library. It'd be nice to be able to get a list of such functions.

Note: the weeder tool already seems to be doing it.

Summary quit editing summary
#
Weird range comparison
possible bug
move item up move item down edit item info delete item
Summary edit summary

Instead of the right 3 < a && a < 5:

if a < 3 && a > 5
if a > 5 && a < 3
...
Summary quit editing summary
#
printf arguments
possible bug
move item up move item down edit item info delete item
Summary edit summary

It's often possible to detect in compile-time when printf or format (from text-format) is used with the wrong number of arguments:

printf "%s %s" a b c
Summary quit editing summary
#
Copypaste
possible bug
move item up move item down edit item info delete item
Summary edit summary
  • Using the same expression with an operator:

    a == a    a /= a    a + a    a - a    a && a    a || a    ...
  • Duplicated expressions in a list:

    [a..a]    [a, a..]
  • Duplicated branches in an if:

    if ... then A else A
  • Very similar but different bindings:

    -- in one function
    chainLength = blockSecurityParam * (length participants - 1)
    
    -- in another function
    chainLength = blockSecurityParam * (length participants + 1)
Summary quit editing summary
#
Discarding data and then using it
possible bug
move item up move item down edit item info delete item
Summary edit summary

The following code usually doesn't make sense because having a single () is rarely useful:

xs <- replicateM_ ...
ys <- forM_ ...

Having a list of ()s isn't useful either:

xs <- replicateM 10 {- something that returns () -}
Summary quit editing summary
#
Having outdated computations in comments
possible bug
move item up move item down edit item info delete item
Summary edit summary

Often people are afraid that the compiler won't simplify some arithmetic expression, and they calculate it, write the answer, and leave the expression in comments. Then someone updates the comment but forgets to recalculate the expression (or vice versa). Here the correct answer is 191, but the number written is 187:

size :: Int
size = 187       -- 4*(28+16) + 15
Summary quit editing summary
#
Floated-out lists
possible perf issue
move item up move item down edit item info delete item
Summary edit summary

GHC will needlessly share [1..10000000] between two computations and cause a memory leak here:

foo = for_ [1..10000000] $ \i ->
  ...

bar = for_ [1..10000000] $ \k ->
  ...

The solution is to use something like loop.

Summary quit editing summary
#
Using (</>) with URLs
possible bug
move item up move item down edit item info delete item
Summary edit summary

This will fail on Windows because </> will append "\", not "/":

foo = "https://example.com/get" </> show a
Summary quit editing summary
#
Using 'pure' or 'return' in the middle of a function
possible bug
move item up move item down edit item info delete item
Summary edit summary

An inexperienced developer might write this, thinking that return means “early exit”. It doesn't.

main = do
  ...
  when p $ return ()
  ...
Summary quit editing summary
#
Weird boolean computations
possible bug
move item up move item down edit item info delete item
Summary edit summary

These are often copypaste errors:

a || (a && b)    ==>    a
a || not a       ==>    True
a && (a || b)    ==>    a
a && not a       ==>    False

Even if not, they still could be simplified.

Summary quit editing summary
#
Using Data.ByteString.Char8.pack
possible bug
move item up move item down edit item info delete item
Summary edit summary

Using BS8.pack is almost always a mistake, because it messes up any Unicode characters. Instead you probably want to encode the string into UTF-8 (with e.g. utf8-string or text).

Summary quit editing summary
#
Modifying state in MonadBaseControl
possible bug
move item up move item down edit item info delete item
Summary edit summary

When you use e.g. bracket from lifted-base, and you do anything with State, Writer, etc in the release action, it will be discarded. Thus, when a release action explicitly modifies state, it's likely a mistake and the developer simply doesn't know that it's not safe to do. An example:

bracket ... (\a -> put (a, 0)) $ do
  ...
Summary quit editing summary
#
Potential infinite recursion
possible bug
move item up move item down edit item info delete item
Summary edit summary

Here's a case where an infinite loop could happen because the n < 0 case wasn't handled:

f :: Int -> something
f n = ... f (n-1) ...

In some cases it's going to be a false positive, but it's still better to report such cases.

Summary quit editing summary
#
Warn about unexpectedly slow functions
pie-in-the-sky
move item up move item down edit item info delete item
Summary edit summary

Data.HashMap.(Strict|Lazy).size and Data.HashSet.size are O(n), which is surprising to many people because corresponding functions for ordinary maps and sets are fast. It would be good to detect when it can likely lead to performance problems and emit a warning.

Data.Text.index and Data.Text.length are in the same category, by the way.

Summary quit editing summary
#
'length', 'null', etc applied to tuples
possible bug
move item up move item down edit item info delete item
Summary edit summary

You almost never want to apply length or null (and most other Foldable methods) to tuples because they treat tuples as one-element containers. So, when an application of one of those methods is detected, it's likely a bug.

Summary quit editing summary
#
Buggy serialization instances
possible bug
move item up move item down edit item info delete item
Summary edit summary

Binary instances are usually written like this:

data Foo = Foo {this, that, other :: Int}

instance Binary Foo where
  put Foo{..) = put that >> put this >> put other
  get = Foo <$> get <*> get <*> get

Here the order was mixed-up: we serialize that, this, other, but deserialize this, that, other (this and that are swapped).

Another, similar, bug is forgetting to serialize one of the fields altogether.

Summary quit editing summary
#
Writing into 'ix'
possible bug
move item up move item down edit item info delete item
Summary edit summary

Sometimes people forget that if they write into ix from lens, the corresponding element does not get created if it didn't exist already. Thus, this (and variants of this) are wrong:

do absent <- isNothing <$> preuse (ix k)
   when absent $ ix k .= blah
Summary quit editing summary
#
[0..length xs]
possible bug
move item up move item down edit item info delete item
Summary edit summary

People often write [0..length xs] when they intend to write [0..length xs - 1].

Summary quit editing summary
#
Infinite loops in instances
possible bug
move item up move item down edit item info delete item
Summary edit summary

This is almost always a mistake (and unfortunately it's more common than it might seem):

instance Foo Int where 
  foo = foo

It should also be possible to detect a trickier variant of this mistake. Here f is a function (or chain of functions) that doesn't change the type:

instance Foo Int where
  foo = f . foo
instance Foo Int where
  foo = foo . f
Summary quit editing summary