Skip to content
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

Merged
merged 4 commits into from
Jan 18, 2023
Merged

Conversation

cristianoc
Copy link
Collaborator

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.

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.
@cristianoc
Copy link
Collaborator Author

cristianoc commented Jan 17, 2023

Note: with this the error message misses a location for:

let a = [1]

module Array = {}

let _ = a[0]

the message being:

% ./bsc tst.res   

  We've found a bug for you!
  tst.res

  The value get can't be found in Array

@@ -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
Copy link
Member

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?

Copy link
Collaborator Author

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.

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)
Copy link
Member

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?

Copy link
Member

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.

Copy link
Collaborator Author

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.

@cristianoc
Copy link
Collaborator Author

@mununki see here ee5fca5 where comment attachments are changed instead. So the location of the array functions does not matter.

@mununki
Copy link
Member

mununki commented Jan 18, 2023

@mununki see here ee5fca5 where comment attachments are changed instead. So the location of the array functions does not matter.

Looks great!
I also thought that we need to look into the comment parsing logics for this kind of expressions, But I couldn't grab a time to look into.

Copy link
Member

@mununki mununki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@cristianoc cristianoc merged commit ada7100 into master Jan 18, 2023
@cristianoc cristianoc deleted the array_comments branch January 18, 2023 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants