Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix roundtrip check to support 5.1 <-> 5.2 migrations #519

Merged
merged 2 commits into from
Sep 23, 2024

Conversation

NathanReb
Copy link
Collaborator

@NathanReb NathanReb commented Sep 4, 2024

Printing to source code and parsing with the compiler before running the upward migration is expected to fail because
fun x y -> z and fun x -> fun y -> z parse to the same AST on 5.01- but have different representations on OCaml 5.02+.

We therefore need to first migrate it to the compiler version before printing it so that it is properly annotated with attributes by the downward migration, allowing the upward one to preserve the original AST.

CHANGES.md Outdated Show resolved Hide resolved
@NathanReb
Copy link
Collaborator Author

CC @patricoferris !

@patricoferris
Copy link
Collaborator

Definitely seems to fix the diffing in the 5.2 AST branch (https://github.com/ocaml-ppx/ppxlib/pull/514/checks?check_run_id=29662622891 -- the errors now are because of something I need to potentially promote it seems). The Astlib.Compiler_pprintast.structure_item is unbound on some platforms it seems/compiler versions

@NathanReb
Copy link
Collaborator Author

Yeah I was kind of expecting Pprintast.structure_item not to exist on older compilers, I'll fix that!

Printing to source code and parsing with the compiler before
running the upward migration is expected to fail because on compilers
older than 5.2, `fun x y -> z` and `fun x -> fun y -> z` parse to
the same AST.

We therefore need to first migrate it to the compiler version before
printing it.

Signed-off-by: Nathan Rebours <[email protected]>
@NathanReb
Copy link
Collaborator Author

NathanReb commented Sep 4, 2024

the errors now are because of something I need to potentially promote it seems

I got those errors when testing on top of the 5.2 bump as well. My guess is that the deriving_inline bits in ast.ml were not updated because of the roundtrip error but I might be misunderstanding what's going on here.

@NathanReb
Copy link
Collaborator Author

NathanReb commented Sep 4, 2024

What puzzles me is that there are two sides to the deriving_inline diff in ast.ml, there are fun x y -> z being turned to fun x -> fun y -> z which I get. I might again be wrong but I'd attribute those to ppx_traverse generating nested Pexp_function in 5.2 instead of generating flat Pexp_function with maximum arity, which in turn gets printed differently.

The other side are those weird formatting changes, such as removed spaces before = which I'm not quite sure where they are coming from.

@patricoferris
Copy link
Collaborator

The other side are those weird formatting changes, such as removed spaces before = which I'm not quite sure where they are coming from.

I thought this too, but this is actually always been the case! I tested on older versions of the codebase -- the = and extra bracketing is removed by a subsequent dune build @fmt --auto and any dune build @lint won't be retriggered afterwards :))

@patricoferris
Copy link
Collaborator

Oh and the fun x -> fun y -> is converted back to fun x y -> by OCamlformat!

@patricoferris
Copy link
Collaborator

Ah that's the problem, sorry for all the messages, that's what is broken here. The traversal and ocamlformat are fighting each other, so a lint followed by a format undoes the work of the lint.

@NathanReb
Copy link
Collaborator Author

We do still have a roundtripping issue though don't we? Can we go ahead and merge this PR?

Copy link
Collaborator

@patricoferris patricoferris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should leave some comment about the logic in this section, but otherwise this is good to go :))

src/code_matcher.ml Outdated Show resolved Hide resolved
@NathanReb
Copy link
Collaborator Author

The trunk build is failing for unrelated reason, I'll try and fix it in a separate PR. Merging!

@NathanReb NathanReb merged commit 538bbe6 into ocaml-ppx:main Sep 23, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants