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

Don't validate Yojson.Safe.t abstract type #263

Open
MisterDA opened this issue Mar 23, 2022 · 4 comments
Open

Don't validate Yojson.Safe.t abstract type #263

MisterDA opened this issue Mar 23, 2022 · 4 comments
Labels
good first time issue packaging Build and packaging issues

Comments

@MisterDA
Copy link
Contributor

MisterDA commented Mar 23, 2022

Yojson rename it's Yojson.Safe.json type to Yojson.Safe.t and deprecated the old type. It also removed the Yojson.Safe.validate_json and deliberately not introduced a new Yojson.Safe.validate_t. I have an atd document using the json type, and I'd like to validation for the values produced, but however I write it, I cannot get atd to turn off validation for that type: it always produces code that call the non-existent Yojson.Safe.validate_t function.

cat > test.atd <<EOF
type json <ocaml module="Yojson.Safe" t="t"> = abstract <ocaml valid="fun _x -> true">
type json' <ocaml module="Yojson.Safe" t="t"> = abstract <ocaml validator="fun _path _x -> None">
EOF
atdgen -v test.atd
ocamlfind ocamlc -o test.exe -linkpkg -package yojson,atdgen test_v.ml

produces

(* Auto-generated from "test.atd" *)
              [@@@ocaml.warning "-27-32-35-39"]

type json' = Yojson.Safe.t

type json = Yojson.Safe.t

let validate_json' = (
  (fun path x -> match ( fun _path _x -> None ) path x with | Some _ as err -> err | None -> (Yojson.Safe.validate_t) path x)
)
let validate_json = (
  (fun path x -> match ( fun path x ->
    let msg = "Failed check by fun _x -> true" in
    if (fun _x -> true) x then
      None
    else
      Some (Atdgen_runtime.Util.Validation.error ~msg path) ) path x with | Some _ as err -> err | None -> (Yojson.Safe.validate_t) path x)
)

and

File "test_v.ml", line 9, characters 91-113:
9 |   (fun path x -> match ( fun _x _y -> None ) path x with | Some _ as err -> err | None -> (Yojson.Safe.validate_t) path x)
                                                                                               ^^^^^^^^^^^^^^^^^^^^^^
Error: Unbound value Yojson.Safe.validate_t
@MisterDA MisterDA changed the title Don't validate abstract Yojson.Safe.t abstract type Don't validate Yojson.Safe.t abstract type Mar 23, 2022
@mjambon mjambon added good first time issue packaging Build and packaging issues labels Mar 23, 2022
@mjambon
Copy link
Collaborator

mjambon commented Mar 23, 2022

I think this change in yojson is going to break user code no matter what we fix in atdgen, because the user code is type json <ocaml module="Yojson.Safe" t="t"> in their ATD file. I don't think atdgen should interpret Yojson.Safe in a special manner either because it's already complicated enough as it is (but at least a warning showing how to fix the problem seems desirable).

Some ideas:

  • Add dedicated support to atd/atdgen for handling raw JSON. The idea would be to make JSON support more built-in or more abstract so that we're not tied to a specific OCaml signature or library in the future.
  • Impose a version constraint on yojson for all past atdgen releases to keep things working.
  • Only relax the package constraint on yojson once atdgen offers a solution for fixing legacy code. This won't fix legacy code automatically, though.

@mjambon
Copy link
Collaborator

mjambon commented Mar 23, 2022

We could also have the atdgen runtime provide its own standardized interface for using raw JSON. It would define the JSON type like Yojson.Safe.t except that it wouldn't change if yojson decides to change its interface. So, instead of

type json <ocaml module="Yojson.Safe" t="t"> = abstract

we would write:

type json <ocaml module="JSON"> = abstract

where JSON is a module provided by the atdgen runtime library. The generated code would start with open Atdgen_runtime (or whatever it's called) so as to have the JSON module directly in scope.

Initially, the interface of JSON would be the same as Yojson.Safe, until the latter changes.

JSON/OCaml adapters would also use the standardized JSON.t type instead of Yojson.Safe.t.

This wouldn't break compatibility with legacy atd files since the types Yojson.Safe.t and JSON.t are equal (at least initially) thanks to the use of polymorphic variants (i.e. structural typing).

@mjambon
Copy link
Collaborator

mjambon commented Mar 23, 2022

Perhaps (almost) all of this and more would be solved by having proper module/multifile support in the ATD language.

@mjambon
Copy link
Collaborator

mjambon commented Mar 24, 2022

I wrote a proposal for managing ATD modules here: #26
Please share your opinions and/or wildest dreams for ATD!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first time issue packaging Build and packaging issues
Projects
None yet
Development

No branches or pull requests

2 participants