review feature(import): add CtImport in the model

[surli]

The goal of this PR is to manage imports properly with CtImport.

[monperrus]

should be getImportKind

[surli]

@monperrus @pvojtechovsky @tdurieux Even if every test is not passing, I'm curious about your insight on this PR. In particular, concerning the changes in the architecture.

CtImport extends CtReference and encapsulate a CtReference: it was easier for managing scanner, but I'm mixed about this architecture. I was first considering a CtImport as only extending CtElement. What seem the better choice for you?

For now I propose to introduce CtImport in keeping the old behaviour of Spoon: imports are only contained in a CompilationUnit and are computed by ImportScanner.
After this PR, I'll start to work on linking CtImport to CtType.

[pvojtechovsky]

CtImport extends CtReference ... I was first considering a CtImport as only extending CtElement.
What seem the better choice for you?

Interesting question!
I vote for CtImport extends CtElement. Why?

What is the difference between CtReference and CtElement?
D1) CtReference has getSimpleName/setSimpleName
The CtComment#getSimpleName() should reflect the importKind, but it doesn't

D2) CtReference has getDeclaration
The CtComment#getDeclaration() should cannot return declaration of all Types of the package if it is of kind STAR_x

D3) CtReference has unsetable Comment
The CtImport can have comments.

So all 3 differences seems to be NOT fitting to CtImport extends CtReference.

WDYT?

Does any code which uses CtImport profits from getSimpleName or getDeclaration?

[pvojtechovsky]

what about

boolean includingChildren

[pvojtechovsky]

what about

if (this.localReference instanceof CtTypeReference) {
  return includingChildren ? STAR_TYPE : TYPE;
}
...

That would assure that code in ElementPrinterHelper#writeImports will always work

			switch (ctImport.getImportKind()) {
				case TYPE:
					CtTypeReference typeRef = (CtTypeReference) ctImport.getReference();
					importTypeStr = typeRef.getQualifiedName();
					if (!isJavaLangClasses(importTypeStr)) {
						setImports.add(importTypeStr);
					}
					break;

				case STAR_PACKAGE:
					CtPackageReference packageRef = (CtPackageReference) ctImport.getReference();
					importTypeStr = packageRef.getQualifiedName() + ".*";
					if (!isJavaLangClasses(importTypeStr)) {
						setImports.add(importTypeStr);
					}
					break;

				case METHOD:
					CtExecutableReference execRef = (CtExecutableReference) ctImport.getReference();
					if (execRef.getDeclaringType() != null) {
						setStaticImports.add(this.removeInnerTypeSeparator(execRef.getDeclaringType().getQualifiedName()) + "." + execRef.getSimpleName());
					}
					break;

				case FIELD:
					CtFieldReference fieldRef = (CtFieldReference) ctImport.getReference();
					setStaticImports.add(this.removeInnerTypeSeparator(fieldRef.getDeclaringType().getQualifiedName()) + "." + fieldRef.getSimpleName());
					break;

				case STAR_TYPE:
					CtTypeReference typeStarRef = (CtTypeReference) ctImport.getReference();
					importTypeStr = typeStarRef.getQualifiedName() + ".*";
					if (!isJavaLangClasses(importTypeStr)) {
						setStaticImports.add(importTypeStr);
					}
					break;
			}

[surli]

That would assure that code in ElementPrinterHelper#writeImports will always work

I'm not sure to understand your point here. You assume that the current implementation of ElementPrinterHelper#writeImports won't work in some cases?

IMHO includingChildren would give less information than ImportKind.

[pvojtechovsky]

I mean that current CtImport API is not safe for client's mistake. Client can create CtImport with kind = package, but providing CtTypeReference ... it is insconsistency.
That inconsistency might be solved if ctImport.getImportKind() would be DERIVED property and we would have instead CtImport#includingChildren which is not derived

[pvojtechovsky]

Anyway I like this PR! It moves in good direction!

[surli]

What is the difference between CtReference and CtElement?
D1) CtReference has getSimpleName/setSimpleName
The CtComment#getSimpleName() should reflect the importKind, but it doesn't

I assume you mean CtImport#getSimpleName() here :)
Actually we can override getSimpleName to use ImportKind, so no big deal on this

D2) CtReference has getDeclaration
The CtComment#getDeclaration() should cannot return declaration of all Types of the package if it is of kind STAR_x

IMHO the CtImport#getDeclaration should always return the contained reference of the CtImport. So even if you have a kind STAR_PACKAGE, it will return the declaration of the package itself. I think it keeps the semantic of the getDeclaration on an import.

