Stop unsafely binding a new variable

[josephcsible]

Currently, hlint will suggest turning zipWith (+) xs (repeat x) into map (\ x -> (+) x x) xs, which is wrong. The problem is that it's not safe to bind a new variable and use user-provided expressions in the same scope. This change results in it suggesting the correct map (+ x) xs instead.

[ndmitchell]

Good catch!

f is an arbitrary expression, so I don't think that works. You probably need a side condition that ``x notIn ...` something - I also can't remember what the membership predicate is.

[josephcsible]

It looks to me like hlint will automatically make a lambda if f is more complicated than a variable. Do you know of any specific cases where the hint as written in this PR won't work?

[josephcsible]

Also, I don't understand why the test is still failing after my second commit. Can you take a look at that?

[ndmitchell]

I had no idea that HLint would automatically make a lambda if the variable was too complicated. Can we add a test for that? Something like:

# foo = zipWith (SymInfo q) [0 ..] (repeat ty) -- map (\ x -> SymInfo q x ty) [0 ..]

The test above is failing because of refactoring not working properly, cc @zliu41 - I think this is the only instance we're aware of where refactoring is broken for an LHS/RHS hint. To suppress it for now add @NoRefactor to the test. The message it gives is:

INPUT: foo = zipWith SymInfo [0 ..] (repeat ty)
Refactor output is expected to contain: map (`SymInfo` ty) [0 .. ]
Actual: foo = map (SymInfo ty) [0 ..]

[zliu41]

Yeah this is a bug in apply-refact. When a substitution variable (f in this case) is used as infix operator, the backticks are dropped after substitution. I raised mpickering/apply-refact#89. For now as @ndmitchell suggested please add @NoRefactor at the end of the line that contains the test case.

[josephcsible]

# foo = zipWith (SymInfo q) [0 ..] (repeat ty) -- map (\ x -> SymInfo q x ty) [0 ..]

Annoyingly, the actual RHS you get from that is map (( \ x_ -> SymInfo q x_ ty)) [0 .. ]. Should I just add those redundant parentheses to the test, or should we hold off on this test until we can make them go away? Also, do we care that it uses x_ instead of x?

[ndmitchell]

Don't really mind that it's x_, although x would have been nicer. The brackets seem like a bug. I'm going to merge this and fix up locally.

[ndmitchell]

I ended up fixing the test suite by copying over the dubious result with the excess brackets. I've also raised #1132 which properly explains the issue, why, and how we might fix it. Might be a bit of work so can be lower priority.