From 8a2753e095df184de2b838a7e47cc7c7dec22441 Mon Sep 17 00:00:00 2001 From: Cristiano Calcagno Date: Tue, 17 Jan 2023 14:30:29 +0100 Subject: [PATCH 1/4] Fix some comments disappearing in array sugar. The ids for `Array.get` and `Array.set` used to desugar array access have location that overlaps with the arguments, stealing the comments. This takes care of some examples similar to the one in https://github.com/rescript-lang/rescript-compiler/issues/5946 but not that specific example. --- res_syntax/src/res_core.ml | 11 ++++------- res_syntax/tests/printer/comments/array.res | 10 ++++++++++ .../tests/printer/comments/expected/array.res.txt | 5 +++++ 3 files changed, 19 insertions(+), 7 deletions(-) create mode 100644 res_syntax/tests/printer/comments/array.res create mode 100644 res_syntax/tests/printer/comments/expected/array.res.txt diff --git a/res_syntax/src/res_core.ml b/res_syntax/src/res_core.ml index 6f054d8f3e..919a43dafd 100644 --- a/res_syntax/src/res_core.ml +++ b/res_syntax/src/res_core.ml @@ -1976,7 +1976,6 @@ and parseFirstClassModuleExpr ~startPos p = and parseBracketAccess p expr startPos = Parser.leaveBreadcrumb p Grammar.ExprArrayAccess; - let lbracket = p.startPos in Parser.expect Lbracket p; let stringStart = p.startPos in match p.Parser.token with @@ -2009,20 +2008,18 @@ and parseBracketAccess p expr startPos = let accessExpr = parseConstrainedOrCoercedExpr p in Parser.expect Rbracket p; Parser.eatBreadcrumb p; - let rbracket = p.prevEndPos in - let arrayLoc = mkLoc lbracket rbracket in match p.token with | Equal -> Parser.leaveBreadcrumb p ExprArrayMutation; Parser.next p; let rhsExpr = parseExpr p in let arraySet = - Location.mkloc (Longident.Ldot (Lident "Array", "set")) arrayLoc + Location.mknoloc (Longident.Ldot (Lident "Array", "set")) in let endPos = p.prevEndPos in let arraySet = Ast_helper.Exp.apply ~loc:(mkLoc startPos endPos) - (Ast_helper.Exp.ident ~loc:arrayLoc arraySet) + (Ast_helper.Exp.ident arraySet) [(Nolabel, expr); (Nolabel, accessExpr); (Nolabel, rhsExpr)] in Parser.eatBreadcrumb p; @@ -2031,8 +2028,8 @@ and parseBracketAccess p expr startPos = let endPos = p.prevEndPos in let e = Ast_helper.Exp.apply ~loc:(mkLoc startPos endPos) - (Ast_helper.Exp.ident ~loc:arrayLoc - (Location.mkloc (Longident.Ldot (Lident "Array", "get")) arrayLoc)) + (Ast_helper.Exp.ident + (Location.mknoloc (Longident.Ldot (Lident "Array", "get")))) [(Nolabel, expr); (Nolabel, accessExpr)] in parsePrimaryExpr ~operand:e p) diff --git a/res_syntax/tests/printer/comments/array.res b/res_syntax/tests/printer/comments/array.res new file mode 100644 index 0000000000..47f83acfd6 --- /dev/null +++ b/res_syntax/tests/printer/comments/array.res @@ -0,0 +1,10 @@ +a[ /* zz */ 0 ] = 7 + +let _ = ( + /* zz */ a + )[0] + +let _ = ( + a // zz + )[0] + diff --git a/res_syntax/tests/printer/comments/expected/array.res.txt b/res_syntax/tests/printer/comments/expected/array.res.txt new file mode 100644 index 0000000000..61efcf1348 --- /dev/null +++ b/res_syntax/tests/printer/comments/expected/array.res.txt @@ -0,0 +1,5 @@ +a[/* zz */ 0] = 7 + +let _ = /* zz */ a[0] + +let _ = a[0] // zz From a41edddd600a6ff9032cd808f72e8942d8508acb Mon Sep 17 00:00:00 2001 From: Cristiano Calcagno Date: Tue, 17 Jan 2023 14:34:57 +0100 Subject: [PATCH 2/4] Original example. --- res_syntax/tests/printer/comments/array.res | 8 ++++++++ res_syntax/tests/printer/comments/expected/array.res.txt | 7 +++++++ 2 files changed, 15 insertions(+) diff --git a/res_syntax/tests/printer/comments/array.res b/res_syntax/tests/printer/comments/array.res index 47f83acfd6..8882f172e8 100644 --- a/res_syntax/tests/printer/comments/array.res +++ b/res_syntax/tests/printer/comments/array.res @@ -8,3 +8,11 @@ let _ = ( a // zz )[0] + ( + incidents + ->Belt.Array.keep(({status}) => status === #OPEN) + // This comment will vanish + ->Belt.SortArray.stableSortBy((a, b) => + compare(a.createdTime, b.createdTime) + ) + )[0] \ No newline at end of file diff --git a/res_syntax/tests/printer/comments/expected/array.res.txt b/res_syntax/tests/printer/comments/expected/array.res.txt index 61efcf1348..90ed9ed5c8 100644 --- a/res_syntax/tests/printer/comments/expected/array.res.txt +++ b/res_syntax/tests/printer/comments/expected/array.res.txt @@ -3,3 +3,10 @@ a[/* zz */ 0] = 7 let _ = /* zz */ a[0] let _ = a[0] // zz + +( + incidents + ->Belt.Array.keep(({status}) => status === #OPEN) + // This comment will vanish + ->Belt.SortArray.stableSortBy((a, b) => compare(a.createdTime, b.createdTime)) +)[0] From ee5fca5d90c2a3f0bfba29c1d560308f1766fb6d Mon Sep 17 00:00:00 2001 From: Cristiano Calcagno Date: Wed, 18 Jan 2023 09:18:56 +0100 Subject: [PATCH 3/4] Change comment attachment instead of parser location of array functions. --- res_syntax/src/res_comments_table.ml | 11 +++++++++++ res_syntax/src/res_core.ml | 11 +++++++---- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/res_syntax/src/res_comments_table.ml b/res_syntax/src/res_comments_table.ml index ed07ec2f97..f6ce468db5 100644 --- a/res_syntax/src/res_comments_table.ml +++ b/res_syntax/src/res_comments_table.ml @@ -1297,6 +1297,17 @@ and walkExpression expr t comments = walkExpression operand2 t inside; (* (List.concat [inside; after]); *) attach t.trailing operand2.pexp_loc after + | Pexp_apply + ( {pexp_desc = Pexp_ident {txt = Longident.Ldot (Lident "Array", "get")}}, + [(Nolabel, parentExpr); (Nolabel, memberExpr)] ) -> + walkList [Expression parentExpr; Expression memberExpr] t comments + | Pexp_apply + ( {pexp_desc = Pexp_ident {txt = Longident.Ldot (Lident "Array", "set")}}, + [(Nolabel, parentExpr); (Nolabel, memberExpr); (Nolabel, targetExpr)] ) + -> + walkList + [Expression parentExpr; Expression memberExpr; Expression targetExpr] + t comments | Pexp_apply (callExpr, arguments) -> let before, inside, after = partitionByLoc comments callExpr.pexp_loc in let after = diff --git a/res_syntax/src/res_core.ml b/res_syntax/src/res_core.ml index 919a43dafd..6f054d8f3e 100644 --- a/res_syntax/src/res_core.ml +++ b/res_syntax/src/res_core.ml @@ -1976,6 +1976,7 @@ and parseFirstClassModuleExpr ~startPos p = and parseBracketAccess p expr startPos = Parser.leaveBreadcrumb p Grammar.ExprArrayAccess; + let lbracket = p.startPos in Parser.expect Lbracket p; let stringStart = p.startPos in match p.Parser.token with @@ -2008,18 +2009,20 @@ and parseBracketAccess p expr startPos = let accessExpr = parseConstrainedOrCoercedExpr p in Parser.expect Rbracket p; Parser.eatBreadcrumb p; + let rbracket = p.prevEndPos in + let arrayLoc = mkLoc lbracket rbracket in match p.token with | Equal -> Parser.leaveBreadcrumb p ExprArrayMutation; Parser.next p; let rhsExpr = parseExpr p in let arraySet = - Location.mknoloc (Longident.Ldot (Lident "Array", "set")) + Location.mkloc (Longident.Ldot (Lident "Array", "set")) arrayLoc in let endPos = p.prevEndPos in let arraySet = Ast_helper.Exp.apply ~loc:(mkLoc startPos endPos) - (Ast_helper.Exp.ident arraySet) + (Ast_helper.Exp.ident ~loc:arrayLoc arraySet) [(Nolabel, expr); (Nolabel, accessExpr); (Nolabel, rhsExpr)] in Parser.eatBreadcrumb p; @@ -2028,8 +2031,8 @@ and parseBracketAccess p expr startPos = let endPos = p.prevEndPos in let e = Ast_helper.Exp.apply ~loc:(mkLoc startPos endPos) - (Ast_helper.Exp.ident - (Location.mknoloc (Longident.Ldot (Lident "Array", "get")))) + (Ast_helper.Exp.ident ~loc:arrayLoc + (Location.mkloc (Longident.Ldot (Lident "Array", "get")) arrayLoc)) [(Nolabel, expr); (Nolabel, accessExpr)] in parsePrimaryExpr ~operand:e p) From 7450c3d42258cde075039f5f041bf1607b6da82c Mon Sep 17 00:00:00 2001 From: Cristiano Calcagno Date: Wed, 18 Jan 2023 16:32:42 +0100 Subject: [PATCH 4/4] Update CHANGELOG.md Fixes https://github.com/rescript-lang/rescript-compiler/issues/5946 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 17181fdfe0..c6b241a9d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,7 @@ These are only breaking changes for unformatted code. - Fix compiler ppx issue when combining `async` and uncurried application https://github.com/rescript-lang/rescript-compiler/pull/5856 - Fix issue where the internal representation of uncurried types would leak when a non-function is applied in a curried way https://github.com/rescript-lang/rescript-compiler/pull/5892 - In GenType, check annotations also in module types to decide whether to produce the `.gen.tsx` file https://github.com/rescript-lang/rescript-compiler/pull/5903 +- Fix some comments disappearing in array access expressions https://github.com/rescript-lang/rescript-compiler/pull/5947 #### :nail_care: Polish