AbstractMethodDeclaration.sourceEnd should be rparen offset not name end

- Errors on constructor/method declarations should cover parameter list - Move sourceEnds table population to GroovyCompilationUnitDeclaration - TODO: Should enum constants with a body get the same treatment? #611 #612
groovy · Jan 20, 2019 · a149c5b · a149c5b
1 parent 72cc3a3
commit a149c5b
Show file tree

Hide file tree

Showing 24 changed files with 316 additions and 355 deletions.
diff --git a/...e.tests.builder/src/org/eclipse/jdt/core/groovy/tests/locations/SourceLocationsTests.java b/...e.tests.builder/src/org/eclipse/jdt/core/groovy/tests/locations/SourceLocationsTests.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2009-2018 the original author or authors.
+ * Copyright 2009-2019 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -24,9 +24,7 @@
 
 import java.util.List;
 
-import org.eclipse.core.resources.IFile;
 import org.eclipse.core.resources.IMarker;
-import org.eclipse.core.resources.ResourcesPlugin;
 import org.eclipse.core.runtime.IPath;
 import org.eclipse.jdt.core.Flags;
 import org.eclipse.jdt.core.ICompilationUnit;
@@ -35,9 +33,7 @@
 import org.eclipse.jdt.core.IMethod;
 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;
@@ -46,6 +42,7 @@
 import org.eclipse.jdt.core.dom.MethodDeclaration;
 import org.eclipse.jdt.core.dom.SimpleName;
 import org.eclipse.jdt.core.dom.VariableDeclarationFragment;
+import org.eclipse.jdt.core.groovy.tests.SimpleProgressMonitor;
 import org.eclipse.jdt.core.groovy.tests.builder.BuilderTestSuite;
 import org.eclipse.jdt.core.tests.builder.Problem;
 import org.eclipse.jdt.groovy.core.util.JavaConstants;
