Bug fixes for 1465

CHANGES.md

@@ -1,3 +1,15 @@
+# Dev (2023-??-??) - ??
+## Features/Changes
+* Misc: Bump magic number for ocaml 5.1
+* Misc: changes to stay compatible with the next version of ppx_expect
+
+## Bug fixes
+* Compiler: fix location for parsing errors when last token is a virtual semicolon
+* Compiler: fix variable renaming with nested const/let decl with identical names
+* Compiler: fix variable renaming inside js method
+* Compiler: consise body should allow any expression but object literals
+* Compiler: preverve [new] without arguments [new C] (vs [new C()]
+
 # 5.2.0 (2023-04-28) - Lille
 ## Features/Changes
 * Compiler: jsoo link archive with -a (#1428)

compiler/lib/js_output.ml

@@ -732,8 +732,6 @@ struct
         PP.string f "new";
         PP.space f;
         expression NewExpression f e;
-        PP.break f;
-        PP.string f "()";
         PP.end_group f;
         if Prec.(l > NewExpression)
         then (

compiler/lib/js_parser.mly

@@ -911,9 +911,7 @@ async_arrow_function:
 (* was called consise body in spec *)
 arrow_body:
  | "{" b=function_body "}" { b }
- | e=assignment_expr_no_stmt { [(Return_statement (Some e), p $symbolstartpos)] }
- (* ugly *)
- | e=function_expr { [(Expression_statement e, p $symbolstartpos)] }
+ | e=assignment_expr_for_consise_body { [(Return_statement (Some e), p $symbolstartpos)] }
 
 (*----------------------------*)
 (* no in                    *)
@@ -982,6 +980,31 @@ assignment_expr_no_stmt:
  | T_YIELD e=assignment_expr { EYield (Some e) }
  | T_YIELD "*" e=assignment_expr { EYield (Some e) }
 
+
+primary_for_consise_body:
+ | function_expr       { $1 }
+ | class_expr          { $1 }
+ (* es6: *)
+ | generator_expr      { $1 }
+ (* es7: *)
+ | async_function_expr { $1 }
+
+assignment_expr_for_consise_body:
+ | conditional_expr(primary_for_consise_body) { $1 }
+ | e1=left_hand_side_expr_(primary_for_consise_body) op=assignment_operator e2=assignment_expr
+    {
+      match assignment_pattern_of_expr (Some op) e1 with
+      | None -> EBin (op, e1, e2)
+      | Some pat -> EBin (op, EAssignTarget pat, e2)
+    }
+ (* es6: *)
+ | arrow_function { $1 }
+ | async_arrow_function { $1 }
+ (* es6: *)
+ | T_YIELD { EYield None }
+ | T_YIELD e=assignment_expr { EYield (Some e) }
+ | T_YIELD "*" e=assignment_expr { EYield (Some e) }
+
 (* no object_literal here *)
 primary_no_stmt: T_ERROR TComment { assert false }
 

compiler/lib/js_traverse.ml

@@ -315,6 +315,8 @@ class type iterator =
 
     method switch_case : Javascript.expression -> unit
 
+    method block : Javascript.statement_list -> unit
+
     method initialiser : Javascript.expression * Javascript.location -> unit
 
     method initialiser_o : (Javascript.expression * Javascript.location) option -> unit
@@ -345,6 +347,8 @@ class iter : iterator =
 
     method early_error _ = ()
 
+    method block l = m#statements l
+
     method statements l = List.iter l ~f:(fun (s, _) -> m#statement s)
 
     method variable_declaration _ x =
@@ -383,7 +387,7 @@ class iter : iterator =
       | CEField (_static, n, i) ->
           m#class_element_name n;
           m#initialiser_o i
-      | CEStaticBLock b -> m#statements b
+      | CEStaticBLock b -> m#block b
 
     method private class_element_name x =
       match x with
@@ -392,7 +396,7 @@ class iter : iterator =
 
     method statement s =
       match s with
-      | Block b -> m#statements b
+      | Block b -> m#block b
       | Variable_statement (k, l) -> List.iter l ~f:(m#variable_declaration k)
       | Function_declaration (id, fun_decl) ->
           m#ident id;
@@ -451,15 +455,15 @@ class iter : iterator =
               m#switch_case e;
               m#statements s)
       | Try_statement (b, catch, final) -> (
-          m#statements b;
+          m#block b;
           (match catch with
           | None -> ()
           | Some (id, b) ->
               Option.iter ~f:m#param id;
-              m#statements b);
+              m#block b);
           match final with
           | None -> ()
-          | Some s -> m#statements s)
+          | Some s -> m#block s)
 
     method statement_o x =
       match x with
@@ -905,42 +909,58 @@ class free =
       super#for_binding k x
   end
 
+type scope =
+  | Lexical_block
+  | Fun_block of ident option
+
 class rename_variable =
-  let declared local_only ident params body =
+  let declared scope params body =
     let declared_names = ref StringSet.empty in
     let decl_var x =
       match x with
       | S { name = Utf8 name; _ } -> declared_names := StringSet.add name !declared_names
       | _ -> ()
     in
-    Option.iter ~f:decl_var ident;
+    (match scope with
+    | Lexical_block -> ()
+    | Fun_block None -> ()
+    | Fun_block (Some x) -> decl_var x);
     List.iter params ~f:(fun x -> decl_var x);
-    (object
+    (object (self)
+       val depth = 0
+
        inherit iter as super
 
        method expression _ = ()
 
+       method fun_decl _ = ()
+
        method statement x =
-         match x with
-         | Function_declaration (id, _) -> if not local_only then decl_var id
-         | _ -> super#statement x
+         match scope, x with
+         | Fun_block _, Function_declaration (id, fd) ->
+             decl_var id;
+             self#fun_decl fd
+         | Lexical_block, Function_declaration (_, fd) -> self#fun_decl fd
+         | (Fun_block _ | Lexical_block), _ -> super#statement x
 
        method variable_declaration k l =
-         if (not local_only)
-            ||
-            match k with
-            | Let | Const -> true
-            | Var -> false
+         if match scope, k with
+            | (Lexical_block | Fun_block _), (Let | Const) -> depth = 0
+            | Lexical_block, Var -> false
+            | Fun_block _, Var -> true
          then
            let ids = bound_idents_of_variable_declaration l in
            List.iter ids ~f:decl_var
 
+       method block l =
+         let m = {<depth = depth + 1>} in
+         m#statements l
+
        method for_binding k p =
-         if (not local_only)
-            ||
-            match k with
-            | Let | Const -> true
-            | Var -> false
+         if match scope, k with
+            | (Lexical_block | Fun_block _), (Let | Const) -> depth = 0
+            | Lexical_block, Var -> false
+            | Fun_block _, Var -> true
          then
            match p with
            | BindingIdent i -> decl_var i
@@ -959,8 +979,8 @@ class rename_variable =
 
     val decl = StringSet.empty
 
-    method private update_state local_only ident params iter_body =
-      let declared_names = declared local_only ident params iter_body in
+    method private update_state scope params iter_body =
+      let declared_names = declared scope params iter_body in
       {<subst = StringSet.fold
                   (fun name subst -> StringMap.add name (Code.Var.fresh_n name) subst)
                   declared_names
@@ -975,18 +995,18 @@ class rename_variable =
 
     method fun_decl (k, params, body, nid) =
       let ids = bound_idents_of_params params in
-      let m' = m#update_state false None ids body in
+      let m' = m#update_state (Fun_block None) ids body in
       k, m'#formal_parameter_list params, m'#function_body body, m#loc nid
 
     method program p =
-      let m' = m#update_state true None [] p in
+      let m' = m#update_state Lexical_block [] p in
       m'#statements p
 
     method expression e =
       match e with
       | EFun (ident, (k, params, body, nid)) ->
           let ids = bound_idents_of_params params in
-          let m' = m#update_state false ident ids body in
+          let m' = m#update_state (Fun_block ident) ids body in
           EFun
             ( Option.map ident ~f:m'#ident
             , (k, m'#formal_parameter_list params, m'#function_body body, m#loc nid) )
@@ -996,23 +1016,23 @@ class rename_variable =
       match s with
       | Function_declaration (id, (k, params, body, nid)) ->
           let ids = bound_idents_of_params params in
-          let m' = m#update_state false None ids body in
+          let m' = m#update_state (Fun_block None) ids body in
           Function_declaration
             ( m#ident id
             , (k, m'#formal_parameter_list params, m'#function_body body, m#loc nid) )
       | Block l ->
-          let m' = m#update_state true None [] l in
+          let m' = m#update_state Lexical_block [] l in
           Block (m'#statements l)
       | Try_statement (block, catch, final) ->
           let block =
-            let m' = m#update_state true None [] block in
+            let m' = m#update_state Lexical_block [] block in
             m'#statements block
           in
           let final =
             match final with
             | None -> None
             | Some final ->
-                let m' = m#update_state true None [] final in
+                let m' = m#update_state Lexical_block [] final in
                 Some (m'#statements final)
           in
           let catch =
@@ -1031,7 +1051,7 @@ class rename_variable =
                       in
                       Some p, l
                 in
-                let m' = m#update_state true None l catch in
+                let m' = m#update_state Lexical_block l catch in
                 let i =
                   match i with
                   | None -> None

compiler/lib/js_traverse.mli

@@ -78,6 +78,8 @@ class type iterator =
 
     method switch_case : Javascript.expression -> unit
 
+    method block : Javascript.statement_list -> unit
+
     method initialiser : Javascript.expression * Javascript.location -> unit
 
     method initialiser_o : (Javascript.expression * Javascript.location) option -> unit

compiler/lib/parse_js.ml

@@ -356,7 +356,11 @@ let rec offer_one t (lexbuf : Lexer.t) =
         | ( Some (((T_RETURN | T_CONTINUE | T_BREAK | T_THROW | T_YIELD), _, _), _)
           , (((T_SEMICOLON | T_VIRTUAL_SEMICOLON), _, _) as tok) ) -> tok
         | Some (((T_RETURN | T_CONTINUE | T_BREAK | T_THROW | T_YIELD), _, _), _), _
-          when nl_separated h tok ->
+          when nl_separated h tok && acceptable t T_VIRTUAL_SEMICOLON ->
+            (* restricted token can also appear as regular identifier such
+               as in [x.return]. In such case, feeding a virtual semicolon
+               could trigger a parser error. Here, we first checkpoint
+               that a virtual semicolon is acceptable. *)
             Lexer.rollback lexbuf;
             semicolon
         (* The practical effect of these restricted productions is as follows:
@@ -496,10 +500,15 @@ let parse_aux the_parser (lexbuf : Lexer.t) =
   let checkpoint = State.create init in
   match loop checkpoint checkpoint with
   | `Ok x -> x
-  | `Error t -> (
-      match State.Cursor.last_token (State.cursor t) with
-      | None -> assert false
-      | Some ((_, p, _), _) -> raise (Parsing_error (Parse_info.t_of_pos p)))
+  | `Error t ->
+      let rec last cursor =
+        match State.Cursor.last_token cursor with
+        | None -> assert false
+        | Some ((T_VIRTUAL_SEMICOLON, _, _), cursor) -> last cursor
+        | Some ((_, p, _), _) -> p
+      in
+      let p = last (State.cursor t) in
+      raise (Parsing_error (Parse_info.t_of_pos p))
 
 let fail_early =
   object

compiler/tests-compiler/js_parser_printer.ml

@@ -609,6 +609,39 @@ if(a) {
      this(is, not, small) + this(is, bigger);
     } |}]
 
+let%expect_test "consise body can be anything that doesn't start with curly brackets" =
+  print
+    ~debuginfo:false
+    ~compact:false
+    ~report:true
+    {|
+var e = function () { } ()
+
+var e = f(x => function () { } ())
+
+var e = f(x => new class f {})
+|};
+  [%expect
+    {|
+    var e = function(){}();
+    var e = f(x=>function(){}());
+    var e = f(x=>new class f{}); |}]
+
+let%expect_test "new kw with no arguments should be preserve" =
+  print
+    ~debuginfo:false
+    ~compact:false
+    ~report:true
+    {|
+var e = new f
+var e = new f()
+var e = new class f {}
+var e = new (class f {})
+|};
+  [%expect
+    {|
+    var e = new f; var e = new f(); var e = new class f{}; var e = new class f{}; |}]
+
 let%expect_test "error reporting" =
   (try
      print ~invalid:true ~compact:false {|