Support for Java 9 project Jigsaw module-info.java files #800

matozoid · 2017-02-24T21:14:37Z

This is a first draft. It seems to parse and print files quite well.

It has one big disadvantage: there is now a second grammar, and it contains a copy of the first. The differences are small, so that should be fixable with some trickery.

CSM or symbol solver support is not there yet.

I think the most interesting thing to look at is the unit test: https://github.com/javaparser/javaparser/pull/800/files#diff-32ad75ad8b271ad317da30653e5b9c83

…ead. * Fix compile errors * Add module support to SourceRoot

matozoid · 2017-02-24T22:04:07Z

Build fails because the CSM wants me to implement module info. Can @ftomassetti help a bit?

ftomassetti · 2017-02-24T22:12:21Z

Sure: to implement it or make CSM not complain?
In the latter case you may want to go to ConcreteSyntaxModel.java and changes these lines:

        List<String> unsupportedNodeClassNames = JavaParserMetaModel.getNodeMetaModels().stream()
                .filter(c -> !c.isAbstract() && !Comment.class.isAssignableFrom(c.getType()) && !concreteSyntaxModelByClass.containsKey(c.getType()))
                .map(nm -> nm.getType().getSimpleName())
                .collect(Collectors.toList());

Maybe you want to create a common ancestor for the ModuleInfo AST classes and put them in a set and make CSM skipping those

matozoid · 2017-02-24T22:26:57Z

No no, to implement them. I can probably do it myself if I get the time, but I'm lazy. The PPV is already implemented.

ftomassetti · 2017-02-24T22:46:17Z

You got your first PR :D

#1

