25

It is considered good practice to enable GHC warnings with -Wall. However, I've found out that fixing those warnings has a negative effect for some types of code constructs.

Example 1:

Using the do-notation equivalent of f >> will generate a warning if I don't explicitly use the _ <- f form:

Warning: A do-notation statement discarded a result of type Char.
         Suppress this warning by saying "_ <- f",
         or by using the flag -fno-warn-unused-do-bind

I understand that I can forget to do something with the result of f. However, it is legitimate to ignore the result (very common in parsers). There is no warning when using >>, right? Using _ <- is heavier than it should.

Example 2:

Naming a pattern variable with the same name of a visible function will give:

Warning: This binding for `map' shadows the existing binding
           imported from Prelude

This is getting worse when using record syntax as namespace gets polluted quickly. The solution is to give an alternate name in the pattern expression. So I end up using a less appropriate name just to avoid a warning. I don't feel it's a good-enough reason.

I know I can use -fno-warn-... options but should I stick with -Wall after all?

gawi
  • 13,940
  • 7
  • 42
  • 78
  • 1
    Can you give more examples on the effects of `-Wall`? Exhaustiveness checking for example seems worth the price, though I agree that, especially in the last point, the option complicates valid expressions. – Dario Nov 13 '10 at 20:59
  • 4
    'Example 1' has only recently become a warning and the change was not universally popular. People seem to add -fno-warn-unused-do-bind to their cabal files to disable it. See this mail thread: http://www.mail-archive.com/haskell-cafe@haskell.org/msg74079.html – stephen tetley Nov 13 '10 at 21:46
  • 2
    In the future please ask one question at a time. What if two answers appear, each answering one of them well. Which would you accept? – luqui Nov 14 '10 at 07:04
  • 2
    @luqui I used two examples to show that `-Wall` has an impact on style. I am not looking for an answer for each one. There really is one question: should I stick with `-Wall`. – gawi Nov 15 '10 at 01:43
  • @Dario I agree for exhaustive pattern check. I've already asked this quiestion: http://stackoverflow.com/questions/3804484/in-haskell-why-non-exhaustive-patterns-are-not-compile-time-errors – gawi Nov 15 '10 at 01:45

6 Answers6

25

Example 1:

I have re-learned to write parsers in Applicative style -- they are much more concise. Eg, instead of:

funCallExpr :: Parser AST
funCallExpr = do
    func <- atom
    token "("
    arg <- expr
    token ")"
    return $ FunCall func arg

I instead write:

funCallExpr :: Parser AST
funCallExpr = FunCall <$> atom <* token "(" <*> expr <* token ")"

But what can I say, if you don't like the warning, disable it as it suggests.

Example 2:

Yeah I find that warning a bit irritating as well. But it has saved me a couple times.

It ties into naming conventions. I like to keep modules pretty small, and keep most imports qualified (except for "notation" imports like Control.Applicative and Control.Arrow). That keeps the chances of name conflict low, and it just makes things easy to work with. hothasktags makes this style tolerable if you are using tags.

If you are just pattern matching on a field with the same name, you can use -XNamedFieldPuns or -XRecordWildCards to reuse the name:

data Foo = Foo { baz :: Int, bar :: String }

-- RecordWildCards
doubleBaz :: Foo -> Int
doubleBaz (Foo {..}) = baz*baz

-- NamedFieldPuns
reverseBar :: Foo -> String
reverseBar (Foo {bar}) = reverse bar

Another common convention is to add a hungarian prefix to record labels:

data Foo = Foo { fooBaz :: Int, fooBar :: String }

But yeah, records are no fun to work with in Haskell. Anyway, keep your modules small and your abstractions tight and this shouldn't be a problem. Consider it as a warning that says simplifyyyy, man.

luqui
  • 59,485
  • 12
  • 145
  • 204
  • 1
    Yes, I definitively want to _evolve_ to applicative parsing. I'm still a bit too green to jump. – gawi Nov 15 '10 at 01:47
  • I tend to use whole words instead of letters or abbrev for variable in pattern matching expression. I believe this increase the risk of conflicts. Using qualified imports may be useful in my case. – gawi Nov 15 '10 at 01:55
  • 2
    I'm coming from the OOP world so using record syntax is something natural for me. It's not clear for me how to rewire my brain to do things otherwise. Maybe that will be another question for StackOverflow some day... – gawi Nov 15 '10 at 02:01
10

I think that use of -Wall may lead to less readable code. Especially, if it is doing some arithmetics.

Some other examples, where the use of -Wall suggests modifications with worse readability.

(^) with -Wall requires type signatures for exponents

Consider this code:

norm2 x y = sqrt (x^2 + y^2)
main = print $ norm2 1 1

With -Wall it gives two warnings like this:

rt.hs:1:18:
    Warning: Defaulting the following constraint(s) to type `Integer'
             `Integral t' arising from a use of `^' at rt.hs:2:18-20
    In the first argument of `(+)', namely `x ^ 2'
    In the first argument of `sqrt', namely `(x ^ 2 + y ^ 2)'
    In the expression: sqrt (x ^ 2 + y ^ 2)

