Skip to content
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.

Sign up for GitHub

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

Some updates for DOM-first code selection #3640

Merged
merged 1 commit into from
Feb 3, 2025
Merged

Some updates for DOM-first code selection #3640

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 20 additions & 7 deletions org.eclipse.jdt.core/codeassist/org/eclipse/jdt/internal/codeassist/DOMCodeSelector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public IJavaElement[] codeSelect(int offset, int length) throws JavaModelExcepti
if (offset + length > this.unit.getSource().length()) {
throw new JavaModelException(new IndexOutOfBoundsException(offset + length), IJavaModelStatusConstants.INDEX_OUT_OF_BOUNDS);
}
org.eclipse.jdt.core.dom.CompilationUnit currentAST = this.unit.getOrBuildAST(this.owner);
org.eclipse.jdt.core.dom.CompilationUnit currentAST = this.unit.getOrBuildAST(this.owner, this.unit.getBuffer().getLength() < 50000 ? -1 : offset);
Copy link
Contributor

@rgrunber rgrunber Jan 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the reason for not applying focal position always (ie. not just when the buffer is >= 50000) ? Is there something that's lost out on large files? Intuitively I'm just thinking that if we need to return a list of IJavaElements that are partly covered by a selection, then do we even need statement nodes completely outside the scope to make that decision ? Maybe if the selection goes across different scopes ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current rationale (that we can discuss as we have a lot of choices here) is to enable some caching. If we always care about focalPosition, then all the doms we get are partial and resolve faster (that is per se not too bad), but in case of codeSelect, we may prefer to build the whole DOM once and reuse it as long as the file is not modified. The size of the buffer was used as a criterion to decide whether to enable caching and ignore focalPosition, or whether to prefer a partial but non-reusable DOM.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Ok, it just hit me. This also caches the AST, and returning a cached AST is more efficient that computing a partial (setFocalPosition) one, but if we always compute a partial, we never get to cache a proper one. So may as well limit ourselves to cases where we know the full parse is particularly bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So may as well limit ourselves to cases where we know the full parse is particularly bad.

Yes, that's the idea, but I'm not sure what is the best resolution of "particularly bad" concretely. I went for document size because it's easy and cheap to get, but I guess some better conditions could be identified.

Copy link
Contributor

@rgrunber rgrunber Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe @datho7561 has some ideas from his look into parsing about some "easy" checks other than document size that can indicate parsing will be expensive.

The only other thing I've looked at is the usage of setFocalPosition(-1) in regular cases as a way of saying, "don't set the focal position and do a full AST parse". It still sets the CompilationUnitResolver.PARTIAL bit, and useSearcher to true, but these are only meaningfully used for determining whether we set IGNORE_METHOD_BODIES. In other words we could never set IGNORE_METHOD_BODIES on a full AST parse. Almost identical to what setFocalPosition would do to begin with except even the elements around the focal position would be ignored if in a method body. Are you fine with this ? Assuming I understood the logic, it is a subtle difference to be aware of. Update: I mean at the very least it's unexpected behaviour going forward.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

particularly bad

lambdas and switch expressions (these require some fancy and costly hacks in the parser to work), and deeply nested expressions. Lambdas and switch expressions could be identified by searching for -> in the file text. However, that check itself could take a while for a large file. I don't know how much time this would save in practice though.

