Modify polymorphic type variables to include location. #64
Conversation
There was a problem hiding this comment.
I have made a minor comment on the implementation. But I have a bigger question: should I not change Ppx_deriving so that you don't have to make those changes in the first place? In particular, is it the case that if we turned fold_left_type_decl to pass a located name under 4.05, then you wouldn't need any conditional?
I'm interesting in finding the API changes that will make it easier for user code to be compatible across OCaml versions. (Normally this need should also drive the evolution of the Parsetree types themselves, but we are too late in the OCaml release cycle for that feedback loop to be effective.)
| @@ -539,8 +545,7 @@ let desu_str_of_type ~options ~path ({ ptype_loc = loc } as type_decl) = | |||
| raise_errorf ~loc "%s cannot be derived for fully abstract types" deriver | |||
| in | |||
| let ty = desu_type_of_decl ~options ~path type_decl in | |||
| let fv = Ppx_deriving.free_vars_in_core_type ty in | |||
| let poly_type = Typ.force_poly @@ Typ.poly fv @@ ty in | |||
| let poly_type = Ppx_deriving.strong_type_of_type ty in | |||
| init_exp td | ||
| #else | ||
| Ppx_deriving.fold_left_type_decl (fun exp name -> Exp.newtype name exp) init_exp td | ||
| #endif |
There was a problem hiding this comment.
I think that it would be nice if you could factor the similar parts of the two blocks out of the conditional (for the two functions above). The way I did it in my patch, but there may be a better way, is to define (conditionally) worker functions, so I would guess mkname for the first function and newtype for the second, and then have the "main" line call these functions: (fun acc name -> mkname name :: acc) and (fun exp name -> newtype name exp). I guess the definitions would look somewhat like:
#if OCAML_VERSION >= (4, 05, 0)
let loc = ... in
let mkname name = mkloc name loc in
#else
let mkname name = name in
#endifThere was a problem hiding this comment.
I guess this is where my naive understanding of the AST will come out, but in your suggested approach wouldn't mkname's loc depend on the type_declaration that we're folding?
In that case, yes, I do think that we should change fold_left_type_decl to have a 'a -> str -> 'a function in 4.05.0 and 'a -> string -> 'a in < 4.05.0.
|
Update: I tried equipping type variables with a location in ppx_deriving and indeed it makes it easier to adapt plugins. I hope to send a PR later today, or maybe in the beginning of the week, and not wait much longer for a ppx_deriving release (or two, maybe one for 4.1 and one for 4.2). |
…g loc` Since 4.05.0, many base functions on type variables expect located strings instead of raw strings. This is also the more informative API -- locations can always be stripped out, but not regained. I have mixed feelings about the choice of reusing the same function names with a different API, which sort of enforces conditional compilation on users as well. On the other hand, this choice was designed to *reduce* the amount of conditional compilation required by providing, for all OCaml versions, the right type for many client usage patterns -- see ocaml-ppx/ppx_deriving_yojson#64
string loc
ocaml-ppx/ppx_deriving#159
…g loc` Since 4.05.0, many base functions on type variables expect located strings instead of raw strings. This is also the more informative API -- locations can always be stripped out, but not regained. I have mixed feelings about the choice of reusing the same function names with a different API, which sort of enforces conditional compilation on users as well. On the other hand, this choice was designed to *reduce* the amount of conditional compilation required by providing, for all OCaml versions, the right type for many client usage patterns -- see ocaml-ppx/ppx_deriving_yojson#64
|
Obsoleted by the ppx_deriving-side change that your work enabled. Thanks a lot! |
|
Thank you! |
This PR builds upon @gasche 4.05 support PR to enable ppx_deriving_yojson. My apologies, in advance, for a bit of naivety about how all of the underlying pieces fit together, I haven't grokked the ppx_deriving & ppx_tools semantics fully, as one can see by
poly_fun_of_exp_and_type_decl.