Some AST nodes have wrong source position when AST transform is run with Groovy Eclipse

I also recently ran into an example where AST position info was incorrect: http://jira.codehaus.org/browse/GRECLIPSE-758

I was thinking it might be a good idea to implement some kind of sanity check (e.g. a node's children should have positions that are inside the node's position). Unfortunately I did not proceed to implement this sanity check because I couldn't find an easy way to "getChildren" on an AST node in a generic way.

Maybe I should look into this again. Any hints on how to get AST children easily would be much appreciated

Interesting...we have had to make several changes to the way that source positions are calculated because the parser contains many mistakes and inconsistencies. It is possible that something we are doing is causing this regression. I'll have a look.

I'm looking at our code for this area and I don't see that we made any changes to the way that map expressions have their source locations calculated. I'd like to compare this against the command line.

Do you know of a way I can get source information about ast nodes from the command line?

Don't know how to do it on the command line, but you could use groovyConsole. Paste in my code, remove the reference to spock.lang.Specification (or add spock-core to the class path), and press CTRL+T to open the AST browser.

Thanks, Peter. I just tried this in the groovy console on version 1.7.2-rc2 (this is the latest version I happen to have downloaded. The source location for the map expression is the same as it is in Groovy-Eclipse (ie- incorrect).

I tried this on 1.6.7, but it seems like the AST viewer is not part of that distribution.

I am downloading 1.7.2 right now to see what the behavior is on the latest release (and this is the release that Eclipse is currently based on).

Ahhh....now I see it. I was looking at the source locations for the [:] expression, and not for the entire assignment expression (map = [:]). The sloc for [:] is incorrect, always. And the sloc for the assignment is correct outside of Eclipse, but incorrect inside of Eclipse.

This is because we are explicitly setting the ending sloc of the binary expression to be the same as the end of the RHS of the assignment. We are doing this because we found that otherwise inside Eclipse all binary expressions were having slocs that coincided with their operator and excluded either the LHS and RHS expressions.

So, this is uncovering another deviation in slocs. It seems that the underlying Antlr AST is different inside Eclipse and out. This could have something to do with our error recovery.

I can easily fix this proximate problem, but I want to find out what the deeper cause is.

I now see what we did:

        // FIXASC (groovychange) binary expression source locations are already set.
        // setting them here again would be incorrect
        // original
        // configureAST(expression, node);
        if (! (expression instanceof BinaryExpression)) {
            configureAST(expression, node);
        }
        // // FIXASC (groovychange) end

So, we are explicitly not setting the source locations for binary expressions because they were (incorrectly) set earlier. We need to rethink this.

Here is another expression where Groovy Eclipse provides wrong line information:

foo == [1, 2] as Set

This one is clipped after "as".

Our changes to the AntlrParserPlugin are not directly tested. They are tested indirectly in other test we have written. So, before making any changes I am going to get a few tests together for this area and see if we can remove the guard expression I describe above.

OK...here is where it gets tricky. Summarizing what I found so far, the groovy compiler has some problems with the source locations of nested BinaryExpressions. Groovy-Eclipse attempts to fix this, but fails in some situations because the groovy compiler fails to create correct source locations for some nested expressions.

The reason why I originally added that guard above is for situations like this:

map = [:] + [:]

There are now 2 BinaryExpressions here. In regular groovy, the first one, the assignment, has correct slocs (ie- the entire line), but the second binary expression ([:] + [:]) has a source location that only covers the '+'. This is wrong.

The fix that was added by Groovy Eclipse is to handle this situation with nested binary expressions. In many situations involving simple arithmetic, eg:

map = 666 + 566

Groovy-Eclipse will produce correct slocs for all binary expressions, but Groovy will not produce a correct sloc for the inner binary expression.

The problem in Groovy-Eclipse comes around when Groovy creates incorrect slocs for expressions inside of a binary expression. This is the problem that you are seeing.

Another thing that I see is that groovy produces incorrect slocs for the 'as' expression here:

foo == [1, 2] as Set

The sloc for the '[1, 2] as Set' is the word 'as'.

There is another pervasive problem with groovy compiler source locations in that they include whitespace after the expression ends and before the next one begins. This has to do with the limitations of the Antlr AST and deserves another bug all to itself.

Now, thinking of a fix...

It is possible to set the sloc of a binary expression in one of several locations in AntlrParserPlugin. In each of these places, there is potentially a different sloc being set. It seems to me that it should always be that the largest sloc should take precedence over any smaller sloc. As we go through potential locations where the sloc of a BinaryExpression is set, we should always check to see if the new sloc is larger than the current one (and only do the actual setting if this holds true).

I don't know if this holds true for other kinds of AST nodes.

Another problem with groovy's slocs:

MapEntryExpressions have the slocs of the ':', rather than of the whole expression. These could be treated in the same way as binary expressions.

I just committed some changes to AntlrParserPlugin. Here are the broad strokes:

• For BinaryExpressions, CastExpressions, and MapEntryExpressions, I am initially setting their slocs to be the start of their left-most child and the end of their right-most child. This may not be entirely correct in all situations, but this is remedied below.

• For these types of expressions, during a 'configureAST' call we do an extra check. We check to see if any existing sloc exists and if it is larger than (ie- contains) the new sloc. If so, then we ignore the request and keep the original sloc.

• For empty map expressions, I push the sloc to have a minimum of 3 characters of length. This is not entirely correct, but it will handle the standard situation where the text looks like '[:]' , but it will not handle situations where there is any whitespace included. Although a little messy, we are not going to be correct part of the time instead of never (a bit of an improvement).

All this will help to ensure that binary expressions will have the correct slocs.

Peter, when the build is available would you be able to try it and see if this covers all of your problems?

when serious changes like this are made, we really should be running the groovy core test suite if we can.

OK. Will do that today.

Haven't heard back from original reporter of problem. We believe this is fixed. Please reopen if you still have issues.

We are still seeing these problems, but I don't have a concrete example at the moment.

OK...reopening. Please attach or describe a reproducible example of this.

Will not get to for the 2.1.0 release.

Managed to reproduce the latest problem we had with this, but it turned out to be a Spock bug. I suggest to close this issue.

OK. Please reopen or create a new bug if you find that Groovy-Eclipse is the cause of any of this.