Skip to content

Commit

Permalink
Fix for #758: ensure offsets of annotation span from '@' to ')'
Browse files Browse the repository at this point in the history
  • Loading branch information
@eric-milles
eric-milles committed Nov 11, 2018
1 parent d3cdb93 commit eef5ff8
Show file tree
Hide file tree
Showing 9 changed files with 202 additions and 163 deletions.
92 changes: 74 additions & 18 deletions ...e.tests.builder/src/org/eclipse/jdt/core/groovy/tests/locations/SourceLocationsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package org.eclipse.jdt.core.groovy.tests.locations;

import static org.codehaus.groovy.runtime.DefaultGroovyMethods.last;
import static org.eclipse.jdt.groovy.core.tests.GroovyBundle.isAtLeastGroovy;
import static org.eclipse.jdt.groovy.core.tests.GroovyBundle.isParrotParser;
import static org.junit.Assert.assertEquals;
Expand All @@ -35,8 +36,10 @@
import org.eclipse.jdt.core.ISourceRange;
import org.eclipse.jdt.core.IType;
import org.eclipse.jdt.core.JavaCore;
import org.eclipse.jdt.core.dom.ASTNode;
import org.eclipse.jdt.core.dom.ASTParser;
import org.eclipse.jdt.core.dom.AbstractTypeDeclaration;
import org.eclipse.jdt.core.dom.Annotation;
import org.eclipse.jdt.core.dom.BodyDeclaration;
import org.eclipse.jdt.core.dom.CompilationUnit;
import org.eclipse.jdt.core.dom.FieldDeclaration;
Expand Down Expand Up @@ -153,6 +156,33 @@ private static void assertDeclaration(IMember decl, BodyDeclaration body, int me
assertEquals(body + "\nhas incorrect source start value", start, body.getStartPosition());
assertEquals(decl + "\nhas incorrect source end value", end, declRange.getOffset() + declRange.getLength());

String modsStartTag = "/*" + astKind + memberNumber + "sm*/";
int modsStart = source.indexOf(modsStartTag);
if (modsStart == -1) {
modsStart = start;
} else {
modsStart += modsStartTag.length();
}

String modsEndTag = "/*" + astKind + memberNumber + "em*/";
int modsEnd = source.indexOf(modsEndTag);
if (modsEnd == -1) {
modsEnd = modsStart;
}

List<ASTNode> mods = body.modifiers();
if (mods != null && !mods.isEmpty()) {
if (!isParrotParser() && last(mods) instanceof Annotation) {
modsEnd += modsEndTag.length(); // antlr2 includes comment
}
int one = mods.get(0).getStartPosition(), two = last(mods).getStartPosition() + last(mods).getLength();
assertEquals(decl + "\nhas incorrect modifiers start value", modsStart, one);
assertEquals(decl + "\nhas incorrect modifiers end value", modsEnd, two);
} else {
assertEquals(decl + "\nhas incorrect modifiers start value", modsStart, start);
assertEquals(decl + "\nhas incorrect modifiers end value", modsEnd, start);
}

String nameStartTag = "/*" + astKind + memberNumber + "sn*/";
int nameStart = source.indexOf(nameStartTag);
if (nameStart == -1) {
Expand All @@ -173,8 +203,8 @@ private static void assertDeclaration(IMember decl, BodyDeclaration body, int me
}

ISourceRange nameDeclRange = decl.getNameRange();
assertEquals(decl + "\nhas incorrect source start value", nameStart, nameDeclRange.getOffset());
assertEquals(decl + "\nhas incorrect source end value", nameEnd, nameDeclRange.getOffset() + nameDeclRange.getLength());
assertEquals(decl + "\nhas incorrect name start value", nameStart, nameDeclRange.getOffset());
assertEquals(decl + "\nhas incorrect name end value", nameEnd, nameDeclRange.getOffset() + nameDeclRange.getLength());

if (body instanceof FieldDeclaration) {
SimpleName name = ((VariableDeclarationFragment) ((FieldDeclaration) body).fragments().get(0)).getName();
Expand Down Expand Up @@ -252,8 +282,8 @@ private ICompilationUnit createCompUnit(String pack, String name, String source)
public void testSourceLocations() throws Exception {
String source =
"package p1\n" +
"/*t0s*/public class /*t0sn*/Hello/*t0en*/ {\n" +
" /*m0s*/public static void /*m0sn*/main/*m0en*/(String[] args) /*m0sb*/{\n" +
"/*t0s*/public/*t0em*/ class /*t0sn*/Hello/*t0en*/ {\n" +
" /*m0s*/public static/*m0em*/ void /*m0sn*/main/*m0en*/(String[] args) /*m0sb*/{\n" +
" System.out.println(\"Hello world\");\n" +
" }/*m0e*/\n" +
" /*f1s*/int /*f1sn*/x/*f1en*/ = 9/*f1e*/;\n" +
Expand All @@ -265,8 +295,8 @@ public void testSourceLocations() throws Exception {
public void testSourceLocationsNoSemiColons() throws Exception {
String source =
"package p1\n" +
"/*t0s*/public class /*t0sn*/Hello/*t0en*/ {\n" +
" /*m0s*/public static void /*m0sn*/main/*m0en*/(String[] args) /*m0sb*/{\n" +
"/*t0s*/public/*t0em*/ class /*t0sn*/Hello/*t0en*/ {\n" +
" /*m0s*/public static/*m0em*/ void /*m0sn*/main/*m0en*/(String[] args) /*m0sb*/{\n" +
" System.out.println(\"Hello world\");\n" +
" }/*m0e*/\n" +
" /*f1s*/int /*f1sn*/x/*f1en*/ = 9/*f1e*/\n" +
Expand All @@ -287,6 +317,32 @@ public void testSourceLocationsNoModifiers() throws Exception {
assertUnitWithSingleType(source, createCompUnit("p1", "Hello", source));
}

@Test // https://github.com/groovy/groovy-eclipse/issues/758
public void testSourceLocationsAnnotationModifiers1() throws Exception {
String source =
"package p1\n" +
"/*t0s*/class /*t0sn*/Hello/*t0en*/ {\n" +
" /*m0s*/@Deprecated/*m0em*/\n" +
" def /*m0sn*/main/*m0en*/(args) /*m0sb*/{\n" +
" println 'Hello world'\n" +
" }/*m0e*/\n" +
"}/*t0e*/\n";
assertUnitWithSingleType(source, createCompUnit("p1", "Hello", source));
}

@Test
public void testSourceLocationsAnnotationModifiers2() throws Exception {
String source =
"package p1\n" +
"/*t0s*/class /*t0sn*/Hello/*t0en*/ {\n" +
" /*m0s*/@Deprecated(/*lalala*/)/*m0em*/\n" +
" def /*m0sn*/main/*m0en*/(args) /*m0sb*/{\n" +
" println 'Hello world'\n" +
" }/*m0e*/\n" +
"}/*t0e*/\n";
assertUnitWithSingleType(source, createCompUnit("p1", "Hello", source));
}

@Test
public void testSourceLocationsMultipleVariableFragments() throws Exception {
String source =
Expand Down Expand Up @@ -347,7 +403,7 @@ public void testSourceLocationsConstructor() throws Exception {
String source =
"package p1\n" +
"/*t0s*/class /*t0sn*/Hello/*t0en*/ {\n" +
" /*m0s*/public /*m0sn*/Hello/*m0en*/() /*m0sb*/{\n" +
" /*m0s*/public/*m0em*/ /*m0sn*/Hello/*m0en*/() /*m0sb*/{\n" +
" System.out.println(\"Hello world\")\n" +
" }/*m0e*/\n" +
"}/*t0e*/\n";
Expand All @@ -359,7 +415,7 @@ public void testSourceLocationsConstructorWithParam() throws Exception {
String source =
"package p1\n" +
"/*t0s*/class /*t0sn*/Hello/*t0en*/ {\n" +
" /*m0s*/public /*m0sn*/Hello/*m0en*/(String x) /*m0sb*/{\n" +
" /*m0s*/public/*m0em*/ /*m0sn*/Hello/*m0en*/(String x) /*m0sb*/{\n" +
" System.out.println(\"Hello world\")\n" +
" }/*m0e*/\n" +
"}/*t0e*/\n";
Expand All @@ -371,7 +427,7 @@ public void testSourceLocationsConstructorWithParamNoType() throws Exception {
String source =
"package p1\n" +
"/*t0s*/class /*t0sn*/Hello/*t0en*/ {\n" +
" /*m0s*/public /*m0sn*/Hello/*m0en*/(x) /*m0sb*/{\n" +
" /*m0s*/public/*m0em*/ /*m0sn*/Hello/*m0en*/(x) /*m0sb*/{\n" +
" System.out.println(\"Hello world\")\n" +
" }/*m0e*/\n" +
"}/*t0e*/\n";
Expand All @@ -383,7 +439,7 @@ public void testSourceLocationsConstructorWithDefaultParam() throws Exception {
String source =
"package p1\n" +
"/*t0s*/class /*t0sn*/Hello/*t0en*/ {\n" +
" /*m0s*/public /*m0sn*/Hello/*m0en*/(args = \"9\") /*m0sb*/{\n" +
" /*m0s*/public/*m0em*/ /*m0sn*/Hello/*m0en*/(args = \"9\") /*m0sb*/{\n" +
" System.out.println(\"Hello world\")\n" +
" }/*m0e*/\n" +
"}/*t0e*/\n";
Expand Down Expand Up @@ -443,7 +499,7 @@ public void testSourceLocationsConstructorWithDefaultParams() throws Exception {
String source =
"package p1\n" +
"/*t0s*/class /*t0sn*/Hello/*t0en*/ {\n" +
" /*m0s*/public /*m0sn*/Hello/*m0en*/(args = \"9\", String blargs = \"8\") /*m0sb*/{\n" +
" /*m0s*/public/*m0em*/ /*m0sn*/Hello/*m0en*/(args = \"9\", String blargs = \"8\") /*m0sb*/{\n" +
" System.out.println(\"Hello world\")\n" +
" }/*m0e*/\n" +
"}/*t0e*/\n";
Expand All @@ -455,7 +511,7 @@ public void testSourceLocationsInterface1() throws Exception {
String source =
"package p1\n" +
"/*t0s*/interface /*t0sn*/Hello/*t0en*/ {\n" +
" /*m0s*/public String /*m0sn*/hello/*m0en*/(args, String blargs)/*m0e*/\n" +
" /*m0s*/public/*m0em*/ String /*m0sn*/hello/*m0en*/(args, String blargs)/*m0e*/\n" +
"}/*t0e*/\n";
assertUnitWithSingleType(source, createCompUnit("p1", "Hello", source));
}
Expand All @@ -474,8 +530,8 @@ public void testSourceLocationsInterface2() throws Exception {
public void testSourceLocationsAbstractClass1() throws Exception {
String source =
"package p1\n" +
"/*t0s*/abstract class /*t0sn*/Hello/*t0en*/ {\n" +
" /*m0s*/public abstract /*m0sn*/hello/*m0en*/(args, String blargs)/*m0e*/\n" +
"/*t0s*/abstract/*t0em*/ class /*t0sn*/Hello/*t0en*/ {\n" +
" /*m0s*/public abstract/*m0em*/ /*m0sn*/hello/*m0en*/(args, String blargs)/*m0e*/\n" +
"}/*t0e*/\n";
assertUnitWithSingleType(source, createCompUnit("p1", "Hello", source));
}
Expand All @@ -484,8 +540,8 @@ public void testSourceLocationsAbstractClass1() throws Exception {
public void testSourceLocationsAbstractClass2() throws Exception {
String source =
"package p1\n" +
"/*t0s*/abstract class /*t0sn*/Hello/*t0en*/ {\n" +
" /*m0s*/abstract /*m0sn*/hello/*m0en*/(args, String blargs) throws Exception/*m0e*/\n" +
"/*t0s*/abstract/*t0em*/ class /*t0sn*/Hello/*t0en*/ {\n" +
" /*m0s*/abstract/*m0em*/ /*m0sn*/hello/*m0en*/(args, String blargs) throws Exception/*m0e*/\n" +
"}/*t0e*/\n";
assertUnitWithSingleType(source, createCompUnit("p1", "Hello", source));
}
Expand Down Expand Up @@ -545,7 +601,7 @@ public void testSourceLocationsTrailingWhitespace3() throws Exception {
String source =
"package p1\n" +
"/*t0s*/class /*t0sn*/Hello/*t0en*/ {\n" +
" /*m0s*/public /*m0sn*/Hello/*m0en*/(int one, int two) /*m0sb*/{\n" +
" /*m0s*/public/*m0em*/ /*m0sn*/Hello/*m0en*/(int one, int two) /*m0sb*/{\n" +
" }/*m0e*/ \t \n" +
"}/*t0e*/\n";
assertUnitWithSingleType(source, createCompUnit("p1", "Hello", source));
Expand All @@ -556,7 +612,7 @@ public void testSourceLocationsTrailingWhitespace4() throws Exception {
String source =
"package p1\n" +
"/*t0s*/class /*t0sn*/Hello/*t0en*/ {\n" +
" /*m0s*/protected String /*m0sn*/bar/*m0en*/(int one, int two) /*m0sb*/{\n" +
" /*m0s*/protected/*m0em*/ String /*m0sn*/bar/*m0en*/(int one, int two) /*m0sb*/{\n" +
" }/*m0e*/ \t \n" +
"}/*t0e*/\n";
assertUnitWithSingleType(source, createCompUnit("p1", "Hello", source));
Expand Down
96 changes: 45 additions & 51 deletions base/org.codehaus.groovy24/src/org/codehaus/groovy/antlr/AntlrParserPlugin.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1569,12 +1569,33 @@ protected int setModifierBit(AST node, int answer, int bit) {
}

protected AnnotationNode annotation(AST annotationNode) {
/* GRECLIPSE edit
AST node = annotationNode.getFirstChild();
String name = qualifiedName(node);
AnnotationNode annotatedNode = new AnnotationNode(ClassHelper.make(name));
// GRECLIPSE edit
//configureAST(annotatedNode, annotationNode);
configureAnnotationAST(annotatedNode, annotationNode);
configureAST(annotatedNode, annotationNode);
*/
AnnotationNode annotatedNode = new AnnotationNode(makeType(annotationNode));
configureAST(annotatedNode, annotationNode.getFirstChild());
// Eclipse wants this for error reporting on name range:
annotatedNode.setEnd(annotatedNode.getEnd() - 1);

// save the full source range of the annotation for future use
int start = locations.findOffset(annotationNode.getLine(), annotationNode.getColumn());
int until = locations.findOffset(((GroovySourceAST) annotationNode).getLineLast(), ((GroovySourceAST) annotationNode).getColumnLast());
// check for trailing whitespace
if (getController() != null) {
char[] sourceChars = getController().readSourceRange(start, until - start);
if (sourceChars != null) {
int i = (sourceChars.length - 1);
while (i >= 0 && Character.isWhitespace(sourceChars[i])) {
i -= 1; until -= 1;
}
}
}
annotatedNode.setNodeMetaData("source.offsets", Long.valueOf(((long) start << 32) | until)); // pack the offsets into one long integer value

AST node = annotationNode.getFirstChild();
// GRECLIPSE end
while (true) {
node = node.getNextSibling();
Expand All @@ -1598,8 +1619,7 @@ protected AnnotationNode annotation(AST annotationNode) {
//-------------------------------------------------------------------------

protected Statement statement(AST node) {
// GRECLIPSE add
// GRECLIPSE-1038 avoid NPE on bad code
// GRECLIPSE add -- avoid NPEs on bad code
if (node == null) return new EmptyStatement();
// GRECLIPSE end
Statement statement = null;
Expand Down Expand Up @@ -2597,16 +2617,6 @@ protected Expression methodPointerExpression(AST node) {
return methodPointerExpression;
}

/* commented out due to groovy.g non-determinisms
protected Expression defaultMethodPointerExpression(AST node) {
AST exprNode = node.getFirstChild();
String methodName = exprNode.toString();
MethodPointerExpression methodPointerExpression = new MethodPointerExpression(null, methodName);
configureAST(methodPointerExpression, node);
return methodPointerExpression;
}
*/

protected Expression listExpression(AST listNode) {
List<Expression> expressions = new ArrayList<Expression>();
AST elist = listNode.getFirstChild();
Expand Down Expand Up @@ -2694,7 +2704,6 @@ protected MapEntryExpression mapEntryExpression(AST node) {
}
}


protected Expression instanceofExpression(AST node) {
AST leftNode = node.getFirstChild();
Expression leftExpression = expression(leftNode);
Expand Down Expand Up @@ -2776,7 +2785,6 @@ protected Expression castExpression(AST castNode) {
return castExpression;
}


protected Expression indexExpression(AST indexNode) {
AST bracket = indexNode.getFirstChild();
AST leftNode = bracket.getNextSibling();
Expand Down Expand Up @@ -3533,7 +3541,7 @@ protected ClassNode makeType(AST typeNode) {
AST node = typeNode.getFirstChild();
if (node != null) {
if (isType(INDEX_OP, node) || isType(ARRAY_DECLARATOR, node)) {
// GRECLIPSE edit
// GRECLIPSE edit -- retain generics
//answer = makeType(node).makeArray();
answer = makeTypeWithArguments(node).makeArray();
// GRECLIPSE end
Expand All @@ -3545,22 +3553,30 @@ protected ClassNode makeType(AST typeNode) {
answer = newAnswer;
}
}
// GRECLIPSE add
if (getController() != null) {
// check for trailing whitespace
GroovySourceAST root = (GroovySourceAST) node;
int start = locations.findOffset(root.getLine(), root.getColumn());
int until = locations.findOffset(root.getLineLast(), root.getColumnLast());
char[] sourceChars = getController().readSourceRange(start, until - start);
if (sourceChars != null) {
int idx = (sourceChars.length - 1), off = until;
while (idx >= 0 && Character.isWhitespace(sourceChars[idx])) {
idx -= 1; off -= 1;
}
if (off < until) {
int[] row_col = locations.getRowCol(off);
root.setLineLast(row_col[0]); root.setColumnLast(row_col[1]);
}
}
}
// GRECLIPSE end
configureAST(answer, node);
}
return answer;
}

/**
* Performs a name resolution to see if the given name is a type from imports,
* aliases or newly created classes
*/
/*protected String resolveTypeName(String name, boolean safe) {
if (name == null) {
return null;
}
return resolveNewClassOrName(name, safe);
}*/

/**
* Extracts an identifier from the Antlr AST and then performs a name resolution
* to see if the given name is a type from imports, aliases or newly created classes
Expand Down Expand Up @@ -3632,7 +3648,6 @@ protected String label(AST labelNode) {
// Helper methods
//-------------------------------------------------------------------------


/**
* Returns true if the modifiers flags contain a visibility modifier
*/
Expand Down Expand Up @@ -3744,7 +3759,6 @@ protected String getFirstChildText(AST node) {
return child != null ? child.getText() : null;
}


public static boolean isType(int typeCode, AST node) {
return node != null && node.getType() == typeCode;
}
Expand Down Expand Up @@ -3906,25 +3920,5 @@ private Statement createSyntheticAfterImports() {
}
return synthetic;
}

protected void configureAnnotationAST(AnnotationNode node, AST ast) {
if (ast == null) {
throw new ASTRuntimeException(ast, "PARSER BUG: Tried to configure " + node.getClass().getName() + " with null AST");
}
if (!(ast instanceof GroovySourceAST)) {
throw new ASTRuntimeException(ast, "PARSER BUG: Expected a GroovySourceAST node for annotation, but got: " + ast.getClass().getName());
}
// Structure of incoming parameter 'ast':
// ANNOTATION
// - down='annotationName' (IDENT:84)
configureAST(node, ast.getFirstChild());
configureAST(node.getClassNode(), ast.getFirstChild());
// save the full source range of the annotation for future use
long start = locations.findOffset(ast.getLine(), ast.getColumn());
long until = locations.findOffset(((GroovySourceAST) ast).getLineLast(), ((GroovySourceAST) ast).getColumnLast());
node.setNodeMetaData("source.offsets", (start << 32) | until); // pack the two offsets into one long integer value

node.setEnd(node.getEnd() - 1); // Eclipse wants this for error reporting
}
// GRECLIPSE end
}
4 changes: 0 additions & 4 deletions base/org.codehaus.groovy24/src/org/codehaus/groovy/ast/ClassCodeVisitorSupport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,10 +198,6 @@ protected void addError(String msg, ASTNode node) {
} else if (!(node instanceof DeclarationExpression)) {
start = node.getStart();
end = node.getEnd() - 1;
if (node instanceof ClassNode && ((ClassNode) node).isArray()) {
// avoid extra whitespace after closing ]
end -= 1;
}
} else {
addError(msg, ((DeclarationExpression) node).getLeftExpression());
return;
Expand Down
Loading

0 comments on commit eef5ff8

Please sign in to comment.