tweag / ormolu

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

Output some diff on "AST differs" errors #877

Closed brandonchinn178 closed 2 years ago

brandonchinn178 commented 2 years ago

Is your feature request related to a problem? Please describe. The current error shown when Ormolu formats things in a way that changes the AST is very opaque. It's perhaps intentionally so, as the error always indicates a bug on Ormolu's side, but it does cause a block in the immediate work waiting on a response after opening an issue. There is also no mention of using --unsafe to bypass it temporarily.

Describe the solution you'd like The error message already shows the lines affected. It would be great to:

  1. Keep the "Please report this as a bug" message
  2. Show a diff between the original version and formatting with --unsafe on the particular lines / show some representation of the AST that differs
    • Alternatively, mention --unsafe as a temporary solution

Describe alternatives you've considered

Additional context

amesgen commented 2 years ago

I agree that sth like this would be useful, and it should not be too difficult as we already have diffing functionality in Ormolu.

So far, I often used the "Show internal parse result" option of Ormolu Live to debug differing ASTs. It seems like this is only really useful for someone intending to contribute to Ormolu/Fourmolu, so maybe we can expect that they know how to look at the GHC AST in some way (e.g. -ddump-parsed-ast)?

There is also no mention of using --unsafe to bypass it temporarily.

I agree, seems reasonable to mention it as a stopgap measure.

mrkkrp commented 2 years ago

This is a great idea.

amesgen commented 2 years ago

Just for clarification: @brandonchinn178 writes above:

Show a diff between the original version and formatting with --unsafe on the particular lines / show some representation of the AST that differs

In #878, we only display the diff of the source code, but not of the AST. Depending on the context/type of bug, both can be useful. Any thoughts @mrkkrp?

mrkkrp commented 2 years ago

Hmm, good point. I somehow missed the AST bit. I am not sure what is the best way handle this. Perhaps we could pretty-print ASTs before/after as text and then use our existing diffing functions to present the result? Do you have other ideas?

amesgen commented 2 years ago

Perhaps we could pretty-print ASTs before/after as text and then use our existing diffing functions to present the result? Do you have other ideas?

Yeah, pretty-printing with showAstData like in Ormolu Live and then diffing would already be pretty useful I think.

Other than that, my only idea is to extract a Tree via the Data instance and then diff it via sth like treeDiff for more precision, but that would require significantly more work for unclear benefit.

brandonchinn178 commented 2 years ago

To be clear, I think anything would be better than the current situation. I don't have too strong a preference what to diff, but just to show something to make the error less opaque.

mrkkrp commented 2 years ago

@brandonchinn178 Can you try #878 and let me know what you think? Do you deem it "good enough"?

@amesgen I have to admit, I am hesitant to add AST diffing because

amesgen commented 2 years ago

Yeah, having only the textual diff is also completely reasonable IMO :+1:

brandonchinn178 commented 2 years ago

Great, thanks!