tweag / ormolu

A formatter for Haskell source code
https://ormolu-live.tweag.io
Other
958 stars 83 forks source link

Diff displays hunks incorrectly in some cases #886

Closed mrkkrp closed 2 years ago

mrkkrp commented 2 years ago

When diffing these two inputs:

testPermParser :: Permutation Parser String
testPermParser =
  f <$> toPermutationWithDefault 'x' (char 'a')
    <*> toPermutationWithDefault 'y' (char 'b')
    <*> toPermutationWithDefault 'z' (char 'c')
  where
    f a b c = [a, b, c]

and

testPermParser :: Permutation Parser String
testPermParser =
  f
    <$> toPermutationWithDefault 'x' (char 'a')
    <*> toPermutationWithDefault 'y' (char 'b')
    <*> toPermutationWithDefault 'z' (char 'c')
  where
    f a b c = [a, b, c]

The result is

@@ -148,5 +148,6 @@
  testPermParser :: Permutation Parser String
  testPermParser =
-   f <$> toPermutationWithDefault 'x' (char 'a')
+   f
+     <$> toPermutationWithDefault 'x' (char 'a')
    where
      f a b c = [a, b, c]

which is not correct. Instead of the line with where it is the line with <*> toPermutationWithDefault 'y' (char 'b') that should follow. Proper diff reports:

@@ -147,7 +147,8 @@ prsp' p = prs' (runPermutation p)

 testPermParser :: Permutation Parser String
 testPermParser =
-  f <$> toPermutationWithDefault 'x' (char 'a')
+  f
+    <$> toPermutationWithDefault 'x' (char 'a')
     <*> toPermutationWithDefault 'y' (char 'b')
     <*> toPermutationWithDefault 'z' (char 'c')
   where

We should match that.

This seems to have been broken ever since I introduced the functionality for nice displaying of diffs.

mrkkrp commented 2 years ago

I'm surprised that this did not come up earlier and I did not notice it before...

amesgen commented 2 years ago

I'm surprised that this did not come up earlier and I did not notice it before...

Personally, I think I never looked at the diffs produced by Ormolu more than in a binary "oh, it is not yet formatted" fashion, but rather used the diffing of the git project I was formatting afterwards for inspection. One possibility would be to just shell out to git diff if available, and don't display diffs otherwise, but the diff printing code in Ormolu is not that large/complex that this seems like a priority.

mrkkrp commented 2 years ago

git diff approach is what was used before. I think there is a problem with it though, git diffing and Ormolu diffing are not always the same, in particular, they are not the same in the presence of intermediate editing. Actually IIRC that was the reason for adding built-in diffing in Ormolu: if I wanted to check on CI with Ormolu I had to use git diff, but my script also did some other editing and it interfered with the results.

amesgen commented 2 years ago

One could use git diff --no-index to just use the diffing part and ignoring any actual git-related functionality.