review feat: Add Java9 CtModule to Spoon model #1730

surli · 2017-11-16T13:12:02Z

No description provided.

surli · 2017-11-16T13:15:47Z

@monperrus @pvojtechovsky I'm starting to work on Java 9 modules to add the concept in Spoon model. Based on the JLS a module has the following definition (see: https://docs.oracle.com/javase/specs/jls/se9/html/jls-7.html#jls-7.7):

ModuleDirective:
requires {RequiresModifier} ModuleName ;
exports PackageName [to ModuleName {, ModuleName}] ;
opens PackageName [to ModuleName {, ModuleName}] ;
uses TypeName ;
provides TypeName with TypeName {, TypeName} ;
RequiresModifier:
(one of)
transitive static

I don't know how to express properly the different directives, without creating new interfaces in the model to represent them. Could you give me a quick opinion about the first proposed hierarchy of classes: then I'll be able to work on it.

monperrus · 2017-11-16T13:54:30Z

A quick proposal: |ModuleDirective: -> CtModule ModuleName -> get/setSimpleName PackageName -> CtPackageReference TypeName -> CtTypeReference transitive static -> add transitive to ModifierKind|

pvojtechovsky · 2017-11-16T18:33:38Z

|ModuleDirective: -> CtModule ModuleName -> get/setSimpleName PackageName -> CtPackageReference
TypeName -> CtTypeReference transitive static -> add transitive to ModifierKind|

@monperrus, I don't understand this language ;-)

Could you give me a quick opinion about the first proposed hierarchy of classes

Note: I have zero exprience with java9 modules. I just read some tutorials and specification linked by Simon. I have some questions ...

Q1) May be we should introduce a concept of a CtModuleMember (similar to CtTypeMember), to have one ORDERED list of all CtModuleMembers. Then each CtModule member should exist as own entity extending CtModuleMember.

Q2) Is CtModuleExport extends CtPackageReference correct? Or should we use

CtModuleExport extends CtModuleMember {
  CtPackageReference package;
  Set<CtModuleReference> targetExport;
}

Q3) Are comments allowed in module-info.java filed? If yes, then each CtModule member should NOT extend a CtReference (reference is not allowed to have comments).

pvojtechovsky · 2017-11-21T18:31:45Z

src/main/java/spoon/reflect/CtModelImpl.java

@@ -39,6 +43,23 @@

 	private static final long serialVersionUID = 1L;

+	@Override
+	public <R extends CtElement> CtQuery filterChildren(Filter<R> filter) {
+		return getUnnamedModule().getFactory().Query().createQuery(this.getAllModules().toArray()).filterChildren(filter);


QueryFactory#createQuery has actually parameter type Object, not Object[], so this code will not work as expected. We have to add new QueryFactory#createQuery(Object[]) method.

My bad, that's because CtQueryImpl constructor as an Object... arg. I'll fix that.

monperrus · 2017-11-22T15:47:14Z

src/main/java/spoon/reflect/CtModel.java

@@ -40,6 +42,16 @@
 	/** returns all packages of the model */
 	Collection<CtPackage> getAllPackages();

+	/**
+	 * returns the unnamed module


returns the unnamed module. Warning: if there are other modules, they are not contained in that unnamed root module.

monperrus · 2017-11-22T15:48:02Z

src/main/java/spoon/reflect/CtModel.java


-	/** returns the root package */
+	/** returns the root package of the unnamed module */


If there are several modules, it throws an exception

monperrus · 2017-11-22T15:55:14Z

src/main/java/spoon/Metamodel.java

@@ -146,6 +146,11 @@ private Metamodel() { }
 		result.add(factory.Type().get(spoon.reflect.reference.CtUnboundVariableReference.class));
 		result.add(factory.Type().get(spoon.reflect.reference.CtVariableReference.class));
 		result.add(factory.Type().get(spoon.reflect.reference.CtWildcardReference.class));
+		result.add(factory.Type().get(spoon.reflect.declaration.CtModule.class));
+		result.add(factory.Type().get(spoon.reflect.declaration.CtModuleRequirement.class));


CtModuleProvidedService should be called CtProvidedService

add CtUsedService

pvojtechovsky · 2017-11-22T21:07:24Z

Q1) May be we should introduce a concept of a CtModuleMember (similar to CtTypeMember), to have one ORDERED list of all CtModuleMembers. Then each CtModule member should exist as own entity extending CtModuleMember.

no reaction here? Did you understood this problem? Will be spoon able to distinguish between these two cases?

module com.example.foo {
    requires com.example.foo.http;
    exports com.example.foo.bar;
}

and

module com.example.foo {
    exports com.example.foo.bar;
    requires com.example.foo.http;
}

