-
Notifications
You must be signed in to change notification settings - Fork 455
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix some comments disappearing in array sugar. #5947
Conversation
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 #5946 but not that specific example.
Note: with this the error message misses a location for: let a = [1]
module Array = {}
let _ = a[0] the message being:
|
res_syntax/src/res_core.ml
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not able to access the computer now so have not run it to see if it works.
Can we use stringStart
position got rbracket for loc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried a few different locations, such as [
or ]
only, but that already breaks some printing tests.
I could get it to work with mkLoc startPos startPos
, which does not break tests. The error message for Array.get
not found has at least the right line in that case.
res_syntax/src/res_core.ml
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, this could be alternatives:
Ast_helper.Exp.apply ~loc:(mkLoc lbracket rbracket)
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example case's ouput looks like this:
// This comment will vanish
(
incidents
->Belt.Array.keep(({status}) => status === #OPEN)
->Belt.SortArray.stableSortBy((a, b) => compare(a.createdTime, b.createdTime))
)[0]
The comment location looks weird, but at least we wouldn't lost the loc for error msg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a separate question. Why does it matter what location one gives to array functions given that they are not printed? I'd like to see where in the printing mechanism that matters, and whether one can make it be ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Fixes #5946
The ids for
Array.get
andArray.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 #5946 but not that specific example.