Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[javasrc2cpg] fix ext method call full name with same name of a scope var #4545

Merged
merged 1 commit into from
May 8, 2024

Conversation

xavierpinho
Copy link
Contributor

When dealing with:

  • external method calls whose receiver type we can't resolve, and
  • if there is a variable in scope with that same name, then

we are resolving to that variable's type, as in:

import org.Client;
import org.Builder;
...
Client foo = new Builder().foo().build(); // build's receiver is org.Client
Client bar = new Builder().somethingElse().build(); // build's receiver is <unresolvedNamespace>

This gets worse: if in the 2nd statement above we had used foo() again instead of somethingElse(), it would resolve again to org.Client.

This patch stinks, though: I'm not a fan of that edge condition. Hopefully we can find something nicer.

@xavierpinho xavierpinho added bug Something isn't working java Relates to javasrc2cpg labels May 7, 2024
Copy link
Collaborator

@DavidBakerEffendi DavidBakerEffendi left a comment

Choose a reason for hiding this comment

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

Ideally, our namespace would be org.Builder right? Is it a large effort (or unsound in some cases) to achieve this result based on imports?

@xavierpinho
Copy link
Contributor Author

Ideally, our namespace would be org.Builder right? Is it a large effort (or unsound in some cases) to achieve this result based on imports?

Not always, would depend on the return type of foo() and somethingElse(). Some libraries would return different types so that the order of operations/methods we can call is coherent.

Copy link
Contributor

@johannescoetzee johannescoetzee left a comment

Choose a reason for hiding this comment

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

This is a good catch! I think this patch is fine to merge to fix it for now. I wonder about matching the NodeWithSimpleName there in general. I originally did it that way to keep the code simple, but I didn't consider all the cases. It might make more sense to only match the specific node types we want to look up (so possibly just identifiers and types, but we'd have to consider all options)

@johannescoetzee
Copy link
Contributor

Ideally, our namespace would be org.Builder right? Is it a large effort (or unsound in some cases) to achieve this result based on imports?

The namespace should already be org.Builder for the foo and somethingElse calls based on the imports. We don't know anything about the return types of those methods though, so can't say anything sensible about the namespace for the build calls.

@xavierpinho xavierpinho merged commit 5acae86 into master May 8, 2024
5 checks passed
@xavierpinho xavierpinho deleted the xavierp/java-ext-meth-stk branch May 8, 2024 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working java Relates to javasrc2cpg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants