Skip to content

Fix unbalanced paren #966

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 5 commits into
base: main
Choose a base branch
from

Conversation

BlackBuck
Copy link

This PR addresses #547. Previously, due to a mix of local and recursive tracking of parentheses balance, ( and ) were not parsed correctly (especially )).

Changes Made

  • Introduced a single global parenBalance variable to track parentheses across all expressions and sub-expressions.
  • Changed the definition of parseExprList() to accept a pointer to parenBalance
  • Updated calls to parseExprList() to use a pointer to the global parenBalance

This takes the parentheses logic, which previously existed only in nextToken, and moves it up to the Parse function.

Copy link
Member

@keegancsmith keegancsmith left a comment

Choose a reason for hiding this comment

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

Thanks! directly tracking paren balance makes sense. I wonder if my alternative implementation may be better to explore given the simplicity of it and it maybe capturing other bad expressions https://github.com/sourcegraph/zoekt/pull/569/files WDYT?

Happy to go along with this change after the feedback is resolved though.

@BlackBuck
Copy link
Author

BlackBuck commented Jun 27, 2025

Thanks! directly tracking paren balance makes sense. I wonder if my alternative implementation may be better to explore given the simplicity of it and it maybe capturing other bad expressions https://github.com/sourcegraph/zoekt/pull/569/files WDYT?

Happy to go along with this change after the feedback is resolved though.

@keegancsmith I was simply in awe when I saw the simplicity of the solution, but since the last draft was in 2023 and you were a bit skeptical about your solution as well, I figured I might propose an alternative for you to compare the two and check which was the better fit. Your solution works perfectly fine. I've also resolved the requested changes. We can go along with any one of them 🙂.

@keegancsmith
Copy link
Member

aaaaw thanks :) TBH I just completely forgot about that PR since it was just a quick hack. I still don't know if it makes sense. Given I am in an end of week mood, I will dive a bit into the code to see if it makes sense. Will get back to you today.

@keegancsmith
Copy link
Member

@BlackBuck thanks for motivating me to look into this again. I read the code and did some more testing, and I think the approach in #569 is solid. I reckon we should prefer that approach given its simplicity and it can cover other potential situation we don't parse the full query string (although from reading the code this is the only one I saw).

@BlackBuck
Copy link
Author

LGTM 👍

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