Skip to content

Conversation

NathanReb
Copy link
Contributor

@NathanReb NathanReb commented Sep 29, 2025

As per usual, starting with a draft to see how revdeps look like.

This should be an improvement over the failed attempt at releasing 0.36.1 as users are less susceptible of producing nodes that will be incorrectly printed.

It is still possible to manually construct them but Ast_builder users should not have that problem anymore.

Changes

@NathanReb
Copy link
Contributor Author

There's this error in vlt that impacts quite a few packages:

# File "ppx/dune", lines 2-11, characters 0-162:
#  2 | (rule
#  3 |  (deps (:< vlt_ppx.ml.cppo))
#  4 |  (targets vlt_ppx.ml)
#  5 |  (action
#  6 |   (with-stdout-to
#  7 |    %{targets}
#  8 |    (run %{bin:cppo} -V "PPXLIB:%{version:ppxlib}" %{<})
#  9 |   )
# 10 |  )
# 11 | )
# (cd _build/default/ppx && /home/opam/.opam/4.14/bin/cppo -V PPXLIB: vlt_ppx.ml.cppo) > _build/default/ppx/vlt_ppx.ml
# Error: Invalid version specification: "PPXLIB:"

I'm not quite sure where that comes from, it seems dune's expanding ppxlib's version to the empty string somehow.

@NathanReb
Copy link
Contributor Author

I added upper bounds on the with-test ppxlib constraint to packages that rely on pprintast for their test as our upgrade to the latest version leads to better but different output.

@NathanReb NathanReb force-pushed the release-ppxlib-0.36.2 branch 2 times, most recently from d405516 to 98ee139 Compare September 30, 2025 10:06
@NathanReb
Copy link
Contributor Author

NathanReb commented Sep 30, 2025

Looking at the CI it seems the test were broken on ppx_factory and ppx_deriving_jsonschema before this release!

@NathanReb NathanReb force-pushed the release-ppxlib-0.36.2 branch from 98ee139 to 307bd6a Compare September 30, 2025 11:35
@NathanReb
Copy link
Contributor Author

The failure on ppx_show is unrelated to this release!

@NathanReb
Copy link
Contributor Author

ping @ocaml/opam-repository! I'd like to get this in before a final release of the 5.4 compatible 0.37.0 release!

I don't think any of the remaining errors are tied to these changes but I might have missed something!

@kit-ty-kate
Copy link
Member

are there any reasons why this is still a draft PR?

@NathanReb NathanReb marked this pull request as ready for review September 30, 2025 14:58
@NathanReb
Copy link
Contributor Author

are there any reasons why this is still a draft PR?

Because I forgot to undraft it... My bad

@raphael-proust
Copy link
Contributor

I'm not quite sure where that comes from, it seems dune's expanding ppxlib's version to the empty string somehow.

Can you check that the generated META file that is installed mentions the version?
There was an issue on Lwt with the version not appearing in the generatede META file because it wasn't mentioned explicitly in the dune-project's package description.

@NathanReb
Copy link
Contributor Author

Indeed, when I run dune build @install, the _build/install/default/lib/ppxlib/META has no mention of the version. I expect that's the version that gets installed.

Should we explicitly set the version in the dune-project when releasing? That's probably something we should fix in dune-release somehow as manually setting it isn't ideal.

@NathanReb
Copy link
Contributor Author

Indeed that seems to fix the META files! It does result in adding the version field to the opam files as well which I thought was discouraged now, am I correct?

CHANGES:

