Address review comments

Author: cknitt

compiler/core/lam_util.ml

@@ -1,5 +1,5 @@
 (* Copyright (C) 2015 - 2016 Bloomberg Finance L.P.
- * Copyright (C) 2017 - Hongbo Zhang, Authors of ReScript 
+ * Copyright (C) 2017 - Hongbo Zhang, Authors of ReScript
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU Lesser General Public License as published by
  * the Free Software Foundation, either version 3 of the License, or
@@ -17,7 +17,7 @@
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU Lesser General Public License for more details.
- * 
+ *
  * You should have received a copy of the GNU Lesser General Public License
  * along with this program; if not, write to the Free Software
  * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. *)
@@ -32,9 +32,9 @@
 
 
 (*
-let add_required_modules ( x : Ident.t list) (meta : Lam_stats.t) = 
+let add_required_modules ( x : Ident.t list) (meta : Lam_stats.t) =
   let meta_require_modules = meta.required_modules in
-  List.iter (fun x -> add meta_require_modules (Lam_module_ident.of_ml x)) x 
+  List.iter (fun x -> add meta_require_modules (Lam_module_ident.of_ml x)) x
 *)
 
 
@@ -45,13 +45,11 @@ let refine_let ~kind param (arg : Lam.t) (l : Lam.t) : Lam.t =
     | Lam_primitive.Pmakeblock _ -> true
     | _ -> false
   in
-  (* Helper for spotting RHS expressions that are safe to inline everywhere.
-     These are the obviously pure shapes where substituting the original value
-     cannot introduce extra work or side effects. *)
-  let is_alias_candidate (lam : Lam.t) =
+  let is_safe_to_inline (lam : Lam.t) =
     match lam with
     | Lvar _ | Lconst _ -> true
-    | Lprim { primitive = Psome_not_nest; _ } -> true
+    | Lprim { primitive = Psome_not_nest; args = [inner]; _ } ->
+        Lam_analysis.no_side_effects inner
     | Lprim { primitive = Pfield (_, Fld_module _); args = [ (Lglobal_module _ | Lvar _) ]; _ } -> true
     | _ -> false
   in
@@ -74,7 +72,7 @@ let refine_let ~kind param (arg : Lam.t) (l : Lam.t) : Lam.t =
          rewrite to `someFn(value)` as long as the callee does not capture `x`.
          This removes the temporary binding while preserving the call semantics. *)
       Lam.apply fn [arg] ap_info ~ap_transformed_jsx