I just sketch something immediately because in the week-end I will unable to do much coding :(

draft of CSM for the Module Info nodes

ftomassetti · 2017-02-25T10:55:59Z

Do you think we will have many conflicts with #654 ?

coveralls · 2017-02-25T10:58:24Z

Coverage decreased (-10.2%) to 50.812% when pulling 3f2f661 on matozoid:issue_554_part_1 into 8771897 on javaparser:master.

matozoid · 2017-02-25T11:10:33Z

@ftomassetti - about conflicts: there is one extra parameter to the CU constructor, a bunch of extra nodes, and I've changed the names of the classes that JavaCC generates. So that may cause a lot of conflicts, but they should be simple to fix (mostly name changes.)

matozoid · 2017-02-25T11:14:51Z

@ftomassetti - I've added a test to ModuleDeclarationTest, but it complains that ModuleProvidesStmt doesn't have a hasName method. Name is required, so I don't know why I would ask that. No time to figure it out right now.

ftomassetti · 2017-02-25T11:15:12Z

@matozoid perfect. I do not plan to touch #654 anymore. If there are improvements to do (aside those that will emerge during code review) I will do as separate PRs

matozoid · 2017-02-27T16:33:57Z

This works, but it isn't pretty. The problem is with the 10 additional keywords. They have to be specified in the list of keyword tokens. When you do so, all the following tokens will get their ID increased by 10 (because we just inserted 10 new ones.) Now we have a problem with the Concrete Syntax Model, which uses the token ID's from the normal grammar, and starts outputting the wrong tokens for the module info grammar.

The quick fix is to define 10 nonsense keywords in the normal grammar. They are named "qqqqqqqqqqqqqqqqqqqqqqq...."

I'll try to merge it back into one grammar, and redefine IDENTIFIER to match those ten keywords too... If that doesn't work either I'm out of options.

# Conflicts: # javaparser-core/src/main/java/com/github/javaparser/ast/visitor/GenericListVisitorAdapter.java # javaparser-core/src/main/java/com/github/javaparser/ast/visitor/ModifierVisitor.java

matozoid · 2017-02-27T16:43:03Z

@ftomassetti what the heck does java.lang.UnsupportedOperationException: kept token(() vs ChildTextElement{foo} mean? I'm getting that from one of the unit tests.

coveralls · 2017-02-27T16:46:02Z

Coverage decreased (-11.9%) to 51.007% when pulling b937b69 on matozoid:issue_554_part_1 into 52814c7 on javaparser:master.

ftomassetti · 2017-02-27T17:50:35Z

It means that according to the difference it has calculated it is expecting to find a certain token and to keep it. It finds instead a token of a different type so it does not know what to do. Note that it is fine to find extra whitespace tokens or comments and they can be skip.

matozoid · 2017-02-27T19:15:39Z

@ftomassetti it seems to be an EOL issue again, since travis doesn't complain and it build on my linux machine.

ftomassetti · 2017-02-27T19:25:36Z

You mean the failures in Appveyor? Also, isn't surprising that coverage is down by almost 12%?

matozoid · 2017-02-27T19:34:38Z

Coverage is explained by having a second copy of the grammar, which causes a second set of generated code.

…are actually just identifiers" trick right.

coveralls · 2017-02-27T19:53:31Z

Coverage increased (+0.03%) to 62.963% when pulling de2adb5 on matozoid:issue_554_part_1 into 52814c7 on javaparser:master.

matozoid · 2017-02-27T19:57:14Z

This was rather surprising: removed the second grammar, and made all the module-info keywords match as identifiers too.

String Identifier():
{
    String ret;
}
{
    // Make sure the module info keywords don't interfere with normal Java parsing by matching them as normal identifiers.
  (<MODULE> | <REQUIRES> | <TO> | <WITH> | <OPEN> | <OPENS> | <USES> | <EXPORTS> | <PROVIDES> | <TRANSITIVE> |
   <IDENTIFIER>) { ret = token.image; }
  { return ret; }
}

So this seems to work correctly!

ftomassetti · 2017-02-27T20:11:50Z

Great!

gunnarmorling

Hey @matozoid looks good to me after a quick look. Put two comments inline, mainly I was wondering why there is not a new version of the grammar file for Java 9.

gunnarmorling · 2017-02-27T20:44:52Z

javaparser-core/src/main/java/com/github/javaparser/ast/Modifier.java

@@ -37,7 +37,8 @@
    VOLATILE,
    SYNCHRONIZED,
    NATIVE,
-    STRICTFP;
+    STRICTFP,
+    TRANSITIVE;


I don't think it should be treated as a regular modifier.

I put it there because JP matches all modifiers in every location where modifiers are allowed. The idea is to check if the correct modifiers are used by validating the AST, which is not implemented yet: #594 . Or did you mean something else?

Ah, I see.

I expected this to resemble javax.lang.model.element.Modifier which doesn't contain TRANSITIVE. I.e. the "module-only" modifiers are kept separately there. There are separate enums for the different module instruction modifiers, e.g. here for Requires.

Hmmm, that page says "mandated" is a modifier. I didn't see that in the JLS...

gunnarmorling · 2017-02-27T20:46:30Z

javaparser-core/src/main/javacc/java_1_8.jj

@@ -25,11 +25,11 @@ options {
  JAVA_UNICODE_ESCAPE=true;
  COMMON_TOKEN_ACTION=true;
  JDK_VERSION = "1.8";


How comes that's art of java_1_8.jj? I'd have assumed you'd create a new file java_9.jj instead.

Well, looking at the changes in Java 9 it seems that it is a superset of 8, so we can keep a single grammar (much less maintenance work!) - but it should be renamed yes :)

I'll not rename it right away because these comments might disappear...

Sounds good.

matozoid · 2017-02-27T20:58:21Z

@gunnarmorling thanks for the feedback - I was hoping you could check this branch out and try it on what you are thinking of using it for (if anything.)

gunnarmorling · 2017-02-27T21:21:52Z

I checked it out and parsed one test descriptor, all looking good.

I noticed though that JavaParser#parse(InputStream) doesn't like module descriptors as input, I had to use parseModuleInfo(). Is that expected?

matozoid · 2017-02-27T21:23:14Z

@gunnarmorling currently it is, since it is a separate production, but I'll look into that tomorrow. Thanks for testing!

gunnarmorling · 2017-02-27T21:28:18Z

I see. Thanks a lot for working on this, @matozoid!

coveralls · 2017-02-28T15:53:44Z

Coverage increased (+0.05%) to 62.983% when pulling a887dbe on matozoid:issue_554_part_1 into 52814c7 on javaparser:master.

matozoid · 2017-02-28T15:58:15Z

javaparser-core/src/main/javacc/java.jj

+            tn = AnnotationTypeDeclaration(modifier) { types = add(types, tn); }
+        |
+            module = ModuleDeclaration(modifier)
+        |


This is what I did to make CompilationUnit() match all possible content. So the grammar thinks it's okay to mix classes, multiple module declarations, enums and annotation declarations in the same file. Yet again, a validator could check if a module declaration is mixed with type declarations and flag a warning or error.

matozoid · 2017-02-28T16:00:24Z

Okay, the main JavaParser.parse(...) methods should now parse module declarations, and the grammar file no longer contains the java version, which wasn't helping anyone anyway.

coveralls · 2017-02-28T16:04:06Z

Coverage increased (+0.05%) to 62.983% when pulling a887dbe on matozoid:issue_554_part_1 into 52814c7 on javaparser:master.

matozoid · 2017-02-28T16:05:54Z

I think this will do as a nice preview. If anything changes in the specs we can change it later.

ftomassetti

Looks good, my only real concern is about the token type being IDENTIFIER when parsing identifiers that are keywords in the context of module declarations

ftomassetti · 2017-02-28T16:12:33Z

...parser-core-generators/src/main/java/com/github/javaparser/generator/core/CoreGenerator.java

+        new TreeStructureVisitorGenerator(sourceRoot).generate();
+        new ModifierVisitorGenerator(sourceRoot).generate();
+
+        new GetNodeListsGenerator(sourceRoot).generate();


Why they are divided in two blocks? Is this worth a comment to explain what the two groups of generators represent?

ftomassetti · 2017-02-28T16:13:22Z

...rators/src/main/java/com/github/javaparser/generator/core/visitor/CloneVisitorGenerator.java

-    public CloneVisitorGenerator(JavaParser javaParser, SourceRoot sourceRoot) {
-        super(javaParser, sourceRoot, "com.github.javaparser.ast.visitor", "CloneVisitor", "void", "A", true);
+    public CloneVisitorGenerator(SourceRoot sourceRoot) {
+        super(sourceRoot, "com.github.javaparser.ast.visitor", "CloneVisitor", "Visitable", "Object", true);


Here I miss Kotlin named parameters :)

True true, but if you really like those then the latest intellij can help you too :)

ftomassetti · 2017-02-28T16:14:32Z

javaparser-core/src/main/java/com/github/javaparser/JavaParser.java

-            return result.getResult().get();
-        }
-        throw new ParseProblemException(result.getProblems());
+        return result.getResult().orElseThrow(() -> new ParseProblemException(result.getProblems()));


ftomassetti · 2017-02-28T16:18:33Z

javaparser-core/src/main/java/com/github/javaparser/ast/nodeTypes/NodeWithAnnotations.java

     */
-    default NormalAnnotationExpr addAnnotation(Class<? extends Annotation> clazz) {
+    default N addAnnotation(Class<? extends Annotation> clazz) {
        ((Node) this).tryAddImportToParentCompilationUnit(clazz);


Is this wise? Should the addImport be optional? What if the annotation is in the same package?

This functionality wasn't changed (what I did was correct the use of the "add" and "addAndGet" pattern) but it is a good idea: #803

ftomassetti · 2017-02-28T16:20:04Z

javaparser-core/src/main/java/com/github/javaparser/ast/visitor/ModifierVisitor.java

+    public Visitable visit(ModuleUsesStmt n, A arg) {
+        Type type = (Type) n.getType().accept(this, arg);
+        Comment comment = n.getComment().map( s -> (Comment) s.accept(this, arg)).orElse(null);
+        if (type == null)


nitpick: even in generated code I would really love to see the then statement between braces

Go submit a PR for your nitpicking :-P

Actually, I have a better answer. The code in the generator is a little more readable because it doesn't contain the braces :)

ftomassetti · 2017-02-28T16:21:12Z

javaparser-core/src/main/java/com/github/javaparser/printer/ConcreteSyntaxModel.java

@@ -64,17 +65,17 @@ private static CsmElement memberAnnotations() {
    }

    private static CsmElement annotations() {
-        return list(ObservableProperty.ANNOTATIONS, none(), none(), newline());
+        return list(ObservableProperty.ANNOTATIONS, space(), none(), newline());


I am surprised this change does not cause tests to fail

I was surprised it was working at all :D

ftomassetti · 2017-02-28T16:21:30Z

javaparser-core/src/main/java/com/github/javaparser/printer/ConcreteSyntaxModel.java

    }

    private static CsmElement typeParameters() {
-        return list(ObservableProperty.TYPE_PARAMETERS, CsmElement.sequence(CsmElement.comma(), CsmElement.space()), CsmElement.token(ASTParserConstants.LT),
-                CsmElement.sequence(CsmElement.token(ASTParserConstants.GT), CsmElement.space()));
+        return list(ObservableProperty.TYPE_PARAMETERS, CsmElement.sequence(CsmElement.comma(), CsmElement.space()), CsmElement.token(GeneratedJavaParserConstants.LT),


Maybe we could use static imports for the token constants

I'm a big fan of static imports, and I thought you were not using them here because there are constants coming in from several sources. PR please :)

Not sure whether you clicked that option or not when creating this PR, but since recently one can have other contributors push to the same PR. I.e. @ftomassetti could add this kind of changes right to this PR (it's a checkbox when submitting a PR). Found it quite useful myself.

I missed it completely, but that's good to know!

ftomassetti · 2017-02-28T16:24:08Z

javaparser-core/src/main/javacc/java.jj

+}
+{
+    // Make sure the module info keywords don't interfere with normal Java parsing by matching them as normal identifiers.
+  (<MODULE> | <REQUIRES> | <TO> | <WITH> | <OPEN> | <OPENS> | <USES> | <EXPORTS> | <PROVIDES> | <TRANSITIVE> |


in this case what would be the token type? If the topken type is not IDENTIFIER we could run into issues with the CSM

That's what I was worried about too, but the tests kept running. Would you like to suggest a test for this?

Anyway, the fix would be to override the token type to IDENTIFIER right here, I think.

@ftomassetti - do you have an idea how to test this? It is the last thing before I pull this.

I would:

parse something that has a name

change it to a name corresponding to a module keyword

print it using lexical preservation

Do the viceversa (starting from something having a name like "requires").

These tests would be very similar to the ones in the lexicalpreservation package

@ftomassetti I added the following tests and they work - am I doing this right?

@Test public void printAModuleInfoSpecificKeywordUsedAsIdentifier1() { considerCode("class module { }"); cu.getClassByName("module").get().setName("xyz"); assertEquals("class xyz { }", lpp.print(cu)); } @Test public void printAModuleInfoSpecificKeywordUsedAsIdentifier2() { considerCode("class xyz { }"); cu.getClassByName("xyz").get().setName("module"); assertEquals("class module { }", lpp.print(cu)); }

ftomassetti · 2017-03-02T18:13:11Z

+1 from me: the tests in LexicalPreservingPrinterTest.java are exactly what I meant

matozoid · 2017-03-02T18:17:18Z

Thank you!

gunnarmorling · 2017-03-02T19:33:14Z

Awesome, thanks a lot for making it happen, @matozoid and @ftomassetti! Any idea when we can get it in a release?

matozoid · 2017-03-02T19:44:38Z

Yes, next Sunday. This project releases every Sunday unless something gets really in the way.

matozoid · 2017-03-02T19:46:04Z

I'm not sure what you want to do it with it, but remember that this is only partial Java 9 support :) Here's the rest: Java 9 support

matozoid added 2 commits February 24, 2017 22:04

Implement Java 9 module info

88d5ca0

Add a module info parsing method to JavaParser.

02b5acd

matozoid closed this Feb 24, 2017

matozoid added 2 commits February 24, 2017 22:57

* Remove JavaParser from all SourceRoot methods, make it a field inst…

6f5fb80

…ead. * Fix compile errors * Add module support to SourceRoot

More fixing

253fcbb

matozoid reopened this Feb 24, 2017

draft of CSM for the Module Info nodes

f3de366

Merge pull request #1 from ftomassetti/issue_554_part_1_csm

3f2f661

draft of CSM for the Module Info nodes

Attempt to get CSM working for modules

ed02cca

matozoid added 2 commits February 27, 2017 17:04

Make CSM work with a hack.

1411f35

Just use the tokens from the second grammar

1e2b42d

matozoid changed the title ~~Support for Java 9 project Jigsaw module-info.java files~~ [DRAFT] Support for Java 9 project Jigsaw module-info.java files Feb 27, 2017

matozoid added 2 commits February 27, 2017 17:34

Merge remote-tracking branch 'javaparser/master' into issue_554_part_1

5bc7c95

# Conflicts: # javaparser-core/src/main/java/com/github/javaparser/ast/visitor/GenericListVisitorAdapter.java # javaparser-core/src/main/java/com/github/javaparser/ast/visitor/ModifierVisitor.java

Try to merge in lex pres.

b937b69

Merge everything back into one grammar and use the "special keywords …

de2adb5

…are actually just identifiers" trick right.

matozoid changed the title ~~[DRAFT] Support for Java 9 project Jigsaw module-info.java files~~ Support for Java 9 project Jigsaw module-info.java files Feb 27, 2017

matozoid mentioned this pull request Feb 27, 2017

Support module-info.java files. #554

Closed

gunnarmorling reviewed Feb 27, 2017

View reviewed changes

matozoid added 2 commits February 28, 2017 16:44

Make the main CompilationUnit rule match ModuleDeclaration too.

5163fc9

Rename the grammar file

a887dbe

matozoid commented Feb 28, 2017

View reviewed changes

ftomassetti reviewed Feb 28, 2017

View reviewed changes

matozoid added 2 commits March 1, 2017 22:55

Move test to the correct package

ecdd6f0

Add test for LPP with module info keywords

1f8b0ba

matozoid merged commit 20dbe9d into javaparser:master Mar 2, 2017

matozoid deleted the issue_554_part_1 branch March 2, 2017 18:17

matozoid added this to the next release milestone Mar 2, 2017

Support for Java 9 project Jigsaw module-info.java files #800

Support for Java 9 project Jigsaw module-info.java files #800

Conversation

matozoid commented Feb 24, 2017 • edited

matozoid commented Feb 24, 2017

ftomassetti commented Feb 24, 2017

matozoid commented Feb 24, 2017

ftomassetti commented Feb 24, 2017

ftomassetti commented Feb 25, 2017

coveralls commented Feb 25, 2017

matozoid commented Feb 25, 2017

matozoid commented Feb 25, 2017

ftomassetti commented Feb 25, 2017

matozoid commented Feb 27, 2017

matozoid commented Feb 27, 2017 • edited

coveralls commented Feb 27, 2017

ftomassetti commented Feb 27, 2017

matozoid commented Feb 27, 2017

ftomassetti commented Feb 27, 2017

matozoid commented Feb 27, 2017

coveralls commented Feb 27, 2017

matozoid commented Feb 27, 2017

ftomassetti commented Feb 27, 2017

gunnarmorling left a 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

matozoid commented Feb 27, 2017

gunnarmorling commented Feb 27, 2017

matozoid commented Feb 27, 2017

gunnarmorling commented Feb 27, 2017

coveralls commented Feb 28, 2017

Choose a reason for hiding this comment

matozoid commented Feb 28, 2017

coveralls commented Feb 28, 2017

matozoid commented Feb 28, 2017

ftomassetti left a 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

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

ftomassetti commented Mar 2, 2017

matozoid commented Mar 2, 2017

gunnarmorling commented Mar 2, 2017

matozoid commented Mar 2, 2017

matozoid commented Mar 2, 2017

matozoid commented Feb 24, 2017 •

edited

matozoid commented Feb 27, 2017 •

edited