Skip to content

Commit

Permalink
improve accuracy of METHOD_CALL and CONSTRUCTOR_CALL
Browse files Browse the repository at this point in the history 
Signed-off-by: Pranav Gaikwad <[email protected]>
  • Loading branch information
@pranavgaikwad
pranavgaikwad committed Jul 8, 2024
1 parent 87175e6 commit 0339cf2
Show file tree
Hide file tree
Showing 5 changed files with 243 additions and 56 deletions.
26 changes: 14 additions & 12 deletions .github/workflows/global-ci.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,17 @@ jobs:
else
echo "::set-output name=ref::refs/pull/$PULL_REQUEST_NUMBER/merge"
fi
- name: checkout
uses: actions/checkout@v3

- name: Checkout tools repo
uses: actions/checkout@v3
with:
repository: konveyor/analyzer-lsp
path: analyzer-lsp
ref: "${{ steps.extract_analyzer_pull_request_number.outputs.ref }}"

- uses: actions/checkout@v3
with:
fetch-depth: 0
Expand All @@ -35,24 +38,23 @@ jobs:

- name: Build bundle base image
run: |
docker build -t quay.io/konveyor/jdtls-server-base:latest .
- name: build analyzer-lsp Dockerfile
run: |
docker build -f analyzer-lsp/Dockerfile -t quay.io/konveyor/analyzer-lsp:latest analyzer-lsp
docker tag quay.io/konveyor/analyzer-lsp:latest analyzer-lsp
- name: Build addon and save image
working-directory: tackle2-addon-analyzer
podman build -t quay.io/konveyor/jdtls-server-base:latest .
- name: build java provider and save image
working-directory: analyzer-lsp
run: |
IMG=quay.io/konveyor/tackle2-addon-analyzer:latest make image-podman
podman save -o /tmp/tackle2-addon-analyzer.tar quay.io/konveyor/tackle2-addon-analyzer:latest
make build-java-provider
podman tag java-provider quay.io/konveyor/java-external-provider:latest
podman save -o /tmp/java-provider.tar quay.io/konveyor/java-external-provider:latest
- name: Upload image as artifact
uses: actions/upload-artifact@v3
with:
name: tackle2-addon-analyzer
path: /tmp/tackle2-addon-analyzer.tar
name: java-provider
path: /tmp/java-provider.tar
retention-days: 1
e2e:
needs: build-addon
uses: konveyor/ci/.github/workflows/global-ci.yml@main
with:
component_name: tackle2-addon-analyzer
component_name: java-provider
36 changes: 23 additions & 13 deletions .../src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,22 @@
import java.util.List;

import org.eclipse.core.runtime.CoreException;
import org.eclipse.jdt.core.IClassFile;
import org.eclipse.jdt.core.ICompilationUnit;
import org.eclipse.jdt.core.IJavaElement;
import org.eclipse.jdt.core.IMethod;
import org.eclipse.jdt.core.dom.AST;
import org.eclipse.jdt.core.dom.ASTParser;
import org.eclipse.jdt.core.dom.ASTVisitor;
import org.eclipse.jdt.core.dom.CompilationUnit;
import org.eclipse.jdt.core.dom.IMethodBinding;
import org.eclipse.jdt.core.dom.ITypeBinding;
import org.eclipse.jdt.core.dom.MethodInvocation;
import org.eclipse.jdt.core.search.MethodReferenceMatch;
import org.eclipse.jdt.core.search.SearchMatch;
import org.eclipse.jdt.internal.core.JavaElement;
import org.eclipse.lsp4j.Location;
import org.eclipse.lsp4j.SymbolInformation;
import org.eclipse.lsp4j.SymbolKind;

import io.konveyor.tackle.core.internal.symbol.CustomASTVisitor.QueryLocation;

