Skip to content

Commit

Permalink
Fix for #1184: check for inner of super, which is implicitly available
Browse files Browse the repository at this point in the history
  • Loading branch information
@eric-milles
eric-milles committed Oct 20, 2020
1 parent a74edbe commit c4bee0c
Show file tree
Hide file tree
Showing 2 changed files with 101 additions and 42 deletions.
31 changes: 31 additions & 0 deletions ...vy.eclipse.tests/src/org/codehaus/groovy/eclipse/test/actions/OrganizeImportsTests.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -946,6 +946,37 @@ final class OrganizeImportsTests extends OrganizeImportsTestSuite {
doContentsCompareTest(contents)
}

@Test // https://github.com/groovy/groovy-eclipse/issues/1184
void testInnerClass11() {
createGroovyType 'p', 'A', '''\
|abstract class A {
| protected static class B {
| }
|}
|'''
String contents = '''\
|import p.A
|import p.A.B
|class C extends A {
| def m(B b) {
| }
|}
|'''
doContentsCompareTest(contents, contents - ~/\|import p.A.B\s+/)
}

@Test // https://github.com/groovy/groovy-eclipse/issues/1184
void testInnerClass12() {
String contents = '''\
|import java.util.Map.Entry
|class C implements Map {
| def m(Entry e) {
| }
|}
|'''
doContentsCompareTest(contents, contents - ~/\|import java.util.Map.Entry\s+/)
}

