rescript-lang / rescript-compiler

The compiler for ReScript.
https://rescript-lang.org
Other
6.65k stars 442 forks source link

Inconsistency in Js output for optional field record #6485

Open mununki opened 10 months ago

mununki commented 10 months ago
type t0 = {x?: int}

let a = None
let fa = v =>
  switch v {
  | Some(v) => {x: v}
  | None => {x: ?a}
  }

Js.log(fa(a)) // {}

let b = None
let fb = v =>
  switch v {
  | Some(v) => {x: v}
  | None as y => {x: ?y}
  }

Js.log(fb(b)) // {x: undefined}
// generated
function fa(v) {
  if (v !== undefined) {
    return {
            x: v
          };
  } else {
    return {};
  }
}

function fb(v) {
  if (v !== undefined) {
    return {
            x: v
          };
  } else {
    return {
            x: v
          };
  }
}

lambda

(let
  (a/1004 = 0a
   fa/1005 =
     (#fn_mk
       (function v/1006
         (if (#is_not_none v/1006)
           (let (v/1007 =a (#val_from_unnest_option v/1006))
             (makeblock [x] (makeblock some_not_nested v/1007)))
           (makeblock [x] a/1004))))
   b/1008 = 0a
   fb/1009 =
     (#fn_mk
       (function v/1010
         (if (#is_not_none v/1010)
           (let (v/1011 =a (#val_from_unnest_option v/1010))
             (makeblock [x] (makeblock some_not_nested v/1011)))
           (let (y/1012 =a v/1010) (makeblock [x] y/1012))))))
mununki commented 10 months ago

Also same happends:

let c = Ok(None)
let fc = v =>
  switch v {
  | Ok(v) => {x: ?v}
  | Error(_) => {}
  }

Js.log(fc(c)) // {x: undefined}
c/1013 = [0: 0a]
   fc/1014 =
     (#fn_mk
       (function v/1015
         (switch* v/1015
          case tag 0:
           (let (v/1016 =a (field:var/0 v/1015)) (makeblock [x] v/1016))
          case tag 1: (let (match/1027 =a (field:var/0 v/1015)) [0])))))
mununki commented 10 months ago

It seems inevitable as per lambda output. Both as y and Ok(v) are making the new assignment to the variable which means that the compiler doesn't know y and v are the same to the original variables b and c.

ellbur commented 8 months ago

Here's a shorter code that demonstrates the issue:


type item = {
  x?: int
}

let x1 = None
let x2 = if Math.random() < 2.0 { None } else { Some(3) }

// {}
Console.log({x: ?x1})

// {x: undefined}
Console.log({x: ?x2})
ellbur commented 8 months ago

My take on this issue is that since Rescript is a functional language, it should usually be true that "equals can be substituted for equals."

So I think the correct way to handle this would be that whenever x: ?y is used in a record constructor, the compiler needs to generate slightly more verbose js:

let rec = { };
if (y) { rec.x = y; }

More succinctly, this could be accomplished using spread syntax:

let rec = {
   ...(y ? {x: y} : {})
};
ellbur commented 8 months ago

The relevant lines appear to be https://github.com/rescript-lang/rescript-compiler/blob/3bb11b4151ffadf9a14802c01cb9869288bab5b8/jscomp/core/js_dump.ml#L743-L750

ellbur commented 8 months ago

Here's a demonstration of how js code generation can be changed to omit None optional fields, regardless of whether the compiler can tell at compile-time that they are None: https://github.com/rescript-lang/rescript-compiler/compare/master...ellbur:rescript-compiler:object-with-spreads

It's a bit messy because I'm not familiar with the rescript code, and I'm not that good with ocaml. If this seems like something that would be useful, I'm happy to clean it up.

ellbur commented 8 months ago

As I was working on this, I had a couple thoughts:

  1. The Record_regular / Record_optional is per-record rather than per-field. I suppose for more finely tuned js-interop, it should be per-field.
  2. Since whether a field appears in the runtime representation is really more of a js-interop issue than a language semantics issue, perhaps the optionality of the field shouldn't be the trigger for when the field is omitted from the runtime representation. Instead, { x?: t } could have the same representation as { x: option<t> }, and a per-field annotation like @omitWhenNone could be created when omitting from the runtime representation is desired.