AST nodes for an empty script contains incorrect line & column numbers in Eclipse #252
Comments
|
I can see the offsets in Eclipse as you indicate. However, I was not able to reproduce the exception. Can you have a look at my screen and let me know if I selected the right thing? Is it a common case to try and run an empty script in the Groovy Console? If you had at least one statement, do you get the same error?
|
|
The source positions are set by AntParserPlugin.fixModuleNodeLocations: // GRECLIPSE add
private void fixModuleNodeLocations() {
output.setStart(0);
output.setEnd(locations.getEnd());
output.setLineNumber(1);
output.setColumnNumber(1);
output.setLastColumnNumber(locations.getEndColumn());
output.setLastLineNumber(locations.getEndLine());
// only occurs if in a script
// start location should be the start of either the first method or statement
// end location should be the end of either the last method or statement
BlockStatement statements = output.getStatementBlock();
List<MethodNode> methods = output.getMethods();
if (hasScriptMethodsOrStatements(statements, methods)) {
ASTNode first = getFirst(statements, methods);
ASTNode last = getLast(statements, methods);
if (hasScriptStatements(statements)) {
statements.setStart(first.getStart());
statements.setLineNumber(first.getLineNumber());
statements.setColumnNumber(first.getColumnNumber());
statements.setEnd(last.getEnd());
statements.setLastLineNumber(last.getLastLineNumber());
statements.setLastColumnNumber(last.getLastColumnNumber());
}
if (output.getClasses().size() > 0) {
ClassNode scriptClass = output.getClasses().get(0);
scriptClass.setStart(first.getStart());
scriptClass.setLineNumber(first.getLineNumber());
scriptClass.setColumnNumber(first.getColumnNumber());
scriptClass.setEnd(last.getEnd());
scriptClass.setLastLineNumber(last.getLastLineNumber());
scriptClass.setLastColumnNumber(last.getLastColumnNumber());
// fix the run method to contain the start and end locations of the statement block
MethodNode runMethod = scriptClass.getDeclaredMethod("run", new Parameter[0]);
runMethod.setStart(first.getStart());
runMethod.setLineNumber(first.getLineNumber());
runMethod.setColumnNumber(first.getColumnNumber());
runMethod.setEnd(last.getEnd());
runMethod.setLastLineNumber(last.getLastLineNumber());
runMethod.setLastColumnNumber(last.getLastColumnNumber());
}
}
} |
|
Hi Eric, thanks for the fast feedback. I used the wording "empty script" because Groovy parses such files as scripts. In real we are talking about For non-empty script files like this one: ...the AST contains correct values in Eclipse: MethodNode - run
I came across the issue with invalid line & column numbers while scanning Groovy files with CodeNarc inside Eclipse. CodeNarc uses a lot of AST visitors and accesses source code via the line & column numbers. When they are not correct I have seen java.lang.StringIndexOutOfBoundsExceptions. So I am interested in a correct AST inside Eclipse. The Exception in Groovy Console is just a side effect of the wrong AST. Looking at your screenshot I can confirm that you selected the You also said that you can see the offsets in Eclipse. Did you in mean in the "Groovy AST Viewer"? Because these values are not visible in the screenshot. So what are the values inside the AST viewer? |
|
@darxriggs Yes, I did launch the Groovy Console from Eclipse. I tried to follow your directions as closely as possible. And, yes the offsets in the AST Viewer match what you describe in the issue. Can you say what may be throwing CodeNarc off by having source positions on the run method node? I'm not sure why it has been given positions, but that is one of the patches made by Greclipse. It may be possible to skip setting the positions in case there are no statements besides the package statement. |
|
If I change hasScriptStatements() to private boolean hasScriptStatements(BlockStatement statements) {
return statements != null && statements.getStatements() != null &&
!statements.getStatements().isEmpty() && statements.getStatements().get(0).getEnd() > 0;
}from private boolean hasScriptStatements(BlockStatement statements) {
return statements != null && statements.getStatements() != null && statements.getStatements().size() > 0;
}I think the desired effect is achieved. |
|
There are cases of files with just package statement, imports and import annotations that are failing with this change. I'll have to look into what position information they require. |
|
CodeNarc accesses exact positions on the source code based on the line & column information from the AST - see for example SpaceAfterOpeningBraceRule::visitMethodEx(). That's also the rule currently failing for package-only files. If you like you can reproduce the StringIndexOutOfBoundsException with this code in Eclipse: It results in this stacktrace: Sure the CodeNarc rules (about 400) could be implemented in a more safer way by always double checking the line & column values against the source code prior to accessing values in the source code. But this would clutter code a lot and would only workaround this AST issue opened here. Other tools using the AST values would still be affected. So my goal was the root cause to be fixed. |
|
I'm trying to see if I can fix the positions. I just need a bit more time to look into the tested cases of files with just imports and the behavior of add/organize imports. |
|
From another angle, could it be the presence of position info on the module, block statement and run method nodes is not the problem? Is the problem that the end offset is 1 past the length of the source? |
|
Here is where the +1 source position comes from: // creates a synthetic, empty statement that starts after the last import or package statement
private Statement createSyntheticAfterImports() {
ASTNode target = null;
Statement synthetic = ReturnStatement.RETURN_NULL_OR_VOID;
if (output.getImports() != null && !output.getImports().isEmpty()) {
target = output.getImports().get(output.getImports().size() - 1);
} else if (output.hasPackage()) {
target = output.getPackage();
}
if (target != null) {
synthetic = new ReturnStatement(ConstantExpression.NULL);
synthetic.setStart(target.getEnd() + 1);
synthetic.setEnd(target.getEnd() + 1);
synthetic.setLineNumber(target.getLastLineNumber());
synthetic.setLastLineNumber(target.getLineNumber());
synthetic.setColumnNumber(target.getLastColumnNumber() + 1);
synthetic.setLastColumnNumber(target.getColumnNumber() + 1);
}
return synthetic;
}This is also in AntParserPlugin as part of the Greclipse additions. Let me experiment with removing the +1s. |
|
I think the "start" and "end" offsets are added via Groovy-Eclipse. They are not available when groovyConsole is run from the commandline - see "Details" section in this issue. Therefore they are not the problem for CodeNarc. |
|
The snippet that I included above is where you are getting an index past the end of the file. |
|
Can you retest with the latest snapshot? Here is a direct link if the update site did not get changed due to unrelated test failure: https://build.spring.io/browse/GRECLIPSE-GE46-178/artifact/JOB1/Zipped-Update-Site/org.codehaus.groovy-2.9.2.xx-201701042322-e46-updatesite.zip |
|
I retested with Groovy-Eclipse 2.9.2.xx-201701080425-e45. With this version "Groovy Console" and "SpaceAfterOpeningBraceRule" CodeNarc rule both work fine inside Eclipse. The AST values are now as follows:
Well done. If possible the value should be -1 to make Groovy-Eclipse as similar as the real Groovy compiler. With the -1 value it's also possible to detect methods that are not present in the source code (generated methods). |
|
The position info is set on the run method and its return statement to enable adding imports and some other editing activities to a nearly empty source file. |
Issue
For the following empty script the line & column numbers are incorrect for the
MethodNodeandBlockStatementof the generated run method. The values are correct for the generated main method.Issue.groovy:
Behaviour
Actual
The AST value in Eclipse for lineNumber, lastLineNumber, columnNumber and lastColumnNumber is different than for Groovy on the commandline.
Expected
The AST value in Eclipse for lineNumber, lastLineNumber, columnNumber and lastColumnNumber is -1 (the same as for Groovy on the commandline).
Steps to reproduce
In "Groovy AST Viewer" (Eclipse)
In Groovy Console (Eclipse and commandline)
Environment
Tested in Eclipse
Tested on commandline
Details
When running
groovyConsolefrom commandline the values in question are:MethodNode - run
When running
groovyConsolefrom Eclipse and selecting "MethodNode - run" the UI throws this exception:That's because some AST values are not valid.
The values can be examined inside "Groovy AST Viewer":
MethodNode - run
The text was updated successfully, but these errors were encountered: