Rescript-compiler: `bs.deriving`: special casing type `t`

[alexfedoseev]

Does it make sense to special case type t when bs.deriving is used?

E.g.:

[@bs.deriving jsConverter]
type t = {foo: string};

/*
 * Generated: toJs & fromJs
 *            (instead of tToJs & tFromJs)
 */

[bobzhang]

I do have similar ideas, would like to hear what other people think

[OvermindDL1]

I'd follow what the usual OCaml deriving PPX's do, which is exactly what @alexfedoseev proposed (ignore the t and just put `to/from for things like the JSON PPX and others).

[bobzhang]

There is a more serious issue about shadowing

type hh = {
   xx : int
} and yy = {
  hh : hh
} [@@bs.deriving abstract]

Here the generated getter hh would shadow the maker hh, so I propose to call maker hhMake or make (when the type is t)

To maintain backward compatibility we are still going to generate hh and hhMake for a while(deprecate hh) and remove hh eventually

[bobzhang]

Note we have to rename hh into hhMake, since it will be shadowed otherwise.
My experience of making an exception for name t is actually not pleasant, mostly because when I have a type t, I want to provide a smart constructor instead of using the provided one, what do you think, is it worth making an exception for t?

[alexfedoseev]

@bobzhang I'm not sure I understand use cases, can you post some code snippets?

[anmonteiro]

FWIW, I do agree special casing t could lead to a lot of confusion. I feel like the proposed solution hhMake handles the shadowing in a nice way.

I truly don't see the point of saving one character when t is used. In signatures we'd be using MyModule.t anyway; I don't think we should move away from MyModule.tToJs either.

[bobzhang]

Actually I want to be a bit more aggressive to end the endless shadowing issues: labelGet, labelSet, tMake
The old functions will still be generated but deprecated
The thing is that it is too easy to get bitten by shadowing when using short names

Edit: it seems we only need change accessor to labelGet to avoid shadowing issues, we can keep calling it t since type name is unique per recursive definitions or even per module (multiple type definition is not allowed)

So here is the proposal:
For the name rules, creator will still be called the type name, but accessor will be suffixed Get to avoid name shadowing

[bobzhang]

fixed in master

[bobzhang]

Note the shadowing issue exists, it has to wait for another release to remove the deprecated generated functions

[rrdelaney]

How does suffixing get avoid name shadowing? I feel like in most cases the types are in a module, and suffixing get only adds line noise.

[anmonteiro]

@rrdelaney

before:

type address = {
  street: string
} [@@bs.deriving abstract]

type person = {
   name : string
   address: address
} [@@bs.deriving abstract]

(* the following produces an error because `address` is actually the getter for address in `person` *)
person ~name:"Tony" ~address:(address ~street:"42 Foo Ave")

after:

type address = {
  street: string
} [@@bs.deriving abstract]

type person = {
   name : string
   address: address
} [@@bs.deriving abstract]

(* no shadowing because the getter is now `addressGet` and the maker is `addressMake` *)
personMake ~name:"Tony" ~address:(addressMake ~street:"42 Foo Ave")

[rrdelaney]

I feel like that’s a really small use case that’s solved by suffixing make (which sounds like a good idea) — the *Get suffix seems superfluous to me.

[cristianoc]

People in practice also experienced shadowing across fields as the same field name is used in different types.
That situation is not unusual in eg uses of GraphQL.

[rrdelaney]

@cristianoc Yea I've had that problem too, but does suffixing Get help with that at all? If I understand correctly (I might not) the transform will look like:

/* input */
[@bs.deriving abstract]
type user = {
  name: string,
  profilePhoto: string,
};

[@bs.deriving abstract]
type post = {
  name: string,
  body: string,
}

/* output */
type user;
external userMake : (~name: string, ~profilePhoto: string) => user = "";
external nameGet : user => string = "";
external profilePhotoGet : user => string = "";

type post;
external postMake : (~name: string, ~body: string) => user = "";
external nameGet : post => string = "";
external bodyGet : post => string = "";

It looks like that has the same field shadowing problem.

[cristianoc]

@rrdelaney changing getters uniformly does not help with overloaded fields. It's just an example of a problem that remains open.

As a principle, if we're not making everything verbose, it seems sensible to make less verbose the most common case. I don't know what's the current proposal for field get/set vs constructor.

[rrdelaney]

I think the Get/Set suffix was checked in though? I'm just curious as to what it helps. I'm in favor of the Make suffix though.

[cristianoc]

From an offline conversation. If a field is called x, then there's a high risk that something called x is already in scope and there could be unintentional shadowing.

[rrdelaney]

The ergonomics still seem really off with this change. It’s getting harder to recommend deriving abstract over something like bs-json, or even just Js.t...

[bobzhang]

note we have to change the current naming scheme because there is no easy work around:

type t1 = { x : int }
and t2 = { t1 : t1}

So we have to suffix either maker or Getter, my experience with short getter is that it is really easy to be shadowed even with parameters or locally defined variables

[rrdelaney]

I don't know if the average usage of deriving abstract looks like that though. Doing a quick search through GitHub it looks like most usage are confined to a module consisting of just types and maybe a ReasonReact binding (no order here, just some samples from a GitHub search):

• AWS_Config.re
• Hapi.re
• SimpleMap.re
• Location.re
• Geography.re
• Marker.re
• AwsLambda.re
• Line.re
• readline.re

To me it just looks like adding the Get suffix will add line noise to each of these, for little benefit. I agree in that simple case the shadowing can look like a problem, but I don't believe that the example represents the average use case.