Better reporting on parsing error caused by idempotency problems

[Niols]

Is your feature request related to a problem? Please describe.

Consider the following piece of code:

let id = if true then Fun.id else function x -> x

It is valid OCaml code. However, running latest Topiary reports a parsing error:

$ nix run github:tweag/topiary/cba1993 -- -l ocaml <<EOF
let id = if true then Fun.id else function x -> x
EOF
[2023-06-22T18:18:45Z ERROR topiary] Parsing error between line 1, column 46 and line 1, column 47

The error comes in fact from the fact that the first pass of Topiary produces an invalid file (cf #529) and the second pass then fails parsing it. In this case, the location is somewhat correct but in other situations the line numbers might be completely wrong (if previous lines were affected by the first pass of formatting), leaving a user clueless.

Describe the solution you'd like

I want an error message that explains that it is an idempotency error and that it is not the user's fault and that it should be reported.

Additional context

If I am not mistaken, v0.1.0 used to do this?

[torhovland]

v0.1.0

❯ echo 'let id = if true then Fun.id else function x -> x' | cargo run -- --language ocaml
let id = if true then Fun.id else function x -> x

v0.2.0 - v0.2.2

❯ echo 'let id = if true then Fun.id else function x -> x' | cargo run -- --language ocaml -s
let id = if true then Fun.id else functionx -> x

❯ echo 'let id = if true then Fun.id else function x -> x' | cargo run -- --language ocaml
[2023-06-26T11:50:01Z ERROR topiary] Parsing error between line 1, column 1 and line 1, column 49

v0.2.3 - master

❯ echo 'let id = if true then Fun.id else function x -> x' | cargo run -- --language ocaml -s
let id = if true then Fun.id else functionx -> x

❯ echo 'let id = if true then Fun.id else function x -> x' | cargo run -- --language ocaml
[2023-06-26T11:43:04Z ERROR topiary] Parsing error between line 1, column 46 and line 1, column 47

I'd really like to test how idempotency errors got reported pre v0.2.0, but I don't know how to reproduce that.

[Niols]

I'd really like to test how idempotency errors got reported pre v0.2.0, but I don't know how to reproduce that.

Me too. I tried but didn't manage.

[torhovland]

I can force it by adding this:

(
  (let_binding) @append_delimiter
  (#delimiter! ".")
)

Then I get:

v0.1.0

❯ echo 'let id = if true then Fun.id else function x -> x' | cargo run -- --language ocaml   
[2023-06-26T11:59:38Z ERROR topiary] Failed idempotence check
[2023-06-26T11:59:38Z ERROR topiary] Diff < left / right > :
    <let id = if true then Fun.id else function x -> x.
    >let id = if true then Fun.id else function x -> x. .
     
    
[2023-06-26T11:59:38Z ERROR topiary] The formatter did not produce the same result when invoked twice (idempotence check).
    It would be helpful if you logged this error at https://github.com/tweag/topiary/issues/new?assignees=&labels=type%3A+bug&template=bug_report.md

Newer versions behave as above. So the idempotency reporting has definitely regressed.

[Niols]

This is not an idempotency issue caused by an unparseable production in the first pass though, is it?

[torhovland]

Actually, no, it hasn't regressed. The behaviour is by design. Even the latest version of Topiary catches an idempotency error like this:

❯ echo 'let id = if true then Fun.id else Fun.id' | cargo run -- --language ocaml
[2023-06-26T12:24:36Z ERROR topiary] Failed idempotence check
[2023-06-26T12:24:36Z ERROR topiary] Diff < left / right > :
    <let id = if true then Fun.id else Fun.idX
    >let id = if true then Fun.id else Fun.idXX
    
[2023-06-26T12:24:36Z ERROR topiary] The formatter did not produce the same result when invoked twice (idempotence check).
    It would be helpful if you logged this error at https://github.com/tweag/topiary/issues/new?assignees=&labels=type%3A+bug&template=bug_report.md

But the issue you are running into isn't regarded as an idempotency error, but a formatting error:

            // If topiary ran smoothly on its own output,
            // but produced a different output, it is a Idempotence error.
            FormatterError::Idempotence => Err(FormatterError::Idempotence),
            // On the other hand, if it failed to run on its output,
            // it means that when formatting the code, topiary somehow broke it.
            // Hence it is a formatting error.
            _ => Err(FormatterError::Formatting(Box::new(err))),

Whether this is good or bad is open to debate.

[Niols]

I'm not particularly attached to reporting it as an idempotency error. I do however think that it is bad to report it as a parsing error without telling the user that it is not their fault and that they should open an issue.

[torhovland]

How about this?

❯ echo 'let id = if true then Fun.id else function x -> x' | cargo run -- --language ocaml
[2023-06-27T08:05:49Z ERROR topiary] The formatter produced invalid output and
    failed when trying to format twice (idempotence check).
    
    If this happened with the built-in query files, it is a bug. It would be
    helpful if you logged this error at
    https://github.com/tweag/topiary/issues/new?assignees=&labels=type%3A+bug&template=bug_report.md
    
    The following is the error received when running the second time, but note
    that any line and column numbers refer to the formatted code, not the
    original input. Run Topiary with the --skip-idempotence flag to see this
    invalid formatted code.
[2023-06-27T08:05:49Z ERROR topiary] Cause: Parsing error between line 1, column 46 and line 1, column 47

[Niols]

Looks amazing to me!