-  | (Strict | StrictOpt), arg, _ when is_alias_candidate arg ->
+  | (Strict | StrictOpt), arg, _ when is_safe_to_inline arg ->
       (* `Strict` and `StrictOpt` bindings both evaluate the RHS immediately
          (with `StrictOpt` allowing later elimination if unused). When that RHS
          is pure — `{ let x = Some(value); ... }`, `{ let x = 3; ... }`, or a module
@@ -96,15 +94,15 @@ let refine_let ~kind param (arg : Lam.t) (l : Lam.t) : Lam.t =
   | kind, _, _ ->
       Lam.let_ kind param arg l
 
-let alias_ident_or_global (meta : Lam_stats.t) (k:Ident.t) (v:Ident.t) 
+let alias_ident_or_global (meta : Lam_stats.t) (k:Ident.t) (v:Ident.t)
     (v_kind : Lam_id_kind.t)  =
-  (* treat rec as Strict, k is assigned to v 
+  (* treat rec as Strict, k is assigned to v
      {[ let k = v ]}
   *)
-  match v_kind with 
+  match v_kind with
   | NA ->
-    begin 
-      match Hash_ident.find_opt meta.ident_tbl v  with 
+    begin
+      match Hash_ident.find_opt meta.ident_tbl v  with
       | None -> ()
       | Some ident_info -> Hash_ident.add meta.ident_tbl k ident_info
     end
@@ -113,59 +111,59 @@ let alias_ident_or_global (meta : Lam_stats.t) (k:Ident.t) (v:Ident.t)
 (* share -- it is safe to share most properties,
     for arity, we might be careful, only [Alias] can share,
     since two values have same type, can have different arities
-    TODO: check with reference pass, it might break 
+    TODO: check with reference pass, it might break
     since it will create new identifier, we can avoid such issue??
 
-    actually arity is a dynamic property, for a reference, it can 
-    be changed across 
+    actually arity is a dynamic property, for a reference, it can
+    be changed across
     we should treat
-    reference specially. or maybe we should track any 
+    reference specially. or maybe we should track any
     mutable reference
 *)
 
 
 
 
 
-(* How we destruct the immutable block 
-   depend on the block name itself, 
+(* How we destruct the immutable block
+   depend on the block name itself,
    good hints to do aggressive destructing
    1. the variable is not exported
       like [matched] -- these are blocks constructed temporary
-   2. how the variable is used 
-      if it is guarateed to be 
-   - non export 
+   2. how the variable is used
+      if it is guarateed to be
+   - non export
    - and non escaped (there is no place it is used as a whole)
-      then we can always destruct it 
-      if some fields are used in multiple places, we can create 
-      a temporary field 
+      then we can always destruct it
+      if some fields are used in multiple places, we can create
+      a temporary field
 
-   3. It would be nice that when the block is mutable, its 
+   3. It would be nice that when the block is mutable, its
        mutable fields are explicit, since wen can not inline an mutable block access
 *)
 
-let element_of_lambda (lam : Lam.t) : Lam_id_kind.element = 
-  match lam with 
-  | Lvar _ 
-  | Lconst _ 
-  | Lprim {primitive = Pfield (_, Fld_module _) ; 
+let element_of_lambda (lam : Lam.t) : Lam_id_kind.element =
+  match lam with
+  | Lvar _
+  | Lconst _
+  | Lprim {primitive = Pfield (_, Fld_module _) ;
            args =  [ Lglobal_module _  | Lvar _ ];
            _} -> SimpleForm lam
   (* | Lfunction _  *)
-  | _ -> NA 
+  | _ -> NA
 
-let kind_of_lambda_block (xs : Lam.t list) : Lam_id_kind.t = 
-  ImmutableBlock( Ext_array.of_list_map xs (fun x -> 
+let kind_of_lambda_block (xs : Lam.t list) : Lam_id_kind.t =
+  ImmutableBlock( Ext_array.of_list_map xs (fun x ->
       element_of_lambda x ))
 
 let field_flatten_get
     lam v i info (tbl : Lam_id_kind.t Hash_ident.t) : Lam.t =
-  match Hash_ident.find_opt tbl v  with 
-  | Some (Module g) -> 
-    Lam.prim ~primitive:(Pfield (i, info)) 
+  match Hash_ident.find_opt tbl v  with
+  | Some (Module g) ->
+    Lam.prim ~primitive:(Pfield (i, info))
       ~args:[ Lam.global_module g ] Location.none
-  | Some (ImmutableBlock (arr)) -> 
-    begin match arr.(i) with 
+  | Some (ImmutableBlock (arr)) ->
+    begin match arr.(i) with
       | NA -> lam ()
       | SimpleForm l -> l
       | exception _ -> lam ()
@@ -182,7 +180,7 @@ let field_flatten_get
       | _ -> lam ()
     )
   | Some (Constant (Const_block (_,_,ls))) ->
-    begin match Ext_list.nth_opt ls i with 
+    begin match Ext_list.nth_opt ls i with
       | None -> lam  ()
       | Some x when not (Lam_constant.is_allocating x) -> Lam.const x
       | Some _ -> lam ()
@@ -191,49 +189,49 @@ let field_flatten_get
   | None -> lam ()
 
 #if (defined BROWSER || defined RELEASE)
-let dump ext  lam = 
+let dump ext  lam =
   ()
 #else
 let log_counter = ref 0
-let dump ext  lam = 
+let dump ext  lam =
   if !Js_config.diagnose
-  then 
+  then
     (* ATTENTION: easy to introduce a bug during refactoring when forgeting `begin` `end`*)
-    begin 
+    begin
       incr log_counter;
       Ext_log.dwarn ~__POS__ "\n@[[TIME:]%s: %f@]@." ext (Sys.time () *. 1000.);
-      Lam_print.serialize  
+      Lam_print.serialize
         (Ext_filename.new_extension
            !Location.input_name
            (Printf.sprintf ".%02d%s.lam" !log_counter ext)
         ) lam;
     end
-#endif      
+#endif
 
 
 
 
 
-let is_function (lam : Lam.t) = 
-  match lam with 
+let is_function (lam : Lam.t) =
+  match lam with
   | Lfunction _ -> true | _ -> false
 
-let not_function (lam : Lam.t) = 
-  match lam with 
-  | Lfunction _ -> false | _ -> true 
-(* 
-let is_var (lam : Lam.t) id =   
-  match lam with 
-  | Lvar id0 -> Ident.same id0 id 
+let not_function (lam : Lam.t) =
+  match lam with
+  | Lfunction _ -> false | _ -> true
+(*
+let is_var (lam : Lam.t) id =
+  match lam with
+  | Lvar id0 -> Ident.same id0 id
   | _ -> false *)
 
 
-(* TODO: we need create 
-   1. a smart [let] combinator, reusable beta-reduction 
-   2. [lapply fn args info] 
+(* TODO: we need create
+   1. a smart [let] combinator, reusable beta-reduction
+   2. [lapply fn args info]
    here [fn] should get the last tail
-   for example 
+   for example
    {[
-     lapply (let a = 3 in let b = 4 in fun x y -> x + y) 2 3 
-   ]}   
+     lapply (let a = 3 in let b = 4 in fun x y -> x + y) 2 3
+   ]}
 *)

packages/@rescript/runtime/lib/es6/Belt_internalAVLset.js

@@ -741,13 +741,15 @@ function rotateWithRightChild(k1) {
 
 function doubleWithLeftChild(k3) {
   let k3l = k3.l;
-  k3.l = rotateWithRightChild(k3l);
+  let v = rotateWithRightChild(k3l);
+  k3.l = v;
   return rotateWithLeftChild(k3);
 }
 
 function doubleWithRightChild(k2) {
   let k2r = k2.r;
-  k2.r = rotateWithLeftChild(k2r);
+  let v = rotateWithLeftChild(k2r);
+  k2.r = v;
   return rotateWithRightChild(k2);
 }
 

packages/@rescript/runtime/lib/js/Belt_internalAVLset.js

@@ -741,13 +741,15 @@ function rotateWithRightChild(k1) {
 
 function doubleWithLeftChild(k3) {
   let k3l = k3.l;
-  k3.l = rotateWithRightChild(k3l);
+  let v = rotateWithRightChild(k3l);
+  k3.l = v;
   return rotateWithLeftChild(k3);
 }
 
 function doubleWithRightChild(k2) {
   let k2r = k2.r;
-  k2.r = rotateWithLeftChild(k2r);
+  let v = rotateWithLeftChild(k2r);
+  k2.r = v;
   return rotateWithRightChild(k2);
 }
 

tests/tests/src/option_wrapping_test.mjs

@@ -12,37 +12,10 @@ let x10 = null;
 
 let x11 = Primitive_option.some(undefined);
 
-let x1 = "hello";
-
-let x2 = 1;
-
-let x3 = {
-  TAG: "Ok",
-  _0: "hi"
-};
-
-let x4 = "polyvar";
-
-let x5 = {
-  x: 42
-};
-
-let x7 = [
-  1,
-  2,
-  3
-];
-
-let x8 = () => {};
-
-let x12 = "test";
-
 let x20 = null;
 
 let x21 = new Date();
 
-let x22 = /test/;
-
 let x23 = new Map();
 
 let x24 = new Set();
@@ -75,8 +48,6 @@ let x37 = new Intl.DateTimeFormat();
 
 let x38 = new Intl.NumberFormat();
 
-let x39 = true;
-
 let x40 = new Intl.Collator();
 
 let x41 = new Intl.RelativeTimeFormat();
@@ -85,19 +56,48 @@ let x42 = new Intl.PluralRules();
 
 let x43 = new Intl.Locale("en");
 
+let x45 = Promise.resolve(true);
+
+let x48 = Stdlib_Lazy.make(() => true);
+
+let x1 = "hello";
+
+let x2 = 1;
+
+let x3 = {
+  TAG: "Ok",
+  _0: "hi"
+};
+
+let x4 = "polyvar";
+
+let x5 = {
+  x: 42
+};
+
+let x7 = [
+  1,
+  2,
+  3
+];
+
+let x8 = () => {};
+
+let x12 = "test";
+
+let x22 = /test/;
+
+let x39 = true;
+
 let x44 = [
   1,
   2
 ];
 
-let x45 = Promise.resolve(true);
-
 let x46 = /* [] */0;
 
 let x47 = {};
 
-let x48 = Stdlib_Lazy.make(() => true);
-
 export {
   x1,
   x2,