Skip to content

Conversation

@jmle
Copy link
Collaborator

@jmle jmle commented Oct 8, 2025

Related to https://issues.redhat.com/browse/MTA-5763

Summary by CodeRabbit

  • Bug Fixes
    • Improved reliability of method call analysis by correctly resolving source context from methods or fields, with a safe fallback to class files.
    • Reduced crashes and errors caused by null references during symbol resolution.
    • Preserved behavior for simple (non-qualified) queries while improving handling for qualified cases.

@coderabbitai
Copy link

coderabbitai bot commented Oct 8, 2025

Walkthrough

Updates MethodCallSymbolProvider to treat matched elements as generic IJavaElement, adding IField handling, null-safe compilation unit resolution (including CLASS_FILE ancestor fallback), and preserving existing behavior for non-qualified queries. Imports adjusted accordingly and unit disposal retained.

Changes

Cohort / File(s) Summary of Changes
Symbol provider null-safe CU resolution
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodCallSymbolProvider.java
Treat match element as IJavaElement; add IField support; remove ITypeRoot import; null-guarded retrieval of compilation unit via IMethod/IField; fallback via CLASS_FILE ancestor when unit is null; preserve behavior for non-qualified queries; maintain unit dispose/close after processing.

Sequence Diagram(s)

sequenceDiagram
    participant Provider as MethodCallSymbolProvider
    participant Matcher as Search/Match
    participant Element as IJavaElement
    participant JDT as JDT (CU/Ancestor)

    Provider->>Matcher: get match element
    Matcher-->>Provider: element (IJavaElement)

    alt element is IMethod
        Provider->>Element: getCompilationUnit()
        Element-->>Provider: unit (maybe null)
    else element is IField
        Provider->>Element: getCompilationUnit()
        Element-->>Provider: unit (maybe null)
    else other element
        Note right of Provider: unit = null
    end

    alt unit is null
        Provider->>JDT: element.getAncestor(CLASS_FILE)
        JDT-->>Provider: classFile (maybe null)
        Provider->>JDT: classFile.getWorkingCopy()/getCU
        JDT-->>Provider: unit (maybe null)
    end

    Provider->>Provider: process query (unchanged for non-qualified)
    Provider-->>JDT: discard/close unit
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • eemcmullan
  • shawn-hurley
  • aufi

Poem

A hop and a skip through code I roam,
I sniff out methods, fields I comb.
If units hide, I nose the trail—
CLASS_FILE crumbs, I will not fail.
With tidy paws I close the door,
Then thump: “Resolved!” and hop for more. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly conveys that the PR improves method matching to catch static methods declared on class fields, aligning directly with the code changes that add IField handling in MethodCallSymbolProvider. It succinctly summarizes the primary enhancement without unnecessary detail or file references. This specificity makes it easy for reviewers to understand the core objective of the changeset.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f83fe79 and c957bc2.

📒 Files selected for processing (1)
  • 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 (3)
java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/symbol/MethodCallSymbolProvider.java (3)

10-10: LGTM! Import needed for IField handling.

The IField import is correctly added to support the new logic for handling static methods declared on class fields.


35-35: LGTM! Correct generalization to IJavaElement.

Changing the cast from IMethod to IJavaElement is the right approach to support both IMethod and IField instances.


44-48: LGTM! IField handling correctly added.

The logic properly extends compilation unit retrieval to IField instances, which is necessary for catching static methods declared on class fields (e.g., Foo.bar.someMethod() where bar is a static field).

However, as noted in the PR comments, please add a test case demonstrating what this PR now catches versus what was previously missed. A test would help verify this behavior and prevent regressions.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@jmle jmle requested a review from shawn-hurley October 8, 2025 12:38
@shawn-hurley
Copy link
Contributor

Is there a test case that shows an example of what this catches now vs what was missing?

Copy link
Contributor

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this looks good, and is a good change. I would like to request a follow-up that includes a test to ensure we don't inadvertently break this fix.

@dymurray dymurray added the cherry-pick/release-0.8 This PR should be cherry-picked to release-0.8 branch label Oct 9, 2025
jmle added 2 commits October 9, 2025 19:24
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>

# Conflicts:
#	java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
@jmle jmle merged commit f0b6d70 into konveyor:main Oct 9, 2025
8 checks passed
github-actions bot pushed a commit that referenced this pull request Oct 9, 2025
…ields (#150)

* Improve method matching to catch static methods declared on class fields

Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>

# Conflicts:
#	java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java

* Remove unnecessary line

Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>

---------

Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
Signed-off-by: Cherry Picker <[email protected]>
jmle added a commit that referenced this pull request Oct 10, 2025
…ields (#150) (#156)

* Improve method matching to catch static methods declared on class fields



# Conflicts:
#	java-analyzer-bundle.core/src/main/java/io/konveyor/tackle/core/internal/SampleDelegateCommandHandler.java

* Remove unnecessary line



---------

Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
Signed-off-by: Cherry Picker <[email protected]>
Co-authored-by: Juan Manuel Leflet Estrada <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherry-pick/release-0.8 This PR should be cherry-picked to release-0.8 branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants