Skip to content

Commit

Permalink
Fix stackoverflow in unparseExprRecurse
Browse files Browse the repository at this point in the history
This is an alternative to reasonml#2289 that also renames `literalAttrs` to
`stylisticAttrs` as request in reasonml#2223's code review
  • Loading branch information
anmonteiro committed Dec 11, 2018
1 parent ec52ded commit 2413278
Show file tree
Hide file tree
Showing 8 changed files with 40 additions and 25 deletions.
3 changes: 3 additions & 0 deletions formatTest/unit_tests/expected_output/jsx.re
Original file line number Diff line number Diff line change
Expand Up @@ -612,3 +612,6 @@ ReasonReact.(<> {string("Test")} </>);
?id className={Cn.make({"a": b})} onClick>
{"Submit" |> ste}
</button>;

// shouldn't result in a stack overflow
<X y={z->Belt.Option.getWithDefault("")} />;
3 changes: 3 additions & 0 deletions formatTest/unit_tests/input/jsx.re
Original file line number Diff line number Diff line change
Expand Up @@ -489,3 +489,6 @@ ReasonReact.(<> {string("Test")} </>);
<button ?id className={Cn.make({"a": b})} onClick>
{"Submit" |> ste}
</button>;

// shouldn't result in a stack overflow
<X y={z->Belt.Option.getWithDefault("")} />;
24 changes: 16 additions & 8 deletions src/reason-parser/reason_attributes.ml
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,15 @@ type attributesPartition = {
docAttrs : attributes;
stdAttrs : attributes;
jsxAttrs : attributes;
literalAttrs : attributes;
stylisticAttrs : attributes;
uncurried : bool
}

(** Partition attributes into kinds *)
let rec partitionAttributes ?(partDoc=false) ?(allowUncurry=true) attrs : attributesPartition =
match attrs with
| [] ->
{arityAttrs=[]; docAttrs=[]; stdAttrs=[]; jsxAttrs=[]; literalAttrs=[]; uncurried = false}
{arityAttrs=[]; docAttrs=[]; stdAttrs=[]; jsxAttrs=[]; stylisticAttrs=[]; uncurried = false}
| (({txt = "bs"}, PStr []) as attr)::atTl ->
let partition = partitionAttributes ~partDoc ~allowUncurry atTl in
if allowUncurry then
Expand All @@ -37,10 +37,10 @@ let rec partitionAttributes ?(partDoc=false) ?(allowUncurry=true) attrs : attrib
{partition with docAttrs=doc::partition.docAttrs}
| (({txt="reason.raw_literal"}, _) as attr) :: atTl ->
let partition = partitionAttributes ~partDoc ~allowUncurry atTl in
{partition with literalAttrs=attr::partition.literalAttrs}
{partition with stylisticAttrs=attr::partition.stylisticAttrs}
| (({txt="reason.preserve_braces"}, _) as attr) :: atTl ->
let partition = partitionAttributes ~partDoc ~allowUncurry atTl in
{partition with literalAttrs=attr::partition.literalAttrs}
{partition with stylisticAttrs=attr::partition.stylisticAttrs}
| atHd :: atTl ->
let partition = partitionAttributes ~partDoc ~allowUncurry atTl in
{partition with stdAttrs=atHd::partition.stdAttrs}
Expand All @@ -59,9 +59,9 @@ let extract_raw_literal attrs =
in
loop [] attrs

let without_literal_attrs attrs =
let without_stylistic_attrs attrs =
let rec loop acc = function
| attr :: rest when (partitionAttributes [attr]).literalAttrs != [] ->
| attr :: rest when (partitionAttributes [attr]).stylisticAttrs != [] ->
loop acc rest
| [] -> List.rev acc
| attr :: rest -> loop (attr :: acc) rest
Expand All @@ -71,6 +71,14 @@ let without_literal_attrs attrs =
let is_preserve_braces_attr ({txt}, _) =
txt = "reason.preserve_braces"

