You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@groovy.apache.org by em...@apache.org on 2022/03/19 13:48:19 UTC
[groovy] branch GROOVY_2_5_X updated: GROOVY-5103: allow plain star imports to pick up static inner types
This is an automated email from the ASF dual-hosted git repository.
emilles pushed a commit to branch GROOVY_2_5_X
in repository https://gitbox.apache.org/repos/asf/groovy.git
The following commit(s) were added to refs/heads/GROOVY_2_5_X by this push:
new e2b6553 GROOVY-5103: allow plain star imports to pick up static inner types
e2b6553 is described below
commit e2b6553c402d9187220f3a450b2871c24c31a4cb
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Sat Jul 18 13:55:12 2020 -0500
GROOVY-5103: allow plain star imports to pick up static inner types
- Java allows "import foo.Bar.*" to resolve "Baz" to "foo.Bar$Baz"
---
.../java/org/codehaus/groovy/ast/ImportNode.java | 69 ++++++++--------
.../codehaus/groovy/control/ResolveVisitor.java | 94 +++++++++-------------
src/test/groovy/ImportTest.groovy | 60 ++++++++++----
src/test/groovy/bugs/Groovy4193Bug.groovy | 31 -------
4 files changed, 119 insertions(+), 135 deletions(-)
diff --git a/src/main/java/org/codehaus/groovy/ast/ImportNode.java b/src/main/java/org/codehaus/groovy/ast/ImportNode.java
index df249bb..e6665aa 100644
--- a/src/main/java/org/codehaus/groovy/ast/ImportNode.java
+++ b/src/main/java/org/codehaus/groovy/ast/ImportNode.java
@@ -18,14 +18,14 @@
*/
package org.codehaus.groovy.ast;
-import org.objectweb.asm.Opcodes;
+import static java.util.Objects.requireNonNull;
/**
- * Represents an import statement of a single class
+ * Represents an import statement.
*/
-public class ImportNode extends AnnotatedNode implements Opcodes {
+public class ImportNode extends AnnotatedNode {
- private final ClassNode type;
+ private ClassNode type;
private final String alias;
private final String fieldName;
private final String packageName;
@@ -33,13 +33,13 @@ public class ImportNode extends AnnotatedNode implements Opcodes {
private final boolean isStatic;
/**
- * Represent an import of an entire package, i.e. import package.Classname
+ * An import of a single type, i.e. {@code import pack.Type} or {@code import pack.Type as Alias}
*
- * @param type the referenced class
+ * @param type the type reference
* @param alias optional alias
*/
- public ImportNode(ClassNode type, String alias) {
- this.type = type;
+ public ImportNode(final ClassNode type, final String alias) {
+ this.type = requireNonNull(type);
this.alias = alias;
this.isStar = false;
this.isStatic = false;
@@ -48,26 +48,26 @@ public class ImportNode extends AnnotatedNode implements Opcodes {
}
/**
- * Represent an import of an entire package, i.e. import package.*
+ * An import of all types in a package, i.e. {@code import pack.*}
*
* @param packageName the name of the package
*/
- public ImportNode(String packageName) {
+ public ImportNode(final String packageName) {
this.type = null;
this.alias = null;
this.isStar = true;
this.isStatic = false;
- this.packageName = packageName;
+ this.packageName = requireNonNull(packageName);
this.fieldName = null;
}
/**
- * Represent a static import of a Class, i.e. import static package.Classname.*
+ * An import of all static members of a type, i.e. {@code import static pack.Type.*}
*
- * @param type the referenced class
+ * @param type the type reference
*/
- public ImportNode(ClassNode type) {
- this.type = type;
+ public ImportNode(final ClassNode type) {
+ this.type = requireNonNull(type);
this.alias = null;
this.isStar = true;
this.isStatic = true;
@@ -76,24 +76,25 @@ public class ImportNode extends AnnotatedNode implements Opcodes {
}
/**
- * Represent a static import of a field or method, i.e. import static package.Classname.name
+ * An import of a static field or method of a type, i.e. {@code import static pack.Type.name} or {@code import static pack.Type.name as alias}
*
- * @param type the referenced class
+ * @param type the type reference
* @param fieldName the field name
* @param alias optional alias
*/
- public ImportNode(ClassNode type, String fieldName, String alias) {
- this.type = type;
+ public ImportNode(final ClassNode type, final String fieldName, final String alias) {
+ this.type = requireNonNull(type);
this.alias = alias;
this.isStar = false;
this.isStatic = true;
this.packageName = null;
- this.fieldName = fieldName;
+ this.fieldName = requireNonNull(fieldName);
}
/**
* @return the text display of this import
*/
+ @Override
public String getText() {
String typeName = getClassName();
if (isStar && !isStatic) {
@@ -114,14 +115,6 @@ public class ImportNode extends AnnotatedNode implements Opcodes {
return "import " + typeName + " as " + alias;
}
- public String getPackageName() {
- return packageName;
- }
-
- public String getFieldName() {
- return fieldName;
- }
-
public boolean isStar() {
return isStar;
}
@@ -134,15 +127,27 @@ public class ImportNode extends AnnotatedNode implements Opcodes {
return alias;
}
+ public String getClassName() {
+ return (type == null ? null : type.getName());
+ }
+
+ public String getFieldName() {
+ return fieldName;
+ }
+
+ public String getPackageName() {
+ return packageName;
+ }
+
public ClassNode getType() {
return type;
}
- public String getClassName() {
- return type == null ? null : type.getName();
+ public void setType(final ClassNode type) {
+ this.type = requireNonNull(type);
}
- public void visit(GroovyCodeVisitor visitor) {
+ @Override
+ public void visit(final GroovyCodeVisitor visitor) {
}
-
}
diff --git a/src/main/java/org/codehaus/groovy/control/ResolveVisitor.java b/src/main/java/org/codehaus/groovy/control/ResolveVisitor.java
index da13d8e..915a242 100644
--- a/src/main/java/org/codehaus/groovy/control/ResolveVisitor.java
+++ b/src/main/java/org/codehaus/groovy/control/ResolveVisitor.java
@@ -29,9 +29,11 @@ import org.codehaus.groovy.ast.ClassCodeExpressionTransformer;
import org.codehaus.groovy.ast.ClassHelper;
import org.codehaus.groovy.ast.ClassNode;
import org.codehaus.groovy.ast.CompileUnit;
+import org.codehaus.groovy.ast.CompileUnit.ConstructedOuterNestedClassNode;
import org.codehaus.groovy.ast.DynamicVariable;
import org.codehaus.groovy.ast.FieldNode;
import org.codehaus.groovy.ast.GenericsType;
+import org.codehaus.groovy.ast.GenericsType.GenericsTypeName;
import org.codehaus.groovy.ast.ImportNode;
import org.codehaus.groovy.ast.InnerClassNode;
import org.codehaus.groovy.ast.MethodNode;
@@ -60,6 +62,7 @@ import org.codehaus.groovy.ast.stmt.BlockStatement;
import org.codehaus.groovy.ast.stmt.CatchStatement;
import org.codehaus.groovy.ast.stmt.ForStatement;
import org.codehaus.groovy.ast.stmt.Statement;
+import org.codehaus.groovy.ast.tools.ClosureUtils;
import org.codehaus.groovy.runtime.memoize.ConcurrentCommonCache;
import org.codehaus.groovy.runtime.memoize.EvictableCache;
import org.codehaus.groovy.syntax.Types;
@@ -79,12 +82,6 @@ import java.util.Map;
import java.util.Objects;
import java.util.Set;
-import static org.codehaus.groovy.ast.CompileUnit.ConstructedOuterNestedClassNode;
-import static org.codehaus.groovy.ast.GenericsType.GenericsTypeName;
-import static org.codehaus.groovy.ast.tools.ClosureUtils.getParametersSafe;
-import static org.codehaus.groovy.ast.tools.GeneralUtils.inSamePackage;
-import static org.codehaus.groovy.ast.tools.GeneralUtils.isDefaultVisibility;
-
/**
* Visitor to resolve Types and convert VariableExpression to
* ClassExpressions if needed. The ResolveVisitor will try to
@@ -701,11 +698,9 @@ public class ResolveVisitor extends ClassCodeExpressionTransformer {
if (importNode != null && importNode != currImportNode) {
// static alias only for inner classes and must be at end of chain
ClassNode tmp = new ConstructedNestedClass(importNode.getType(), importNode.getFieldName());
- if (resolve(tmp, false, false, true)) {
- if ((tmp.getModifiers() & Opcodes.ACC_STATIC) != 0) {
- type.setRedirect(tmp.redirect());
- return true;
- }
+ if (resolve(tmp, false, false, true) && (tmp.getModifiers() & Opcodes.ACC_STATIC) != 0) {
+ type.setRedirect(tmp.redirect());
+ return true;
}
}
}
@@ -727,9 +722,8 @@ public class ResolveVisitor extends ClassCodeExpressionTransformer {
// It is either completely a inner static class or it is not.
// Since we do not want to have useless lookups we create the name
// completely and use a ConstructedClassWithPackage to prevent lookups against the package.
- String className = aliasedNode.getNameWithoutPackage() + '$' +
- name.substring(pname.length() + 1).replace('.', '$');
- ConstructedClassWithPackage tmp = new ConstructedClassWithPackage(aliasedNode.getPackageName() + ".", className);
+ String className = aliasedNode.getNameWithoutPackage() + '$' + name.substring(pname.length() + 1).replace('.', '$');
+ ClassNode tmp = new ConstructedClassWithPackage(aliasedNode.getPackageName() + ".", className);
if (resolve(tmp, true, true, false)) {
type.setRedirect(tmp.redirect());
return true;
@@ -786,55 +780,47 @@ public class ResolveVisitor extends ClassCodeExpressionTransformer {
// check package this class is defined in. The usage of ConstructedClassWithPackage here
// means, that the module package will not be involved when the
// compiler tries to find an inner class.
- ConstructedClassWithPackage tmp = new ConstructedClassWithPackage(module.getPackageName(), name);
+ ClassNode tmp = new ConstructedClassWithPackage(module.getPackageName(), name);
if (resolve(tmp, false, false, false)) {
ambiguousClass(type, tmp, name);
- type.setRedirect(tmp.redirect());
return true;
}
}
-
- // check module static imports (for static inner classes)
+ // check static imports for static inner types
for (ImportNode importNode : module.getStaticImports().values()) {
if (importNode.getFieldName().equals(name)) {
ClassNode tmp = new ConstructedNestedClass(importNode.getType(), name);
- if (resolve(tmp, false, false, true)) {
- if ((tmp.getModifiers() & Opcodes.ACC_STATIC) != 0) {
- type.setRedirect(tmp.redirect());
- return true;
- }
+ if (resolve(tmp, false, false, true) && (tmp.getModifiers() & Opcodes.ACC_STATIC) != 0) {
+ type.setRedirect(tmp.redirect());
+ return true;
}
}
}
-
- // check module node import packages
- for (ImportNode importNode : module.getStarImports()) {
- String packagePrefix = importNode.getPackageName();
- // We limit the inner class lookups here by using ConstructedClassWithPackage.
- // This way only the name will change, the packagePrefix will
- // not be included in the lookup. The case where the
- // packagePrefix is really a class is handled elsewhere.
- ConstructedClassWithPackage tmp = new ConstructedClassWithPackage(packagePrefix, name);
- if (resolve(tmp, false, false, true)) {
+ for (ImportNode importNode : module.getStaticStarImports().values()) {
+ ClassNode tmp = new ConstructedNestedClass(importNode.getType(), name);
+ if (resolve(tmp, false, false, true) && (tmp.getModifiers() & Opcodes.ACC_STATIC) != 0) {
ambiguousClass(type, tmp, name);
- type.setRedirect(tmp.redirect());
return true;
}
}
-
- // check for star imports (import static pkg.Outer.*) matching static inner classes
- for (ImportNode importNode : module.getStaticStarImports().values()) {
- ClassNode tmp = new ConstructedNestedClass(importNode.getType(), name);
- if (resolve(tmp, false, false, true)) {
- if ((tmp.getModifiers() & Opcodes.ACC_STATIC) != 0) {
+ // check star imports ("import foo.*" or "import foo.Bar.*")
+ for (ImportNode importNode : module.getStarImports()) {
+ if (importNode.getType() != null) {
+ ClassNode tmp = new ConstructedNestedClass(importNode.getType(), name);
+ if (resolve(tmp, false, false, true) && (tmp.getModifiers() & Opcodes.ACC_STATIC) != 0) {
+ ambiguousClass(type, tmp, name);
+ return true;
+ }
+ } else {
+ ClassNode tmp = new ConstructedClassWithPackage(importNode.getPackageName(), name);
+ if (resolve(tmp, false, false, true)) {
ambiguousClass(type, tmp, name);
- type.setRedirect(tmp.redirect());
return true;
}
}
-
}
}
+
return false;
}
@@ -1226,7 +1212,7 @@ public class ResolveVisitor extends ClassCodeExpressionTransformer {
protected Expression transformClosureExpression(ClosureExpression ce) {
boolean oldInClosure = inClosure;
inClosure = true;
- for (Parameter para : getParametersSafe(ce)) {
+ for (Parameter para : ClosureUtils.getParametersSafe(ce)) {
ClassNode t = para.getType();
resolveOrFail(t, ce);
visitAnnotations(para);
@@ -1426,19 +1412,17 @@ public class ResolveVisitor extends ClassCodeExpressionTransformer {
currImportNode = null;
addError("unable to resolve class " + type.getName(), type);
}
- for (ImportNode importNode : module.getStaticStarImports().values()) {
- ClassNode type = importNode.getType();
- if (resolve(type, false, false, true)) continue;
- // Maybe this type belongs in the same package as the node that is doing the
- // static import. In that case, the package may not have been explicitly specified.
- // Try with the node's package too. If still not found, revert to original type name.
- if (type.getPackageName() == null && node.getPackageName() != null) {
- String oldTypeName = type.getName();
- type.setName(node.getPackageName() + "." + oldTypeName);
- if (resolve(type, false, false, true)) continue;
- type.setName(oldTypeName);
+ for (ImportNode importNode : module.getStarImports()) {
+ if (importNode.getLineNumber() > 0) {
+ currImportNode = importNode;
+ String importName = importNode.getPackageName();
+ importName = importName.substring(0, importName.length()-1);
+ ClassNode type = ClassHelper.makeWithoutCaching(importName);
+ if (resolve(type, false, false, true)) {
+ importNode.setType(type);
+ }
+ currImportNode = null;
}
- addError("unable to resolve class " + type.getName(), type);
}
for (ImportNode importNode : module.getStaticImports().values()) {
ClassNode type = importNode.getType();
diff --git a/src/test/groovy/ImportTest.groovy b/src/test/groovy/ImportTest.groovy
index 8eda3b9..17690f3 100644
--- a/src/test/groovy/ImportTest.groovy
+++ b/src/test/groovy/ImportTest.groovy
@@ -18,26 +18,52 @@
*/
package groovy
-class ImportTest extends GroovyTestCase {
+import org.junit.Test
+import static groovy.test.GroovyAssert.assertScript
+
+final class ImportTest {
+
+ @Test
void testImportAll() {
- def file = new File("foo.txt")
- assert file instanceof File
- assert file.getClass().name == "java.io.File"
+ assertScript '''
+ def file = new File('foo')
+ assert file instanceof File
+ '''
}
-
+
+ @Test
void testImportByName() {
- def x = [:]
- assert x instanceof Map
- /**
- * For maps, map.getClass() should be used instead of map.class,
- * when map has no member, named as "class"
- */
- assert x.getClass() != null
- assert x.getClass().name.startsWith("java.util.")
-
- def y = [1, 2, 3]
- assert y instanceof List
- assert y.getClass().name.startsWith("java.util.")
+ assertScript '''
+ def map = [foo:'bar']
+ assert map instanceof Map
+ '''
+
+ assertScript '''
+ def list = [1, 2, 3]
+ assert list instanceof List
+ '''
+ }
+
+ @Test
+ void testImportStaticInnerClass() {
+ assertScript '''
+ import java.util.Map.Entry
+ Entry entry = [foo:'bar'].entrySet().first()
+ '''
+
+ // GROOVY-5103
+ assertScript '''
+ import java.util.Map.*
+ Entry entry = [foo:'bar'].entrySet().first()
+ '''
+ }
+
+ @Test // GROOVY-4193
+ void testNullSafetyOfGetClassName() {
+ def imp = new org.codehaus.groovy.ast.ImportNode('foo.bar')
+ assert imp.packageName == 'foo.bar' // constructor routing
+ assert imp.className == null // no NullPointerException
+ assert imp.text != null // no NullPointerException
}
}
diff --git a/src/test/groovy/bugs/Groovy4193Bug.groovy b/src/test/groovy/bugs/Groovy4193Bug.groovy
deleted file mode 100644
index efc11a5..0000000
--- a/src/test/groovy/bugs/Groovy4193Bug.groovy
+++ /dev/null
@@ -1,31 +0,0 @@
-/*
- * 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.
- */
-package groovy.bugs
-
-import org.codehaus.groovy.ast.ImportNode
-
-class Groovy4193Bug extends GroovyTestCase {
- void testNPEOnImportNode() {
- def imp = new ImportNode(null, "GROOVY-4193")
-
- assertNull imp.className // no NPE
-
- assertNotNull imp.text // no NPE
- }
-}
\ No newline at end of file