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

Make the parentheses optional in if else statement #4417

Merged
merged 3 commits into from Jan 24, 2024

Conversation

Chen1Plus
Copy link
Contributor

@Chen1Plus Chen1Plus commented Jan 24, 2024

resolve #4062

Not complete, this will break formatter.
It seems like formatter can't identify if without parentheses.

@CLAassistant
Copy link

CLAassistant commented Jan 24, 2024

CLA assistant check
All committers have signed the CLA.

@ogoffart
Copy link
Member

Thanks for the PR.

If I paste the code that you try to format in a .slint file and try to compile it, i get the following error:

error: Syntax error: expected '}'
 --> tmp/foo.slint:2:20
  |
2 | A := B { c => { if !abc{nothing}else   if  true{if 0== 8 {}    } else{  } } }
  |                    ^

So it looks like the formater fails to format because it wasn't parsed properly in the first place
The bug is not in the formatter, but in the parser.

That is because parse_if_statement is not even being called for this code.

That is because the code in parse_statement expect a parenthesis after the if

if p.peek().as_str() == "if" && p.nth(1).kind() == SyntaxKind::LParent {

We could remove the requirement for parentheses there.

The thing is that there is no keyword in slint so one can have property <bool> if; or if := Rectangle { }
And this would break code like this: xxx: if; or xxx: if.bar;
But i'm pretty sure nobody has a property called if at this point.

It is slightly more complicated, but the solution can be:

  1. just remove the test for the ( in parse_statement. That's easy and can worry about properties and element with if later.
  2. Try to parse an expression, and if that fails roll back. This is the most correct but also most complicated solution because we don't have way to rollback at this point.
  3. instead of (, try to match any token that can start an expression. So (, ! and all unary opps, identifiers, and maybe i miss some.
  4. instead of matching for (, try to match for token which are not starting an expression for sure. For example . or ;. And also maybe or , or ). That's actually what i'd do.

Please copy the test that is in tests/cases/conditional/stm.slint into tests/cases/conditional/stm2.slint and remove the parentheses, so we have test for the non-parentheses if.
You can also extend the test to include things that have property or an element named if.

Thanks again for contributing to Slint 🤗

@Chen1Plus
Copy link
Contributor Author

thanks for explanation and support

Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

Thanks for the change. Looks good to me.

Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

Actually maybe there is some documentation that needs adjustments

@ogoffart ogoffart merged commit 367b5c5 into slint-ui:master Jan 24, 2024
35 checks passed
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.

if statements shouldn't require both parentheses and curly braces
3 participants