if (currentAST == null) {
return new IJavaElement[0];
}
Expand Down Expand Up @@ -197,6 +197,10 @@ public IJavaElement[] codeSelect(int offset, int length) throws JavaModelExcepti
return reorderedOverloadedMethods;
}
return new IJavaElement[] { importBinding.getJavaElement() };
} else if (node instanceof MethodDeclaration decl && offset > decl.getName().getStartPosition()) {
// most likely inside and empty `()`
// case for TypeHierarchyCommandTest.testTypeHierarchy()
return null;
} else if (findTypeDeclaration(node) == null) {
IBinding binding = resolveBinding(node);
if (binding != null && !binding.isRecovered()) {
Expand Down Expand Up @@ -372,7 +376,7 @@ public IJavaElement[] codeSelect(int offset, int length) throws JavaModelExcepti
return new IJavaElement[0];
}

static IBinding resolveBinding(ASTNode node) {
public static IBinding resolveBinding(ASTNode node) {
if (node instanceof MethodDeclaration decl) {
return decl.resolveBinding();
}
Expand Down Expand Up @@ -450,7 +454,7 @@ static IBinding resolveBinding(ASTNode node) {
}
IMethod methodModel = ((IMethod)methodBinding.getJavaElement());
boolean allowExtraParam = true;
if ((methodModel.getFlags() & Flags.AccStatic) != 0) {
if (methodModel != null && (methodModel.getFlags() & Flags.AccStatic) != 0) {
allowExtraParam = false;
if (methodRef.getExpression() instanceof ClassInstanceCreation) {
return null;
Expand All @@ -463,15 +467,21 @@ static IBinding resolveBinding(ASTNode node) {
while (type == null && cursor != null) {
if (cursor.getParent() instanceof VariableDeclarationFragment declFragment) {
type = declFragment.resolveBinding().getType();
}
else if (cursor.getParent() instanceof MethodInvocation methodInvocation) {
} else if (cursor.getParent() instanceof MethodInvocation methodInvocation) {
IMethodBinding methodInvocationBinding = methodInvocation.resolveMethodBinding();
int index = methodInvocation.arguments().indexOf(cursor);
type = methodInvocationBinding.getParameterTypes()[index];
if (methodInvocationBinding != null) {
int index = methodInvocation.arguments().indexOf(cursor);
type = methodInvocationBinding.getParameterTypes()[index];
} else {
cursor = null;
}
} else {
cursor = cursor.getParent();
}
}
if (type == null) {
return null;
}

IMethodBinding boundMethod = type.getDeclaredMethods()[0];

Expand Down Expand Up @@ -562,6 +572,9 @@ private static boolean matchSignatures(IMethodBinding invocation, IMethodBinding
}

private IJavaElement[] findTypeInIndex(String packageName, String simpleName) throws JavaModelException {
if (simpleName == null) {
return new IJavaElement[0];
}
List<IType> indexMatch = new ArrayList<>();
TypeNameMatchRequestor requestor = new TypeNameMatchRequestor() {
@Override
Expand Down
13 changes: 12 additions & 1 deletion org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/CompilationUnit.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,10 @@ protected boolean buildStructure(OpenableElementInfo info, final IProgressMonito
ASTNode dom = null;
try {
dom = astParser.createAST(pm);
if (computeProblems) {
// force resolution of bindings to load more problems
dom.getAST().resolveWellKnownType(Object.class.getName());
}
} catch (AbortCompilationUnit e) {
var problem = e.problem;
if (problem == null && e.exception instanceof IOException ioEx) {
Expand Down Expand Up @@ -472,20 +476,27 @@ public IJavaElement[] codeSelect(int offset, int length, WorkingCopyOwner workin
}
}

public org.eclipse.jdt.core.dom.CompilationUnit getOrBuildAST(WorkingCopyOwner workingCopyOwner) throws JavaModelException {
public org.eclipse.jdt.core.dom.CompilationUnit getOrBuildAST(WorkingCopyOwner workingCopyOwner, int focalPosition) throws JavaModelException {
if (this.ast != null) {
return this.ast;
}
Map<String, String> options = getOptions(true);
ASTParser parser = ASTParser.newParser(new AST(options).apiLevel()); // go through AST constructor to convert options to apiLevel
// but we should probably instead just use the latest Java version
// supported by the compiler
parser.setWorkingCopyOwner(workingCopyOwner);
parser.setSource(this);
// greedily enable everything assuming the AST will be used extensively for edition
parser.setResolveBindings(true);
parser.setStatementsRecovery(true);
parser.setBindingsRecovery(true);
parser.setCompilerOptions(options);
parser.setFocalPosition(focalPosition);
if (parser.createAST(null) instanceof org.eclipse.jdt.core.dom.CompilationUnit newAST) {
if (focalPosition >= 0) {
// do not store
return newAST;
}
this.ast = newAST;
}
return this.ast;
Expand Down
Loading