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

feat: allow to put type arguments in calling expressions #59

Merged
merged 5 commits into from
Jan 6, 2022

Conversation

kawaemon
Copy link
Contributor

@kawaemon kawaemon commented Jan 5, 2022

I realized that I forgot to allow type arguments in calling expressions on #57. I'm so sorry.
This PR fixes it, but there is a little concern. Can I ask for your opinions?

Due to syntax conflict, this thing occurs:

someFunction[foo]()
~~~~~~~~~~~~~~~~~ treated as indexing expression
someFunction[foo, bar]()
~~~~~~~~~~~~~~~~~~~~~~ treated as generic function

It is painful on editor highlighting: (notice that both of Collect and Map are function, but have different highlighting)

image

Are there anything we can do? I know this is tough task but I wonder what other parsers do for this kind of problems.

Checklist:

  • All tests pass in CI.
  • There are sufficient tests for the new fix/feature.
  • Grammar rules have not been renamed unless absolutely necessary.
  • The conflicts section hasn't grown too much.
  • The parser size hasn't grown too much (check the value of STATE_COUNT in src/parser.c).
    1220 → 1232

@maxbrunsfeld
Copy link
Contributor

We should use dynamic precedence to always treat those constructs as type argument lists, as opposed to index expressions. I think a ‘prec.dynamic’ with a positive number in the type arguments rule will do the trick.

@kawaemon
Copy link
Contributor Author

kawaemon commented Jan 6, 2022

Really prec.dynamic did fix the issue and it keeps misunderstood codes minimal. Thanks for your kind advice!
This pull request is now ready to review.

),

type_arguments: $ => seq(
type_arguments: $ => prec.dynamic(2, seq(
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a comment would be useful here to explain the need for prec.dynamic, that is to prefer call_expression to index_expression, and why this works with the value 2.

Copy link
Contributor Author

@kawaemon kawaemon Jan 6, 2022

Choose a reason for hiding this comment

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

I think it is because _type_identifier rule in _simple_type rule has its dynamic precedence -1. For example, if type_arguments rule had 1 dynamic precedence typical syntax tree like (type_arguments (_simple_type (type_identifier))) will have 0 dynamic precedence so it makes no difference between index_expression.
Can I open a pull request to write that comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think those comments are useful for the next person that sees those prec.dynamic(2)
and wonder what it is.
Conflicts are hard to understand, so the more we have comments like this the better I think.

@aryx aryx merged commit 0fa917a into tree-sitter:master Jan 6, 2022
@kawaemon kawaemon deleted the generics-followup branch January 6, 2022 14:31
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.

3 participants