@@ -58,8 +55,8 @@
  * Source locations are deteremined by special marker comments in the code:<pre>
  * markers /*m1s* / /*f1s* / /*t1s* / indicate start of method, field and type
  * markers /*m1e* / /*f1e* / /*t1e* / indicate end of method, field and type
- * markers /*m1sn* / /*f1sn* / /*t1sn* / indicate start of method, field and type names
- * markers /*m1en* / /*f1en* / /*t1en* / indicate end of method, field and type names
+ * markers /*m1sn* / /*f1sn* / /*t1sn* / indicate start of method, field and type name
+ * markers /*m1en* / /*f1en* / /*t1en* / indicate end of method, field and type name
  * markers /*m1sb* / indicate the start of a method body</pre>
  *
  * NOTE: The start of a type body is not being calculated correctly.
@@ -69,9 +66,16 @@ public final class SourceLocationsTests extends BuilderTestSuite {
     private static void assertUnitWithSingleType(String source, ICompilationUnit unit) throws Exception {
         assertUnit(unit, source);
 
-        ASTParser newParser = ASTParser.newParser(JavaConstants.AST_LEVEL);
-        newParser.setSource(unit);
-        CompilationUnit ast = (CompilationUnit) newParser.createAST(null);
+        CompilationUnit ast = null;
+        unit.becomeWorkingCopy(null);
+        try {
+            SimpleProgressMonitor monitor = new SimpleProgressMonitor("reconcile");
+            ast = unit.reconcile(JavaConstants.AST_LEVEL, true, null, monitor);
+            monitor.waitForCompletion();
+        } finally {
+            unit.discardWorkingCopy();
+        }
+
         IType decl = unit.getTypes()[0];
         AbstractTypeDeclaration typeDecl = (AbstractTypeDeclaration) ast.types().get(0);
 
@@ -256,6 +260,15 @@ private static void assertUnit(ICompilationUnit unit, String source) throws Exce
         assertEquals(unit + "\nhas incorrect source end value", source.length(), unit.getSourceRange().getLength());
     }
 
+    private ICompilationUnit createCompUnit(String pack, String name, String text) throws Exception {
+        IPath root = createGenericProject();
+        IPath path = env.addGroovyClass(root, pack, name, text);
+
+        fullBuild();
+        expectingNoProblems();
+        return env.getUnit(path);
+    }
+
     private IPath createGenericProject() throws Exception {
         IPath projectPath = env.addProject("Project");
         env.addGroovyJars(projectPath);
@@ -266,16 +279,6 @@ private IPath createGenericProject() throws Exception {
         return root;
     }
 
-    private ICompilationUnit createCompUnit(String pack, String name, String source) throws Exception {
-        IPath root = createGenericProject();
-        IPath path = env.addGroovyClass(root, pack, name, source);
-
-        fullBuild();
-        expectingNoProblems();
-        IFile groovyFile = ResourcesPlugin.getWorkspace().getRoot().getFile(path);
-        return JavaCore.createCompilationUnitFrom(groovyFile);
-    }
-
     //--------------------------------------------------------------------------
 
     @Test

diff --git a/...vy.core.tests.compiler/src/org/eclipse/jdt/groovy/core/tests/basic/GroovySimpleTests.java b/...vy.core.tests.compiler/src/org/eclipse/jdt/groovy/core/tests/basic/GroovySimpleTests.java
@@ -1132,7 +1132,7 @@ public void testOverriding_MissingAnnotation1() {
         "----------\n" +
         "1. ERROR in Foo.groovy (at line 2)\n" +
         "\tboolean equals(that) { false }\n" +
-        "\t        ^^^^^^\n" +
+        "\t        ^^^^^^^^^^^^\n" +
         "The method equals(Object) of type Foo should be tagged with @Override since it actually overrides a superclass method\n" +
         "----------\n",
         options);
@@ -1156,7 +1156,7 @@ public void testOverriding_MissingAnnotation2() {
         "----------\n" +
         "1. ERROR in Foo.groovy (at line 3)\n" +
         "\tdef baz = new Object() {      boolean equals(that) { false }\n" +
-        "\t                                      ^^^^^^\n" +
+        "\t                                      ^^^^^^^^^^^^\n" +
         "The method equals(Object) of type new Object(){} should be tagged with @Override since it actually overrides a superclass method\n" +
         "----------\n",
         options);
@@ -1178,7 +1178,7 @@ public void testOverriding_MissingAnnotation3() {
         "----------\n" +
         "1. ERROR in Foo.groovy (at line 2)\n" +
         "\tIterator iterator() { null }\n" +
-        "\t         ^^^^^^^^\n" +
+        "\t         ^^^^^^^^^^\n" +
         "The method iterator() of type Foo should be tagged with @Override since it actually overrides a superinterface method\n" +
         "----------\n",
         options);
@@ -1204,7 +1204,7 @@ public void testOverriding_MissingAnnotation4() {
         "----------\n" +
         "1. ERROR in Foo.groovy (at line 3)\n" +
         "\tdef baz = new Iterable() {      Iterator iterator() { null }\n" +
-        "\t                                         ^^^^^^^^\n" +
+        "\t                                         ^^^^^^^^^^\n" +
         "The method iterator() of type new Iterable(){} should be tagged with @Override since it actually overrides a superinterface method\n" +
         "----------\n",
         options);

diff --git a/base/org.codehaus.groovy24/src/org/codehaus/groovy/antlr/AntlrParserPlugin.java b/base/org.codehaus.groovy24/src/org/codehaus/groovy/antlr/AntlrParserPlugin.java
@@ -1232,6 +1232,7 @@ protected void constructorDef(AST constructorDef) {
         if (parameters == null) parameters = Parameter.EMPTY_ARRAY;
         // GRECLIPSE add
         int nameEnd = locations.findOffset(node.getLine(), node.getColumn()) - 1;
+        groovySourceAST = (GroovySourceAST) node;
         // GRECLIPSE end
         node = node.getNextSibling();
 
@@ -1259,6 +1260,8 @@ protected void constructorDef(AST constructorDef) {
         // GRECLIPSE add
         constructorNode.setNameStart(nameStart);
         constructorNode.setNameEnd(nameEnd - 1);
+        constructorNode.putNodeMetaData("rparen.offset",
+            locations.findOffset(groovySourceAST.getLineLast(), groovySourceAST.getColumnLast()));
         // GRECLIPSE end
     }
 

diff --git a/base/org.codehaus.groovy24/src/org/codehaus/groovy/classgen/Verifier.java b/base/org.codehaus.groovy24/src/org/codehaus/groovy/classgen/Verifier.java
@@ -820,6 +820,8 @@ public void visitVariableExpression(VariableExpression expression) {
                 newMethod.setOriginal(method);
                 newMethod.setNameEnd(method.getNameEnd());
                 newMethod.setNameStart(method.getNameStart());
+                Integer value = method.getNodeMetaData("rparen.offset");
+                if (value != null) newMethod.putNodeMetaData("rparen.offset", value);
                 // GRECLIPSE end
                 newMethod.setGenericsTypes(method.getGenericsTypes());
                 newMethod.putNodeMetaData(DEFAULT_PARAMETER_GENERATED, Boolean.TRUE);
@@ -840,6 +842,8 @@ public void call(ArgumentListExpression arguments, Parameter[] newParams, Method
                 ctor.setOriginal(method);
                 ctor.setNameEnd(method.getNameEnd());
                 ctor.setNameStart(method.getNameStart());
+                Integer value = method.getNodeMetaData("rparen.offset");
+                if (value != null) ctor.putNodeMetaData("rparen.offset", value);
                 ctor.putNodeMetaData(DEFAULT_PARAMETER_GENERATED, Boolean.TRUE);
                 // GRECLIPSE end
             }

diff --git a/base/org.codehaus.groovy25/src/org/codehaus/groovy/antlr/AntlrParserPlugin.java b/base/org.codehaus.groovy25/src/org/codehaus/groovy/antlr/AntlrParserPlugin.java
@@ -1234,6 +1234,7 @@ protected void constructorDef(AST constructorDef) {
         if (parameters == null) parameters = Parameter.EMPTY_ARRAY;
         // GRECLIPSE add
         int nameEnd = locations.findOffset(node.getLine(), node.getColumn()) - 1;
+        groovySourceAST = (GroovySourceAST) node;
         // GRECLIPSE end
         node = node.getNextSibling();
 
@@ -1261,6 +1262,8 @@ protected void constructorDef(AST constructorDef) {
         // GRECLIPSE add
         constructorNode.setNameStart(nameStart);
         constructorNode.setNameEnd(nameEnd - 1);
+        constructorNode.putNodeMetaData("rparen.offset",
+            locations.findOffset(groovySourceAST.getLineLast(), groovySourceAST.getColumnLast()));
         // GRECLIPSE end
     }
 

diff --git a/base/org.codehaus.groovy25/src/org/codehaus/groovy/classgen/Verifier.java b/base/org.codehaus.groovy25/src/org/codehaus/groovy/classgen/Verifier.java
@@ -887,6 +887,8 @@ public void visitVariableExpression(VariableExpression expression) {
                 newMethod.setOriginal(method);
                 newMethod.setNameEnd(method.getNameEnd());
                 newMethod.setNameStart(method.getNameStart());
+                Integer value = method.getNodeMetaData("rparen.offset");
+                if (value != null) newMethod.putNodeMetaData("rparen.offset", value);
                 // GRECLIPSE end
                 newMethod.setGenericsTypes(method.getGenericsTypes());
                 newMethod.putNodeMetaData(DEFAULT_PARAMETER_GENERATED, Boolean.TRUE);
@@ -907,6 +909,8 @@ public void call(ArgumentListExpression arguments, Parameter[] newParams, Method
                 ctor.setOriginal(method);
                 ctor.setNameEnd(method.getNameEnd());
                 ctor.setNameStart(method.getNameStart());
+                Integer value = method.getNodeMetaData("rparen.offset");
+                if (value != null) ctor.putNodeMetaData("rparen.offset", value);
                 ctor.putNodeMetaData(DEFAULT_PARAMETER_GENERATED, Boolean.TRUE);
                 // GRECLIPSE end
             }

diff --git a/base/org.codehaus.groovy30/src/org/codehaus/groovy/antlr/AntlrParserPlugin.java b/base/org.codehaus.groovy30/src/org/codehaus/groovy/antlr/AntlrParserPlugin.java
@@ -1322,6 +1322,7 @@ protected void constructorDef(AST constructorDef) {
         if (parameters == null) parameters = Parameter.EMPTY_ARRAY;
         // GRECLIPSE add
         int nameEnd = locations.findOffset(node.getLine(), node.getColumn()) - 1;
+        groovySourceAST = (GroovySourceAST) node;
         // GRECLIPSE end
         node = node.getNextSibling();
 
@@ -1349,6 +1350,8 @@ protected void constructorDef(AST constructorDef) {
         // GRECLIPSE add
         constructorNode.setNameStart(nameStart);
         constructorNode.setNameEnd(nameEnd - 1);
+        constructorNode.putNodeMetaData("rparen.offset",
+            locations.findOffset(groovySourceAST.getLineLast(), groovySourceAST.getColumnLast()));
         // GRECLIPSE end
     }
 

diff --git a/base/org.codehaus.groovy30/src/org/codehaus/groovy/classgen/Verifier.java b/base/org.codehaus.groovy30/src/org/codehaus/groovy/classgen/Verifier.java
@@ -886,6 +886,8 @@ public void visitVariableExpression(VariableExpression expression) {
                 newMethod.setOriginal(method);
                 newMethod.setNameEnd(method.getNameEnd());
                 newMethod.setNameStart(method.getNameStart());
+                Integer value = method.getNodeMetaData("rparen.offset");
+                if (value != null) newMethod.putNodeMetaData("rparen.offset", value);
                 // GRECLIPSE end
                 newMethod.setGenericsTypes(method.getGenericsTypes());
                 newMethod.putNodeMetaData(DEFAULT_PARAMETER_GENERATED, Boolean.TRUE);
@@ -906,6 +908,8 @@ public void call(ArgumentListExpression arguments, Parameter[] newParams, Method
                 ctor.setOriginal(method);
                 ctor.setNameEnd(method.getNameEnd());
                 ctor.setNameStart(method.getNameStart());
+                Integer value = method.getNodeMetaData("rparen.offset");
+                if (value != null) ctor.putNodeMetaData("rparen.offset", value);
                 ctor.putNodeMetaData(DEFAULT_PARAMETER_GENERATED, Boolean.TRUE);
                 // GRECLIPSE end
             }

diff --git a/...ovy.core/src/org/codehaus/jdt/groovy/integration/internal/MultiplexingIndexingParser.java b/...ovy.core/src/org/codehaus/jdt/groovy/integration/internal/MultiplexingIndexingParser.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2009-2018 the original author or authors.
+ * Copyright 2009-2019 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -27,13 +27,9 @@
 import org.eclipse.jdt.internal.compiler.ISourceElementRequestor;
 import org.eclipse.jdt.internal.compiler.SourceElementNotifier;
 import org.eclipse.jdt.internal.compiler.SourceElementParser;
-import org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration;
 import org.eclipse.jdt.internal.compiler.ast.CompilationUnitDeclaration;
-import org.eclipse.jdt.internal.compiler.ast.FieldDeclaration;
-import org.eclipse.jdt.internal.compiler.ast.TypeDeclaration;
 import org.eclipse.jdt.internal.compiler.env.ICompilationUnit;
 import org.eclipse.jdt.internal.compiler.impl.CompilerOptions;
-import org.eclipse.jdt.internal.compiler.util.HashtableOfObjectToInt;
 import org.eclipse.jdt.internal.core.search.indexing.IndexingParser;
 import org.eclipse.jdt.internal.core.util.Util;
 
@@ -70,8 +66,7 @@ public CompilationUnitDeclaration parseCompilationUnit(ICompilationUnit unit, bo
             CompilationResult compilationResult = new CompilationResult(unit, 0, 0, options.maxProblemsPerUnit);
 
             // FIXASC Is it ok to use a new parser here everytime? If we don't we sometimes recurse back into the first one
-            GroovyCompilationUnitDeclaration cud = (GroovyCompilationUnitDeclaration)
-                new GroovyParser(options, problemReporter, false, true).dietParse(unit, compilationResult);
+            GroovyCompilationUnitDeclaration cud = new GroovyParser(options, problemReporter, false, true).dietParse(unit, compilationResult);
 
             if (cud.getModuleNode() != null) {
                 try {
@@ -82,42 +77,8 @@ public CompilationUnitDeclaration parseCompilationUnit(ICompilationUnit unit, bo
                 }
             }
 
-            notifier.notifySourceElementRequestor(cud, 0, unit.getContents().length, groovyReportReferenceInfo, createSourceEnds(cud),
-                /* We don't care about the @category tag, so pass empty map */ Collections.EMPTY_MAP);
+            notifier.notifySourceElementRequestor(cud, 0, unit.getContents().length, groovyReportReferenceInfo, cud.sourceEnds, Collections.EMPTY_MAP);
             return cud;
         }
     }
