🐛 Improve accuracy of METHOD_CALL and CONSTRUCTOR_CALL matches

.github/workflows/global-ci.yaml

@@ -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
@@ -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 }}"

Dockerfile

@@ -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

java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java

@@ -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<>();
@@ -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()) {
@@ -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;
+    }
 }

java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java

@@ -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;
+    }
+}