From acdb072d8b01a4cfdef1f306eb7a21c55dd6d3c7 Mon Sep 17 00:00:00 2001 From: Pranav Gaikwad Date: Wed, 10 Jul 2024 11:00:58 -0400 Subject: [PATCH] :bug: Improve accuracy of METHOD_CALL and CONSTRUCTOR_CALL matches (#100) * :bug: improve accuracy of MethodReference matches Signed-off-by: Pranav Gaikwad * improve accuracy of constructor call Signed-off-by: Pranav Gaikwad * :ghost: improve logic / structure a bit Signed-off-by: Pranav Gaikwad * improve accuracy of METHOD_CALL and CONSTRUCTOR_CALL Signed-off-by: Pranav Gaikwad * :bug: add a way to specify api tests ref Signed-off-by: Pranav Gaikwad * :bug: fix query that ends in a * Signed-off-by: Pranav Gaikwad * Adding query replace to make java regex in qualification match, bumping LS Signed-off-by: Shawn Hurley * update Signed-off-by: Pranav Gaikwad --------- Signed-off-by: Pranav Gaikwad Signed-off-by: Shawn Hurley Co-authored-by: Shawn Hurley --- .github/workflows/global-ci.yaml | 48 +++-- Dockerfile | 2 +- .../symbol/ConstructorCallSymbolProvider.java | 45 ++++- .../internal/symbol/CustomASTVisitor.java | 188 ++++++++++++++++++ .../symbol/MethodCallSymbolProvider.java | 36 +++- .../core/internal/symbol/SymbolProvider.java | 66 ++++++ .../java-analyzer-bundle.target | 4 +- 7 files changed, 361 insertions(+), 28 deletions(-) create mode 100644 java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java diff --git a/.github/workflows/global-ci.yaml b/.github/workflows/global-ci.yaml index 84f8882..76c680d 100644 --- a/.github/workflows/global-ci.yaml +++ b/.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 }}" diff --git a/Dockerfile b/Dockerfile index e6e5169..813e0eb 100644 --- a/Dockerfile +++ b/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 diff --git a/java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java b/java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java index 2b016b1..6fa9fee 100644 --- a/java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/ConstructorCallSymbolProvider.java +++ b/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 get(SearchMatch match) throws CoreException { List symbols = new ArrayList<>(); @@ -22,6 +33,7 @@ public List 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 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; + } } diff --git a/java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java b/java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java new file mode 100644 index 0000000..33568b4 --- /dev/null +++ b/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("(? 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)); - symbols.add(symbol); + symbol.setLocation(location); + if (this.query.contains(".")) { + ICompilationUnit unit = e.getCompilationUnit(); + 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); + } } catch (Exception e) { logInfo("unable to convert for variable: " + e); } diff --git a/java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProvider.java b/java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProvider.java index 382574b..f69ad34 100644 --- a/java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProvider.java +++ b/java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/SymbolProvider.java @@ -12,9 +12,11 @@ import org.eclipse.jdt.core.IClassFile; import org.eclipse.jdt.core.ICompilationUnit; import org.eclipse.jdt.core.IField; +import org.eclipse.jdt.core.IImportDeclaration; import org.eclipse.jdt.core.IJavaElement; import org.eclipse.jdt.core.IMethod; import org.eclipse.jdt.core.IOpenable; +import org.eclipse.jdt.core.IPackageDeclaration; import org.eclipse.jdt.core.IType; import org.eclipse.jdt.core.JavaModelException; import org.eclipse.jdt.core.SourceRange; @@ -167,4 +169,68 @@ private static void setPosition(Position position, int[] coords) { position.setLine(coords[0]); position.setCharacter(coords[1]); } + + /* + * Given a query, class and location of a Match, tells whether CompilationUnit of the match + * matches the qualification part of the query. For example, if the query is `konveyor.io.Util.get*`, + * qualification means `konveyor.io.Util`. This is so that we can improve accuracy of a match of + * queries that are looking for FQNs. For query `konveyor.io.Util.get*`, returns true if either one is true: + * 1. match is found in the package `konveyor.io` or class `konveyor.io.Util` + * 2. the compilation unit imports package `konveyor.io.Util` or `konveyor.io.*` + * 3. the compilation unit has a package declaration as `konveyor.io.Util` + * we do this so that we can rule out a lot of matches before going the AST route + */ + default boolean queryQualificationMatches(String query, ICompilationUnit unit, Location location) { + query = query.replaceAll("(? 0) { + // for a query, java.io.paths.File*, queryQualification is java.io.paths + queryQualification = query.substring(0, dotIndex); + } + // check if the match was found in the same package as the query was looking for + if (queryQualification != "" && location.getUri().contains(queryQualification.replaceAll(".", "/"))) { + return true; + } + if (unit != null) { + try { + // check if the package declaration on the unit matches query + for (IPackageDeclaration packageDecl : unit.getPackageDeclarations()) { + if (queryQualification != "" && packageDecl.getElementName().matches(queryQualification)) { + return true; + } + } + for (IImportDeclaration importDecl : unit.getImports()) { + String importElement = importDecl.getElementName(); + String importQualification = ""; + int importDotIndex = query.lastIndexOf('.'); + if (importDotIndex > 0) { + importQualification = query.substring(0, dotIndex); + } + // import can be absolute like java.io.paths.FileReader + if (query.matches(importElement)) { + return true; + } + if (importElement.matches(query)) { + return true; + } + // an import can be java.io.paths.* or java.io.* + if (importElement.contains("*")) { + if (queryQualification != "") { + // query is java.io.paths.File*, import is java.io.paths.* + if (queryQualification.startsWith(importQualification)) { + return true; + } + if (importElement.replaceAll(".*", "").matches(queryQualification)) { + return true; + } + } + } + } + } catch (Exception e) { + logInfo("unable to determine accuracy of the match"); + } + } + return false; + } } diff --git a/java-analyzer-bundle.tp/java-analyzer-bundle.target b/java-analyzer-bundle.tp/java-analyzer-bundle.target index 26d76b4..a5f9114 100644 --- a/java-analyzer-bundle.tp/java-analyzer-bundle.target +++ b/java-analyzer-bundle.tp/java-analyzer-bundle.target @@ -7,7 +7,7 @@ - + @@ -61,7 +61,7 @@ - +