Add API for collecting problems without building AST to ICompilationUnitResolver

[datho7561]

Limitations:

• The method of collecting Javadoc errors requires converting the AST, so the current implementation for collecting errors using javac doesn't collect Javadoc errors

[mickaelistria]

I would really like to avoid creating/changing API is we can avoid it.

The method of collecting Javadoc errors requires converting the AST

That is not per se an issue. The conversion of AST is a cheap and easy operation and is neglectable compared to many other processing steps involved earlier to get Javac errors, it's not worth changing the API to avoid it.

the current implementation for collecting errors using javac doesn't collect Javadoc errors

Isn't it "just" a bug in the ProblemConverter?

[datho7561]

That is not per se an issue.

Yes it is. Converting the javac AST to the JDT AST is cheap but for some reason it's causing issues in jdt.ls : eclipse-jdtls/eclipse.jdt.ls#3322 (comment) . I don't understand exactly what the problem is. I think it has something to do with AST caching but I'm not sure. Perhaps @snjeza can explain?

Isn't it "just" a bug in the ProblemConverter?

I guess, but until it's solved, my current implementation won't handle Javadoc errors properly. We can split out collecting the Javadoc errors into a separate visit of the javac AST, and indeed I'll need to do that if we go forward with this PR.

[mickaelistria]

I still believe that we shouldn't try to change the API until we don't know what exact problem we're trying to fix.
The Javadoc problem report is a bug that wouldn't require new API.

[snjeza]

I don't understand exactly what the problem is. I think it has something to do with AST caching but I'm not sure. Perhaps @snjeza can explain?

When opening or activating a file, Java LS calls the following code:

int flags = ICompilationUnit.FORCE_PROBLEM_DETECTION | ICompilationUnit.ENABLE_BINDINGS_RECOVERY | ICompilationUnit.ENABLE_STATEMENTS_RECOVERY;
		synchronized(reconcileLock) {
			unit.reconcile(ICompilationUnit.NO_AST, flags, wcOwner, monitor);
		}

https://github.com/eclipse-jdtls/eclipse.jdt.ls/blob/ba8d0910917bab70b8565af15d7960c2e1ae8058/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/BaseDocumentLifeCycleHandler.java#L330-L333

This code calls unit.makeConsistent twice.

// make working copy consistent if needed and compute AST if needed
			makeConsistent(workingCopy);

			// notify reconcile participants only if working copy was not consistent or if forcing problem detection
			// (see https://bugs.eclipse.org/bugs/show_bug.cgi?id=177319)
			if (!wasConsistent || ((this.reconcileFlags & ICompilationUnit.FORCE_PROBLEM_DETECTION) != 0)) {
				notifyParticipants(workingCopy);

				// recreate ast if one participant reset it
				if (this.ast == null)
					makeConsistent(workingCopy);
			}

https://github.com/eclipse-jdt/eclipse.jdt.core/blob/74ec8f9c7953ec048a83b29e9ff7e5bb547375d6/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/ReconcileWorkingCopyOperation.java#L108-L118

ECJ doesn't create AST if astLevel == 0
https://github.com/eclipse-jdt/eclipse.jdt.core/blob/74ec8f9c7953ec048a83b29e9ff7e5bb547375d6/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/ReconcileWorkingCopyOperation.java#L247

javac creates AST twice
https://github.com/eclipse-jdt/eclipse.jdt.core/blob/74ec8f9c7953ec048a83b29e9ff7e5bb547375d6/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/ReconcileWorkingCopyOperation.java#L208

If we replace

if (this.astLevel != ICompilationUnit.NO_AST) {
							this.ast = newAST;
						}

https://github.com/eclipse-jdt/eclipse.jdt.core/blob/74ec8f9c7953ec048a83b29e9ff7e5bb547375d6/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/ReconcileWorkingCopyOperation.java#L221
with

this.ast = newAST;

See eclipse-jdtls/eclipse.jdt.ls#3322 (comment)

The AST will be saved and not be created when calling unit.makeconsistent again.

[mickaelistria]

ECJ doesn't create AST if astLevel == 0

But it still parses and resolves; whether an AST is created or not doesn't matter. What matters is whether the AST is stored or not.

javac creates AST twice

Why twice? Parsing and resolution are still run only once per call as far as I can see; just like with JDT.

If we replace...

then we always keep a copy of the AST and get memory retained while the request was really to not store the AST. That's not really what's desired.

[snjeza]

Why twice? Parsing and resolution are still run only once per call as far as I can see; just like with JDT.

unit.makeconsistent is called twice

// make working copy consistent if needed and compute AST if needed
			makeConsistent(workingCopy);

			// notify reconcile participants only if working copy was not consistent or if forcing problem detection
			// (see https://bugs.eclipse.org/bugs/show_bug.cgi?id=177319)
			if (!wasConsistent || ((this.reconcileFlags & ICompilationUnit.FORCE_PROBLEM_DETECTION) != 0)) {
				notifyParticipants(workingCopy);

				// recreate ast if one participant reset it
				if (this.ast == null)
					makeConsistent(workingCopy);
			}

https://github.com/eclipse-jdt/eclipse.jdt.core/blob/74ec8f9c7953ec048a83b29e9ff7e5bb547375d6/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/ReconcileWorkingCopyOperation.java#L108-L118

... just like with JDT.

JDT/ECJ calls unit.makeconsistent twice, too. However, it doesn't create AST if astlevel == 0

[snjeza]

Then we always keep a copy of the AST and get memory retained while the request was really to not store the AST. That's not really what's desired.

ok. I understand.

[datho7561]

whether an AST is created or not doesn't matter

I'm starting to think we should ignore NO_AST and always build and set the AST (to avoid the double parse)

However, looking at the code, it seems that there is some callback that is invoked when the AST is created. Maybe this is what the NO_AST is really trying to avoid?

if (this.ast != null) {
	if (this.deltaBuilder.delta == null) {
		this.deltaBuilder.delta = new JavaElementDelta(workingCopy);
	}
	this.deltaBuilder.delta.changedAST(this.ast);
}

[datho7561]

Okay wait I think I get it now, my bad:

ECJ doesn't avoid the double parse itself, right? So it should be okay if the DOM_BASED_OPERATIONS doesn't avoid double parsing.

[mickaelistria]

ECJ doesn't avoid the double parse itself, right?

My understanding is that it does not prevent either from re-parsing same content in case makeConsistent is called multiple time and NO_AST is set.

[datho7561]

Okay cool. I guess this isn't needed, we can keep the existing behaviour.