... one of the bigger problem which avoids using spoon productive is that it changes formatting of the pretty printed source files. So it cannot be easily applied to existing sources to let it change few things only.

I think it makes sense to keep order of module directives to minimize changes between origin and pretty printed code.

monperrus · 2017-11-22T21:41:32Z

May be we should introduce a concept of a CtModuleMember (similar to CtTypeMember), to have one ORDERED list of all CtModuleMembers.

That's a good idea.

pvojtechovsky · 2017-11-26T14:42:25Z

src/main/java/spoon/experimental/modelobs/FineModelChangeListener.java

@@ -51,6 +52,8 @@

 	void onSetAdd(CtElement currentElement, CtRole role, Set field, ModifierKind newValue);

+	void onSetAdd(CtElement currentElement, CtRole role, Set field, CtModuleRequirement.RequiresModifier newValue);


Are we really going to have copy of this method (and others) for each non CtElement type?

pvojtechovsky · 2017-11-28T16:07:22Z

I propose, at least, to remove the implementation of CtRootPackage even if we keep the class, for backward compatibility reason.

I am ok wit that idea. Optionaly we might just modify implementation of root package and

to have rootPackage.getParent() == module
to have one instance of root package for each module.
It would keep old concept of root package together with new modules. But may be discuss it with @monperrus. I do not know concept of java modules well enough.

pvojtechovsky · 2017-11-28T16:07:26Z

I propose, at least, to remove the implementation of CtRootPackage even if we keep the class, for backward compatibility reason.

I am ok wit that idea. Optionaly we might just modify implementation of root package and

to have rootPackage.getParent() == module
to have one instance of root package for each module.
It would keep old concept of root package together with new modules. But may be discuss it with @monperrus. I do not know concept of java modules well enough.

pvojtechovsky · 2017-11-28T17:05:35Z

src/main/java/spoon/reflect/factory/ModuleFactory.java

+			if (allModules.containsKey(module.getSimpleName())) {
+				return false;
+			}
+			allModules.put(module.getSimpleName(), module);


I see one more problem. Later call of CtModule.setSimpleName(...) will cause inconsistency in modules, because the module is registered under old name.
May be we should store modules as List and getModule should always search that list by name.
Then you can add the module into that list sooner directly in factory.Core().createModule() call - at time when it has no name. WDYT?

I see one more problem. Later call of CtModule.setSimpleName(...) will cause inconsistency in modules, because the module is registered under old name.

Agree that's a problem. I'll try to produce a test to show the bug, and fix it the way you propose.

pvojtechovsky · 2017-11-28T17:06:41Z

src/main/java/spoon/reflect/factory/ModuleFactory.java

