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

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

Open
gasche opened this issue Jul 14, 2022 · 10 comments
Open

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

gasche opened this issue Jul 14, 2022 · 10 comments

Comments

@gasche
Copy link

gasche commented Jul 14, 2022

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
Copy link
Author

gasche commented Jul 14, 2022

I also reported this as a Sexplib issue: janestreet/sexplib#44 .

@rgrinberg
Copy link
Contributor

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

@gasche
Copy link
Author

gasche commented Jul 14, 2022

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
Copy link
Contributor

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
Copy link
Author

gasche commented Jul 18, 2022

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:

  • whether s-expression are a good format to standardize upon (why not use JSON instead, etc.)
  • whether the proposed definition is the right one (why not include location information, etc.)

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
Copy link
Contributor

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
Copy link

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
Copy link

mro commented Jan 17, 2023

who is on it?

@ceastlund
Copy link

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
Copy link

@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?

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

No branches or pull requests

6 participants