Make Groovy Parser correctly handle nested parenthesis

[jevanlingen]

What's changed?

Nested parentheses around method invocations are supported as well for the groovy parser.

What's your motivation?

• Groovy parser does not correctly handle nested parenthesis #4703
• Gradle parser fails with extra parens around an expression #4615

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[Laurens-W]

LGTM but since it's an extensive change let's let Tim, Sam or Knut take a look too!

[timtebeek]

I'm somewhat surprised to see source and cursor as separate arguments here when they are merely used to do this substring. Would it make sense to change the method signature here to

Suggested change

	public static @Nullable Integer getInsideParenthesesLevel(MethodCallExpression node, String source, int cursor) {
	String sourceFromCursor = source.substring(cursor);
	public static @Nullable Integer getInsideParenthesesLevel(MethodCallExpression node, String sourceFromCursor) {

[jevanlingen]

I had that before, but reverted it back to current state. I think it more clear if the GroovyParserParentheseDiscoverer is a completely separate entity. So no former logic is needed to determine the wrapping. Just pass everything you have (thus the node, the source code of the entire file and the current place you are parsing right now) and let the function discover it all.

Having the cursor argument also prevents from misinterpretation to function, because you need to provide the starting point (that is the cursor) of parsing.

[knutwannheden]

I wonder if this can be simplified. For Python I had the same basic problem to solve, but I didn't have any "parentheses level" from the AST or similar. Additionally, in Python the parentheses for tuples and generator expressions are sometimes optional, which doesn't make things simpler.

Anyway, rather than counting the "parentheses levels" up front, I added something like a maybeParenthesized(node, delegate) method which does something like this:

1. Save cursor and use whitespace() to parse whitespace (and comments) at cursor into a Space
2. Check if the next character is a (
3. If not, reset cursor to saved cursor and return result from delegate and we are done
4. Otherwise push a lambda on a parentheses stack, which is a field on the parser visitor
5. Call maybeParenthesized() recursively
6. Again, save cursor, call whitespace() and check if we are at a )
7. If yes, pop lambda from parentheses stack and apply it to result, which will wrap it in a J.Parentheses
8. If not, reset cursor and return result

For a language like Java or Python, where we can expect the source files to be reasonably large, I would definitely do something like this, as the substringing and regexp matching won't be free in terms of performance. But for Groovy that would probably not be a big concern, as scripts tend to be small and few.

I will let you think a bit about this and don't want to force you into a different solution.

[knutwannheden]

For reference, here is the Python code in question. It isn't pretty, so in case you decide to give this approach a try, I would only look at the Python code if you get stuck, as you will probably find a cleaner solution for your parser: https://github.com/openrewrite/rewrite-python/blob/b42101961bbc9daf21d54fe803dd9eaec59c3184/rewrite/rewrite/python/_parser_visitor.py#L1950-L2025

[sambsnyd]

I second Knut's comments and like the approach he describes. I worry that these regexes will be slow and error prone.

[knutwannheden]

The one tricky situation I can think of are Java-style type casts, as the first token there is also a (. So if the AST node is a Java-style type cast (assuming they can be distinguished from Groovy-style type casts in the AST), the logic could for example pop that last element from the stack and reset the cursor before calling the corresponding visit method (or do it in the method itself). There are also other alternative solutions.

[jevanlingen]

I agree, your implementation seems like a better idea @knutwannheden. I would rather first merge this and then do a second round with improvements (or better said, go for the new/better implementation). Currently we don't support nested parenthesis, which just breaks scripts that use them. Having this sub-ideal implementation makes them at least runnable, though with a performance loss.

Then I can start working on the better implementation

--

Edit: Talked with @knutwannheden about this, we concluded it's better to not go forward yet:

If we aren't in a hurry to get this done, it will just make the Git history messier to go through, when someone tries to find out why some change was made in the past.

[knutwannheden]

It isn't quite as easy as I made it out to be and I now remember the struggles I had with Python here. The difficulty arises because we don't know at what AST level the parentheses are. Here that parentheses level may come in handy.

[sambsnyd]

I haven't reviewed in detail but at first glance it doesn't seem to be aware of comments so any parentheses in comments might throw it off

[shanman190]

This is just my 2c, but the several regexes makes me feel like the solution here isn't quite right. For trivial source files found in the test cases, it works. But I fear that for actual source files that this would bring G-based parsing to a crawl.

[timtebeek]

• Fixed in Make Groovy Parser correctly handle nested parenthesis #4801