-
Notifications
You must be signed in to change notification settings - Fork 9
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.
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
Stubunit #3
Stubunit #3
Conversation
… declarations before the package declaration.
…s in implemented in the CompilationUnit - Implemented VoidVisitorAdapter#visit method for StubUnit to visit all the compilation units StubUnit has.
…n exception with a message indicating that it is not implemented.
… the VoidVisitor of the javaparser.
…ub files as well as to the class. This change is require because stub files could contain additional package declarations and it is valid to have annotations on package in Java.
…gning new empty list of Annotations.
- Added looking ahead while starting parsing a compilation unit because it is possible that CompilationUnit consists only with package declaration without any classes/interfaces/enums. - Removed redundant code that no more needed after adding LOOKAHEAD(PackageDeclaration())
…mpilationUnit contains only package declaration; - Added support for top-level static interfaces in the stub files; - Adjusted javaparser-test to not give an error for top-level interfaces.
- Changed LOOKAHEAD expecting PackageDeclaration
- Added ability to pass receiver annotations to the method;
…ly pass receiver annotations.
…ge is needed for stub files without package declarations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good overall. Here are a few comments, with a couple things to fix before it is merged.
@@ -158,7 +161,15 @@ public MethodDeclaration setName(final SimpleName name) { | |||
} | |||
|
|||
@Override | |||
public MethodDeclaration setParameters(final NodeList<Parameter> parameters) { | |||
public MethodDeclaration setParameters(NodeList<Parameter> parameters) { | |||
if (parameters != null && parameters.size() > 0 && parameters.get(0).getNameAsString().equals("this")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Removing receiver parameter as it should only be added to pass the annotations for receiver | ||
parameters.remove(0); | ||
} | ||
if (parameters == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why this test is necessary. Your code does not set parameters
to null, and neither does the existing JavaParser. Remove it if it cannot be executed.
@@ -49,7 +49,7 @@ public void visit(ClassOrInterfaceDeclaration n, ProblemReporter reporter) { | |||
|
|||
private void validateClassModifiers(ClassOrInterfaceDeclaration n, ProblemReporter reporter) { | |||
if (n.isTopLevelType()) { | |||
validateModifiers(n, reporter, PUBLIC, ABSTRACT, FINAL, STRICTFP); | |||
validateModifiers(n, reporter, PUBLIC, ABSTRACT, STATIC, FINAL, STRICTFP); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is static
permitted on a top-level type?
If the old Stub Parser permitted that, it was probably to handle inner classes, which the new Stub Parser should handle correctly.
You might be able to drop the special handling of static here and elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new stubparser does not treat inner classes that declared on the top-level (e.g Runtime$One).
Possible resolutions could be to treat all classes with "$" in a name as inner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping that we could change the stub file format to be the same as a Java file. That is, instead of a stub file containing
public class Outer { ... }
public class Outer$Inner { ... }
the stub file would contain
public class Outer { ...
public class Inner { ... }
}
Is that possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is definitely not possible in java.jj. But it is possible to add a code that will go through the StubUnit after it is parsed and look for classes that contain "$" in names and try put it inside another if found the match for outer class. But this will happen only after the stub file is already parsed into the StubUnit.
Should I work on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you saying that java.jj does not handle nested/inner classes? Or that your changes to java.jj break java.jj's handling of nested/inner classes?
We would like every Java file to be a legal stub file. That goal is not possible if stub files prohibit nested/inner classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does handle nested/inner classes if they written inside the outer classes.
In the case when the class is written in the top-level, but with name Outer$Inner it is treated as a top-level class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is an example of this in the Checker Framework.
https://github.com/Bohdankm22/checker-framework/blob/master/checker/jtreg/nullness/issue824/First.astub
Inner interfaces are written at the top level. Same way static nested classes could be declared.
Here are two ways of the issues resolution:
- Handle static classes/interfaces at the top-level (already implemented)
- After parsing a stub file look for the inner/nested classes/interfaces at the top-level and try to move it inside outer classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example of the nested static class written in top-level in the Checker Framework
https://github.com/Bohdankm22/checker-framework/blob/master/framework/src/org/checkerframework/common/basetype/flow.astub#L103
@@ -0,0 +1,337 @@ | |||
package com.github.javaparser.ast.visitor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this new class used anywhere by the Stub Parser. Can you delete the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -443,7 +443,7 @@ private static ObservableProperty findNodeListName(NodeList nodeList) { | |||
if (m.getParameterCount() == 0 && m.getReturnType().getCanonicalName().equals(NodeList.class.getCanonicalName())) { | |||
try { | |||
Object raw = m.invoke(parent); | |||
if (!(raw instanceof NodeList)) { | |||
if (raw != null && !(raw instanceof NodeList)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this could never be null in the JavaParser, write a short comment about why it can be null in the Stub Parser.
@@ -711,36 +711,46 @@ StubUnit StubUnit(): | |||
ModifierHolder modifier; | |||
TypeDeclaration tn = null; | |||
ModuleDeclaration module = null; | |||
NodeList<AnnotationExpr> annotations = new NodeList<AnnotationExpr>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be helpful to write a brief comment saying what each of these two variables represents.
} | ||
{ | ||
( LOOKAHEAD(2) ";" )* | ||
[ LOOKAHEAD(PackageDeclaration()) pakage = PackageDeclaration() ] | ||
/* | ||
* Reading leading imports to support the old stub files that contain import declarations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still a documented, supported feature of the stub file format? I think it is, in which case this comment should be updated.
If it's not a documented feature of the stub file format, then remove the feature from the code.
(LOOKAHEAD(3, PackageDeclaration()) | ||
( | ||
{ | ||
cu = new CompilationUnit(range(token_source.getHomeToken(), token()), pakage, imports, types, module); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Write a comment that this is the previous compilation unit, which has been completely read already.
module = null; | ||
} | ||
pakage = PackageDeclaration() | ||
(in = ImportDeclaration() { imports = add(imports, in); })* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if two different compilation units within a single stub file have different, conflicting import statements? That is, one imports a.b.C
and the other imports d.e.C
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the imports found in a stub file are collected into a single list of imports. Each of the compilations units has the reference to the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that. I'm asking what happens if the combined list contains conflicts that none of the individual lists would have.
(Maybe nothing bad happens? Or maybe it does, and there should be a global list for imports before the first package
and there should be a local list for each compilation unit, and a compilation unit gets the concatenation of those two rather than the concatenation of every import that precedes it in the file.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the Checker Framework manual, all imports declaration should be at the beginning of a stub file.
https://checkerframework.org/manual/#stub
Other imports readings were added because some of the legacy stub files have imports declarations between the compilation units.
I am not sure how does it affect the conflict.
However, in the old stubparser, it also collects all the imports into a single list.
It could be left like that or I can remove redundant imports readings and change all the legacy stub files to declare imports only at the beginning of files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it is easy to do, let's continue permitting import statements throughout the stub file. Please update the spec.
Leave the implementation as is, and if there is a problem in the future, we'll deal with it then.
… list of receiver annotations to the variable. - Changed the condition in the LexicalPreservingPrinter#findNodeListName
- Adjusted the comments on the parser StubUnit parse.
- Added back tests to avoid top-level static classes/interfaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Please address my comments, then merge.
@@ -158,11 +158,6 @@ public MethodDeclaration setName(final SimpleName name) { | |||
} | |||
|
|||
@Override | |||
public MethodDeclaration setParameters(final NodeList<Parameter> parameters) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this code deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code was added before to handle receiver annotations. Then it was moved to the super class setParameters method.
// parts of the array rather than two sources of information about the same parts of the | ||
// array. | ||
if (arrayBracketPairLists.length > 0) { | ||
type.setArrayAnnotations(arrayBracketPairLists[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this ignore everything but the outermost set of array brackets?
Does this pass tests that you have written that contain multi-dimensional arrays with annotations on various levels?
@@ -146,4 +152,27 @@ public boolean replace(Node node, Node replacementNode) { | |||
} | |||
return super.replace(node, replacementNode); | |||
} | |||
|
|||
public List<AnnotationExpr> getAnnotationsAtLevel(int level) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every new method needs Javadoc, even if not all of JavaParser is properly documented. For example, what are the legal arguments? (Why is -1 legal for level
?) What are the possible return values (it might return null).
Should the method throw an exception if level is greater than the number of array dimensions?
/* | ||
* Stub file format described in the Checker Framework Manual: | ||
* https://checkerframework.org/manual/#stub-format . | ||
* All imports must be at the beginning of the file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete the next three lines; it's adequate to refer to the manual.
Should we relax the restriction that all imports must be at the beginning of the file?
I think you have already done this, in which case you can delete the restriction from the Checker Framework manual.
| | ||
( | ||
{ | ||
modifier = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary, given that 5 lines later modifier is also set?
Update with all changes in javaparser master
No description provided.