- Make Ast_builder's default `value_binding` constructor generate the proper
  `pvb_constraint` from the pattern and expression arguments.
  (ocaml-ppx/ppxlib#589, @NathanReb)
- Fix pprintast to output correct syntax from `Ppat_constraint (pat, Ptyp_poly ...)`
  nodes until they are completely dropped. (ocaml-ppx/ppxlib#588, @NathanReb)
@NathanReb NathanReb force-pushed the release-ppxlib-0.36.2 branch from a4bb466 to b75aaf2 Compare October 1, 2025 08:28
@NathanReb
Copy link
Contributor Author

dune-release does strip the version field from the opam files for the release so that's all good.

@smorimoto
Copy link
Member

  • ppx_yojson.1.3.0
    Test failure due to discrepancies in generated code.
    Example: Bool (true) became Bool true, i.e. parentheses were omitted.
  • ppx_factory.0.2.0
    Test suite failure caused by changes in formatting of optional arguments.
    Example: ?(tup0= 0) () became ?(tup0= 0) (), with a difference in spacing.
  • ppx_deriving_jsonschema.0.0.1
    Test failure arising from altered formatting of function arguments.
    Example: ?definitions ?id ?title became ?definitions ?id ?title, again due to spacing adjustments.

@raphael-proust
Copy link
Contributor

Nice to see the version issue is fixed!

@raphael-proust raphael-proust merged commit fd0ab3e into ocaml:master Oct 2, 2025
3 of 4 checks passed
@mtzguido
Copy link
Contributor

mtzguido commented Oct 2, 2025

Hi there, after this upgrade building package fstar on OCaml 4.14.2 fails with:

# (cd _build/.sandbox/00ec5e456450602bb7cb450c3c741875/default && .ppx/0f34692bc2b997ae22916e489048416a/ppx.exe --cookie 'library-name="fstarcompiler"' -o fstar-guts/fstarc.ml/FStarC_TypeChecker_Primops.pp.ml --impl fstar-guts/fstarc.ml/FStarC_TypeChecker_Primops.ml -corrected-suffix .ppx-corrected -diff-cmd - -dump-ast)
# File "fstar-guts/fstarc.ml/FStarC_TypeChecker_Primops.ml", line 221, characters 53-54:
# 221 |                                 (division_modulus_op \#mod)
#                                                            ^
# Error: Illegal character (\\)

See here. It seems like 4.14.2 does not have this syntax for the mod operator. Should the OCaml version constraint here be tighter? (I'm not familiar with this syntax, not sure when it was introduced.)

@NathanReb
Copy link
Contributor Author

Hi there, after this upgrade building package fstar on OCaml 4.14.2 fails

I'll take a look and see if I can reproduce this and track down the cause but from the look of it, I'm not sure that strictly ppxlib's doing. I need to know a bit more about fstar first!

@NathanReb
Copy link
Contributor Author

NathanReb commented Oct 3, 2025

@smorimoto to circle back on those errors, ppx_yojson's error comes from our late update to 5.2's pprintast which is probably causing the change in how the drivers prints out the same AST, leading to the test failure you mention.

The other two packages though seem to have had those test failures before ppxlib.0.36.2, I assume their test suite have been unmaintained for a little while! I can take a look and send PRs upstream if that helps with opam's revdeps CI in the future.

@mtelvers
Copy link
Contributor

mtelvers commented Oct 3, 2025

Hi there, after this upgrade building package fstar on OCaml 4.14.2 fails

I'll take a look and see if I can reproduce this and track down the cause but from the look of it, I'm not sure that strictly ppxlib's doing. I need to know a bit more about fstar first!

@NathanReb fstar.2025.08.07 and fstar.2025.09.04 fails on ocaml.4.14.2, ocaml.5.0.0, ocaml.5.1.1 after the new release of ppxlib

@mtzguido
Copy link
Contributor

mtzguido commented Oct 3, 2025

Hi there, after this upgrade building package fstar on OCaml 4.14.2 fails

I'll take a look and see if I can reproduce this and track down the cause but from the look of it, I'm not sure that strictly ppxlib's doing. I need to know a bit more about fstar first!

F*'s build is a bit complicated, but I think what's happening here is that we used to print (mod) for the modulus operator, but now we print \#mod, which doesn't seem to work in older OCaml's. I'm happy to help debug further if needed.

@NathanReb
Copy link
Contributor Author

Can you open an issue on ppxlib please? That'll give us a more suited discussion channel for this!

@mseri
Copy link
Member

mseri commented Oct 3, 2025

Should we add a conflict at least to fstar here on the repo in the meantime?

@avsm
Copy link
Member

avsm commented Oct 3, 2025

@mtzguido wrote:

F*'s build is a bit complicated, but I think what's happening here is that we used to print (mod) for the modulus operator, but now we print #mod, which doesn't seem to work in older OCaml's

This \#mod syntax was only added in OCaml 5.2, so failure in earlier OCaml versions is expected isn't it? ocaml/ocaml#11252

@mseri
Copy link
Member

mseri commented Oct 3, 2025

but now we print \#mod

When was this change introduced in ppxlib?

@NathanReb
Copy link
Contributor Author

NathanReb commented Oct 3, 2025

We didn't properly update our copy of Pprintast in 0.36.0 and this was causing troubles so we updated it in 0.36.1 which was followed by some bug fixes in 0.36.2 and reworks of some helpers to prevent users from generating inconsistent AST nodes that lead to invalid source code when printed via Pprintast.

Note that this should not impact regular ppx users as we don't use the driver's source output but instead marshall out the AST.

From a quick look it seems fstar uses ppxlib's library for some Ocaml code generation which is leading to this.

One possible fix for this in fstar would be to pretty print using the compiler's pprintast, i.e. migrate the AST to the installed compiler version and use its Pprintast to make sure the syntax is compatible.

Note that this bit you with the identifier compat feature but it could also for other syntax changes.

@Octachron
Copy link
Member

This is the raw literal notation added in 5.2 to make it possible to use an old library using a new keyword (aka effect) as function in a new library by compiling the old library with -keywords 5.0 and then using \#effect in the old library.
Unfortunately, this complicates the rules for printing identifiers in -dsource (see ocaml/ocaml#13604 for instance), and it seems that the current -dsource code uses \#mod for (mod) in an expression. I would have a look at a fix in the compiler Pprintast module.

@NathanReb
Copy link
Contributor Author

In the meantime, in ppxlib, we could prevent this syntax from being used pre 5.2 which would fix the issue for fstar.

@NathanReb
Copy link
Contributor Author

I've made a patch here: NathanReb/ppxlib@230ffe1 just in case but I'm not quite sold on what's the best approach from ppxlib's perspective here.

@Octachron
Copy link
Member

See also ocaml/ocaml#14279 for the pretty-printer patch that makes sure to print (mod) rather than \#mod whenever possible.

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.

9 participants