Skip to content

Commit

Permalink
🐛 Improve accuracy of METHOD_CALL and CONSTRUCTOR_CALL matches (#100)
Browse files Browse the repository at this point in the history
* 🐛 improve accuracy of MethodReference matches

Signed-off-by: Pranav Gaikwad <[email protected]>

* improve accuracy of constructor call

Signed-off-by: Pranav Gaikwad <[email protected]>

* 👻 improve logic / structure a bit

Signed-off-by: Pranav Gaikwad <[email protected]>

* improve accuracy of METHOD_CALL and CONSTRUCTOR_CALL

Signed-off-by: Pranav Gaikwad <[email protected]>

* 🐛 add a way to specify api tests ref

Signed-off-by: Pranav Gaikwad <[email protected]>

* 🐛 fix query that ends in a *

Signed-off-by: Pranav Gaikwad <[email protected]>

* Adding query replace to make java regex in qualification match, bumping LS

Signed-off-by: Shawn Hurley <[email protected]>

* update

Signed-off-by: Pranav Gaikwad <[email protected]>

---------

Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Co-authored-by: Shawn Hurley <[email protected]>
  • Loading branch information
pranavgaikwad and shawn-hurley authored Jul 10, 2024
1 parent d4e0ab0 commit acdb072
Show file tree
Hide file tree
Showing 7 changed files with 361 additions and 28 deletions.
48 changes: 30 additions & 18 deletions .github/workflows/global-ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,26 +6,38 @@ jobs:
build-addon:
name: Build tackle2-addon-analyzer
runs-on: ubuntu-20.04
outputs:
api_tests_ref: ${{ steps.extract_info.outputs.API_TESTS_REF }}
strategy:
fail-fast: false
steps:
- name: Extract pull request number from PR description
id: extract_analyzer_pull_request_number
id: extract_info
run: |
PULL_REQUEST_NUMBER=$(echo "${{ github.event.pull_request.body }}" | grep -oP '[A|a]nalyzer.?[P|p][R|r]: \K\d+' || true)
if [ -z "$PULL_REQUEST_NUMBER" ]; then
echo "::set-output name=ref::main"
ANALYZER_PR=$(echo "${{ github.event.pull_request.body }}" | grep -oP '[A|a]nalyzer.?[P|p][R|r]: \K\d+' || true)
if [ -z "$ANALYZER_PR" ]; then
echo "ANALYZER_REF=${GITHUB_BASE_REF}" >> $GITHUB_OUTPUT
else
echo "::set-output name=ref::refs/pull/$PULL_REQUEST_NUMBER/merge"
echo "ANALYZER_REF=refs/pull/$ANALYZER_PR/merge" >> $GITHUB_OUTPUT
fi
API_TESTS_PR=$(echo "${{ github.event.pull_request.body }}" | grep -oP '[A|a]pi *[T|t]ests *[P|p][R|r]: \K\d+' || true)
if [ -z "$API_TESTS_PR" ]; then
echo "API_TESTS_REF=${GITHUB_BASE_REF}" >> $GITHUB_OUTPUT
else
echo "API_TESTS_REF=refs/pull/$API_TESTS_PR/merge" >> $GITHUB_OUTPUT
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 }}"
ref: "${{ steps.extract_info.outputs.ANALYZER_REF }}"

- uses: actions/checkout@v3
with:
fetch-depth: 0
Expand All @@ -35,24 +47,24 @@ 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
api_tests_ref: "${{ needs.build-addon.outputs.api_tests_ref }}"
2 changes: 1 addition & 1 deletion Dockerfile
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
FROM registry.access.redhat.com/ubi9/ubi AS jdtls-download
WORKDIR /jdtls
RUN curl -s -o jdtls.tar.gz https://download.eclipse.org/jdtls/milestones/1.16.0/jdt-language-server-1.16.0-202209291445.tar.gz &&\
RUN curl -s -o jdtls.tar.gz https://download.eclipse.org/jdtls/milestones/1.36.0/jdt-language-server-1.36.0-202405301306.tar.gz &&\
tar -xvf jdtls.tar.gz --no-same-owner &&\
chmod 755 /jdtls/bin/jdtls &&\
rm -rf jdtls.tar.gz
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,25 @@
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.CompilationUnit;
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;

public class ConstructorCallSymbolProvider implements SymbolProvider {
import io.konveyor.tackle.core.internal.symbol.CustomASTVisitor.QueryLocation;

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

@Override
public List<SymbolInformation> get(SearchMatch match) throws CoreException {
List<SymbolInformation> symbols = new ArrayList<>();
Expand All @@ -22,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 @@ -31,12 +43,39 @@ public List<SymbolInformation> get(SearchMatch match) throws CoreException {
return null;
}
symbol.setContainerName(mod.getParent().getElementName());
symbol.setLocation(getLocation(mod, match));
symbols.add(symbol);
symbol.setLocation(location);
if (this.query.contains(".")) {
ICompilationUnit unit = mod.getCompilationUnit();
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);
}
} catch (Exception e) {
logInfo("unable to get constructor: " + e);
return null;
}
return symbols;
}

@Override
public void setQuery(String query) {
// TODO Auto-generated method stub
this.query = query;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,188 @@
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;
import org.eclipse.jdt.core.dom.MethodInvocation;
import org.eclipse.jdt.core.search.SearchMatch;

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, 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
* by .* so that java regex works as expected on them
*/
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 / 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
* 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 {
IMethodBinding binding = node.resolveMethodBinding();
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("method fqn " + fullyQualifiedName + " did not match with " + query);
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 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;
}
}
Loading

0 comments on commit acdb072

Please sign in to comment.