Should refactoring for "Use guard" be disabled?

[zliu41]

From a bug reported by @jneira in mpickering/apply-refact#93

The problem with "Use guard" refactoring is that it can lose comments, e.g.

-- input
bar x y =
  -- a comment
  if isJust x then "1"
  else if Prelude.null y then "2"
  -- another comment
  else "3"

-- output
bar x y
  | isJust x = "1"
  | Prelude.null y = "2"
  | otherwise = "3"

The comments are lost because they are originally attached to HsIf nodes which don't appear in the output.

In general, any hint that removes stuff can cause comments to be lost during refactoring, for instance

-- input
foo = f {- comment attached to <$> -} <$> {- comment attached to x -} x >>= g

-- output
foo =  {- comment attached to x -}x >>= g . f

But "Use guard" is particularly bad because it is not uncommon for people to write comments above "if" and "else". This also means it is probably not a good idea to add specific logic for "Use guard" to save the comments; rather, it should be solved in a principled way.

@ndmitchell do you think the refactoring for "Use guard" should be turned off because of this, or is it still worth having it?

[googleson78]

Is it possible to make hlint keep attached comments instead? i.e. are they available during parsing in the ast?

[ndmitchell]

Is there some automatic way to figure out when a refactoring would lose comments and disable it automatically? In the same way we automatically disable a hint if it causes additional free variables.

[zliu41]

Is there some automatic way to figure out when a refactoring would lose comments and disable it automatically?

Ah that's a good idea. I think this can be done in apply-refact without modiyfing HLint, so I'll just close this issue.

@googleson78 Yeah the comments are available in ApiAnns, so that should be possible. But I think it's not easy to implement, and would likely require modifying not only HLint but also apply-refact and refact.

[josephcsible]

In the same way we automatically disable a hint if it causes additional free variables.

What exactly do you mean by this?

[ndmitchell]

In the same way we automatically disable a hint if it causes additional free variables.

@josephcsible - for match hints, we run the hint, and if the RHS had new free variables that the LHS didn't have, we assume variables got captured/uncaptured or otherwise screwed up, and don't make the suggestion. I think the suggestion is analogous to that - try it, if it appears to have gone wrong, don't do it.