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

Implement parser for arithmetic expressions like a++ #155

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

Conversation

Armd04
Copy link

@Armd04 Armd04 commented Sep 20, 2024

No description provided.

Copy link
Contributor

@prsabahrami prsabahrami 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!

I also think cases such as echo $(++2) are being parsed and created a part of the AST. I think we can raise error for this while creating the AST.

(parentheses_expr | VARIABLE | NUMBER) ~ post_arithmetic_op
}


unary_arithmetic_op = _{
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change unary_arithmetic_op to pre_arithmetic_op for better clarity?

})
}
Rule::unary_pre_arithmetic_expr => unary_pre_arithmetic_expr(first),
Rule::unary_post_arithmetic_expr => unary_post_arithmetic_expr(first),
_ => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this result in an error given that unary_arithmetic_expr can only be one of the unary_pre_arithmetic_expr or unary_post_arithmetic_expr?


fn unary_pre_arithmetic_expr(pair: Pair<Rule>) -> Result<ArithmeticPart> {
let mut inner = pair.into_inner();
let first = inner.next().unwrap();
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 it's better to replace unwraps with proper error handling.

Comment on lines 1438 to 1444
fn parse_unary_arithmetic_op_type(pair: Pair<Rule>) -> Result<bool> {
match pair.as_rule() {
Rule::unary_arithmetic_op => Ok(true),
Rule::post_arithmetic_op => Ok(false),
_ => Err(miette!(
"Unexpected rule in unary arithmetic operator: {:?}",
pair.as_rule()
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels fairly hacky to return true of false for the type. Maybe you can try replacing this with match statement in unary_pre_arithmetic_expr?

@Armd04
Copy link
Author

Armd04 commented Sep 21, 2024

I also think cases such as echo $(++2) are being parsed and created a part of the AST. I think we can raise error for this while creating the AST.

I believe in such examples, bash raises an error while compiling. I thnik it would be better to do the same here as well.

@Armd04 Armd04 marked this pull request as ready for review September 24, 2024 22:13
@Armd04 Armd04 changed the title WIP Implement parser for arithmetic expressions like a++ Implement parser for arithmetic expressions like a++ Sep 24, 2024
@Armd04 Armd04 requested a review from prsabahrami September 24, 2024 23:20
crates/deno_task_shell/src/parser.rs Outdated Show resolved Hide resolved
@Armd04 Armd04 requested a review from prsabahrami September 26, 2024 17:28
@prsabahrami
Copy link
Contributor

@Armd04 Can you please take care of the issues on Windows?

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