feat: Support parameter-specific matching for METHOD_CALL patterns #183
Conversation
Resolves konveyor#182 This commit adds the ability to distinguish between overloaded methods with different parameter signatures in METHOD_CALL and CONSTRUCTOR_CALL patterns. Previous behavior: - Patterns with parameter types (e.g., setProperty(String, String)) were stripped and matched all overloads of the method New behavior: - Parameter types are extracted and compared against actual method signatures - Supports wildcard matching (*) for any parameter type - Supports subtype matching (Object pattern matches String actual) - Supports generic signature matching (List<String> only matches exact type) Implementation details: - Added extractParameterTypes() to parse parameter types from query patterns - Added matchesParameterTypes() to validate actual vs. query parameters - Added type matching with wildcard, subtype, and generic support - Updated visit() methods for MethodInvocation, ConstructorInvocation, and ClassInstanceCreation to check parameter types Examples: 1. Exact matching: Pattern: DriverManager.getConnection(String, String, String) Matches: DriverManager.getConnection("url", "user", "password") ✓ Rejects: DriverManager.getConnection("url") ✗ 2. Wildcard matching: Pattern: method(*, String) Matches: method with any first param and String second param 3. Subtype matching: Pattern: method(Object) Matches: method(String), method(Integer), etc. Backward compatibility: - Patterns without parameters continue to match all overloads - Existing rules without parameter types are unaffected Signed-off-by: tsanders <tsanders@redhat.com>
Add comprehensive unit tests for the extractParameterTypes() method covering various scenarios: - No parameters, empty parameters, single/multiple parameters - Wildcard parameters - Simple and nested generics - Primitive types and arrays - Mixed types with whitespace handling - Real-world examples and varargs All 16 tests pass successfully. Signed-off-by: tsanders <tsanders@redhat.com>
Add comprehensive documentation explaining the new parameter-specific matching feature for METHOD_CALL and CONSTRUCTOR_CALL patterns. Covers: - Basic usage and syntax - Advanced features (wildcards, subtype matching, generics) - Constructor calls - Backward compatibility - Edge cases and migration examples - Technical details Signed-off-by: tsanders <tsanders@redhat.com>
Implement multiple performance optimizations to minimize overhead: 1. Early returns in matchesParameterTypes(): - Zero overhead for backward compatibility (no parameters specified) - Quick length check before iteration - Special case for zero parameters - Early exit on first parameter mismatch 2. Primitive type optimization in typeMatches(): - Primitives can only match exactly, skip expensive hierarchy traversal - Check exact match before erasure check - Check erasure before full hierarchy traversal 3. Type hierarchy traversal optimizations in isSubtypeOf(): - Added cycle detection with visited set to prevent infinite loops - Cache actualTypeName to avoid recomputation - Quick erasure name checks before recursing into superclass/interfaces - Reduces repeated getQualifiedTypeName() calls 4. Parameter extraction optimizations in extractParameterTypes(): - Early return on no opening parenthesis - Pre-size ArrayList to common case (4 params) to avoid resizing - Cache string length to avoid repeated method calls Performance characteristics: - 0% overhead for queries without parameters (backward compatibility) - Minimal overhead for exact matches (most common case) - Bounded complexity for type hierarchy traversal with cycle detection - Reduced allocations and method calls throughout All 16 unit tests continue to pass. Signed-off-by: tsanders <tsanders@redhat.com>
WalkthroughAdds parameter-aware matching for METHOD_CALL and CONSTRUCTOR_CALL patterns: the visitor extracts parameter-type hints from query patterns, resolves call bindings from the AST, and compares actual parameter types against queried types (supporting wildcards, subtypes, generics, arrays, primitives, and varargs). Also adds documentation and unit tests for parameter parsing. Changes
Sequence Diagram(s)sequenceDiagram
participant Q as Query Pattern
participant V as CustomASTVisitor
participant P as Parameter Parser
participant AST as AST / Binding
participant M as Type Matcher
Q->>V: provide pattern with params (e.g. Foo.bar(String, List<String>[]))
activate V
V->>P: extractParameterTypes(query)
P-->>V: parsed queryParamTypes
V->>V: strip params for FQN matching
V->>AST: resolve method/ctor binding
AST-->>V: binding + actual parameter types
V->>M: matchesParameterTypes(queryParamTypes, actualTypes)
alt match
M-->>V: MATCH (wildcard/subtype/generic/array/varargs rules satisfied)
V-->>Q: report matched node
else mismatch
M-->>V: NO MATCH
V-->>Q: skip node (or fallback if binding unresolved)
end
deactivate V
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Resolved conflicts by keeping parameter extraction enhancement which is a superset of PR konveyor#179's parameter stripping fix. The resolved code: - Extracts parameters for matching (enhancement for konveyor#182) - Strips parameters from query (fix from PR konveyor#179) Both functionalities are preserved. Signed-off-by: tsanders <tsanders@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitorTest.java (2)
19-26: Consider makingextractParameterTypesstatic or using a helper for direct testing
extractParameterTypesdoes not depend on instance state, and the test fixture constructsCustomASTVisitoronly to call it. Consider making this methodstatic(or exposing it via a small helper) so tests don’t need a dummy visitor instance. This is purely a cleanliness/maintainability tweak; behavior is fine as-is.
28-153: Good coverage of parameter parsing; optionally add malformed-input caseThe test suite nicely exercises the main parsing cases (no/empty params, primitives, arrays, generics, wildcard, whitespace, varargs). One optional enhancement is to add a malformed query case (e.g. missing closing parenthesis) to lock in the current
nullbehavior for invalid patterns.java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (1)
149-193: Integration of parameter checks into visit() methods is sound; consider minor DRYingThe new calls to
matchesParameterTypes(binding.getParameterTypes())invisit(MethodInvocation),visit(ConstructorInvocation), andvisit(ClassInstanceCreation)correctly:
- Apply parameter filtering only after the FQN pattern matches.
- Preserve the previous fallback behavior (accepting matches when bindings can’t be resolved).
- Gate both method and constructor matches on parameter count and type.
There is some duplication between the constructor paths (
ConstructorInvocationvsClassInstanceCreation). If you touch this area again, consider factoring out a small helper that takes the resolvedIMethodBindingand a log prefix to keep behavior in one place. Not urgent, but could simplify future changes.Also applies to: 201-239, 247-287
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/PARAMETER_MATCHING.md(1 hunks)java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java(8 hunks)java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitorTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
java-analyzer-bundle.test/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitorTest.java (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (1)
CustomASTVisitor(33-524)
🔇 Additional comments (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (1)
38-65: Parameter extraction and query preprocessing look correct and backward‑compatibleThe introduction of
queryParameterTypesplus the constructor’s two-phase processing:
queryParameterTypes = extractParameterTypes(query);processedQuery = query.replaceAll("\\([^)]*\\)", "");- wildcard expansion on the stripped
processedQuerygives you three clear states:
queryParameterTypes == null→ no parameter filter, match all overloads (legacy behavior).queryParameterTypes.isEmpty()→ only zero-arg methods/constructors (method()).- Non-empty list → parameter-specific matching.
extractParameterTypesitself handles nested generics via the</>depth counter, trims whitespace, and correctly distinguishes between “no params” and “empty param list”. That aligns well with the documented semantics.Also applies to: 289-365
...yzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java
Show resolved
Hide resolved
Fix issue identified by coderabbitai where varargs notation (Type...) in query patterns would not match JDT's array representation (Type[]). JDT's ITypeBinding.getQualifiedTypeName() returns array types as Type[] but users may write varargs as Type... in patterns. Now we normalize Type... → Type[] before matching to support both notations. Uses endsWith() check for precise normalization of only trailing varargs, avoiding potential issues with ... appearing elsewhere in type names. This ensures the documented varargs example works correctly: pattern: method(java.lang.Object...) matches: method with Object[] (varargs) parameter All 16 unit tests continue to pass. Signed-off-by: tsanders <tsanders@redhat.com>
tsanders-rh
force-pushed
the
feat/parameter-specific-matching
branch
from
5e78ed6 to
dda95e8
Compare
When binding.getParameterTypes() returns null (which can happen with certain method bindings), we were getting a NullPointerException when trying to access actualTypes.length. This fix adds a null check before accessing the array length. When actualTypes is null, we match only if the query expects no parameters (i.e., queryParameterTypes is empty). This fixes the CI failure where the java-provider was crashing during analysis due to this NPE. Signed-off-by: tsanders <tsanders@redhat.com>
shawn-hurley
left a comment
shawn-hurley
left a comment
There was a problem hiding this comment.
+1 thank you!
I do think that we are going to have to update the documentation in analyzer-lsp to remove the query pattern syntax because the ? notation will no longer work.
The extractParameterTypes method was incorrectly treating regex alternation groups like (FileWriter|FileReader) as method parameters. This caused patterns like: java.io.(FileWriter|FileReader|PrintStream|File|PrintWriter|RandomAccessFile)* To be parsed as having parameters, which then filtered out valid constructor calls that didn't match the bogus parameter list. Fix: Add heuristics to detect regex alternation: - Check for pipe (|) character inside parentheses - Verify parentheses are at the end of the pattern (method params) vs in the middle (regex groups) This fixes the missing violations in local-storage-00001 and other rules that use regex alternation in CONSTRUCTOR_CALL patterns. Signed-off-by: tsanders <tsanders@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (2)
297-356: Consider normalizing whitespace in extracted parameter types for robustness.The parsing logic correctly handles nested generics and distinguishes method parameters from regex alternation. However, queries with extra whitespace like
method(Map<String , Integer>)won't match the consistently-formatted output ofgetQualifiedTypeName(Map<String, Integer>), causing false negatives.To improve robustness, consider normalizing whitespace after extraction:
// Add the last parameter - params.add(paramsString.substring(start).trim()); + String param = paramsString.substring(start).trim(); + params.add(normalizeWhitespace(param));And add a helper:
/** * Normalizes whitespace in type parameters: "Map<String , Integer>" -> "Map<String, Integer>" */ private String normalizeWhitespace(String type) { return type.replaceAll("\\s+", " ") // Collapse multiple spaces .replaceAll("\\s*<\\s*", "<") // Remove spaces around < .replaceAll("\\s*>\\s*", ">") // Remove spaces around > .replaceAll("\\s*,\\s*", ", "); // Normalize comma spacing }Alternatively, document that query patterns must use standard Java type formatting (no extra spaces).
495-553: Consider defensive handling of null type keys (optional).The hierarchy traversal correctly implements cycle detection and performance optimizations (erasure checks before recursion). However, Line 503's condition skips cycle detection if
typeKeyis null:if (typeKey != null && !visited.add(typeKey))While
ITypeBinding.getKey()should always return non-null and valid Java has no type hierarchy cycles, defensive programming would prevent theoretical infinite loops:String typeKey = actualType.getKey(); - if (typeKey != null && !visited.add(typeKey)) { - return false; // Already visited, avoid infinite loop + if (typeKey == null) { + // Defensive: no key available, can't track cycles for this type + // Log or assign a synthetic key based on qualified name if needed + typeKey = actualTypeName; // Use cached name as fallback key + } + if (!visited.add(typeKey)) { + return false; // Already visited, avoid infinite loop }This ensures cycle detection never fails silently, though the risk is minimal in practice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java(8 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 (8)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/CustomASTVisitor.java (8)
5-9: LGTM!The new imports support the parameter-matching feature:
ListforqueryParameterTypes,ArrayListfor pre-sized parameter lists,HashSet/Setfor cycle detection inisSubtypeOf, andCollectionsfor the empty-list sentinel.
38-38: LGTM!The
queryParameterTypesfield correctly stores the extracted parameter type list, enabling backward-compatible filtering (null means no parameter filter specified).
50-70: LGTM!The constructor correctly extracts parameter types before stripping them from the query pattern, preserving backward compatibility for name-based matching while enabling parameter-specific filtering.
167-175: LGTM!Parameter validation correctly filters overloads after name matching, with appropriate logging and fallback behavior when bindings are unavailable.
214-222: LGTM!Parameter validation in both constructor visit methods mirrors the logic in
visit(MethodInvocation), ensuring consistent behavior across method and constructor calls.Also applies to: 261-269
362-392: LGTM!Excellent performance optimizations: zero overhead for queries without parameters (null check), early length mismatch exit, and loop avoidance for empty parameter lists. The logic correctly validates each parameter position.
401-443: LGTM! Varargs normalization correctly implemented.The type-matching logic is comprehensive and well-optimized:
- Wildcards (
*) short-circuit immediately- Varargs are correctly normalized (
Type...→Type[]) to match JDT's array representation (lines 411-416) ✅- Primitives use exact name matching without hierarchy traversal
- Exact matches (including full generic signatures) are checked before expensive operations
- Erasure matching allows raw-type queries to match parameterized types
- Subtype matching leverages cached type names and quick erasure checks before hierarchy walks
The varargs fix from the previous review is correctly implemented and will ensure patterns like
method(String...)match varargs parameters.
449-484: LGTM!The method correctly reconstructs fully-qualified type names with proper handling of arrays (including multi-dimensional), primitives, and nested generic type arguments. The recursive approach elegantly handles arbitrary nesting depth.
|
Thanks for the review and the heads up about the ? notation! You're absolutely right - we should update the analyzer-lsp documentation to reflect this. I did some
So the implementation is correct for all actual usage. The docs just need to be clarified to avoid I can help with the analyzer-lsp documentation update if needed. |
2 participants
Summary
This PR adds parameter-specific matching for METHOD_CALL and CONSTRUCTOR_CALL patterns, resolving #182.
Currently, METHOD_CALL patterns cannot distinguish between overloaded methods with different parameter signatures. This PR implements parameter type extraction and matching to enable precise targeting of specific method overloads.
Changes
Core Implementation (
CustomASTVisitor.java)extractParameterTypes()to parse parameter types from query patterns, handling nested generics likeMap<String, Integer>matchesParameterTypes()to compare query parameters against actual method signaturestypeMatches()with support for:*matches any type)Objectmatches actualString)List<String>only matches exact generic type)getQualifiedTypeName(): Extracts fully qualified type names including generic parametersisSubtypeOf(): Recursively checks type hierarchies (superclasses and interfaces)visit(MethodInvocation): Now validates parameter types after method name matchingvisit(ConstructorInvocation): Parameter validation for constructor callsvisit(ClassInstanceCreation): Parameter validation fornewexpressionsPerformance Optimizations
Multiple optimizations ensure minimal performance impact:
Zero overhead for backward compatibility:
matchesParameterTypes()when no parameters specifiedOptimized matching path:
Type hierarchy optimizations:
getQualifiedTypeName()callsAllocation optimizations:
Performance characteristics:
Testing
extractParameterTypes()Documentation
docs/PARAMETER_MATCHING.mdUse Cases
Example 1: Exact Parameter Matching
Pattern:
Results:
DriverManager.getConnection("url", "user", "password")DriverManager.getConnection("url")DriverManager.getConnection("url", new Properties())Example 2: Wildcard Matching
Pattern:
Results:
method(123, "value")(any first param, String second)method("value", 123)(wrong order)Example 3: Subtype Matching
Pattern:
Results:
method(String),method(Integer), etc. (any Object subtype)Backward Compatibility
Commits
feat: Support parameter-specific matching for METHOD_CALL patterns- Core implementationtest: Add unit tests for parameter type extraction- Comprehensive test coveragedocs: Add parameter matching feature documentation- User-facing documentationperf: Add performance optimizations to parameter matching- Performance improvementsTesting
mvn clean compileRelated
Checklist
Summary by CodeRabbit
Documentation
New Features
Tests