🐛 Improve method matching to catch static methods declared on class fields #144
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
WalkthroughExpands search scope to include system libraries when not in source-only mode and generalizes symbol resolution to handle both methods and fields when determining the compilation unit for AST analysis. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant CommandHandler as SampleDelegateCommandHandler
participant JDT as JDT Search
rect rgb(240,248,255)
note over CommandHandler: Build search scope
Client->>CommandHandler: search(query, sourceOnly=false)
alt sourceOnly
CommandHandler->>JDT: Scope = SOURCES
else not sourceOnly
CommandHandler->>JDT: Scope = SOURCES | REFERENCED_PROJECTS | APPLICATION_LIBRARIES | SYSTEM_LIBRARIES
note right of CommandHandler: SYSTEM_LIBRARIES newly included
end
JDT-->>CommandHandler: Search results
CommandHandler-->>Client: Results
end
sequenceDiagram
autonumber
participant Client
participant Provider as MethodCallSymbolProvider
participant JDT as JDT Model
participant AST as AST Parser
Client->>Provider: provideSymbols(query)
Provider->>JDT: resolve element for query
JDT-->>Provider: IJavaElement (IMethod or IField or ClassFile)
alt element is IMethod
Provider->>Provider: cu = method.getCompilationUnit()
else element is IField
Provider->>Provider: cu = field.getCompilationUnit()
else no CU
Provider->>JDT: classFile = element.getAncestor(ClassFile)
Provider->>JDT: cu = classFile.getWorkingCopy()
end
Provider->>AST: parse(cu)
AST-->>Provider: AST root
Provider-->>Client: filtered symbols
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (1)
206-214: Scope now includes SYSTEM_LIBRARIES — verify perf/recall trade-offs and guard NPE on analysisMode.Including system libraries can drastically increase search time and noise. Gate it explicitly and make the equals check NPE-safe.
- // For Partial results, we are going to filter out based on a list in the engine - int s = IJavaSearchScope.SOURCES | IJavaSearchScope.REFERENCED_PROJECTS | IJavaSearchScope.APPLICATION_LIBRARIES | IJavaSearchScope.SYSTEM_LIBRARIES; - if (analysisMode.equals(sourceOnlyAnalysisMode)) { + // For partial results, filter based on a list in the engine + int s = IJavaSearchScope.SOURCES | IJavaSearchScope.REFERENCED_PROJECTS | IJavaSearchScope.APPLICATION_LIBRARIES; + if (sourceOnlyAnalysisMode.equals(analysisMode)) { logInfo("KONVEYOR_LOG: source-only analysis mode only scoping to Sources"); s = IJavaSearchScope.SOURCES; } else { logInfo("KONVEYOR_LOG: waiting for source downloads"); waitForJavaSourceDownloads(); logInfo("KONVEYOR_LOG: waited for source downloads"); + // Only include system libraries when not in source-only mode + s |= IJavaSearchScope.SYSTEM_LIBRARIES; }Action items:
- Benchmark large workspaces (cold vs warm) to ensure acceptable latency when SYSTEM_LIBRARIES is included.
- If needed, expose a toggle for system libraries or scope-inclusion per location (e.g., method_call only).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java(2 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodCallSymbolProvider.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build tackle2-addon-analyzer
🔇 Additional comments (2)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java (1)
230-230: Whitespace-only change.No functional impact.
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodCallSymbolProvider.java (1)
43-48: Good generalization to handle matches originating from fields.Allowing IField-backed elements helps catch method references that surface via field initializers or class fields.
Comment on lines
+43
to
+49
| ICompilationUnit unit = null; | ||
| if (m.getElement() instanceof IMethod) { | ||
| unit = ((IMethod) m.getElement()).getCompilationUnit(); | ||
| } else if (m.getElement() instanceof IField) { | ||
| unit = ((IField) m.getElement()).getCompilationUnit(); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid NPEs and only dispose working copies you created; wrap in try/finally.
unit can be null; queryQualificationMatches, setSource, discardWorkingCopy, and close can throw. Also don’t close primary compilation units you didn’t open.
- ICompilationUnit unit = null;
+ ICompilationUnit unit = null;
+ boolean createdWorkingCopy = false;
if (m.getElement() instanceof IMethod) {
unit = ((IMethod) m.getElement()).getCompilationUnit();
} else if (m.getElement() instanceof IField) {
unit = ((IField) m.getElement()).getCompilationUnit();
}
if (unit == null) {
- IClassFile cls = (IClassFile) ((IJavaElement) e).getAncestor(IJavaElement.CLASS_FILE);
+ IClassFile cls = (IClassFile) e.getAncestor(IJavaElement.CLASS_FILE);
if (cls != null) {
- unit = cls.getWorkingCopy(new WorkingCopyOwnerImpl(), null);
+ unit = cls.getWorkingCopy(new WorkingCopyOwnerImpl(), null);
+ createdWorkingCopy = unit != null && unit.isWorkingCopy();
}
}
- 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);
- // Under tests, resolveConstructorBinding will return null if there are problems
- IProblem[] problems = cu.getProblems();
- if (problems != null && problems.length > 0) {
- logInfo("KONVEYOR_LOG: " + "Found " + problems.length + " problems while compiling");
- int count = 0;
- for (IProblem problem : problems) {
- logInfo("KONVEYOR_LOG: Problem - ID: " + problem.getID() + " Message: " + problem.getMessage());
- count++;
- if (count >= SymbolProvider.MAX_PROBLEMS_TO_LOG) {
- logInfo("KONVEYOR_LOG: Only showing first " + SymbolProvider.MAX_PROBLEMS_TO_LOG + " problems, " +
- (problems.length - SymbolProvider.MAX_PROBLEMS_TO_LOG) + " more not displayed");
- break;
- }
- }
- }
- cu.accept(visitor);
- if (visitor.symbolMatches()) {
- symbols.add(symbol);
- }
- }
- unit.discardWorkingCopy();
- unit.close();
+ if (unit == null) {
+ // No source/working copy available to qualify; skip deep check.
+ } else {
+ try {
+ 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);
+ IProblem[] problems = cu.getProblems();
+ if (problems != null && problems.length > 0) {
+ logInfo("KONVEYOR_LOG: " + "Found " + problems.length + " problems while compiling");
+ int count = 0;
+ for (IProblem problem : problems) {
+ logInfo("KONVEYOR_LOG: Problem - ID: " + problem.getID() + " Message: " + problem.getMessage());
+ count++;
+ if (count >= SymbolProvider.MAX_PROBLEMS_TO_LOG) {
+ logInfo("KONVEYOR_LOG: Only showing first " + SymbolProvider.MAX_PROBLEMS_TO_LOG + " problems, " +
+ (problems.length - SymbolProvider.MAX_PROBLEMS_TO_LOG) + " more not displayed");
+ break;
+ }
+ }
+ }
+ cu.accept(visitor);
+ if (visitor.symbolMatches()) {
+ symbols.add(symbol);
+ }
+ }
+ } finally {
+ if (createdWorkingCopy && unit.isWorkingCopy()) {
+ unit.discardWorkingCopy();
+ }
+ }
+ }Notes:
- Removing unit.close() avoids closing shared primary CUs the search engine or other code may rely on.
- createdWorkingCopy ensures we only dispose of what we created.
Also applies to: 50-56, 56-61, 82-84
🤖 Prompt for AI Agents
In
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodCallSymbolProvider.java
around lines 43-49 (and similarly for the other ranges noted: 50-56, 56-61,
82-84), the current code can NPE when unit is null and may improperly close or
discard compilation units it didn't create; wrap the working-copy logic in a
try/finally, null-check unit before using it, call createdWorkingCopy to record
whether you created the working copy and only call discardWorkingCopy/close when
you actually created it, and catch or let checked exceptions propagate as
appropriate so setSource/queryQualificationMatches/discardWorkingCopy/close are
executed safely in finally without closing primary CUs you didn't open.
aufi
approved these changes
Sep 12, 2025
Member
aufi
left a comment
aufi
left a comment
There was a problem hiding this comment.
I don't have all background on java, but overall looks good.
shawn-hurley
approved these changes
Sep 12, 2025
Collaborator
Author
This was referenced Sep 17, 2025
This was referenced Sep 29, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to https://issues.redhat.com/browse/MTA-5763
Summary by CodeRabbit