Writing (^(2::Int) everywhere instead of (^2) is not nice.

Type signatures are required for all top-levels

When writing quick and dirty code, it's annoying. For simple code, where there are at most one or two data types in use (for exapmle, I know that I work only with Doubles), writing type signatures everywhere may complicate reading. In the example above there are two warnings just for the lack of type signature:

rt.hs:1:0:
    Warning: Definition but no type signature for `norm2'
             Inferred type: norm2 :: forall a. (Floating a) => a -> a -> a
...

rt.hs:2:15:
    Warning: Defaulting the following constraint(s) to type `Double'
             `Floating a' arising from a use of `norm2' at rt.hs:2:15-23
    In the second argument of `($)', namely `norm2 1 1'
    In the expression: print $ norm2 1 1
    In the definition of `main': main = print $ norm2 1 1

As a distraction, one of them refers to the line different from the one where the type signature is needed.

Type signatures for intermediate calculations with Integral are necessary

This is a general case of the first problem. Consider an example:

stripe x = fromIntegral . round $ x - (fromIntegral (floor x))
main = mapM_ (print . stripe) [0,0.1..2.0]

It gives a bunch of warnings. Everywhere with fromIntegral to convert back to Double:

rt2.hs:1:11:
    Warning: Defaulting the following constraint(s) to type `Integer'
             `Integral b' arising from a use of `fromIntegral' at rt2.hs:1:11-22
    In the first argument of `(.)', namely `fromIntegral'
    In the first argument of `($)', namely `fromIntegral . round'
    In the expression:
            fromIntegral . round $ x - (fromIntegral (floor x))

And everyone knows how often one needs fromIntegral in Haskell...

There are more cases like these the numeric code risks to become unreadable just to fulfill the -Wall requirements. But I still use -Wall on the code I'd like to be sure of.

sastanin
  • 40,473
  • 13
  • 103
  • 130
  • defaulting is horrible, but yes, having to deal with it is horrible too. I don't think that fixing these issues makes code unreadable, just slightly more noisy (but clear). – sclv Nov 15 '10 at 21:12
  • 1
    I sometimes define monomorphic versions of the functions I use, to avoid specifying types elsewhere. – sastanin Nov 15 '10 at 21:27
7

I would recommend continuing to use '-Wall' as the default option, and disable any checks you need to on local, per-module basis using an OPTIONS_GHC pragma at the top of relevant files.

The one I might make an exception for is indeed '-fno-warn-unused-do-bind', but one suggestion might be to use an explicit 'void' function ... writing 'void f' seems nicer than '_ <- f'.

As for name shadowing - I think it's generally good to avoid if you can - seeing 'map' in the middle of some code will lead most Haskellers to expect the standard library fn.

BenMos
  • 216
  • 2
  • 4
6

Name shadowing can be quite dangerous. In particular, it can become difficult to reason about what scope a name is introduced in.

Unused pattern binds in do notation are not as bad, but can indicate that a less efficient function than necessary is being used (e.g. mapM instead of mapM_).

As BenMos pointed out, using void or ignore to explicitly discard unused values is a nice way to be explicit about things.

It would be quite nice to be able to disable warnings for just a section of code, rather than for everything at once. Also, cabal flags and command line ghc flags take precedence over flags in a file, so I can't have -Wall by default everywhere and even easily just disable it for the entirety of a single file.

sclv
  • 38,665
  • 7
  • 99
  • 204
  • This happens always when I want to use `printf`. There's no way to stop this warning. But a function `void = (>> return ())` would be very good. – fuz Nov 15 '10 at 05:49
  • 3
    `void` is 4 characters long. `_ <-` is 4 character long also. The former requires an import while the latter is supported by the syntax. `_ <-` is already a common idiom. If I had to choose between `_ <-` and `void`, I would prefer `_ <-` – gawi Nov 15 '10 at 14:43
4

All these warnings help to prevent mistakes and should be respected, not suppressed. If you want to define a function with a name from Prelude, you can hide it using

import Prelude hiding (map)

'Hiding' syntax should only be used for Prelude and modules of the same package, otherwise you risk code breakage by API changes in the imported module.

See: http://www.haskell.org/haskellwiki/Import_modules_properly

Lemming
  • 41
  • 1
4

There is also the much less intrusive -W option, which enables a set of reasonable warnings mostly related to general coding style (unused imports, unused variables, incomplete pattern matches, etc.).

In particular it does not include the two warnings you mentioned.

ertes
  • 41
  • 1