@Test
void testStaticImport1() {
String contents = '''\
Expand Down
112 changes: 70 additions & 42 deletions ...eclipse.ui/src/org/codehaus/groovy/eclipse/refactoring/actions/OrganizeGroovyImports.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,32 @@ private IType[] resolveMissingTypes(IProgressMonitor monitor) throws JavaModelEx
}
}

/**
* Determines if organize imports is unsafe due to syntax errors or other conditions.
*/
private static boolean isUnclean(ModuleNodeInfo info, GroovyCompilationUnit unit) {
try {
if (info.module.encounteredUnrecoverableError() || !unit.isConsistent()) {
return true;
}
CategorizedProblem[] problems = info.result.getProblems();
if (problems != null && problems.length > 0) {
for (CategorizedProblem problem : problems) {
if (problem.isError() && problem.getCategoryID() == CategorizedProblem.CAT_INTERNAL) {
String message = problem.getMessage();
if (message.contains("unexpected token")) {
trace("Stopping due to error in compilation unit: %s", message);
return true;
}
}
}
}
} catch (Exception e) {
return true;
}
return false;
}

/**
* GRECLIPSE-1390
* Reorganizing imports (ie- sorting and grouping them) will remove annotations on import statements
Expand All @@ -376,16 +402,16 @@ private static boolean isSafeToReorganize(Iterable<ImportNode> allImports) {
return true;
}

private static boolean isAliased(ImportNode imp) {
String alias = imp.getAlias();
private static boolean isAliased(ImportNode node) {
String alias = node.getAlias();
if (alias == null) {
return false;
}
String fieldName = imp.getFieldName();
String fieldName = node.getFieldName();
if (fieldName != null) {
return !fieldName.equals(alias);
}
String className = imp.getClassName();
String className = node.getClassName();
if (className != null) {
// it is possible to import from the default package
boolean aliasIsSameAsClassName = className.endsWith(alias) &&
Expand All @@ -395,30 +421,14 @@ private static boolean isAliased(ImportNode imp) {
return false;
}

/**
* Determines if organize imports is unsafe due to syntax errors or other conditions.
*/
private static boolean isUnclean(ModuleNodeInfo info, GroovyCompilationUnit unit) {
try {
if (info.module.encounteredUnrecoverableError() || !unit.isConsistent()) {
return true;
}
CategorizedProblem[] problems = info.result.getProblems();
if (problems != null && problems.length > 0) {
for (CategorizedProblem problem : problems) {
if (problem.isError() && problem.getCategoryID() == CategorizedProblem.CAT_INTERNAL) {
String message = problem.getMessage();
if (message.contains("unexpected token")) {
trace("Stopping due to error in compilation unit: %s", message);
return true;
}
}
}
}
} catch (Exception e) {
return true;
private static String getTypeName(ClassNode node) {
ClassNode type = GroovyUtils.getBaseType(node);
// unresolved name may have dots and/or dollars (e.g. 'a.b.C$D' or 'C$D' or even 'C.D')
if (!type.getName().matches(".*\\b" + type.getUnresolvedName().replace('$', '.'))) {
// synch up name and unresolved name (e.g. 'java.util.Map$Entry as Foo$Entry')
return type.getName() + " as " + type.getUnresolvedName().replace('.', '$');
}
return false;
return type.getName();
}

private static void trace(String message, Object... arguments) {
Expand Down Expand Up @@ -691,10 +701,9 @@ private void handleTypeReference(ClassNode node, boolean isAnnotation) {
name = name.replaceAll(Pattern.quote(name.substring(i)) + "(?= |$)", "");
}
doNotRemoveImport(name.replace('$', '.'));
return;
}

if (!node.isResolved() && !current.getModule().getClasses().contains(node.redirect())) {
} else if (current.getModule().getClasses().contains(node)) {
// keep in importsSlatedForRemoval and leave out of missingTypes
} else if (!node.isResolved()) {
String[] parts = name.split("\\.");
if (Character.isUpperCase(name.charAt(0))) {
name = parts[0]; // 'Map.Entry' -> 'Map'
Expand All @@ -714,15 +723,17 @@ private void handleTypeReference(ClassNode node, boolean isAnnotation) {
while (i < chars.length && Character.isJavaIdentifierPart(chars[i])) {
i += 1;
}
// for a 'Map.Entry' reference find '.Map' in 'java.util.Map' or '.Map$' in 'java.util.Map$Entry'
m = Pattern.compile("(?:\\A|\\$|\\.)" + String.valueOf(chars, 0, i) + "(?=\\$|$)").matcher(name);
if (m.find()) {
// 'java.util.Map$Entry' -> 'java.util.Map'
String partialName = name.substring(0, m.end()).replace('$', '.');
doNotRemoveImport(partialName);
String partialName = name.substring(0, m.end());
if (!isInnerOfSuper(partialName))
doNotRemoveImport(partialName.replace('$', '.'));
}
} else {
// We don't know exactly what the text is. We just know how
// it resolves. This can be a problem if an inner class. We
// We do not know exactly what the text is. We just know how
// it resolves. This can be a problem for an inner class. We
// don't really know what is in the text nor what the import
// is, so just ensure that none are slated for removal.
String partialName = name.replace('$', '.');
Expand All @@ -740,14 +751,31 @@ private void handleTypeReference(ClassNode node, boolean isAnnotation) {
}
}

private String getTypeName(ClassNode node) {
ClassNode type = GroovyUtils.getBaseType(node);
// unresolved name may have dots and/or dollars (e.g. 'a.b.C$D' or 'C$D' or even 'C.D')
if (!type.getName().matches(".*\\b" + type.getUnresolvedName().replace('$', '.'))) {
// synch up name and unresolved name (e.g. 'java.util.Map$Entry as Foo$Entry')
return type.getName() + " as " + type.getUnresolvedName().replace('.', '$');
private boolean isInnerOfSuper(String name) {
if (name.lastIndexOf('$') > 0) {
for (ClassNode node = current.getSuperClass(); node != null && !node.equals(ClassHelper.OBJECT_TYPE); node = node.getSuperClass()) {
for (Iterator<? extends ClassNode> it = node.redirect().getInnerClasses(); it.hasNext();) {
if (it.next().getName().equals(name)) {
return true;
}
}
}

java.util.Queue<ClassNode> todo = new java.util.ArrayDeque<>();
java.util.Collections.addAll(todo, current.getInterfaces());
Set<ClassNode> done = new LinkedHashSet<>();
ClassNode node;

while ((node = todo.poll()) != null) { if (!done.add(node)) continue;
for (Iterator<? extends ClassNode> it = node.redirect().getInnerClasses(); it.hasNext();) {
if (it.next().getName().equals(name)) {
return true;
}
}
java.util.Collections.addAll(todo, node.getInterfaces());
}
}
return type.getName();
return false;
}

private boolean checkRetainImport(String name) {
Expand Down

0 comments on commit c4bee0c

Please sign in to comment.