fix: prevent importing classes which are already imported via *

[algomaster99]

Reference: ASSERT-KTH/sorald#598

While debugging the above issue, I saw that types in java.util.* are always imported even though they have been imported via a wildcard - import java.util.*.

[algomaster99]

Sorry for the long commit messages, but I think they describe the changes quite well.

Apparently, classes like java.util.List were imported even though import java.util.* was present in the file. I found out that it was happening because import java.util.* was unresolved and there wasn't any check if java.util.List is encompassed in the wildcard import. I have simply checked it using ctImport.toString().contains(packageReference.toString(), but if there is a better API to do so, please let me know.

I also tested for unresolved static import statements, but they were working correctly with/without this change so I didn't bother pushing that test case.

[algomaster99]

@MartinWitt @slarse can you please take a look?

[slarse]

Had some quick thoughts on this.

[slarse]

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.

[slarse]

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.

[algomaster99]

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.

[slarse]

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.

[algomaster99]

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.

[algomaster99]

New finding: import java.util.List can be resolved, but import java.util.* cannot be. I think we should go in this direction rather than checking if the package reference is part of the wildcard import.

This is where the package is not getting resolved.

I found the reason, but I don't understand the comments written just above it:

// only get package from the model by traversing from rootPackage the model
// it does not use reflection to achieve that

Why are we looking for java.util (abstracted from java.util.*) package in the rootPackage of the model being built? It definitely won't be there because we are not building the model for java.util.

When we have to resolve the import for specific classes, say import java.util.List, we use the ClassLoader to do so. I can infer from the documentation that it most probably looks for the class in the filesystem which makes sense.

@monperrus @slarse @MartinWitt @I-Al-Istannen could you throw some light here?

[algomaster99]

An approach to this problem could be to use getDefinedPackage and then we add get(Package) method in PackageFactory to get the equivalent spoon CtPackage. This is completely analogous to how we load a class. I am not sure if this is the correct way to load packages and I have intuition the above two comments explain why this analogous approach hasn't been adopted.

[monperrus]

Go for it!

[algomaster99]

I have tried to load a package analogous to how we load classes. Apparently, there wasn't a method called loadPackage so I have created my own.

I first fetch all the packages in the current class loader and the ones accessible from it. This would include all in-built packages and the ones passed via the classpath. Then I look up that array for packageName and if it exists, I create a package (but I don't add it to the model being built). This ensures that import java.util.* is resolved.

I am unsure why packages corresponding to wildcard imports were only looked up in the model being built. It is not necessary that package is part of the model and thus, we had to look up using class loaders.

[monperrus]

thanks a lot @algomaster99

[algomaster99]

I found another way, perhaps better, to write loadPackage:

private CtPackage loadPackage(String packageName) {
	LookupEnvironment environment = this.declarationUnit.scope.environment();
	HashtableOfPackage<PlainPackageBinding> pbs = this.sourceUnit.module(environment).declaredPackages;

	if (pbs.containsKey(packageName.toCharArray())) {
		PlainPackageBinding pb = pbs.get(packageName.toCharArray());
		CtPackage ctPackage = factory.createPackage();
		ctPackage.setSimpleName(new String(pb.readableName()));
		return ctPackage;
	}

	// get package by traversing the model
	return factory.Package().get(packageName);
}

This looks up all the references to the packages already built by the ReferenceBuilder. Upon replacing the method in this PR, all tests still pass :)