Skip to content

Refactor _expression rule #16

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

vanessa-rodrigues
Copy link
Contributor

(+) Accepts '\r\n'.
(+) Hide _decimal_integer_literal and _hexadecimal_integer_literal rules

@dolphinau
Copy link
Member

Hi @vanessa-rodrigues !
The '\r\n' support is very interesting and we might want to merge it!

However, I don't understand why you hide _decimal_integer_literal and _hexadecimal_integer_literal rules.
We use actively these rules, and I'm curious about why you want to hide them.

Sorry for the delay, and thanks again for you contribution!

@vanessa-rodrigues
Copy link
Contributor Author

Hello @atxr,

Thanks for catching that. It was not meant to be committed. I just pushed an update.

@dolphinau dolphinau self-assigned this Mar 14, 2025
Copy link
Member

@dolphinau dolphinau left a comment

Choose a reason for hiding this comment

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

Thanks @vanessa-rodrigues !
But there are still a lot of diff that refactor the _expression
I just want to merge the \r\n feature for now

I understand the tree is currently deep and complex, but for our usages, we need this granularity.
Your _expression rule version simplifies greatly the tree but it also breaks the operators priority.

I propose a patch for this PR that only includes the \r\n feature.
I didn't applied it yet so you can review it (git apply pr16_patch.txt) to confirm I didn't break any \r\n feature.

pr16_patch.txt

Comment on lines 669 to 777
multiplicative_expression: $ => prec.left(choice(
$.format_expression,
seq (
$.multiplicative_expression,
choice("/", "\\", "%", "*"), $.format_expression
multiplicative_expression: $ => prec.left(
seq(
$._expression,
choice("/", "\\", "%", "*"),
$._expression,
)
)),
),

format_expression: $ => prec.left(choice(
$.range_expression,
seq (
$.format_expression,
$.format_operator, $.range_expression
format_expression: $ => prec.left(
seq(
$._expression,
$.format_operator,
$._expression,
)
)),
),

range_expression: $ => prec.left(choice(
$.array_literal_expression,
seq (
$.range_expression,
"..", $.array_literal_expression
range_expression: $ => prec.left(
seq(
$._expression,
"..",
$._expression,
)
)),
),

array_literal_expression: $ => prec.left(seq(
$.unary_expression,
repeat (
$._unary_expression,
repeat(
seq(
",",
$.unary_expression
$._unary_expression
)
)
)),

_unary_expression: $ => prec.right(choice(
$.unary_expression,
$._primary_expression
)),

unary_expression: $ => prec.right(choice(
$._primary_expression,
$.expression_with_unary_operator
$._expression_with_unary_operator
)),

expression_with_unary_operator: $ => choice(
seq(",", $.unary_expression),
seq(reservedWord("-not"), $.unary_expression),
seq("!", $.unary_expression),
seq(reservedWord("-bnot"), $.unary_expression),
seq("+", $.unary_expression),
seq("-", $.unary_expression),
_expression_with_unary_operator: $ => choice(
seq(",", $._unary_expression),
seq(reservedWord("-not"), $._unary_expression),
seq("!", $._unary_expression),
seq(reservedWord("-bnot"), $._unary_expression),
seq("+", $._unary_expression),
seq("-", $._unary_expression),
$.pre_increment_expression,
$.pre_decrement_expression,
$.cast_expression,
seq(reservedWord("-split"), $.unary_expression),
seq(reservedWord("-join"), $.unary_expression)
seq(reservedWord("-split"), $._unary_expression),
seq(reservedWord("-join"), $._unary_expression)
),

pre_increment_expression: $ => seq("++", $.unary_expression),
pre_decrement_expression: $ => seq("--", $.unary_expression),

pre_increment_expression: $ => seq("++", $._unary_expression),
pre_decrement_expression: $ => seq("--", $._unary_expression),

cast_expression: $ => prec(PREC.CAST, seq($.type_literal, $.unary_expression)),
cast_expression: $ => prec(PREC.CAST, seq($.type_literal, $._unary_expression)),

attributed_variable: $ => seq($.type_literal, $.variable),

Copy link
Member

Choose a reason for hiding this comment

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

Cannot accept this

Comment on lines 905 to 914
)),

range_argument_expression: $ => prec.left(choice(
$.unary_expression,
$._unary_expression,
seq (
$.range_argument_expression,
"..", $.unary_expression
"..", $._unary_expression
)
)),

Copy link
Member

Choose a reason for hiding this comment

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

Cannot accept this

@@ -25,11 +26,11 @@ module.exports = grammar({
[$.class_property_definition, $.attribute],
[$.class_method_definition, $.attribute],
[$.expandable_string_literal],
[$.path_command_name, $._value]
[$.path_command_name, $._value],
[$._expression, $.array_literal_expression]
Copy link
Member

Choose a reason for hiding this comment

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

Cannot accept that

Copy link
Member

Choose a reason for hiding this comment

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

Cannot accept that

Copy link
Member

Choose a reason for hiding this comment

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

Cannot accept that

Copy link
Member

Choose a reason for hiding this comment

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

Cannot accept that

Copy link
Member

Choose a reason for hiding this comment

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

Cannot accept that


label_expression: $ => choice(
$.label,
$.unary_expression
$._unary_expression
Copy link
Member

Choose a reason for hiding this comment

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

Cannot accept that

Comment on lines 809 to 820

hash_entry: $ => seq(
$.key_expression,
"=",
$.key_expression,
"=",
$._statement, $._statement_terminator, repeat(";")
),

key_expression: $ => choice(
$.simple_name,
$.unary_expression
$._unary_expression
),

Copy link
Member

Choose a reason for hiding this comment

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

Cannot accept this

@@ -828,7 +830,7 @@ module.exports = grammar({
member_name: $ => choice(
$.simple_name,
$.string_literal,
$.expression_with_unary_operator,
$._expression_with_unary_operator,
Copy link
Member

Choose a reason for hiding this comment

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

Cannot accept this

@vanessa-rodrigues
Copy link
Contributor Author

vanessa-rodrigues commented Mar 14, 2025

I created a separated PR with the "\r\n" change only. But I would like to keep this other PR open as well.
Do you have an example where the expression rule used is incorrect due priority? Or is it just that it is not shown in the tree? I would like to fix that on my end if the implementation is incorrect :-)

@dolphinau
Copy link
Member

Thanks for splitting the PR!
I currently have no example, sorry. I'll work on it asap and get back to you if I find something!
I'll keep the PR open until then

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.

2 participants