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

Fix: lex now throws error on unbalanced closing parentheses (issue #11982) #12098

Merged
merged 4 commits into from Mar 7, 2024

Conversation

dj-sourbrough
Copy link
Contributor

Description

Expressions with unbalanced parenthesis [excess closing ')' parenthesis] will throw an error instead of interpreting ')' as a string.

Solved he same way as closing braces '}' are handled.

Screenshot 2024-03-06 at 14 53 46

Screenshot 2024-03-06 at 14 48 27

User-Facing Changes

  • Trailing closing parentheses ')' which do not match the number of opening parentheses '(' will lead to a parse error.
  • From what I have found in the documentation this is the intended behavior, thus no documentation has been updated on my part

Tests + Formatting

  • Two tests added in src/tests/test_parser.rs
  • All previous tests are still passing
  • cargo fmt, clippy and test have been run

Unable to get the following command run

  • cargo run -- -c "use std testing; testing run-tests --path crates/nu-std" to run the tests for the standard library
    Screenshot 2024-03-06 at 20 06 25

Copy link
Contributor

@rgwood rgwood left a comment

Choose a reason for hiding this comment

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

Wow, this is an awesome first PR thanks! LGTM (from reading the code and trying it out locally).

@fdncred
Copy link
Collaborator

fdncred commented Mar 7, 2024

Thanks @dj-sourbrough. The reason the use std testing part didn't work was because that has been removed from the nushell std-lib. There was a recent PR about this in the docs repo nushell/nushell.github.io#1284 that helps explain how to use it.

@fdncred fdncred merged commit 48fca1c into nushell:main Mar 7, 2024
19 checks passed
@kubouch kubouch added this to the v0.92.0 milestone Mar 7, 2024
@kubouch kubouch added pr:bugfix This PR fixes some bug pr:language This PR makes some language changes labels Mar 7, 2024
@dj-sourbrough
Copy link
Contributor Author

After reading up in the docs on string interpolation (https://www.nushell.sh/book/working_with_strings.html#string-interpolation) I got interested to see how this fix would work in this case.

From my understanding parentheses need to be escaped for string interpolation, however trailing unclosed parentheses ')' are still treated as strings and not solved by my patch

Also, it is not possible to wrap an escaped parentheses block inside a regular parentheses block, ie ( (test) )

Screenshot 2024-03-07 at 14 57 10

Would you like a new issue raised for this or #11982 should remain open?

@fdncred
Copy link
Collaborator

fdncred commented Mar 7, 2024

Typically I use (char lp) and (char rp) instead of trying to use \( and \), although I think the escaping should work too. But also you need to remember that parens are for evaluation like (2 + 2) evaluates to 4 (blah blah) doesn't evaluate to anything.

I think what's going on is that the parser sees ( but not \( and it's trying to eval it.

You could just create a new issue.

@fdncred fdncred added the parser Issues related to parsing label Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parser Issues related to parsing pr:bugfix This PR fixes some bug pr:language This PR makes some language changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants