From 0bb8fedbdb04fa28f696d1e0cf6ffe14d6739768 Mon Sep 17 00:00:00 2001 From: Simon Urli Date: Mon, 25 Jun 2018 14:51:48 +0200 Subject: [PATCH] fix: the imports are properly computed after processing (#2083) --- .../visitor/DefaultJavaPrettyPrinter.java | 2 +- .../spoon/reflect/visitor/ImportScanner.java | 5 + .../reflect/visitor/ImportScannerImpl.java | 145 +++++++++++++++++- .../spoon/test/imports/ImportScannerTest.java | 22 +-- .../java/spoon/test/imports/ImportTest.java | 16 +- .../spoon/test/processing/ProcessingTest.java | 31 ++++ .../processors/RenameProcessor.java | 39 +++++ .../spoon/test/processor/test/B.java | 7 + .../spoon/test/processor/test/sub/A.java | 4 + 9 files changed, 250 insertions(+), 21 deletions(-) create mode 100644 src/test/java/spoon/test/processing/processors/RenameProcessor.java create mode 100644 src/test/resources/spoon/test/processor/test/B.java create mode 100644 src/test/resources/spoon/test/processor/test/sub/A.java diff --git a/src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java b/src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java index 02789408a2d..98cb13ca19f 100644 --- a/src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java +++ b/src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java @@ -1937,7 +1937,7 @@ public void calculate(CompilationUnit sourceCompilationUnit, List> typ this.sourceCompilationUnit = sourceCompilationUnit; this.imports = new HashSet<>(); if (sourceCompilationUnit != null) { - imports.addAll(sourceCompilationUnit.getImports()); + this.importsContext.initWithImports(sourceCompilationUnit.getImports()); } for (CtType t : types) { diff --git a/src/main/java/spoon/reflect/visitor/ImportScanner.java b/src/main/java/spoon/reflect/visitor/ImportScanner.java index c0511721abb..adf4e01e240 100644 --- a/src/main/java/spoon/reflect/visitor/ImportScanner.java +++ b/src/main/java/spoon/reflect/visitor/ImportScanner.java @@ -43,4 +43,9 @@ public interface ImportScanner { * Checks if the type is already imported. */ boolean isImported(CtReference ref); + + /** + * Specify the original imports to use before computing new imports. + */ + void initWithImports(Collection importCollection); } diff --git a/src/main/java/spoon/reflect/visitor/ImportScannerImpl.java b/src/main/java/spoon/reflect/visitor/ImportScannerImpl.java index 362afd93694..d312eedc8d9 100644 --- a/src/main/java/spoon/reflect/visitor/ImportScannerImpl.java +++ b/src/main/java/spoon/reflect/visitor/ImportScannerImpl.java @@ -21,6 +21,8 @@ import spoon.reflect.code.CtFieldAccess; import spoon.reflect.code.CtFieldRead; import spoon.reflect.code.CtInvocation; +import spoon.reflect.code.CtJavaDoc; +import spoon.reflect.code.CtJavaDocTag; import spoon.reflect.code.CtLiteral; import spoon.reflect.declaration.CtAnnotationType; import spoon.reflect.declaration.CtClass; @@ -76,6 +78,7 @@ public class ImportScannerImpl extends CtScanner implements ImportScanner { private Map namesPresentInJavaLang = new HashMap<>(); private Set fieldAndMethodsNames = new HashSet(); private Set exploredReferences = new HashSet<>(); // list of explored references + private Map usedImport = new HashMap<>(); // defined if imports had been used or not @Override public void visitCtFieldRead(CtFieldRead fieldRead) { @@ -120,6 +123,22 @@ public void visitCtTypeReference(CtTypeReference reference) { typeReference = reference; } else { typeReference = reference.getAccessType(); + + // in case of a reference to an inner class, the user might want to import it + // instead of importing the access type (which is the default behaviour) + // we cannot guess it: so we check against the existing imports + if (!typeReference.equals(reference)) { + for (CtImport ctImport : this.usedImport.keySet()) { + switch (ctImport.getImportKind()) { + case TYPE: + if (ctImport.getReference().getSimpleName().equals(reference.getSimpleName())) { + this.setImportUsed(ctImport); + super.visitCtTypeReference(reference); + } + } + } + + } } if (!this.isTypeInCollision(typeReference, false)) { @@ -132,11 +151,33 @@ public void visitCtTypeReference(CtTypeReference reference) { @Override public void scan(CtElement element) { - if (element != null && !element.isImplicit()) { + if (element != null) { element.accept(this); } } + @Override + public void visitCtJavaDoc(CtJavaDoc ctJavaDoc) { + StringBuilder stringBuilder = new StringBuilder(); + stringBuilder.append(ctJavaDoc.getContent()); + + for (CtJavaDocTag ctJavaDocTag : ctJavaDoc.getTags()) { + stringBuilder.append(ctJavaDocTag.getContent()); + } + + String javadoc = stringBuilder.toString(); + for (CtImport ctImport : this.usedImport.keySet()) { + switch (ctImport.getImportKind()) { + case TYPE: + if (javadoc.contains(ctImport.getReference().getSimpleName())) { + this.setImportUsed(ctImport); + } + break; + } + } + + } + @Override public void visitCtAnnotationType( CtAnnotationType annotationType) { @@ -182,10 +223,27 @@ public void visitCtCatchVariable(CtCatchVariable catchVariable) { super.visitCtCatchVariable(catchVariable); } + @Override + public void visitCtInvocation(CtInvocation invocation) { + this.scan(invocation.getTypeCasts()); + this.scan(invocation.getExecutable()); + if (!this.isImportedInMethodImports(invocation.getExecutable())) { + this.scan(invocation.getTarget()); + } + + this.scan(invocation.getArguments()); + } + @Override public Collection getAllImports() { Collection listallImports = new ArrayList<>(); + for (Map.Entry entry : this.usedImport.entrySet()) { + if (entry.getValue()) { + listallImports.add(entry.getKey()); + } + } + for (CtReference reference : this.classImports.values()) { listallImports.add(reference.getFactory().Type().createImport(reference)); } @@ -228,6 +286,13 @@ public boolean isImported(CtReference ref) { } } + @Override + public void initWithImports(Collection importCollection) { + for (CtImport ctImport : importCollection) { + this.usedImport.put(ctImport, Boolean.FALSE); + } + } + private boolean isThereAnotherClassWithSameNameInAnotherPackage(CtTypeReference ref) { for (CtTypeReference typeref : this.exploredReferences) { if (typeref.getSimpleName().equals(ref.getSimpleName()) && !typeref.getQualifiedName().equals(ref.getQualifiedName())) { @@ -324,7 +389,9 @@ protected boolean addClassImport(CtTypeReference ref) { CtPackageReference pack = targetType.getPackage(); if (pack != null && ref.getPackage() != null && !ref.getPackage().isUnnamedPackage()) { // ignore java.lang package - if (!ref.getPackage().getSimpleName().equals("java.lang")) { + if (ref.getPackage().getSimpleName().equals("java.lang")) { + return false; + } else { // ignore type in same package if (ref.getPackage().getSimpleName() .equals(pack.getSimpleName())) { @@ -338,7 +405,32 @@ protected boolean addClassImport(CtTypeReference ref) { return true; } + private boolean setImportUsed(CtImport ctImport) { + this.usedImport.put(ctImport, true); + return true; + } + protected boolean isImportedInClassImports(CtTypeReference ref) { + for (CtImport ctImport : this.usedImport.keySet()) { + switch (ctImport.getImportKind()) { + case TYPE: + if (ctImport.getReference().equals(ref)) { + return this.setImportUsed(ctImport); + } + break; + + case ALL_TYPES: + String packQualifiedName = ref.getPackage().getQualifiedName(); + String typeImportQualifiedName = ctImport.getReference().getSimpleName(); + + typeImportQualifiedName = typeImportQualifiedName.substring(0, typeImportQualifiedName.lastIndexOf(".")); + if (packQualifiedName.equals(typeImportQualifiedName)) { + return this.setImportUsed(ctImport); + } + break; + } + } + if (targetType != null) { CtPackageReference pack = targetType.getPackage(); @@ -429,8 +521,8 @@ protected boolean addMethodImport(CtExecutableReference ref) { if (ref.getFactory().getEnvironment().getComplianceLevel() < 5) { return false; } - if (this.methodImports.containsKey(ref.getSimpleName())) { - return isImportedInMethodImports(ref); + if (this.isImportedInMethodImports(ref)) { + return true; } // if the whole class is imported: no need to import the method. @@ -456,6 +548,30 @@ protected boolean addMethodImport(CtExecutableReference ref) { } protected boolean isImportedInMethodImports(CtExecutableReference ref) { + for (CtImport ctImport : this.usedImport.keySet()) { + switch (ctImport.getImportKind()) { + case METHOD: + CtExecutableReference execRef = (CtExecutableReference) ctImport.getReference(); + CtTypeReference declaringType = execRef.getDeclaringType(); + if (execRef.getSimpleName().equals(ref.getSimpleName()) && declaringType != null && declaringType.equals(ref.getDeclaringType())) { + return this.setImportUsed(ctImport); + } + break; + + case ALL_STATIC_MEMBERS: + if (ref.getDeclaringType() != null) { + String typeMethodQualifiedName = ref.getDeclaringType().getQualifiedName(); + String typeImportQualifiedName = ctImport.getReference().getSimpleName(); + + typeImportQualifiedName = typeImportQualifiedName.substring(0, typeImportQualifiedName.lastIndexOf(".")); + if (typeMethodQualifiedName.equals(typeImportQualifiedName)) { + return this.setImportUsed(ctImport); + } + } + break; + } + } + if (!(ref.isImplicit()) && methodImports.containsKey(ref.getSimpleName())) { CtExecutableReference exist = methodImports.get(ref.getSimpleName()); if (getSignature(exist).equals( @@ -490,6 +606,27 @@ protected boolean addFieldImport(CtFieldReference ref) { } protected boolean isImportedInFieldImports(CtFieldReference ref) { + for (CtImport ctImport : this.usedImport.keySet()) { + switch (ctImport.getImportKind()) { + case FIELD: + if (ctImport.getReference().equals(ref)) { + return this.setImportUsed(ctImport); + } + break; + + case ALL_STATIC_MEMBERS: + String typeFieldQualifiedName = ref.getQualifiedName(); + String typeImportQualifiedName = ctImport.getReference().getSimpleName(); + + typeFieldQualifiedName = typeFieldQualifiedName.substring(0, typeFieldQualifiedName.lastIndexOf(".")); + typeImportQualifiedName = typeImportQualifiedName.substring(0, typeImportQualifiedName.lastIndexOf(".")); + if (typeFieldQualifiedName.equals(typeImportQualifiedName)) { + return this.setImportUsed(ctImport); + } + break; + } + } + if (!(ref.isImplicit()) && fieldImports.containsKey(ref.getSimpleName())) { CtFieldReference exist = fieldImports.get(ref.getSimpleName()); try { diff --git a/src/test/java/spoon/test/imports/ImportScannerTest.java b/src/test/java/spoon/test/imports/ImportScannerTest.java index 6a3ca579de2..ac54c272f14 100644 --- a/src/test/java/spoon/test/imports/ImportScannerTest.java +++ b/src/test/java/spoon/test/imports/ImportScannerTest.java @@ -142,11 +142,10 @@ public void testImportOnSpoon() throws IOException { Launcher.LOGGER.warn("ImportScannerTest: Import scanner imports " + countUnusedImports + " unused imports and misses " + countMissingImports + " imports"); // Uncomment for the complete list - /* - Set keys = new HashSet<>(unusedImports.keySet()); - keys.addAll(missingImports.keySet()); - for (CtType type : keys) { + Set missingKeys = new HashSet<>(missingImports.keySet()); + + for (CtType type : missingKeys) { System.err.println(type.getQualifiedName()); if (missingImports.containsKey(type)) { List imports = missingImports.get(type); @@ -154,6 +153,15 @@ public void testImportOnSpoon() throws IOException { System.err.println("\t" + anImport + " missing"); } } + } + + assertEquals("Import scanner missed " + countMissingImports + " imports",0, countMissingImports); + + /* + Set unusedKeys = new HashSet<>(unusedImports.keySet()); + + for (CtType type : unusedKeys) { + System.err.println(type.getQualifiedName()); if (unusedImports.containsKey(type)) { List imports = unusedImports.get(type); for (String anImport : imports) { @@ -162,9 +170,6 @@ public void testImportOnSpoon() throws IOException { } } */ - - assertEquals("Import scanner missed " + countMissingImports + " imports",0, countMissingImports); - // FIXME: the unused imports should be resolved //assertEquals("Import scanner imports " + countUnusedImports + " unused imports", // 0, countUnusedImports); @@ -235,8 +240,7 @@ public void testComputeImportsInClass() throws Exception { importContext.computeImports(theClass); Collection imports = importContext.getAllImports(); - // java.lang are also computed - assertEquals(4, imports.size()); + assertEquals(2, imports.size()); } @Test diff --git a/src/test/java/spoon/test/imports/ImportTest.java b/src/test/java/spoon/test/imports/ImportTest.java index 935a249a66c..e09fe5a46d2 100644 --- a/src/test/java/spoon/test/imports/ImportTest.java +++ b/src/test/java/spoon/test/imports/ImportTest.java @@ -269,14 +269,16 @@ public void testSpoonWithImports() throws Exception { importScanner = new ImportScannerImpl(); importScanner.computeImports(anotherClass); //ClientClass needs 2 imports: ChildClass, PublicInterface2 - assertEquals(2, importScanner.getAllImports().size()); + Collection allImports = importScanner.getAllImports(); + assertEquals(2, allImports.size()); //check that printer did not used the package protected class like "SuperClass.InnerClassProtected" assertTrue(anotherClass.toString().indexOf("InnerClass extends ChildClass.InnerClassProtected")>0); importScanner = new ImportScannerImpl(); importScanner.computeImports(classWithInvocation); // java.lang imports are also computed - assertEquals("Spoon ignores the arguments of CtInvocations", 3, importScanner.getAllImports().size()); + allImports = importScanner.getAllImports(); + assertEquals("Spoon ignores the arguments of CtInvocations", 1, allImports.size()); } @Test @@ -372,10 +374,9 @@ public void testNotImportExecutableType() throws Exception { Collection imports = importContext.getAllImports(); - // java.lang.Object is considered as imported but it will never be output - assertEquals(3, imports.size()); + assertEquals(2, imports.size()); Set expectedImports = new HashSet<>( - Arrays.asList("spoon.test.imports.testclasses.internal3.Foo", "java.io.File", "java.lang.Object")); + Arrays.asList("spoon.test.imports.testclasses.internal3.Foo", "java.io.File")); Set actualImports = imports.stream().map(CtImport::getReference).map(CtReference::toString).collect(Collectors.toSet()); assertEquals(expectedImports, actualImports); } @@ -1202,7 +1203,7 @@ public void testSortImportPutStaticImportAfterTypeImport() { int countImports = 0; int nbStaticImports = 2; - int nbStandardImports = 4; + int nbStandardImports = 3; boolean startStatic = false; @@ -1224,7 +1225,8 @@ public void testSortImportPutStaticImportAfterTypeImport() { } } - assertEquals("Exactly "+nbStandardImports+nbStaticImports+" should have been counted.", (nbStandardImports+nbStaticImports), countImports); + int totalImports = nbStandardImports + nbStaticImports; + assertEquals("Exactly "+totalImports+" should have been counted.", (nbStandardImports+nbStaticImports), countImports); } @Test diff --git a/src/test/java/spoon/test/processing/ProcessingTest.java b/src/test/java/spoon/test/processing/ProcessingTest.java index 5418a8ff446..45dd5388116 100644 --- a/src/test/java/spoon/test/processing/ProcessingTest.java +++ b/src/test/java/spoon/test/processing/ProcessingTest.java @@ -1,5 +1,7 @@ package spoon.test.processing; +import com.google.common.io.Files; +import org.apache.commons.lang3.StringUtils; import org.apache.log4j.Level; import org.junit.Test; import spoon.Launcher; @@ -18,11 +20,15 @@ import spoon.reflect.declaration.CtType; import spoon.reflect.visitor.filter.TypeFilter; import spoon.support.compiler.jdt.JDTBasedSpoonCompiler; +import spoon.test.processing.processors.RenameProcessor; import spoon.test.processing.testclasses.CtClassProcessor; import spoon.test.processing.testclasses.CtInterfaceProcessor; import spoon.test.processing.testclasses.CtTypeProcessor; import spoon.testing.utils.ProcessorUtils; +import java.io.File; +import java.io.IOException; +import java.nio.charset.Charset; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; @@ -383,4 +389,29 @@ public void testCallProcessorWithMultipleTypes() { } } } + + @Test + public void testProcessDontMessWithImports() throws IOException { + // contract: after processing the imports are properly computed + Launcher launcher = new Launcher(); + launcher.getEnvironment().setNoClasspath(false); + launcher.getEnvironment().setAutoImports(true); + launcher.addInputResource("src/test/resources/spoon/test/processor/test"); + File tempDir = new File("target/testprocess"); //Files.createTempDir(); + launcher.setSourceOutputDirectory(tempDir); + launcher.addProcessor(new RenameProcessor("A", "D")); + launcher.run(); + + File Dfile = new File(tempDir, "spoon/test/processing/testclasses/test/sub/D.java"); + assertTrue(Dfile.exists()); + + File Bfile = new File(tempDir, "spoon/test/processing/testclasses/test/B.java"); + assertTrue(Bfile.exists()); + + String fileContent = StringUtils.join(Files.readLines(Bfile, Charset.defaultCharset()), "\n"); + + assertFalse(fileContent.contains("import spoon.test.processing.testclasses.test.sub.A;")); + assertTrue(fileContent.contains("import spoon.test.processing.testclasses.test.sub.D;")); + assertTrue(fileContent.contains("private D a = new D();")); + } } diff --git a/src/test/java/spoon/test/processing/processors/RenameProcessor.java b/src/test/java/spoon/test/processing/processors/RenameProcessor.java new file mode 100644 index 00000000000..9c5242e8cf1 --- /dev/null +++ b/src/test/java/spoon/test/processing/processors/RenameProcessor.java @@ -0,0 +1,39 @@ +package spoon.test.processing.processors; + +import spoon.processing.AbstractProcessor; +import spoon.reflect.declaration.CtElement; +import spoon.reflect.declaration.CtNamedElement; +import spoon.reflect.reference.CtReference; + +public class RenameProcessor extends AbstractProcessor { + private String oldName; + private String newName; + + public RenameProcessor(String oldName, String newName) { + this.oldName = oldName; + this.newName = newName; + } + + @Override + public boolean isToBeProcessed(CtElement candidate) { + if (candidate instanceof CtNamedElement) { + CtNamedElement namedElement = (CtNamedElement) candidate; + return namedElement.getSimpleName().equals(oldName); + } else if (candidate instanceof CtReference) { + CtReference reference = (CtReference) candidate; + return reference.getSimpleName().equals(oldName); + } + return false; + } + + @Override + public void process(CtElement element) { + if (element instanceof CtNamedElement) { + CtNamedElement namedElement = (CtNamedElement) element; + namedElement.setSimpleName(this.newName); + } else if (element instanceof CtReference) { + CtReference reference = (CtReference) element; + reference.setSimpleName(this.newName); + } + } +} diff --git a/src/test/resources/spoon/test/processor/test/B.java b/src/test/resources/spoon/test/processor/test/B.java new file mode 100644 index 00000000000..924044e8318 --- /dev/null +++ b/src/test/resources/spoon/test/processor/test/B.java @@ -0,0 +1,7 @@ +package spoon.test.processing.testclasses.test; + +import spoon.test.processing.testclasses.test.sub.A; + +public class B { + private A a = new A(); +} diff --git a/src/test/resources/spoon/test/processor/test/sub/A.java b/src/test/resources/spoon/test/processor/test/sub/A.java new file mode 100644 index 00000000000..f38cf4796d6 --- /dev/null +++ b/src/test/resources/spoon/test/processor/test/sub/A.java @@ -0,0 +1,4 @@ +package spoon.test.processing.testclasses.test.sub; + +public class A { +}