Community
Participate
Working Groups
I was initially alerted because a DOM bug in the OT branch was not detected by the jdt.core.tests.model suite. I found that non of the tests of the ASTConverter* tests checked the created nodes for isMalformed(), except when expecting true. I added a corresponding check to ConverterTestSetup.checkSourceRange(..) to catch as many test nodes as possible. For 18 test cases I had to set the expectation explicitly to true (malformed), and I checked that these indeed test sources with syntax errors. All other tests still pass. However, from these 18 cases one test revealed that malformed AST was wrongly flagged (plus duplicates in other testclasses): ASTConverterTest.test0339() Here the example class has a syntax error and in this situation a regular interface method has a wrong source position: int doQuery(boolean x); It shows that in recovery mode the MethodDeclaration.bodyEnd is set on the ';' whereas it normally points to the ')'. As a result the ASTConverter signals this method as malformed, because it can't find the ';' at bodyEnd+1.
Created attachment 184424 [details] test changes and parser fix This patch contains the test changes and the fix in the parser.
Olivier, do you want to give it a quick look? This one should be easy and not take much of your time :)
Targeting M5 as there is a change in the parser.
I only see: org.eclipse.jdt.internal.compiler.ast.JavadocArgumentExpression.internalResolveType(Scope) that might require more work. I wrote a small test case that produced: java.lang.NullPointerException at org.eclipse.jdt.internal.compiler.ast.JavadocArgumentExpression.internalResolveType(JavadocArgumentExpression.java:53) at org.eclipse.jdt.internal.compiler.ast.JavadocArgumentExpression.resolveType(JavadocArgumentExpression.java:97) at org.eclipse.jdt.internal.compiler.ast.JavadocMessageSend.internalResolveType(JavadocMessageSend.java:64) at org.eclipse.jdt.internal.compiler.ast.JavadocMessageSend.resolveType(JavadocMessageSend.java:200) at org.eclipse.jdt.internal.compiler.ast.Javadoc.resolveReference(Javadoc.java:367) at org.eclipse.jdt.internal.compiler.ast.Javadoc.resolve(Javadoc.java:269) at org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration.resolveJavadoc(AbstractMethodDeclaration.java:440) at org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration.resolve(AbstractMethodDeclaration.java:420) at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.resolve(TypeDeclaration.java:1147) at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.resolve(TypeDeclaration.java:1257) at org.eclipse.jdt.internal.compiler.ast.CompilationUnitDeclaration.resolve(CompilationUnitDeclaration.java:540) at org.eclipse.jdt.internal.compiler.Compiler.resolve(Compiler.java:899) at org.eclipse.jdt.internal.compiler.Compiler.resolve(Compiler.java:944) at org.eclipse.jdt.internal.core.CompilationUnitProblemFinder.process(CompilationUnitProblemFinder.java:202) at org.eclipse.jdt.internal.core.CompilationUnitProblemFinder.process(CompilationUnitProblemFinder.java:268) at org.eclipse.jdt.internal.core.ReconcileWorkingCopyOperation.makeConsistent(ReconcileWorkingCopyOperation.java:190) at org.eclipse.jdt.internal.core.ReconcileWorkingCopyOperation.executeOperation(ReconcileWorkingCopyOperation.java:89) at org.eclipse.jdt.internal.core.JavaModelOperation.run(JavaModelOperation.java:728) at org.eclipse.jdt.internal.core.JavaModelOperation.runOperation(JavaModelOperation.java:788) at org.eclipse.jdt.internal.core.CompilationUnit.reconcile(CompilationUnit.java:1244) at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy.reconcile(JavaReconcilingStrategy.java:126) at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy.access$0(JavaReconcilingStrategy.java:108) at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy$1.run(JavaReconcilingStrategy.java:89) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42) at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy.reconcile(JavaReconcilingStrategy.java:87) at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy.initialReconcile(JavaReconcilingStrategy.java:178) at org.eclipse.jdt.internal.ui.text.CompositeReconcilingStrategy.initialReconcile(CompositeReconcilingStrategy.java:114) at org.eclipse.jdt.internal.ui.text.JavaCompositeReconcilingStrategy.initialReconcile(JavaCompositeReconcilingStrategy.java:133) at org.eclipse.jface.text.reconciler.MonoReconciler.initialProcess(MonoReconciler.java:105) at org.eclipse.jdt.internal.ui.text.JavaReconciler.initialProcess(JavaReconciler.java:398) at org.eclipse.jface.text.reconciler.AbstractReconciler$BackgroundThread.run(AbstractReconciler.java:173) So I'll target 6pm build and not 1pm.
Sorry wrong bug.
This is a problem with the ASTConverter that doesn't expect recovered nodes. In this case the method inside the interface is well formed, but it is built during the error recovery because of the syntax error at the end of the code. I'll investigate.
(In reply to comment #6) > This is a problem with the ASTConverter that doesn't expect recovered nodes. So you mean it's OK if recovery sets different source positions even for a valid element than normal parsing would do? For normal abstract methods (bodyEnd+1) points to ';', which is not true for recovered methods.
(In reply to comment #7) > So you mean it's OK if recovery sets different source positions even for a > valid element than normal parsing would do? > For normal abstract methods (bodyEnd+1) points to ';', which is not true for > recovered methods. Once you end up with recovery in headers, the method inside the interface is handled through error recovery. So this is why its bodyEnd and declaration source end values are the same. I don't want to play too much inside the recovery just to handle this case. I think we can do a pretty local fix inside the ASTConverter. Investigating.
Created attachment 184817 [details] Proposed fix + regression tests This patch makes changes only inside the ASTConverter by handling differently the case where bodyEnd == declarationSourceEnd. I am not sure we need to do more than that for now.
Stephan, I would release that patch for now. No need to spend lot of time on this issue. Comment ?
Released for 3.7M5.
Verified for 3.7M5 using code inspection.