public class ConstructorCallSymbolProvider implements SymbolProvider, WithQuery {
public String query;

Expand All @@ -32,6 +33,7 @@ public List<SymbolInformation> get(SearchMatch match) throws CoreException {
MethodReferenceMatch m = (MethodReferenceMatch) match;
var mod = (IMethod) m.getElement();
SymbolInformation symbol = new SymbolInformation();
Location location = getLocation(mod, match);
symbol.setName(mod.getElementName());
// If the search match is for a constructor, the enclosing element may not be a constructor.
if (m.isConstructor()) {
Expand All @@ -41,17 +43,25 @@ public List<SymbolInformation> get(SearchMatch match) throws CoreException {
return null;
}
symbol.setContainerName(mod.getParent().getElementName());
symbol.setLocation(getLocation(mod, match));
symbol.setLocation(location);
if (this.query.contains(".")) {
ICompilationUnit unit = mod.getCompilationUnit();
ASTParser astParser = ASTParser.newParser(AST.getJLSLatest());
astParser.setSource(unit);
astParser.setResolveBindings(true);
CompilationUnit cu = (CompilationUnit) astParser.createAST(null);
CustomASTVisitor visitor = new CustomASTVisitor(query, match);
cu.accept(visitor);
if (visitor.symbolMatches()) {
symbols.add(symbol);
if (unit == null) {
IClassFile cls = (IClassFile) ((IJavaElement) mod).getAncestor(IJavaElement.CLASS_FILE);
if (cls != null) {
unit = cls.becomeWorkingCopy(null, null, null);
}
}
if (this.queryQualificationMatches(this.query, unit, location)) {
ASTParser astParser = ASTParser.newParser(AST.getJLSLatest());
astParser.setSource(unit);
astParser.setResolveBindings(true);
CompilationUnit cu = (CompilationUnit) astParser.createAST(null);
CustomASTVisitor visitor = new CustomASTVisitor(query, match, QueryLocation.CONSTRUCTOR_CALL);
cu.accept(visitor);
if (visitor.symbolMatches()) {
symbols.add(symbol);
}
}
} else {
symbols.add(symbol);
Expand Down
134 changes: 118 additions & 16 deletions ...r-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package io.konveyor.tackle.core.internal.symbol;

import org.eclipse.jdt.core.dom.ASTVisitor;
import org.eclipse.jdt.core.dom.ClassInstanceCreation;
import org.eclipse.jdt.core.dom.ConstructorInvocation;
import org.eclipse.jdt.core.dom.ASTNode;
import org.eclipse.jdt.core.dom.IMethodBinding;
import org.eclipse.jdt.core.dom.ITypeBinding;
Expand All @@ -9,13 +11,30 @@

import static org.eclipse.jdt.ls.core.internal.JavaLanguageServerPlugin.logInfo;

/*
* SearchEngine we use often gives us more matches than needed when
* query contains a * and/or contains a fqn. e.g. java.io.paths.get*
* This class exists to help us get accurate results for such queries
* For different type of symbols we get from a match, we try to
* get fqn of that symbol and ensure it matches the given query
* (pgaikwad): if you can, please make the visit() functions DRYer
*/
public class CustomASTVisitor extends ASTVisitor {
private String query;
private SearchMatch match;
private boolean symbolMatches;
private QueryLocation location;

/*
* we re-use this same class for different locations in a query
* we should only be visiting nodes specific to a location, not all
*/
public enum QueryLocation {
METHOD_CALL,
CONSTRUCTOR_CALL,
}

public CustomASTVisitor(String query, SearchMatch match) {
public CustomASTVisitor(String query, SearchMatch match, QueryLocation location) {
/*
* When comparing query pattern with an actual java element's fqn
* we need to make sure that * not preceded with a . are replaced
Expand All @@ -24,34 +43,36 @@ public CustomASTVisitor(String query, SearchMatch match) {
this.query = query.replaceAll("(?<!\\.)\\*", ".*");
this.symbolMatches = false;
this.match = match;
// depending on which location the query was for we only want to
// visit certain nodes
this.location = location;
}

/*
* When visiting AST nodes, it may happen that we visit more nodes
* than needed. We need to ensure that we are only visiting ones
* that are found in the given search match. I wrote this for methods
* where I observed that a node starts at the beginning of line whereas match
* starts at an offset within that line. However, both end on the same position.
* This could differ for other locations. In that case, change logic based
* on type of the node you get.
* When visiting AST nodes, it may happen that we visit more nodes than
* needed. We need to ensure that we are only visiting ones that are found
* in the given search match. I wrote this for methods / constructors where
* I observed that node starts at the beginning of line whereas match starts
* at an offset within that line. However, both end on the same position. This
* could differ for other locations. In that case, change logic based on type of
* the node you get.
*/
private boolean shouldVisit(ASTNode node) {
return (this.match.getOffset() + this.match.getLength()) ==
(node.getStartPosition() + node.getLength());
}

/*
* This is to get information from a MethodInvocation
* used for METHOD_CALL and CONSTRUCTOR_CALL matches
* we only discard a match only when we can tell for sure
* This is to get information from a MethodInvocation, used for METHOD_CALL
* we discard a match only when we can tell for sure. otherwise we accept
* returning false stops further visits
*/
@Override
public boolean visit(MethodInvocation node) {
if (this.location != QueryLocation.METHOD_CALL || !this.shouldVisit(node)) {
return true;
}
try {
if (!this.shouldVisit(node)) {
return true;
}
IMethodBinding binding = node.resolveMethodBinding();
if (binding != null) {
// get fqn of the method being called
Expand All @@ -67,18 +88,99 @@ public boolean visit(MethodInvocation node) {
return true;
}
}
}
}
logInfo("failed to get accurate info for MethodInvocation, falling back");
// sometimes binding or declaring class cannot be found, usually due to errors
// in source code. in that case, we will fallback and accept the match
this.symbolMatches = true;
return false;
} catch (Exception e) {
logInfo("error visiting MethodInvocation node: " + e);
// this is so that we fallback to old approach and we dont lose a result
// this is so that we fallback and don't lose a match when we fail
this.symbolMatches = true;
return false;
}
}

/*
* This is to get information from a ConstructorInvocation, used for CONSTRUCTOR_CALL
* we discard a match only when we can tell for sure. otherwise we accept
* returning false stops further visits
*/
@Override
public boolean visit(ConstructorInvocation node) {
if (this.location != QueryLocation.CONSTRUCTOR_CALL || !this.shouldVisit(node)) {
return true;
}
try {
IMethodBinding binding = node.resolveConstructorBinding();
if (binding != null) {
// get fqn of the method being called
ITypeBinding declaringClass = binding.getDeclaringClass();
if (declaringClass != null) {
String fullyQualifiedName = declaringClass.getQualifiedName() + "." + binding.getName();
// match fqn with query pattern
if (fullyQualifiedName.matches(this.query)) {
this.symbolMatches = true;
return false;
} else {
logInfo("constructor fqn " + fullyQualifiedName + " did not match with " + query);
return true;
}
}
}
logInfo("failed to get accurate info for ConstructorInvocation, falling back");
// sometimes binding or declaring class cannot be found, usually due to errors
// in source code. in that case, we will fallback and accept the match
this.symbolMatches = true;
return false;
} catch (Exception e) {
logInfo("error visiting ConstructorInvocation node: " + e);
// this is so that we fallback and don't lose a match when we fail
this.symbolMatches = true;
return false;
}
}

/*
* This is to get information from a ClassInstanceCreation, used for CONSTRUCTOR_CALL
* we discard a match only when we can tell for sure. otherwise we accept
* returning false stops further visits
*/
@Override
public boolean visit(ClassInstanceCreation node) {
if (this.location != QueryLocation.CONSTRUCTOR_CALL || !this.shouldVisit(node)) {
return true;
}
try {
IMethodBinding binding = node.resolveConstructorBinding();
if (binding != null) {
// get fqn of the method being called
ITypeBinding declaringClass = binding.getDeclaringClass();
if (declaringClass != null) {
String fullyQualifiedName = declaringClass.getQualifiedName() + "." + binding.getName();
// match fqn with query pattern
if (fullyQualifiedName.matches(this.query)) {
this.symbolMatches = true;
return false;
} else {
logInfo("constructor fqn " + fullyQualifiedName + " did not match with " + query);
return true;
}
}
}
logInfo("failed to get accurate info for ClassInstanceCreation, falling back");
// sometimes binding or declaring class cannot be found, usually due to errors
// in source code. in that case, we will fallback and accept the match
this.symbolMatches = true;
return false;
} catch (Exception e) {
logInfo("error visiting ConstructorInvocation node: " + e);
// this is so that we fallback and don't lose a match when we fail
this.symbolMatches = true;
return false;
}
}

public boolean symbolMatches() {
return this.symbolMatches;
Expand Down
37 changes: 22 additions & 15 deletions ....core/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodCallSymbolProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,21 @@
import java.util.ArrayList;
import java.util.List;

import org.eclipse.jdt.core.IClassFile;
import org.eclipse.jdt.core.ICompilationUnit;
import org.eclipse.jdt.core.IJavaElement;
import org.eclipse.jdt.core.IMethod;
import org.eclipse.jdt.core.IType;
import org.eclipse.jdt.core.ITypeRoot;
import org.eclipse.jdt.core.dom.AST;
import org.eclipse.jdt.core.dom.ASTParser;
import org.eclipse.jdt.core.dom.ASTVisitor;
import org.eclipse.jdt.core.dom.CompilationUnit;
import org.eclipse.jdt.core.dom.IMethodBinding;
import org.eclipse.jdt.core.dom.ITypeBinding;
import org.eclipse.jdt.core.dom.MethodInvocation;
import org.eclipse.jdt.core.search.MethodReferenceMatch;
import org.eclipse.jdt.core.search.SearchMatch;
import org.eclipse.lsp4j.Location;
import org.eclipse.lsp4j.SymbolInformation;
import org.eclipse.lsp4j.SymbolKind;

import io.konveyor.tackle.core.internal.symbol.CustomASTVisitor.QueryLocation;

public class MethodCallSymbolProvider implements SymbolProvider, WithQuery {
private String query;

Expand All @@ -34,20 +32,29 @@ public List<SymbolInformation> get(SearchMatch match) {
MethodReferenceMatch m = (MethodReferenceMatch) match;
IMethod e = (IMethod) m.getElement();
SymbolInformation symbol = new SymbolInformation();
Location location = getLocation((IJavaElement) match.getElement(), match);
symbol.setName(e.getElementName());
symbol.setKind(convertSymbolKind(e));
symbol.setContainerName(e.getParent().getElementName());
symbol.setLocation(getLocation(e, match));
symbol.setLocation(location);
if (this.query.contains(".")) {
ICompilationUnit unit = e.getCompilationUnit();
ASTParser astParser = ASTParser.newParser(AST.getJLSLatest());
astParser.setSource(unit);
astParser.setResolveBindings(true);
CompilationUnit cu = (CompilationUnit) astParser.createAST(null);
CustomASTVisitor visitor = new CustomASTVisitor(query, match);
cu.accept(visitor);
if (visitor.symbolMatches()) {
symbols.add(symbol);
if (unit == null) {
IClassFile cls = (IClassFile) ((IJavaElement) e).getAncestor(IJavaElement.CLASS_FILE);
if (cls != null) {
unit = cls.becomeWorkingCopy(null, null, null);
}
}
if (this.queryQualificationMatches(this.query, unit, location)) {
ASTParser astParser = ASTParser.newParser(AST.getJLSLatest());
astParser.setSource(unit);
astParser.setResolveBindings(true);
CompilationUnit cu = (CompilationUnit) astParser.createAST(null);
CustomASTVisitor visitor = new CustomASTVisitor(query, match, QueryLocation.METHOD_CALL);
cu.accept(visitor);
if (visitor.symbolMatches()) {
symbols.add(symbol);
}
}
} else {
symbols.add(symbol);
Expand Down
Loading

0 comments on commit 0339cf2

Please sign in to comment.