Avoid unnecessary zip since it walks the list twice and breaks fusion

[josephcsible]

No description provided.

[josephcsible]

Wait, this will have the same problem that I fixed in #1131. Is there a way that can be fixed in hlint itself to do the right thing in this case, or do I really have to use traverse (fmap . (,) <*> f) x as the RHS?

[ndmitchell]

The RHS traverse (fmap . (,) <*> f) x seems meaningfully more complicated than the LHS, which doesn't make it a great hint.

Does it have the same problem? I thought the condition to cause that bug was having a substitution variable in backticks, which I don't see in your examples. Can you confirm they don't have that issue, and perhaps add a test to check they don't?

As to the hints, the zip/map ones look reasonable. The traverse one looks fairly complex though - it's doing a lot of complication to avoid that zip, and in a way that looks harder to read. I would be tempted to prefer the LHS for readability over the RHS.

[josephcsible]

I thought the condition to cause that bug was having a substitution variable in backticks, which I don't see in your examples. Can you confirm they don't have that issue, and perhaps add a test to check they don't?

I think you're thinking of a different issue. This issue is that if you use a variable from the LHS (in this case f) in a scope where you bound a new variable (in this case y), the new variable won't ever be renamed, resulting in incorrect hints like suggesting that zip xs (map y xs) be changed to map (\y -> (y, y y)) xs. I just checked, and all three of these new hints do have this problem as of HLint 3.2.3.

[ndmitchell]

Hmm - do we have a tracking issue for that? If not, should we create one? And I guess these hints get blocked behind that?

[josephcsible]

Opened #1192. And our choices are either (1) these hints get blocked behind it, or (2) we add these pointfree forms for now instead, and then switch them back to how they are now once it's fixed:

    - warn: {lhs: zip x (map f x), rhs: "map ((,) <*> f) x"}
    - warn: {lhs: zip (map f x) x, rhs: "map ((,) =<< f) x"}
    - warn: {lhs: zip x <$> traverse f x, rhs: "traverse (fmap . (,) <*> f) x"}

[ndmitchell]

Thanks! I don't think the point-free versions are as clear, so I think we should wait for #1192.