Skip to content

Conversation

aspeddro
Copy link
Contributor

@aspeddro aspeddro commented Aug 13, 2023

Adding attribute because the internal representation of arr[idx] is Array.get and we don't have a way to distinguish both when issuing the token on the LSP.

See rescript-lang/rescript-vscode#801

@aspeddro aspeddro changed the title Add attr res.syntaxSugar to `Array.{get, set Add attr res.syntaxSugar to Array.{get, set} Aug 13, 2023
@cknitt
Copy link
Member

cknitt commented Sep 3, 2024

@aspeddro Shall we resurrect this PR?

Would you rebase it, add a comment somewhere explaining what the attribute is used for, and a CHANGELOG entry?

@aspeddro aspeddro force-pushed the add-attr-syntax-sugar-array branch from e4785d1 to 01926ab Compare October 11, 2024 17:08
@aspeddro
Copy link
Contributor Author

I added a comment and updated the PR

@cknitt
Copy link
Member

cknitt commented Oct 13, 2024

Thanks a lot! 👍

Regarding the comment, I meant in the code, where this attribute is set.

Something else I am wondering about: The name res.syntaxSugar seems very generic to me. Wouldn't a more specific name like res.arrayGet/ res.arraySet be better maybe?

@aspeddro aspeddro changed the title Add attr res.syntaxSugar to Array.{get, set} Add attr res.array_{Get,Set} to Array.{get, set} Dec 1, 2024
@aspeddro aspeddro changed the title Add attr res.array_{Get,Set} to Array.{get, set} Add attr res.array{Get,Set} to Array.{get, set} Dec 1, 2024
@aspeddro
Copy link
Contributor Author

aspeddro commented Dec 1, 2024

After a long time I made the adjustments

Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

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

Thanks a lot! Looks good to me!
Maybe @cristianoc or @zth would like to have a final look, too?

@cristianoc
Copy link
Collaborator

Looks good to me.
What is this for?

@aspeddro
Copy link
Contributor Author

aspeddro commented Dec 3, 2024

@cristianoc
Copy link
Collaborator

@aspeddro would you like to explore this on top of #7185 instead? I.e. changing the parse tree.
I'll soon add a PR demonstrating a real change to the AST, and can link it here for visibility.

@cristianoc
Copy link
Collaborator

This is the PR for optional fields in record type declarations: #7190
In this case of array access, I guess the extension should be lightweight and resolved very early, but after the logic for printing.

mediremi added a commit to mediremi/rescript-compiler that referenced this pull request Aug 22, 2025
…access bracket syntax Based on rescript-lang#6343 Co-authored-by: Pedro Castro <aspeddro@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants