ocaml-dune / csexp

Minimal support for Canonical S-expressions
MIT License
27 stars 7 forks source link

Csexp.t and Sexplib.Type.t are incompatible #19

Open gasche opened 2 years ago

gasche commented 2 years ago

Csexp.t and Sexplib.Type.t are two identical but incompatible datatype definitions. This is inconvenient (see for example https://gitlab.inria.fr/bmontagu/sexp_decode/-/issues/1). Would it be possible to share the same type definition, by having one library depend on the other?

gasche commented 2 years ago

I also reported this as a Sexplib issue: https://github.com/janestreet/sexplib/issues/44 .

rgrinberg commented 2 years ago

I think we already provide a Csexp.Make functor that allows you to use the library with whatever type representation you prefer.

gasche commented 2 years ago

Yes, but Csexp.t does provide a default definition which is incompatible with Sexplib. I don't see a good reason for this incompatibility (except possibly history) and it does create inconveniences -- for example Sexp_decode programming against Csexp.t for convenience, and being inconvenient to use together with Sexplib parsers.

rgrinberg commented 2 years ago

Honestly, I'm a bit hesitant to depend on sexplib0 in csexp. I have no doubt it's a quality library, but portability is not as high of a concern for JST as it is for the packages that rely on csexp (dune, merlin, lsp).

Is solving the problem at the root of the dependency tree (the stdlib) still not possible?

gasche commented 2 years ago

Could you upload a sexplib-type package in opam that contains just Csexp's type definition, and then we would kindly ask the Sexplib people to also depend on it?

It's a lot more work to convince people to put in the stdlib than it should be to convince two users of the same datatype definition to share it instead of having incompatible identical definitions. That change is absolutely painless for users, does not require any fresh API design, etc.

If we wanted to include a sexp type in the stdlib, people would ask:

None of those two questions have obvious answers, as far as I can tell, and they are also completely irrelevant for Csexp and Sexplib(0).

rgrinberg commented 2 years ago

Could you upload a sexplib-type package in opam that contains just Csexp's type definition, and then we would kindly ask the Sexplib people to also depend on it?

I'd rather get their agreement first before doing all that work.

None of those two questions have obvious answers, as far as I can tell,

Does the stdlib set itself the goal of solving incompatibilities in the ecosystem? If it does, then this seems like an obvious opportunity where it can step in and solve this concrete problem for the community. I fail to see why the existence of alternative formats or type definitions should prevent the stdlib of achieving this (imo, far more important) goal.

snowleopard commented 1 year ago

I agree with @rgrinberg that this seems like a great opportunity for Stdlib to solve an important issue for the ecosystem. Dune, Merlin and LSP all rely on Csexp.t so the case is pretty strong.

I also don't like introducing any unnecessary dependencies, particularly on Sexplib(0). Maybe a new tiny library just for the type definition is OK, but I'd rather we invest time into putting it into Stdlib.

mro commented 1 year ago

who is on it?

ceastlund commented 1 year ago

We're minting Sexp_type internally at janestreet, it'll go out by our usual release process and Sexplib0 will be updated. Once that's out, dune can be updated to depend on it as well.

tdelvecchio-jsc commented 1 year ago

@rgrinberg Sexplib0 was made specifically with portability in mind for mirage. The library doesn't have any dependencies and doesn't use any C code. Is there anything specific about Sexplib0 that is not portable?