ppx: derive signatures #32

andreypopp · 2024-11-06T17:50:38Z

Fixes #31.

jchavarri

This is awesome, impressive how fast you fixed it. Thanks!

jchavarri · 2024-11-07T09:05:55Z

ppx/tools/ppx_deriving_tools.mli

@@ -11,11 +11,16 @@ class virtual deriving : object
    loc:location -> path:label -> core_type -> expression
  (** a deriver can be applied to as type expression as extension node. *)

-  method virtual generator :
+  method virtual str_type_decl :
    ctxt:Expansion_context.Deriver.t ->
    rec_flag * type_declaration list ->
    structure
  (** or it can be attached to a type declaration. *)


Off-topic, but I don't understand this comment.

jchavarri · 2024-11-07T09:09:53Z

ppx/tools/ppx_deriving_tools.ml

+                | Ptyp_var txt -> { txt; loc }
+                | _ ->
+                    Location.raise_errorf ~loc
+                      "type variable is not a variable"


Maybe something like:

Suggested change

"type variable is not a variable"

"unsupported type description: only type variables such as `'a` are supported"

(feel free to ignore, i saw this error msg was used already)

jchavarri · 2024-11-07T09:15:20Z

ppx/tools/ppx_deriving_tools.ml

+              let name =
+                match t.ptyp_desc with
+                | Ptyp_var txt -> { txt; loc }
+                | _ ->


Why do derive_of_type_declaration and gen_type_ascription support Ptyp_any but not here?

Edit: oh, because we need a name to keep drilling I guess 🤔

jchavarri · 2024-11-12T15:33:04Z

@andreypopp is this good to merge? none of my comments is blocking.

jchavarri · 2024-11-12T15:33:29Z

Ah, we're missing a changelog entry I guess.

andreypopp · 2024-11-12T16:02:35Z

Actually I think this doesn’t work properly in case you are extending a polymorphic variant decoder. This is because it generates an extra function with _poly suffix which is not being exposed in signatures.

anmonteiro · 2024-11-16T23:04:49Z

ppx/tools/ppx_deriving_tools.ml

@@ -732,52 +586,6 @@ module Conv = struct
               :: next)
         in
         pexp_match ~loc x cases
-
-       method! derive_of_type_declaration td =


what exactly are we deleting? aren't we reducing the scope of what's supported?

i thought we would be losing the polyvar composition, but from what i could check (i tested the test in the comment below), we don't, the test keeps passing fine.

I’m going to work a bit more on the last commit but the gist is — instead of generating a separate function for polyvariants which decodes into None on unmatched tag, we extend error type to have a case for unmatched tag. This is why we removed code here.

ppx/test/example.ml

jchavarri · 2024-11-18T09:07:49Z

src/Json_decode.mli

@@ -13,7 +13,11 @@ third-party libraries.
 type 'a decoder = Js.Json.t -> 'a
 (** The type of a decoder combinator *)

-exception DecodeError of string
+type error = Json_error of string | Unpexpected_variant of string


Doesn't seem that the second branch of the variant is used, but:

Suggested change

type error = Json_error of string | Unpexpected_variant of string

type error = Json_error of string | Unexpected_variant of string

The second case is generated by ppx

andreypopp · 2024-11-26T17:23:22Z

I think this can be merged once we figure out the issue with melc in be1e907

It's not longer generic, hardcodes JSON specifics but that's ok.

jchavarri · 2024-11-29T11:02:38Z

ftr melange-atdgen-codec-runtime, which depends on melange-json, breaks when I try to install using commit c669c07 above:

# File "src/atdgen_codec_decode.ml", line 1:
# Error: This expression has type string but an expression was expected of type
#          error = Json__Json_decode.error

andreypopp · 2024-11-29T16:07:29Z

yeah, that's a breaking change...

ppx/test/example.ml

andreypopp · 2024-12-01T12:44:29Z

Yes, I’ve added poly.t test case instead.Von meinem iPhone gesendetAm 01.12.2024 um 16:02 schrieb Javier Chávarri ***@***.***>: @jchavarri commented on this pull request. In ppx/test/example.ml:

+(*module Polyvar : sig*)