-
-    // FIXASC this code is copied from MultiplexingSourceElementParser. Should combine
-    // FIXASC This should be calculated in GroovyCompilationUnitDeclaration
-    private HashtableOfObjectToInt createSourceEnds(CompilationUnitDeclaration cDecl) {
-        HashtableOfObjectToInt table = new HashtableOfObjectToInt();
-        if (cDecl.types != null) {
-            for (TypeDeclaration tDecl : cDecl.types) {
-                createSourceEndsForType(tDecl, table);
-            }
-        }
-        return table;
-    }
-
-    // FIXASC this code is copied from MultiplexingSourceElementParser. Should combine
-    // FIXASC This should be calculated in GroovyCompilationUnitDeclaration
-    private void createSourceEndsForType(TypeDeclaration tDecl, HashtableOfObjectToInt table) {
-        table.put(tDecl, tDecl.sourceEnd);
-        if (tDecl.fields != null) {
-            for (FieldDeclaration fDecl : tDecl.fields) {
-                table.put(fDecl, fDecl.sourceEnd);
-            }
-        }
-        if (tDecl.methods != null) {
-            for (AbstractMethodDeclaration mDecl : tDecl.methods) {
-                table.put(mDecl, mDecl.sourceEnd);
-            }
-        }
-        if (tDecl.memberTypes != null) {
-            for (TypeDeclaration innerTDecl : tDecl.memberTypes) {
-                createSourceEndsForType(innerTDecl, table);
-            }
-        }
-    }
 }
diff --git a/...core/src/org/codehaus/jdt/groovy/integration/internal/MultiplexingMatchLocatorParser.java b/...core/src/org/codehaus/jdt/groovy/integration/internal/MultiplexingMatchLocatorParser.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2009-2017 the original author or authors.
+ * Copyright 2009-2019 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -40,9 +40,8 @@ public CompilationUnitDeclaration dietParse(ICompilationUnit sourceUnit, Compila
                 .isGroovyLikeFileName(sourceUnit.getFileName())) {
             // FIXASC Is it ok to use a new parser here everytime? If we don't we sometimes recurse back into the first one
             // FIXASC ought to reuse to ensure types end up in same groovy CU
-            return new GroovyParser(this.groovyParser.getCompilerOptions(), this.groovyParser.problemReporter, false, true)
-                    .dietParse(sourceUnit, compilationResult);
-            // return groovyParser.dietParse(sourceUnit, compilationResult);
+            GroovyParser groovyParser = new GroovyParser(this.groovyParser.getCompilerOptions(), this.groovyParser.problemReporter, false, true);
+            return groovyParser.dietParse(sourceUnit, compilationResult);
         } else {
             return super.dietParse(sourceUnit, compilationResult);
         }