make Ast_builder.{e,p}list able to build lists such as a :: b :: c

[v-gb]

No description provided.

[NathanReb]

In its current form this could potentially be a breaking change for some users.

@patricoferris this could be a nice thing to try with a revdeps CI!

Just to err on the safe side, I think it would be best to introduce new functions accepting the optional tail argument and rewrite the old ones on top of those.

[v-gb]

What kind of code do you imagine this would break?

Given that 1) there aren't many ppxes 2) they break regularly due to upstream ast changes 3) the optional argument will be erased in the vast majority of cases, I don't really the reason to go out of our way to provide absolute backwards compatibilty (if whatever then elist else esequence would break, but that's not realistic).

[NathanReb]

ppxlib has ~250 reverse dependencies on opam, adding to that any users that are not providing opam packages.

I agree it is unlikely to break most of those, hence my wording above, but I want to avoid unnecessary breakage. As you pointed out, ppxlib does break when we update the internal AST so I'd rather avoid breaking changes outside of those releases as much as possible and it's unclear when the next one will happen. In particular, since we're going to merge support for OCaml trunk in the main branch, introducing breaking changes will impact anyone trying to test the dev version of the compiler.

I understand it's annoying to have to care about the API stability in such cases but ppxlib is a rather central piece of the ecosystem and therefore one where we can't take this lightly.

If you do not want to add separate functions, you can open a PR to opam-repository to trigger a build there to ensure this breaks no opam rev deps, otherwise I'll take care of it a bit later. If you're interested you can see how to
do a "draft" release here, using github archives rather than going through the regular release process.

[v-gb]

I did a grep for elist through opam, and obviously the bulk of its uses are full applications, but there are enough uses that it's hard to know for sure the dependent libs will build.

Just looking at this information, which is a very natural thing to want to do, seems to be wholly unsupported by the tools: opam source or something is slow, opam grep can't seem to limit grepping to the libs that directly use ppxlib despite its flag that seems like it should do exactly that, and even then opam-grep only seems to give you a boolean, not the actual bit of matching text.

So I ended up doing this the low-tech way:

opam search --depends-on ppxlib -s | while read name; do echo $name; (rm -rf /tmp/z; opam source --quiet $name --dir /tmp/z; (cd /tmp/z; rg '\belist')) < /dev/null; done |& tee /tmp/opam-grep

Is there a good way to do this, or do people never look through opam for how something is used?

If you do not want to add separate functions, you can open a PR to opam-repository to trigger a build there to ensure this breaks no opam rev deps, otherwise I'll take care of it a bit later. If you're interested you can see how to
do a "draft" release here, using github archives rather than going through the regular release process.

How is the opam file you point to created ? Is it manually copied from elsewhere in the repo and manually tweaked, or is it generated by dune-release with some specific arguments ?

[NathanReb]

https://sherlocode.com/ can be a useful tool for this!

The opam file I linked is just a copy of ppxlib's opam file with the url field added manually.

[v-gb]

Thanks. I created ocaml/opam-repository#26104, we'll see if I set it up correctly or not in a while apparently.

[NathanReb]

It indeed breaks ppx_deriving_yojson and potentially more packages whose builds are locked behind it. It also confirms my fear that proprietary code could break in a similar fashion.

Could you add separate functions? In the long run we can devise a plan to drop the old versions smoothly.

[v-gb]

It indeed causes breakage, although I think this breakage is a compiler bug, as it doesn't make a whole lot of sense.

let foo ?tail:_ ~loc l =
  Ast_builder.Default.elist ~loc l
let gen_def_loc f x =
  let loc : location = assert false in
  f ~loc x
let _foo = gen_def_loc foo

9 | let _foo = gen_def_loc foo
                           ^^^
Error: This expression has type
         ?tail:'a -> loc:location -> expression list -> expression
       but an expression was expected of type loc:location -> 'b -> 'c

But if ~loc wasn't labelled, then it's all good:

let foo ?tail:_ loc l =
  Ast_builder.Default.elist ~loc l
let gen_def_loc f x =
  let loc : location = assert false in
  f loc x
let _foo = gen_def_loc foo
(* compiles fines *)

In any case, I'll add other functions because even if the compiler gets fixed, it's going months in the best case scenario.

[v-gb]

It's not clear what name to give a function that's exactly the same as another one. I used elist_tail, which is terrible, but other thoughts I have are not particularly better.

[NathanReb]

It's not a compiler bug but a limitation of type inference when it comes to labeled arguments. It's all explained in detail here: https://ocaml.org/manual/5.2/lablexamples.html#s:label-inference.

I agree naming isn't easy here. We've been using V2 modules in some places for this so that's one option to consider. Nothing's perfect here since the name we'd like to use is taken.

[v-gb]

I guess we can just call the function elist_v2 ?

[NathanReb]

I guess the one advantage of using a separate module here is that we could imagine in the future re-exposing the whole Ast_builder.Default api in V2 (with potential later deprecation of the root values) so that users could use let open Ast_builder.Default.V2 in and keep the rest of the code unchanged. It's particularly useful here since, as you originally pointed out, the addition of the optional argument won't break the code of most users.

I don't have a strong opinion on this so I'll leave to you to decide what you prefer here.

[v-gb]

I was imagining bundling the replacement of elist by elist_v2 with the next breaking change that gets introduced upstream, so the elist_v2 function is short lived. Changing Ast_builder.Default to Ast_builder.Default.V2 would touch way more ppxes than the places where the addition of the optional argument causes issues.

But in any case, since I'm not going to be the one doing any of this, I think it'd be simpler if you told me what you prefer.

[NathanReb]

Let's merge this as it is, we can always change it when we release.

[v-gb]

Thanks !

[ceastlund]

We could just make separate constructors for improper lists, and have elist_tail head_exps tail_exp and plist_tail head_pats tail_pat. Then there's no need for v1/v2. In a separate PR, obviously, since this one was merged.