+(* type t = [`A | `B] [@@deriving json]*) +(*end = struct*) +(* type t = [`A | `B] [@@deriving json]*) +(*end*) +(**) +(*type polyvar = [Polyvar.t|`C] [@@deriving json]*) Can we remove this commented code? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

jchavarri · 2024-12-03T15:05:35Z

@andreypopp Is there anything remaining on the PR before it's merged? The only thing I can think of is CHANGES.md entry.

(ftr the bug with melange is being tracked in melange-re/melange#1238).

jchavarri · 2024-12-03T15:07:34Z

Re: the merge of Json and ppx runtime module discussed in #36 I think we should hold a public release until that's done, so we can bundle all breaking changes in a single version.

andreypopp · 2024-12-03T15:19:47Z

@jchavarri updated CHANGES, this PR is good to go, I think

agreed about doing #36 first before the next release

anmonteiro · 2024-12-03T20:45:55Z

@andreypopp I think there are some snapshots to verify/accept -- check the build failures

andreypopp · 2024-12-04T03:59:09Z

I’m not sure why they are failing, did the ocamlformat update?

anmonteiro · 2024-12-04T04:13:24Z

they did merge an ocamlformat release within the last 24 hours ocaml/opam-repository#26998

and the test are using it:

...
  ∗ install opam-solver             2.1.6
  ∗ install opam-state              2.1.6
  ∗ install ocamlformat             0.27.0
  ∗ install ppx_base                v0.17.0
...

andreypopp · 2024-12-04T04:22:08Z

Should we pin ocamlformat or avoid using it for tests?

jchavarri · 2024-12-04T08:18:02Z

There's some issue with ocamlformat because it was upgraded today. Will fix in a new PR.

jchavarri · 2024-12-04T08:21:22Z

#44

andreypopp requested review from jchavarri, anmonteiro and davesnx November 6, 2024 17:50

andreypopp force-pushed the ppx-intf branch from 1d53502 to db0ab3c Compare November 6, 2024 17:50

pedrobslisboa mentioned this pull request Nov 6, 2024

ppx: support interface files #31

Closed

jchavarri approved these changes Nov 7, 2024

View reviewed changes

anmonteiro reviewed Nov 16, 2024

View reviewed changes

jchavarri reviewed Nov 18, 2024

View reviewed changes

ppx/test/example.ml Outdated Show resolved Hide resolved

jchavarri reviewed Nov 18, 2024

View reviewed changes

andreypopp mentioned this pull request Nov 21, 2024

Fix unused-var warning with polyvars without own cases #38

Open

andreypopp added 6 commits November 27, 2024 10:10

ppx: derive signatures

d0d2820

ppx: remove poly special case

09c710e

ppx: move ppx_deriving_tools to ppx/native

80b3c2b

It's not longer generic, hardcodes JSON specifics but that's ok.

ppx: rm unused arg in Ppx_deriving_tools

c404950

ppx: qualify Unexpected_variant usage

ed3a632

MELANGE BUG?! ppx: add more tests for polyvariants

c669c07

andreypopp force-pushed the ppx-intf branch from be1e907 to c669c07 Compare November 27, 2024 06:11

fix melange runtime error by adding runtime dep to ppx

01269a5

jchavarri reviewed Dec 1, 2024

View reviewed changes

ppx/test/example.ml Outdated Show resolved Hide resolved

cleanup unused code in example.ml

deff3ae

Update CHANGES

261aa26

jchavarri merged commit 438f922 into main Dec 4, 2024
1 of 7 checks passed

jchavarri deleted the ppx-intf branch December 4, 2024 08:18

jchavarri restored the ppx-intf branch December 4, 2024 08:18

jchavarri deleted the ppx-intf branch December 4, 2024 08:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ppx: derive signatures #32

ppx: derive signatures #32

andreypopp commented Nov 6, 2024 •

edited by jchavarri

Loading

jchavarri left a comment

jchavarri Nov 7, 2024

jchavarri Nov 7, 2024

jchavarri Nov 7, 2024

jchavarri commented Nov 12, 2024

jchavarri commented Nov 12, 2024

andreypopp commented Nov 12, 2024

anmonteiro Nov 16, 2024

jchavarri Nov 18, 2024

andreypopp Nov 18, 2024 •

edited

Loading

jchavarri Nov 18, 2024

andreypopp Nov 18, 2024

andreypopp commented Nov 26, 2024

jchavarri commented Nov 29, 2024

andreypopp commented Nov 29, 2024

andreypopp commented Dec 1, 2024 via email

jchavarri commented Dec 3, 2024

jchavarri commented Dec 3, 2024

andreypopp commented Dec 3, 2024

anmonteiro commented Dec 3, 2024

andreypopp commented Dec 4, 2024

anmonteiro commented Dec 4, 2024 •

edited

Loading

andreypopp commented Dec 4, 2024

jchavarri commented Dec 4, 2024

jchavarri commented Dec 4, 2024

	"type variable is not a variable"
	"unsupported type description: only type variables such as `'a` are supported"

	type error = Json_error of string \| Unpexpected_variant of string
	type error = Json_error of string \| Unexpected_variant of string

ppx: derive signatures #32

ppx: derive signatures #32

Conversation

andreypopp commented Nov 6, 2024 • edited by jchavarri Loading

jchavarri left a comment

Choose a reason for hiding this comment

jchavarri Nov 7, 2024

Choose a reason for hiding this comment

jchavarri Nov 7, 2024

Choose a reason for hiding this comment

jchavarri Nov 7, 2024

Choose a reason for hiding this comment

jchavarri commented Nov 12, 2024

jchavarri commented Nov 12, 2024

andreypopp commented Nov 12, 2024

anmonteiro Nov 16, 2024

Choose a reason for hiding this comment

jchavarri Nov 18, 2024

Choose a reason for hiding this comment

andreypopp Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

jchavarri Nov 18, 2024

Choose a reason for hiding this comment

andreypopp Nov 18, 2024

Choose a reason for hiding this comment

andreypopp commented Nov 26, 2024

jchavarri commented Nov 29, 2024

andreypopp commented Nov 29, 2024

andreypopp commented Dec 1, 2024 via email

jchavarri commented Dec 3, 2024

jchavarri commented Dec 3, 2024

andreypopp commented Dec 3, 2024

anmonteiro commented Dec 3, 2024

andreypopp commented Dec 4, 2024

anmonteiro commented Dec 4, 2024 • edited Loading

andreypopp commented Dec 4, 2024

jchavarri commented Dec 4, 2024

jchavarri commented Dec 4, 2024

andreypopp commented Nov 6, 2024 •

edited by jchavarri

Loading

andreypopp Nov 18, 2024 •

edited

Loading

anmonteiro commented Dec 4, 2024 •

edited

Loading