D3) CtReference has unsetable Comment
The CtImport can have comments.

This is a real problem. I did not check that when I implement it.

Does any code which uses CtImport profits from getSimpleName or getDeclaration?

It's hard to say as CtImport uses the scanner for CtReference, as in the current implementation it is a CtReference.
I'd say that getSimpleName is really used, at least for comparing CtImports in sets (see #1717) but I'm really not sure for getDeclaration.

So maybe I can inherit CtImport from CtNamedElement: it makes sense to me that a CtImport has a name.

[pvojtechovsky]

So maybe I can inherit CtImport from CtNamedElement

Interesting idea ...
@monperrus WDYT about all these concepts above?

[surli]

So maybe I can inherit CtImport from CtNamedElement

I just made the changes: it shows that the getDeclaration was really never used.
I also change implementation of getSimpleName.
The whole bunch of test is passing so it looks like a good solution for me.

Concerning CtComment I did not test for now to put comments on an import, I propose to see that later.

[monperrus]

STAR_PACKAGE->ALL_TYPES
STAR_TYPE -> ALL_STATIC_MEMBERS

[monperrus]

why is this PR?

[spoon-bot]

Detected changes by Revapi: 8.

Old API: fr.inria.gforge.spoon:spoon-core:jar:6.1.0-20171123.234626-7

New API: fr.inria.gforge.spoon:spoon-core:jar:6.1.0-SNAPSHOT

Name	Change 1
Old	method DefaultJavaPrettyPrinter::computeImports(CtType)
New	method DefaultJavaPrettyPrinter::computeImports(CtType)
Code	java.method.returnTypeTypeParametersChanged
Description	The return type changed from 'java.util.Collection<spoon.reflect.reference.CtReference>' to 'java.util.Collection<spoon.reflect.declaration.CtImport>'.
Breaking	binary: non_breaking,

Name	Change 2
Old	none
New	method CoreFactory::createImport()
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	binary: non_breaking,

Name	Change 3
Old	none
New	method Factory::createImport(CtReference)
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	binary: non_breaking,

Name	Change 4
Old	none
New	method CoreFactory::createWildcardStaticTypeMemberReference()
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	binary: non_breaking,

Name	Change 5
Old	none
New	method Factory::createWildcardStaticTypeMemberReference(CtTypeReference)
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	binary: non_breaking,

Name	Change 6
Old	method ImportScanner::getAllImports()
New	method ImportScanner::getAllImports()
Code	java.method.returnTypeTypeParametersChanged
Description	The return type changed from 'java.util.Collection<spoon.reflect.reference.CtReference>' to 'java.util.Collection<spoon.reflect.declaration.CtImport>'.
Breaking	binary: non_breaking,

Name	Change 7
Old	method CompilationUnit::getImports()
New	method CompilationUnit::getImports()
Code	java.method.returnTypeTypeParametersChanged
Description	The return type changed from 'java.util.Collection<spoon.reflect.reference.CtReference>' to 'java.util.Collection<spoon.reflect.declaration.CtImport>'.
Breaking	binary: non_breaking,

Name	Change 8
Old	none
New	method CtVisitor::visitCtImport(CtImport)
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	binary: non_breaking,

[surli]

@monperrus I propose to merge the PR as it is: your last comment concerning DefaultJavaPrettyPrinterTest is actually already checked few lines later, but not exactly with a call to E1.ENUM (it still use the name of the class).

Once we have this PR merged I want to check first if #1365 is improved, and then to completely refactor ImportScanner classes: those are a mess, and I can simplify them by using CtImport.

[monperrus]

Week-end merge!

Thanks a lot @surli

[Rahamathulla]

Version : Spoon 7.0.0

I am facing the similar or same issue in Spoon 7.0.0.
I m having sample code like this

package com.somelocalpack;
import javax.ejb.*;
public class BankMdb implements MessageDrivenBean, MessageListener {
......
}

When I parse this class

Set<CtTypeReference<?>> superInterfaces = element.getSuperInterfaces();

The superInterfaces ( MessageDrivenBean, MessageListener ) are identified as
com.somelocalpack.MessageDrivenBean & com.somelocalpack.MessageListener instead of
javax.ejb.MessageDrivenBean and javax.ejb.MessageListener

[monperrus]

Thanks for the bug report. This is normal, because the jar file containing javax.ejb.* is not in the classpath. If you add it, it will work as expected. Next time, could you please create a new issue? Thanks!