Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prevent importing classes which are already imported via * #4320

Merged
merged 5 commits into from
Dec 6, 2021
Merged

fix: prevent importing classes which are already imported via * #4320

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c07aafd
Add test for checking if classes are imported if they are already imp…
algomaster99 Dec 1, 2021
c4a86f9
Ignore adding imports if wildcard import is already present but is un…
algomaster99 Dec 1, 2021
b1c2ff4
Refactor method
algomaster99 Dec 1, 2021
a76ff26
Revert "Ignore adding imports if wildcard import is already present b…
algomaster99 Dec 3, 2021
ed2de44
Load package via looking up in the class loader
algomaster99 Dec 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/main/java/spoon/reflect/visitor/ImportCleaner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,9 @@ void addImport(CtReference ref) {
//java.lang is always imported implicitly. Ignore it
return;
}
if (isPackageImportedViaWildcardAndUnresolved(packageRef)) {
return;
}
if (Objects.equals(packageQName, packageRef.getQualifiedName()) && !isStaticExecutableRef(ref)) {
//it is reference to a type of the same package. Do not add it
return;
Expand All @@ -201,6 +204,12 @@ void addImport(CtReference ref) {
}
}

private boolean isPackageImportedViaWildcardAndUnresolved(CtPackageReference packageReference) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name is a bit weird to me; you can't import a package. Maybe something like this?

Suggested change
private boolean isPackageImportedViaWildcardAndUnresolved(CtPackageReference packageReference) {
private boolean isUsedInWildcardImport(CtPackageReference packageReference) {

Not crazy about that name either, but as a package itself can't be imported, I don't think we should... imply that it can.

return compilationUnit.getImports().stream().anyMatch(
ctImport -> ctImport.toString().contains(packageReference.toString())
&& ctImport.getImportKind() == CtImportKind.UNRESOLVED);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When toString() is used for matching like this, my spidey sense always tingles a bit.

Imagine that packageReference points to java.util, and ctImport is java.util.concurrent.*. Don't we get a false positive there? I.e. "java.util.concurrent.*".contains("java.util") is true, and java.util.concurrent.* is a wildcard import.

Copy link
Contributor Author

@algomaster99 algomaster99 Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I delved into ImportCleaner further and I was wondering about its existence. We use ImportCleaner to add/remove import statements, but I can't figure out why that is a function of Spoon. If the imports in a compilation unit are missing or extra, that will cause a compilation error and spoon is not expected to behave correctly on such models.

You are right that this case will be a false-positive. The ImportCleaner will not add import, say java.util.List, if java.util.concurrent.* is present and List interface is used somewhere in the code. But is it Spoon's job to do so? The program would not compile even before it's processed with Spoon.

Anyway, of course, we need a better way to know if the wildcard import exists corresponding to the packageReference. It might be sufficient to look at the declaring type and then compare but since the imports are unresolved, we will have to do that via splitting string and that sounds ugly to me.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right that this case will be a false-positive. The ImportCleaner will not add import, say java.util.List, if java.util.concurrent.* is present and List interface is used somewhere in the code. But is it Spoon's job to do so? The program would not compile even before it's processed with Spoon.

You're thinking of this from the perspective of this being the input, but Spoon is made for AST transformations. The purpose of auto-import functionality is to automatically add imports after one has transformed the AST. For example, in a Spoon model, I can insert a variable declaration with type java.util.List, but I don't want to find this in the printed output:

java.util.List<Integer> myNewList = new java.util.ArrayList<>();

No one writes Java like that. Rather, I want this:

List<Integer> myNewList = new ArrayList<>();

Since imports aren't really part of the AST (they belong to the compilation unit), it's kind of awkward to get that right.

As a concrete example closer to what you work with, look at the XxeProcessingProcessor in Sorald (rule 2755). It inserts references to the java.xml.XMLConstants class, which must sometimes be imported (see for example this PR). But if you look at the implementation of that processor, it does nothing with import statements, that's handled later during output processing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense.

As a concrete example closer to what you work with, look at the XxeProcessingProcessor in Sorald (rule 2755). It inserts references to the java.xml.XMLConstants class, which must sometimes be imported (see for example this PR). But if you look at the implementation of that processor, it does nothing with import statements, that's handled later during output processing.

Yes, I disabled the ImportCleaner and saw a flurry of failed test cases. Rules like 2116 also required import statements (java.util.Arrays) to be added.

}

void onCompilationUnitProcessed(CtCompilationUnit compilationUnit) {
ModelList<CtImport> existingImports = compilationUnit.getImports();
Set<CtImport> computedImports = new HashSet<>(this.computedImports.values());
Expand Down
12 changes: 9 additions & 3 deletions src/test/java/spoon/reflect/visitor/ImportCleanerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package spoon.reflect.visitor;

import org.junit.Test;
import org.junit.jupiter.api.Test;
import spoon.Launcher;
import spoon.reflect.CtModel;
import spoon.reflect.declaration.CtCompilationUnit;
Expand All @@ -16,13 +16,19 @@
public class ImportCleanerTest {

@Test
public void testDoesNotDuplicateUnresolvedImports() {
void testDoNotImportClassesIfAlreadyImportedViaWildcard() {
// contract: The import cleaner should not add classes from java.util if they are imported via wildcard
testImportCleanerDoesNotAlterImports("src/test/resources/wildcard-import/WildcardImport.java", "WildcardImport");
}

@Test
void testDoesNotDuplicateUnresolvedImports() {
// contract: The import cleaner should not duplicate unresolved imports
testImportCleanerDoesNotAlterImports("./src/test/resources/unresolved/UnresolvedImport.java", "UnresolvedImport");
}

@Test
public void testDoesNotImportInheritedStaticMethod() {
void testDoesNotImportInheritedStaticMethod() {
// contract: The import cleaner should not import static attributes that are inherited
testImportCleanerDoesNotAlterImports("./src/test/resources/inherit-static-method", "Derived");
}
Expand Down
5 changes: 5 additions & 0 deletions src/test/resources/wildcard-import/WildcardImport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import java.util.*;

public class WildcardImport {
private static List<Integer> x = new ArrayList<>();
}