let has_preserve_braces_attrs literalAttrs =
(List.filter is_preserve_braces_attr literalAttrs) != []
let has_preserve_braces_attrs stylisticAttrs =
(List.filter is_preserve_braces_attr stylisticAttrs) != []

let maybe_remove_stylistic_attrs attrs should_preserve =
if should_preserve then
attrs
else
List.filter (function
| ({txt="reason.raw_literal"}, _) -> true
| _ -> false)
attrs
15 changes: 8 additions & 7 deletions src/reason-parser/reason_pprint_ast.ml
Original file line number Diff line number Diff line change
Expand Up @@ -3919,15 +3919,16 @@ let printer = object(self:'self)
let x = self#process_underscore_application x in
(* If there are any attributes, render unary like `(~-) x [@ppx]`, and infix like `(+) x y [@attr]` *)
let {arityAttrs; stdAttrs; jsxAttrs; literalAttrs; uncurried} =
let {arityAttrs; stdAttrs; jsxAttrs; stylisticAttrs; uncurried} =
partitionAttributes ~allowUncurry:(Reason_heuristics.bsExprCanBeUncurried x) x.pexp_attributes
in
let stylisticAttrs = Reason_attributes.maybe_remove_stylistic_attrs stylisticAttrs preserve_braces in
let () = if uncurried then Hashtbl.add uncurriedTable x.pexp_loc true in
let x = {x with pexp_attributes = (literalAttrs @ arityAttrs @ stdAttrs @ jsxAttrs) } in
let x = {x with pexp_attributes = (stylisticAttrs @ arityAttrs @ stdAttrs @ jsxAttrs) } in
(* If there's any attributes, recurse without them, then apply them to
the ends of functions, or simplify infix printings then append. *)
if stdAttrs != [] then
let withoutVisibleAttrs = {x with pexp_attributes=(literalAttrs @ arityAttrs @ jsxAttrs)} in
let withoutVisibleAttrs = {x with pexp_attributes=(stylisticAttrs @ arityAttrs @ jsxAttrs)} in
let attributesAsList = (List.map self#attribute stdAttrs) in
let itms = match self#unparseExprRecurse withoutVisibleAttrs with
| SpecificInfixPrecedence ({reducePrecedence}, wrappedRule) ->
Expand Down Expand Up @@ -4412,7 +4413,7 @@ let printer = object(self:'self)
let dotdotdotChild = match expr with
| {pexp_desc = Pexp_apply (funExpr, args)}
when printedStringAndFixityExpr funExpr == Normal &&
Reason_attributes.without_literal_attrs expr.pexp_attributes == [] ->
Reason_attributes.without_stylistic_attrs expr.pexp_attributes == [] ->
begin match (self#formatFunAppl ~prefix:(atom "...") ~wrap:("{", "}") ~jsxAttrs:[] ~args ~funExpr ~applicationExpr:expr ()) with
| [x] -> x
| xs -> makeList xs
Expand Down Expand Up @@ -4451,7 +4452,7 @@ let printer = object(self:'self)
(self#formatNonSequencyExpression e)
| Pexp_apply ({pexp_desc = Pexp_ident _} as funExpr, args)
when printedStringAndFixityExpr funExpr == Normal &&
Reason_attributes.without_literal_attrs expression.pexp_attributes == [] ->
Reason_attributes.without_stylistic_attrs expression.pexp_attributes == [] ->
let lhs = makeList [atom lbl; atom "="] in
begin match (
self#formatFunAppl
Expand Down Expand Up @@ -6065,13 +6066,13 @@ let printer = object(self:'self)
| _ -> assert false
method should_preserve_requested_braces expr =
let {literalAttrs} = partitionAttributes expr.pexp_attributes in
let {stylisticAttrs} = partitionAttributes expr.pexp_attributes in
match expr.pexp_desc with
| Pexp_ifthenelse _
| Pexp_try _ -> false
| _ ->
preserve_braces &&
Reason_attributes.has_preserve_braces_attrs literalAttrs
Reason_attributes.has_preserve_braces_attrs stylisticAttrs
method simplest_expression x =
let {stdAttrs; jsxAttrs} = partitionAttributes x.pexp_attributes in
Expand Down
14 changes: 7 additions & 7 deletions src/reason-parser/reason_syntax_util.ml
Original file line number Diff line number Diff line change
Expand Up @@ -395,34 +395,34 @@ let identifier_mapper f super =
end;
}

let remove_literal_attrs_mapper_maker super =
let remove_stylistic_attrs_mapper_maker super =
let open Ast_404 in
let open Ast_mapper in
{ super with
expr = begin fun mapper expr ->
let {Reason_attributes.literalAttrs; arityAttrs; docAttrs; stdAttrs; jsxAttrs} =
let {Reason_attributes.stylisticAttrs; arityAttrs; docAttrs; stdAttrs; jsxAttrs} =
Reason_attributes.partitionAttributes ~allowUncurry:false expr.pexp_attributes
in
let expr = if literalAttrs != [] then
let expr = if stylisticAttrs != [] then
{ expr with pexp_attributes = arityAttrs @ docAttrs @ stdAttrs @ jsxAttrs }
else expr
in
super.expr mapper expr
end;
pat = begin fun mapper pat ->
let {Reason_attributes.literalAttrs; arityAttrs; docAttrs; stdAttrs; jsxAttrs} =
let {Reason_attributes.stylisticAttrs; arityAttrs; docAttrs; stdAttrs; jsxAttrs} =
Reason_attributes.partitionAttributes ~allowUncurry:false pat.ppat_attributes
in
let pat = if literalAttrs != [] then
let pat = if stylisticAttrs != [] then
{ pat with ppat_attributes = arityAttrs @ docAttrs @ stdAttrs @ jsxAttrs }
else pat
in
super.pat mapper pat
end;
}

let remove_literal_attrs_mapper =
remove_literal_attrs_mapper_maker Ast_mapper.default_mapper
let remove_stylistic_attrs_mapper =
remove_stylistic_attrs_mapper_maker Ast_mapper.default_mapper

(** escape_stars_slashes_mapper escapes all stars and slashes in an AST *)
let escape_stars_slashes_mapper =
Expand Down
2 changes: 1 addition & 1 deletion src/reason-parser/reason_syntax_util.mli
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ val syntax_error_extension_node :
Ast_404.Location.t ->
string -> string Ast_404.Location.loc * Ast_404.Parsetree.payload

val remove_literal_attrs_mapper : Ast_404.Ast_mapper.mapper
val remove_stylistic_attrs_mapper : Ast_404.Ast_mapper.mapper

val escape_stars_slashes_mapper :
Ast_404.Ast_mapper.mapper -> Ast_404.Ast_mapper.mapper
Expand Down
2 changes: 1 addition & 1 deletion src/reason-parser/reason_toolchain.ml
Original file line number Diff line number Diff line change
Expand Up @@ -495,7 +495,7 @@ module OCaml_syntax = struct
(To_current.copy_signature signature)
let format_implementation_with_comments (structure, _) formatter =
let structure =
Reason_syntax_util.(apply_mapper_to_structure structure remove_literal_attrs_mapper)
Reason_syntax_util.(apply_mapper_to_structure structure remove_stylistic_attrs_mapper)
in
Pprintast.structure formatter
(To_current.copy_structure structure)
Expand Down
2 changes: 1 addition & 1 deletion src/refmt/reason_implementation_printer.ml
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ let print printtype filename parsedAsML output_chan output_formatter =
)
| `Binary -> fun (ast, _) ->
let ast =
Reason_syntax_util.(apply_mapper_to_structure ast remove_literal_attrs_mapper)
Reason_syntax_util.(apply_mapper_to_structure ast remove_stylistic_attrs_mapper)
in
Ast_io.to_channel output_chan filename
(Ast_io.Impl ((module OCaml_current),
Expand Down

0 comments on commit 2413278

Please sign in to comment.