tweag/ormolu

Normalize parens

mboes opened this issue · 8 comments

mboes commented

A common deviation in styles is the following:

foo :: (Show a) => a -> IO ()
bar :: Show a => a -> IO ()

There are also contrived examples, where extra parens cannot possibly help readability:

baz :: ((Show a)) => a -> IO ()

Some might argue that constraints should always have parens around them, even singleton constraints, because it's more regular and less work when additional constraints have to be added to the set. Others prefer to always write the minimal number of lexemes. Point is, this is a common matter of style, that could be normalized away.

We already put parens around constraints in deriving clauses, so it would be consistent to do it here as well.

I just want to mention that the problem with removing parentheses is that it changes the AST and since we are checking that AST stays the same before/after formatting, we'd need to be clever about checking too.

Just as an additional data point, prettier (in JavaScript land) defaults to adding redundant parens around one thing, because it makes it easier for a human to add a second thing: https://prettier.io/docs/en/options.html#arrow-function-parentheses

At first glance, avoiding parentheses may look like a better choice because of less visual noise. However, when Prettier removes parentheses, it becomes harder to add type annotations, extra arguments or default values as well as making other changes. Consistent use of parentheses provides a better developer experience when editing real codebases, which justifies the default value for the option.

This is a very good point.

Just as an additional data point, prettier (in JavaScript land) defaults to adding redundant parens around one thing, because it makes it easier for a human to add a second thing

I originally supported the idea that (Show a) => should format to Show a =>, but this has convinced me that I'd really rather Show a => be formatted to (Show a) =>. I frequently find myself annoyed that I have to wrap some constraint in parenthesis first, and I'd much rather take the tiny bit of ugliness of extra parens over this extra editing busywork.

One downside I haven't seen mentioned (just ran into this): This falls afoul of an hlint rule. For example:

foo :: (HasCallStack) => ()
foo = ()
MyModule.hs:5:8-21: Warning: Redundant bracket
Found:
  (HasCallStack)
Perhaps:
  HasCallStack

1 hint

Thus if you run default hlint and ormolu on the same code, one of them will complain. I don't feel strongly regarding this new rule, but I felt this issue at least merits a comment for visibility. Luckily, hlint is accommodating:

-- entire module, after imports
{-# ANN module "HLint: ignore Redundant bracket" #-}

-- OR: {-# ANN foo "HLint: ignore Redundant bracket" #-}
foo :: (HasCallStack) => ()
foo = ()

Perhaps this should be documented somewhere.

That is a pretty unfortunate one, because that's a very useful hint at the expression level. I wonder if HLint could be taught to allow this for constraints somehow, too.

Yes, remedying this isn't as trivial as I thought (brackets on type class methods are tricky), so I would welcome a blanket rule (e.g. "Redundant constraint brackets"). That hlint already does not error for redundant brackets on deriving clauses adds some motivation imo.