+		CtModule ctModule = getUnnamedModule().getModule(moduleName);
+		if (ctModule == null) {
+			ctModule = factory.Core().createModule().setSimpleName(moduleName).setParent(getUnnamedModule());
+			getUnnamedModule().addModule(ctModule);


What about move of this code getUnnamedModule().addModule(ctModule); into factory.Core().createModule(). WDYT?

pvojtechovsky · 2017-11-28T17:12:16Z

src/main/java/spoon/reflect/CtModel.java

@@ -41,6 +42,17 @@
 	/** returns all packages of the model */
 	Collection<CtPackage> getAllPackages();

+	/**
+	 * Returns the unnamed module.
+	 * Warning: if there are other modules, they are not contained in that unnamed root module, use getAllModules instead.


Is it still true? I guess getUnnamedModule().getAllModules() now works same like Model.getAllModules(). OR?

No I think you're right.

pvojtechovsky · 2017-11-28T17:13:39Z

src/main/java/spoon/reflect/CtModelImpl.java

+
+	@Override
+	public Collection<CtModule> getAllModules() {
+		return this.unnamedModule.getFactory().Module().getAllModules();


Shorter return this.unnamedModule.getAllModules(); should return the same result now.

pvojtechovsky · 2017-11-28T17:36:27Z

src/main/java/spoon/support/reflect/declaration/CtModuleImpl.java

+			return (T) this;
+		}
+		if (this.moduleDirectives == CtElementImpl.<CtModuleDirective>emptyList()) {
+			this.moduleDirectives = new SortedList<>(new CtLineElementComparator());


I suggest to sort directives only once at time when CtModule is created by JDT builder. I think, later automatic sorting of client specific module directives by position is not expected by clients. WDYT?

I don't agree. I think we should document that the module directives are ordered by position. Now I think users don't put position attribute in the element when they're modifying the model. So they will use the method addModuleDirectiveAtPosition when they want to specify a position.

pvojtechovsky · 2017-11-28T17:40:42Z

src/main/java/spoon/support/reflect/declaration/CtModuleImpl.java

+		if (!this.getSimpleName().equals(CtModule.TOP_LEVEL_MODULE_NAME) && parent != getFactory().Module().getUnnamedModule()) {
+			throw new SpoonException("The parent of a module should necessarily be the unnamed module.");
+		}
+		return (T) super.setParent(parent);


May be we can mark setParent here as Derived property and ignore call of setParent here?
I guess there is no case when somebody should be able to set different parent then unnamedModule...

I agree, it will avoid unexpected behaviours there.

surli · 2017-11-29T09:25:10Z

I am ok wit that idea. Optionaly we might just modify implementation of root package and
1. to have rootPackage.getParent() == module
2. to have one instance of root package for each module.
It would keep old concept of root package together with new modules.

I'm ok with those idea. I'll try to implement it in that way.

surli · 2017-11-29T17:38:39Z

So I'm kind of stuck with two problems related to the JDT usage right now:

I don't manage to compile source code containing modules with JDT
I don't manage to get the module of a given type with JDT.

The master problem around those is that I don't find any proper documentation on how JDT handles modules. Actually I got some piece of information in JDT bug tracker, but I don't know if it's reliable and my experiments don't show many improvement: I suspect that the next release of JDT should add improvement on modules, but I'm not sure right now.

I was even trying to compile myself JDT from the sources to see if it helps, but even then the documentation is either hidden either inexistent.
So, my guess would be to put the two failing tests with a @Ignore, and to wait the next JDT release to see if it improves things.

WDYT @monperrus @pvojtechovsky?

pvojtechovsky · 2017-11-29T19:50:13Z

So, my guess would be to put the two failing tests with a @ignore, and to wait the next JDT release to see if it improves things.

I agree that we should merge this work even if it is not 100% finished.

surli · 2017-12-01T10:27:56Z

So I added documentation and ignored the failing tests. For me it's ready for merging @pvojtechovsky @monperrus

Once it's merged I'll create issues for the remaining unsupported features associated with modules.

spoon-bot · 2017-12-01T12:40:09Z

Detected changes by Revapi: 28.

Old API: fr.inria.gforge.spoon:spoon-core:jar:6.1.0-20171201.110051-26

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Name	Change 18
Old	method EmptyModelChangeListener::onSetAdd(CtElement, CtRole, Set, ModifierKind)
New	method EmptyModelChangeListener::onSetAdd(CtElement, CtRole, Set, EmptyModelChangeListener.Method.T)
Code	java.generics.elementNowParameterized
Description	Element now defines formal type parameters.
Breaking	binary: non_breaking,
Name	Change 18
:---:	:---:
Old	method EmptyModelChangeListener::onSetAdd(CtElement, CtRole, Set, ModifierKind)
New	method EmptyModelChangeListener::onSetAdd(CtElement, CtRole, Set, EmptyModelChangeListener.Method.T)
Code	java.generics.formalTypeParameterAdded
Description	A new formal type parameter added to element: 'T extends java.lang.Enum'.
Breaking	binary: non_breaking

Name	Change 19
Old	none
New	method PrettyPrinter::printModuleInfo(CtModule)
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	binary: non_breaking,

Name	Change 20
Old	none
New	method CompilationUnit::setDeclaredModule(CtModule)
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	binary: non_breaking,

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

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

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

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

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

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

Name	Change 27
Old	parameter EmptyModelChangeListener::onSetAdd(CtElement, CtRole, Set, ===ModifierKind===)
New	parameter EmptyModelChangeListener::onSetAdd(CtElement, CtRole, Set, ===EmptyModelChangeListener.Method.T===)
Code	java.method.parameterTypeChanged
Description	The type of the parameter changed from 'spoon.reflect.declaration.ModifierKind' to 'T extends java.lang.Enum'.
Breaking	binary: breaking

Name	Change 28
Old	parameter FineModelChangeListener::onSetAdd(CtElement, CtRole, Set, ===ModifierKind===)
New	parameter FineModelChangeListener::onSetAdd(CtElement, CtRole, Set, ===FineModelChangeListener.Method.T===)
Code	java.method.parameterTypeChanged
Description	The type of the parameter changed from 'spoon.reflect.declaration.ModifierKind' to 'T extends java.lang.Enum'.
Breaking	binary: breaking

monperrus · 2017-12-02T18:50:41Z

excellent! if everybody agrees I'll merge it on Monday.

pvojtechovsky · 2017-12-03T18:14:48Z

I agree, Nice code and documentation! Thank You Simon!

surli · 2017-12-04T10:14:30Z

@monperrus it's ready for me too

monperrus · 2017-12-04T11:15:23Z

Thanks a lot for the hard and deep work!

monperrus changed the title ~~WiP feat: Add CtModule to Spoon model~~ WiP feat: Add Java9 CtModule to Spoon model Nov 17, 2017

INRIA deleted a comment from spoon-bot Nov 21, 2017

pvojtechovsky reviewed Nov 21, 2017

View reviewed changes

surli mentioned this pull request Nov 21, 2017

bug: ElementPrinterHelper#createListPrinter "startPrefixSpace" is not supported #1735

Closed

INRIA deleted a comment from spoon-bot Nov 21, 2017

INRIA deleted a comment from spoon-bot Nov 22, 2017

monperrus reviewed Nov 22, 2017

View reviewed changes

monperrus mentioned this pull request Nov 22, 2017

review refactor: add CtModel#getRootElement and deprecates CtModel#getRootPackage #1741

Closed

monperrus reviewed Nov 22, 2017

View reviewed changes

INRIA deleted a comment from spoon-bot Nov 22, 2017

surli force-pushed the add-ctmodule branch from 8e98371 to 961a199 Compare November 24, 2017 21:41

INRIA deleted a comment from spoon-bot Nov 25, 2017

pvojtechovsky reviewed Nov 26, 2017

View reviewed changes

pvojtechovsky reviewed Nov 28, 2017

View reviewed changes

surli added 4 commits November 29, 2017 13:30

Fix some things after Pavel's review

ef3bd97

Fix an infinite loop and the checkstyle

c6b4a74

Add more tests. Fix some things

64eb78f

Fix serializable test

9425cbb

surli added 2 commits December 1, 2017 10:52

Add documentation and ignore failing tests

a8a0765

Export back good image code-elements

3959e62

INRIA deleted a comment from spoon-bot Dec 1, 2017

trigger CI

d5daf59

INRIA deleted a comment from spoon-bot Dec 1, 2017

monperrus merged commit b26167e into INRIA:master Dec 4, 2017

monperrus mentioned this pull request Dec 4, 2017

feature: add support for Java9 constructs in the metamodel #904

Closed

surli mentioned this pull request Dec 20, 2017

OHOHOH Christmas Release! Spoon 6.1.0 is coming #1789

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

review feat: Add Java9 CtModule to Spoon model #1730

review feat: Add Java9 CtModule to Spoon model #1730

surli commented Nov 16, 2017

surli commented Nov 16, 2017

monperrus commented Nov 16, 2017 via email

pvojtechovsky commented Nov 16, 2017

pvojtechovsky Nov 21, 2017

surli Nov 21, 2017

monperrus Nov 22, 2017

monperrus Nov 22, 2017

monperrus Nov 22, 2017

pvojtechovsky commented Nov 22, 2017

monperrus commented Nov 22, 2017

pvojtechovsky Nov 26, 2017

pvojtechovsky commented Nov 28, 2017

pvojtechovsky commented Nov 28, 2017

pvojtechovsky Nov 28, 2017

surli Nov 29, 2017

pvojtechovsky Nov 28, 2017

pvojtechovsky Nov 28, 2017

surli Nov 29, 2017

pvojtechovsky Nov 28, 2017

surli Nov 29, 2017

pvojtechovsky Nov 28, 2017

surli Nov 29, 2017

pvojtechovsky Nov 28, 2017

surli Nov 29, 2017

surli commented Nov 29, 2017

surli commented Nov 29, 2017

pvojtechovsky commented Nov 29, 2017

surli commented Dec 1, 2017

spoon-bot commented Dec 1, 2017

monperrus commented Dec 2, 2017

pvojtechovsky commented Dec 3, 2017

surli commented Dec 4, 2017

monperrus commented Dec 4, 2017


		/** returns the root package */
		/** returns the root package of the unnamed module */

		@@ -51,6 +52,8 @@

		void onSetAdd(CtElement currentElement, CtRole role, Set field, ModifierKind newValue);

		void onSetAdd(CtElement currentElement, CtRole role, Set field, CtModuleRequirement.RequiresModifier newValue);

review feat: Add Java9 CtModule to Spoon model #1730

review feat: Add Java9 CtModule to Spoon model #1730

Conversation

surli commented Nov 16, 2017

surli commented Nov 16, 2017

monperrus commented Nov 16, 2017 via email

pvojtechovsky commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvojtechovsky commented Nov 22, 2017

monperrus commented Nov 22, 2017

Choose a reason for hiding this comment

pvojtechovsky commented Nov 28, 2017

pvojtechovsky commented Nov 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surli commented Nov 29, 2017

surli commented Nov 29, 2017

pvojtechovsky commented Nov 29, 2017

surli commented Dec 1, 2017

spoon-bot commented Dec 1, 2017

monperrus commented Dec 2, 2017

pvojtechovsky commented Dec 3, 2017

surli commented Dec 4, 2017

monperrus commented Dec 4, 2017