diff --git a/CHANGES.md b/CHANGES.md index 15e9d49c77..5949892780 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -21,6 +21,7 @@ profile. This started with version 0.26.0. - Fix invalid syntax generated with `ocp-indent-compat` (#2445, @Julow) - Fixed bug with attributes on sub-expressions of infix operators (#2459, @tdelvecchio-jsc) - \* Fix cinaps comment formatting to not change multiline string contents (#2463, @tdelvecchio-jsc) +- Fix position of comments around function parameters (#2471, @gpetiot) ## 0.26.1 (2023-09-15) diff --git a/lib/Ast.ml b/lib/Ast.ml index 43f1073c4e..0f8df8b54d 100644 --- a/lib/Ast.ml +++ b/lib/Ast.ml @@ -177,7 +177,7 @@ module Exp = struct , (Non_apply | Sequence | Then | ThenElse) ) |( { pexp_desc= ( Pexp_function _ | Pexp_match _ | Pexp_try _ - | Pexp_fun (_, _, _, {pexp_desc= Pexp_constraint _; _}) ) + | Pexp_fun (_, {pexp_desc= Pexp_constraint _; _}) ) ; _ } , (Match | Let_match | Non_apply) ) |( { pexp_desc= @@ -1072,7 +1072,7 @@ end = struct let rec loop = function | {pexp_desc= Pexp_newtype (_, e); _} -> loop e | {pexp_desc= Pexp_constraint (_, t); _} -> t == typ - | {pexp_desc= Pexp_fun (_, _, _, e); _} -> loop e + | {pexp_desc= Pexp_fun (_, e); _} -> loop e | _ -> false in (match topt with None -> false | Some t -> typ == t) @@ -1237,6 +1237,11 @@ end = struct let check_bindings l = List.exists l ~f:(fun {pvb_pat; _} -> check_subpat pvb_pat) in + let check_function_param param = + match param.pparam_desc with + | Pparam_val (_, _, p) -> p == pat + | Pparam_newtype _ -> false + in match ctx with | Pld (PPat (p1, _)) -> assert (p1 == pat) | Pld _ -> assert false @@ -1293,13 +1298,9 @@ end = struct List.exists cases ~f:(function | {pc_lhs; _} when pc_lhs == pat -> true | _ -> false ) ) - | Pexp_for (p, _, _, _, _) | Pexp_fun (_, _, p, _) -> assert (p == pat) - ) - | Fp ctx -> - assert ( - match ctx.pparam_desc with - | Pparam_val (_, _, p) -> p == pat - | Pparam_newtype _ -> false ) + | Pexp_for (p, _, _, _, _) -> assert (p == pat) + | Pexp_fun (p, _) -> assert (check_function_param p) ) + | Fp ctx -> assert (check_function_param ctx) | Lb x -> assert (x.pvb_pat == pat) | Mb _ -> assert false | Md _ -> assert false @@ -1346,6 +1347,11 @@ end = struct | PStr [{pstr_desc= Pstr_eval (e, _); _}] -> e == exp | _ -> false in + let check_function_param param = + match param.pparam_desc with + | Pparam_val (_, e, _) -> Option.exists e ~f:(fun x -> x == exp) + | Pparam_newtype _ -> false + in match ctx with | Pld (PPat (_, Some e1)) -> assert (e1 == exp) | Pld _ -> assert false @@ -1374,8 +1380,8 @@ end = struct | {pc_guard= Some g; _} when g == exp -> true | {pc_rhs; _} when pc_rhs == exp -> true | _ -> false ) ) - | Pexp_fun (_, default, _, body) -> - assert (Option.value_map default ~default:false ~f || body == exp) + | Pexp_fun (param, body) -> + assert (check_function_param param || body == exp) | Pexp_indexop_access {pia_lhs; pia_kind= Builtin idx; pia_rhs; _} -> assert ( pia_lhs == exp || idx == exp @@ -1424,11 +1430,7 @@ end = struct | Pexp_for (_, e1, e2, _, e3) -> assert (e1 == exp || e2 == exp || e3 == exp) | Pexp_override e1N -> assert (List.exists e1N ~f:snd_f) ) - | Fp ctx -> - assert ( - match ctx.pparam_desc with - | Pparam_val (_, e, _) -> Option.exists e ~f:(fun x -> x == exp) - | Pparam_newtype _ -> false ) + | Fp ctx -> assert (check_function_param ctx) | Lb x -> assert (x.pvb_expr == exp) | Mb _ -> assert false | Md _ -> assert false @@ -1487,7 +1489,7 @@ end = struct match x with | {pexp_desc= Pexp_newtype (_, e); _} -> loop e | {pexp_desc= Pexp_constraint (e, _); _} -> loop e - | {pexp_desc= Pexp_fun (_, _, _, e); _} -> loop e + | {pexp_desc= Pexp_fun (_, e); _} -> loop e | _ -> false in loop e @@ -1983,7 +1985,7 @@ end = struct match exp.pexp_desc with | Pexp_assert e |Pexp_construct (_, Some e) - |Pexp_fun (_, _, _, e) + |Pexp_fun (_, e) |Pexp_ifthenelse (_, Some e) |Pexp_prefix (_, e) |Pexp_infix (_, _, e) @@ -2066,7 +2068,7 @@ end = struct |Pexp_newtype (_, e) |Pexp_open (_, e) |Pexp_letopen (_, e) - |Pexp_fun (_, _, _, e) + |Pexp_fun (_, e) |Pexp_sequence (_, e) |Pexp_setfield (_, _, e) |Pexp_setinstvar (_, e) diff --git a/lib/Fmt_ast.ml b/lib/Fmt_ast.ml index d1a968b406..1dae4955b3 100644 --- a/lib/Fmt_ast.ml +++ b/lib/Fmt_ast.ml @@ -708,7 +708,12 @@ and type_constr_and_body c xbody = ~after:exp.pexp_loc ; let typ_ctx = Exp body in let exp_ctx = - Exp Ast_helper.(Exp.fun_ Nolabel None (Pat.any ()) exp) + let pat = Ast_helper.Pat.any () in + let param = + { pparam_desc= Pparam_val (Nolabel, None, pat) + ; pparam_loc= pat.ppat_loc } + in + Exp Ast_helper.(Exp.fun_ param exp) in ( Some (fmt_type_cstr c ~constraint_ctx:`Fun (sub_typ ~ctx:typ_ctx typ)) , sub_exp ~ctx:exp_ctx exp ) @@ -1236,6 +1241,8 @@ and fmt_pattern ?ext c ?pro ?parens ?(box = false) and fmt_fun_args c args = let fmt_fun_arg (a : function_param) = let ctx = Fp a in + Cmts.fmt c a.pparam_loc + @@ match a.pparam_desc with | Pparam_val ( ((Labelled l | Optional l) as lbl) @@ -1963,7 +1970,7 @@ and fmt_expression c ?(box = true) ?(pro = noop) ?eol ?parens if parens || not dock_fun_arg then (noop, pro) else (pro, noop) in match last_arg.pexp_desc with - | Pexp_fun (_, _, _, eN1_body) + | Pexp_fun (_, eN1_body) when List.for_all args_before ~f:(fun (_, eI) -> is_simple c.conf (fun _ -> 0) (sub_exp ~ctx eI) ) -> (* Last argument is a [fun _ ->]. *) diff --git a/lib/Sugar.ml b/lib/Sugar.ml index 3da55f7a19..f692d4eb7d 100644 --- a/lib/Sugar.ml +++ b/lib/Sugar.ml @@ -14,37 +14,9 @@ open Asttypes open Ast open Extended_ast -(* Temporary. Won't be necessary once the type [function_param] is used in - [Pexp_fun] and [Pcl_fun]. *) -let mk_function_param pparam_desc = - let pparam_loc = - let init, locs = - match pparam_desc with - | Pparam_val (lbl, e, p) -> - let locs = - match lbl with - | Nolabel -> [] - | Labelled x -> [x.loc] - | Optional x -> [x.loc] - in - let locs = - match e with Some e -> e.pexp_loc :: locs | None -> locs - in - (p.ppat_loc, locs) - | Pparam_newtype types -> ( - match types with - | [] -> failwith "Pparam_newtype always contains at least one type" - | hd :: tl -> - let locs = List.map tl ~f:(fun x -> x.loc) in - (hd.loc, locs) ) - in - let min acc x = if Location.compare_start acc x < 0 then acc else x in - let max acc x = if Location.compare_end acc x > 0 then acc else x in - let loc_start = (List.fold_left locs ~init ~f:min).loc_start in - let loc_end = (List.fold_left locs ~init ~f:max).loc_end in - {Location.loc_start; loc_end; loc_ghost= true} - in - {pparam_desc; pparam_loc} +let mk_function_param {Location.loc_start; _} {Location.loc_end; _} p = + let pparam_loc = {Location.loc_start; loc_end; loc_ghost= true} in + {pparam_desc= p; pparam_loc} let fun_ cmts ?(will_keep_first_ast_node = true) xexp = let rec fun_ ?(will_keep_first_ast_node = false) ({ast= exp; _} as xexp) = @@ -52,13 +24,12 @@ let fun_ cmts ?(will_keep_first_ast_node = true) xexp = let {pexp_desc; pexp_loc; pexp_attributes; _} = exp in if will_keep_first_ast_node || List.is_empty pexp_attributes then match pexp_desc with - | Pexp_fun (label, default, pattern, body) -> + | Pexp_fun (p, body) -> if not will_keep_first_ast_node then - Cmts.relocate cmts ~src:pexp_loc ~before:pattern.ppat_loc + Cmts.relocate cmts ~src:pexp_loc ~before:p.pparam_loc ~after:body.pexp_loc ; let xargs, xbody = fun_ (sub_exp ~ctx body) in - ( mk_function_param (Pparam_val (label, default, pattern)) :: xargs - , xbody ) + (p :: xargs, xbody) | Pexp_newtype (name, body) -> if not will_keep_first_ast_node then Cmts.relocate cmts ~src:pexp_loc ~before:body.pexp_loc @@ -66,9 +37,12 @@ let fun_ cmts ?(will_keep_first_ast_node = true) xexp = let xargs, xbody = fun_ (sub_exp ~ctx body) in let xargs = match xargs with - | {pparam_desc= Pparam_newtype names; _} :: xargs -> - mk_function_param (Pparam_newtype (name :: names)) :: xargs - | xargs -> mk_function_param (Pparam_newtype [name]) :: xargs + | {pparam_desc= Pparam_newtype names; pparam_loc} :: xargs -> + let param = Pparam_newtype (name :: names) in + mk_function_param name.loc pparam_loc param :: xargs + | xargs -> + let param = Pparam_newtype [name] in + mk_function_param name.loc name.loc param :: xargs in (xargs, xbody) | _ -> ([], xexp) @@ -83,12 +57,12 @@ let cl_fun ?(will_keep_first_ast_node = true) cmts xexp = if will_keep_first_ast_node || List.is_empty pcl_attributes then match pcl_desc with | Pcl_fun (label, default, pattern, body) -> + let before = pattern.ppat_loc and after = body.pcl_loc in if not will_keep_first_ast_node then - Cmts.relocate cmts ~src:pcl_loc ~before:pattern.ppat_loc - ~after:body.pcl_loc ; + Cmts.relocate cmts ~src:pcl_loc ~before ~after ; let xargs, xbody = fun_ (sub_cl ~ctx body) in - ( mk_function_param (Pparam_val (label, default, pattern)) :: xargs - , xbody ) + let param = Pparam_val (label, default, pattern) in + (mk_function_param before after param :: xargs, xbody) | _ -> ([], xexp) else ([], xexp) in @@ -230,7 +204,11 @@ module Let_binding = struct won't be necessary once the normalization is moved to [Extended_ast]. *) let pat = Ast_helper.Pat.any () in - Exp (Ast_helper.Exp.fun_ Nolabel None pat exp) + let param = + { pparam_desc= Pparam_val (Nolabel, None, pat) + ; pparam_loc= pat.ppat_loc } + in + Exp (Ast_helper.Exp.fun_ param exp) in (xargs, `Other (sub_typ ~ctx:typ_ctx typ), sub_exp ~ctx:exp_ctx exp) (* The type constraint is always printed before the declaration for diff --git a/test/passing/tests/class_expr.ml b/test/passing/tests/class_expr.ml index 5040c62ba5..cc5a371a3a 100644 --- a/test/passing/tests/class_expr.ml +++ b/test/passing/tests/class_expr.ml @@ -3,3 +3,5 @@ class c (`I i) = x class c `I = x class c i = x + +class c (* xx *) i (* yy *) = x diff --git a/test/passing/tests/label_option_default_args.ml.ref b/test/passing/tests/label_option_default_args.ml.ref index 09efc00fa6..0f3af3adea 100644 --- a/test/passing/tests/label_option_default_args.ml.ref +++ b/test/passing/tests/label_option_default_args.ml.ref @@ -4,94 +4,65 @@ let (* 0 *) f (* 1 *) x (* 2 *) = (* 3 *) e let f ~x = e -let (* 0 *) f ~(* 1 *) x (* 2 *) = (* 3 *) e +let (* 0 *) f (* 1 *) ~x (* 2 *) = (* 3 *) e let f ~(x : t) = e -let (* 0 *) f - ~(* 1 *) - (* 2 *) - (x (* 3 *) : (* 4 *) t (* 5 *)) = - (* 6 *) - (* 7 *) - e +let (* 0 *) f (* 1 *) ~(* 2 *) (x (* 3 *) : (* 4 *) t (* 5 *)) (* 6 *) = + (* 7 *) e let f ~l:x = e -let (* 0 *) f - ~l: - (* 1 *) - (* 2 *) x (* 3 *) = - (* 4 *) e +let (* 0 *) f (* 1 *) ~l:(* 2 *) x (* 3 *) = (* 4 *) e let f ~l:{f; g} = e -let (* 0 *) f - ~l: - (* 1 *) - (* 2 *) {(* 3 *) f (* 4 *); (* 5 *) g (* 6 *)} (* 7 *) = +let (* 0 *) f (* 1 *) ~l:(* 2 *) {(* 3 *) f (* 4 *); (* 5 *) g (* 6 *)} + (* 7 *) = e let f ~x:({f; g} as x) = e -let (* 0 *) f - ~x:(* 1 *) ((* 2 *) {f; g} (* 3 *) as (* 4 *) x (* 5 *)) (* 6 *) = +let (* 0 *) f (* 1 *) ~x:((* 2 *) {f; g} (* 3 *) as (* 4 *) x (* 5 *)) + (* 6 *) = e let f ?x = e -let (* 0 *) f - ?(* 1 *) - (* 2 *) x (* 3 *) = - e +let (* 0 *) f (* 1 *) ?(* 2 *) x (* 3 *) = e let f ?(x : t) = e -let (* 0 *) f - ?(* 1 *) - (* 2 *) - (x (* 3 *) : (* 4 *) t (* 5 *)) = - (* 6 *) e +let (* 0 *) f (* 1 *) ?(* 2 *) (x (* 3 *) : (* 4 *) t (* 5 *)) (* 6 *) = e let f ?l:x = e -let (* 0 *) f - ?l: - (* 1 *) - (* 2 *) x (* 3 *) = - e +let (* 0 *) f (* 1 *) ?l:(* 2 *) x (* 3 *) = e let f ?l:(C x) = e -let (* 0 *) f +let (* 0 *) f (* 1 *) ?l: - ((* 1 *) - (* 2 *) - (* 3 *) C (* 4 *) x (* 5 *) ) = - (* 6 *) e + ((* 2 *) + (* 3 *) C (* 4 *) x (* 5 *) ) (* 6 *) = + e let f ?(x = d) = e -let (* 0 *) f - ?((* 1 *) - (* 2 *) x (* 3 *) = (* 4 *) d (* 5 *)) = - (* 6 *) e +let (* 0 *) f (* 1 *) ?((* 2 *) x (* 3 *) = (* 4 *) d (* 5 *)) (* 6 *) = e let f ?(x : t = d) = e -let (* 0 *) f - ?((* 1 *) - (* 2 *) - x (* 3 *) : (* 4 *) t (* 5 *) = (* 6 *) d (* 7 *)) = - (* 8 *) e +let (* 0 *) f (* 1 *) + ?((* 2 *) x (* 3 *) : (* 4 *) t (* 5 *) = (* 6 *) d (* 7 *)) (* 8 *) = + e let f ?(x = (d : t)) = e -let (* 0 *) f - ?((* 1 *) - (* 2 *) x (* 3 *) = - (* 4 *) ((* 5 *) d (* 6 *) : (* 7 *) t (* 8 *)) (* 9 *)) = - (* 10 *) e +let (* 0 *) f (* 1 *) + ?((* 2 *) x (* 3 *) = + (* 4 *) ((* 5 *) d (* 6 *) : (* 7 *) t (* 8 *)) (* 9 *)) (* 10 *) = + e let f ?l:(x = d) = e @@ -99,23 +70,21 @@ let f ?l:(x = (d : t)) = e let f ?l:(x : t = d) = e -let (* 0 *) f +let (* 0 *) f (* 1 *) ?l: - ((* 1 *) - (* 2 *) + ((* 2 *) (* 3 *) - x (* 4 *) : (* 5 *) t (* 6 *) = (* 7 *) d (* 8 *)) = - (* 9 *) e + x (* 4 *) : (* 5 *) t (* 6 *) = (* 7 *) d (* 8 *)) (* 9 *) = + e let f ?l:(C x = d) = e -let (* 0 *) f +let (* 0 *) f (* 1 *) ?l: - ((* 1 *) - (* 2 *) + ((* 2 *) (* 3 *) - C (* 4 *) x (* 5 *) = (* 6 *) d (* 7 *)) = - (* 8 *) e + C (* 4 *) x (* 5 *) = (* 6 *) d (* 7 *)) (* 8 *) = + e (* Regression tests for https://github.com/ocaml-ppx/ocamlformat/issues/1260 (optional argument rebound to non-variable without necessary parens). *) diff --git a/vendor/parser-extended/ast_helper.ml b/vendor/parser-extended/ast_helper.ml index 442ce1b63f..130031920f 100644 --- a/vendor/parser-extended/ast_helper.ml +++ b/vendor/parser-extended/ast_helper.ml @@ -122,7 +122,7 @@ module Exp = struct let ident ?loc ?attrs a = mk ?loc ?attrs (Pexp_ident a) let constant ?loc ?attrs a = mk ?loc ?attrs (Pexp_constant a) let let_ ?loc ?attrs a b = mk ?loc ?attrs (Pexp_let (a, b)) - let fun_ ?loc ?attrs a b c d = mk ?loc ?attrs (Pexp_fun (a, b, c, d)) + let fun_ ?loc ?attrs a b = mk ?loc ?attrs (Pexp_fun (a, b)) let function_ ?loc ?attrs a = mk ?loc ?attrs (Pexp_function a) let apply ?loc ?attrs a b = mk ?loc ?attrs (Pexp_apply (a, b)) let match_ ?loc ?attrs a b = mk ?loc ?attrs (Pexp_match (a, b)) diff --git a/vendor/parser-extended/ast_mapper.ml b/vendor/parser-extended/ast_mapper.ml index 00e1fe4e35..5d12a04110 100644 --- a/vendor/parser-extended/ast_mapper.ml +++ b/vendor/parser-extended/ast_mapper.ml @@ -502,11 +502,9 @@ module E = struct | Pexp_let (lbs, e) -> let_ ~loc ~attrs (sub.value_bindings sub lbs) (sub.expr sub e) - | Pexp_fun (lab, def, p, e) -> + | Pexp_fun (p, e) -> fun_ ~loc ~attrs - (sub.arg_label sub lab) - (map_opt (sub.expr sub) def) - (sub.pat sub p) + (map_function_param sub p) (sub.expr sub e) | Pexp_function pel -> function_ ~loc ~attrs (sub.cases sub pel) | Pexp_apply (e, l) -> diff --git a/vendor/parser-extended/parser.mly b/vendor/parser-extended/parser.mly index dc65b85aa2..62ff0d7912 100644 --- a/vendor/parser-extended/parser.mly +++ b/vendor/parser-extended/parser.mly @@ -2268,9 +2268,8 @@ expr: Pexp_letopen(od, $7), $4 } | FUNCTION ext_attributes match_cases { Pexp_function $3, $2 } - | FUN ext_attributes labeled_simple_pattern fun_def - { let (l,o,p) = $3 in - Pexp_fun(l, o, p, $4), $2 } + | FUN ext_attributes fun_param fun_def + { Pexp_fun($3, $4), $2 } | FUN ext_attributes LPAREN TYPE lident_list RPAREN fun_def { (mk_newtypes ~loc:$sloc $5 $7).pexp_desc, $2 } | MATCH ext_attributes seq_expr WITH match_cases @@ -2579,8 +2578,8 @@ fun_binding: strict_binding: EQUAL seq_expr { $2 } - | labeled_simple_pattern fun_binding - { let (l, o, p) = $1 in ghexp ~loc:$sloc (Pexp_fun(l, o, p, $2)) } + | fun_param fun_binding + { ghexp ~loc:$sloc (Pexp_fun($1, $2)) } | LPAREN TYPE lident_list RPAREN fun_binding { mk_newtypes ~loc:$sloc $3 $5 } ; @@ -2596,6 +2595,11 @@ match_case: | pattern MINUSGREATER DOT { Exp.case $1 (Exp.unreachable ~loc:(make_loc $loc($3)) ()) } ; +fun_param: + | labeled_simple_pattern + { let l, o, p = $1 in + { pparam_loc = make_loc $sloc; pparam_desc = Pparam_val (l, o, p) } } +; fun_def: MINUSGREATER seq_expr { $2 } @@ -2603,11 +2607,8 @@ fun_def: { Pexp_constraint ($4, $2) }) { $1 } /* Cf #5939: we used to accept (fun p when e0 -> e) */ - | labeled_simple_pattern fun_def - { - let (l,o,p) = $1 in - ghexp ~loc:$sloc (Pexp_fun(l, o, p, $2)) - } + | fun_param fun_def + { ghexp ~loc:$sloc (Pexp_fun($1, $2)) } | LPAREN TYPE lident_list RPAREN fun_def { mk_newtypes ~loc:$sloc $3 $5 } ; diff --git a/vendor/parser-extended/parsetree.mli b/vendor/parser-extended/parsetree.mli index f1ec4ec835..3725a4e02f 100644 --- a/vendor/parser-extended/parsetree.mli +++ b/vendor/parser-extended/parsetree.mli @@ -321,28 +321,13 @@ and expression_desc = when [flag] is {{!Asttypes.rec_flag.Recursive}[Recursive]}. *) | Pexp_function of case list (** [function P1 -> E1 | ... | Pn -> En] *) - | Pexp_fun of arg_label * expression option * pattern * expression - (** [Pexp_fun(lbl, exp0, P, E1)] represents: - - [fun P -> E1] - when [lbl] is {{!Asttypes.arg_label.Nolabel}[Nolabel]} - and [exp0] is [None] - - [fun ~l:P -> E1] - when [lbl] is {{!Asttypes.arg_label.Labelled}[Labelled l]} - and [exp0] is [None] - - [fun ?l:P -> E1] - when [lbl] is {{!Asttypes.arg_label.Optional}[Optional l]} - and [exp0] is [None] - - [fun ?l:(P = E0) -> E1] - when [lbl] is {{!Asttypes.arg_label.Optional}[Optional l]} - and [exp0] is [Some E0] - - Notes: - - If [E0] is provided, only - {{!Asttypes.arg_label.Optional}[Optional]} is allowed. - - [fun P1 P2 .. Pn -> E1] is represented as nested - {{!expression_desc.Pexp_fun}[Pexp_fun]}. - - [let f P = E] is represented using - {{!expression_desc.Pexp_fun}[Pexp_fun]}. + | Pexp_fun of function_param * expression + (** [Pexp_fun(P, E)] represents: + - [fun P -> E] + - [fun ~l:P -> E] + - [fun ?l:P -> E] + - [fun ?l:(P = E0) -> E] + - [fun (type t) -> E] *) | Pexp_apply of expression * (arg_label * expression) list (** [Pexp_apply(E0, [(l1, E1) ; ... ; (ln, En)])] diff --git a/vendor/parser-extended/printast.ml b/vendor/parser-extended/printast.ml index 32e671b0b7..f6f1404514 100644 --- a/vendor/parser-extended/printast.ml +++ b/vendor/parser-extended/printast.ml @@ -345,11 +345,9 @@ and expression i ppf x = | Pexp_function l -> line i ppf "Pexp_function\n"; list i case ppf l; - | Pexp_fun (l, eo, p, e) -> + | Pexp_fun (p, e) -> line i ppf "Pexp_fun\n"; - arg_label i ppf l; - option i expression ppf eo; - pattern i ppf p; + function_param i ppf p; expression i ppf e; | Pexp_apply (e, l) -> line i ppf "Pexp_apply\n";