Skip to content

Commit

Permalink
GROOVY-8551: simplify parser rules for array
Browse files Browse the repository at this point in the history
  • Loading branch information
daniellansun committed Jan 21, 2025
1 parent 8d94b6f commit 3084279
Show file tree
Hide file tree
Showing 5 changed files with 71 additions and 17 deletions.
16 changes: 3 additions & 13 deletions src/antlr/GroovyParser.g4
Original file line number Diff line number Diff line change
Expand Up @@ -1141,22 +1141,12 @@ options { baseContext = mapEntryLabel; }
creator[int t]
: createdName
( nls arguments anonymousInnerClassDeclaration[0]?
| dim[0]+ nls arrayInitializer
| dim[1]+ dim[0]*
| dim+ (nls arrayInitializer)?
)
;

/**
* n 0: dim without size expression; 1: dim with size expression
*/
dim[int n]
: annotationsOpt LBRACK
(
{ $n == 1 }?
expression
|
)
RBRACK
dim
: annotationsOpt LBRACK expression? RBRACK
;

arrayInitializer
Expand Down
32 changes: 28 additions & 4 deletions src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -3482,7 +3482,15 @@ public Expression visitCreator(final CreatorContext ctx) {
final int nDim = dims.size();
if (asBoolean(ctx.arrayInitializer())) { // create array: new Type[][]{ ... }
List<List<AnnotationNode>> typeAnnotations = new ArrayList<>(nDim);
for (var dim : dims) typeAnnotations.add(this.visitAnnotationsOpt(dim.annotationsOpt()));
for (var dim : dims) {
final ExpressionContext dimExpressionContext = dim.expression();
if (asBoolean(dimExpressionContext)) {
throw createParsingFailedException(
"Unsupported array dimension expression: " + dimExpressionContext.getText(),
dimExpressionContext);
}
typeAnnotations.add(this.visitAnnotationsOpt(dim.annotationsOpt()));
}

ClassNode elementType = classNode;
for (int i = nDim - 1; i > 0; i -= 1) {
Expand All @@ -3500,10 +3508,26 @@ public Expression visitCreator(final CreatorContext ctx) {
} else { // create array: new Type[n][]
final List<Expression> sizeExpressions = new ArrayList<>(nDim);
final List<List<AnnotationNode>> typeAnnotations = new ArrayList<>(nDim);
for (var dim : dims) {
ExpressionContext lastDimExpressionContext = null;
for (int i = 0, n = dims.size(); i < n; i += 1) {
var dim = dims.get(i);
final ExpressionContext dimExpressionContext = dim.expression();
if (i == 0) {
if (!asBoolean(dimExpressionContext)) {
throw createParsingFailedException("array dimension expression is expected", dim);
}
} else {
if (asBoolean(dimExpressionContext) && !asBoolean(lastDimExpressionContext)) {
throw createParsingFailedException(
"Unsupported array dimension expression: " + dimExpressionContext.getText(),
dimExpressionContext);
}
}
lastDimExpressionContext = dimExpressionContext;

final Expression sizeExpression;
if (asBoolean(dim.expression())) {
sizeExpression = (Expression) this.visit(dim.expression());
if (asBoolean(dimExpressionContext)) {
sizeExpression = (Expression) this.visit(dimExpressionContext);
} else {
sizeExpression = ConstantExpression.EMPTY_EXPRESSION;
}
Expand Down
19 changes: 19 additions & 0 deletions src/test-resources/fail/Array_03x.groovy
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
def foo = new double[2][][5]
19 changes: 19 additions & 0 deletions src/test-resources/fail/Array_04x.groovy
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
def foo = new double[2] { 1.0, 2.0 }
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,8 @@ final class SyntaxErrorTest {
void 'groovy core - Array'() {
TestUtils.doRunAndShouldFail('fail/Array_01x.groovy')
TestUtils.doRunAndShouldFail('fail/Array_02x.groovy')
TestUtils.doRunAndShouldFail('fail/Array_03x.groovy')
TestUtils.doRunAndShouldFail('fail/Array_04x.groovy')
}

@Test
Expand Down

6 comments on commit 3084279

@eric-milles
Copy link
Member

Choose a reason for hiding this comment

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

Why was this change made? It was very intentional to have dimensions with an expression (dim1) followed by empty dimensions (dim0). It made for very clean matching and processing.

This should have come with a separate issue and separate discussion and review.

@daniellansun
Copy link
Contributor Author

@daniellansun daniellansun commented on 3084279 Jan 22, 2025

Choose a reason for hiding this comment

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

We should try our best to avoid using similar rules, e.g. dim0, dim1. It is hard for parser to choose which branch to go. Though they can work finally, parsing performance is impacted.

The simplifed rules have same function and better prompt for syntax errors.

BTW, we had friendly prompt for missing right parenthesis, but the similar rules make parsing performance very poor, so we have to remove it.

It was very intentional to have dimensions with an expression (dim1) followed by empty dimensions (dim0).

Did you create any PR or discussion for the changes?

@eric-milles
Copy link
Member

Choose a reason for hiding this comment

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

The rules were designed so that the choice is clear. One rule has expression and one does not. I sat on GROOVY-8551 for a very long time. Then as soon as I make a change that satisfies all the tests, you go in and change it, calling such change "simpler". I do not agree that the ASB builder change that you needed to support this is simpler.

If you feel a change does not meet your expectations, a test case or discussion point is the wat forward. You should not make it a practice to modify others changes without at least discussing why such a change is needed. This goes for all the "trivial tweak" changes as well.

A smile or smirk is not a good answer to why this comes in on master with no tracking or notification.

@daniellansun
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"simpler" is for parser and not for AST walker, i.e. AstBuilder.

Apart from the parsing performance, friendly prompt is important for parser, the simplifed/tweaked version can achieve the goal.

I really appreciate your hard and amazing work on Groovy development for years. As we all know, you are also the main energy. Go as you want.

Just smile, no smirk, sincerely.

@paulk-asert
Copy link
Contributor

@paulk-asert paulk-asert commented on 3084279 Jan 23, 2025

Choose a reason for hiding this comment

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

You both provide wonderful contributions to Groovy! Thanks for keeping the conversation civil. I think it is really important that folks of all backgrounds feel comfortable discussing design choices in all our forums.

In this case, it looks like an opportunity to potentially progress this iteratively since you both have different thoughts but lots of knowledge. Actually for my benefit, can either of you point me to something which changes for Groovy users between the two implementations? Like error message Daniel mentioned?

@daniellansun
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Paul,

Let me explain a bit more.

I had thought Eric did not find a graceful solution to implement the feature of GROOVY-8551, because I found similar rules violating DRY principle, e.g. dim0, dim1. and no friendly error prompt provided. so I thought I could give a hand, and I do know it is not easy to pursue both goals.

Luckily, I tried my best and achieved both goal in the early hours of morning, and pushed the improvement directly to master, which "reverted" some intentional changes by Eric. To be frank, I did not know the changes are intentional. I want to apologize sincerely if Eric is still angry now.

Here is one of friendly error prompt:
3084279#diff-03f7d07e002c1a3986a4913911349066b6f85fc3cf1d18506dc85c4ef83a1478R3517

It's very important to keep grammar rules simple even if increasing complexity of AST walker because the grammar rules control the behavior of parser generated by antlr4 but the AST walker is written by us, i.e. under our control. Also, friendly prompt is important for users, multi-dimentions array is not easy to use, friendly prompt can help users a lot.

Please sign in to comment.