Skip to content

Rust: Resolve function calls to traits methods - #19575

Merged
paldepind merged 3 commits into
github:mainfrom
paldepind:rust/function-call-method
May 27, 2025
Merged

Rust: Resolve function calls to traits methods#19575
paldepind merged 3 commits into
github:mainfrom
paldepind:rust/function-call-method

Conversation

@paldepind

@paldepindpaldepind commented May 24, 2025

Copy link
Copy Markdown
Contributor

This PR adds resolution for function calls to trait methods ATrait::some_method(obj, ...).

Key changes:

  • Introduce an abstract MethodCall abstract class that is extended by the different types of expressions that can lead to method calls. Later this class can also be extended by overloaded operators that perform method calls.
  • Added a methodCandidateTrait predicate, which is similar to the existing methodCandidate predicate but which also takes a trait that the method should come from. potentialInstantiationOf now accounts both for method calls that target a specific trait.

@github-actionsgithub-actionsBot added the Rust Pull requests that update Rust code label May 24, 2025
Comment threadrust/ql/lib/codeql/rust/internal/TypeInference.qll Fixed
Comment threadrust/ql/lib/codeql/rust/internal/TypeInference.qll Fixed
Comment threadrust/ql/lib/codeql/rust/internal/TypeInference.qll Fixed
@paldepind
paldepindforce-pushed the rust/function-call-method branch from 051938a to e778cbeCompareMay 24, 2025 08:26

@BoomchainLabsBoomchainLabs left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Lgtm

// trait bound on the type parameter.
result = getTypeParameterMethod(receiver.getTypeAt(TypePath::nil()), receiver.getField())
)
Function resolveMethodCallTarget(MethodCall mc) {

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This resolveMethodCallTarget was previously getStaticTarget inside MethodCallExpr. But since we now need for CallExpr as well, I've moved it in here.

methodCandidateTrait(rootType, mc.getTrait(), name, arity, impl)
or
not exists(mc.getTrait()) and
methodCandidate(rootType, name, arity, impl)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This new disjunct and methodCandidateTrait is the crux of the change.

@paldepind
paldepind marked this pull request as ready for review May 26, 2025 07:11
@paldepind
paldepind requested a review from a team as a code ownerMay 26, 2025 07:11
hvitved
hvitved previously approved these changes May 26, 2025

@hvitvedhvitved left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Let's wait for DCA before merging.

*/
pragma[nomagic]
private predicate methodCandidateTrait(Type type, Trait trait, string name, int arity, Impl impl) {
trait = resolvePath(impl.getTrait().(PathTypeRepr).getPath()) and

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Alternatively, trait = resolvePath(impl.(ImplItemNode).getTraitPath())

@paldepind

Copy link
Copy Markdown
ContributorAuthor

DCA seems fine to me.

"Missing call targets" went down, but I don't think these trait calls where counted as missing before (because there where resolved to the trait itself), so I think the reduction in missing call targets is a secondary effect since some types are now more accurate, and hence some normal method calls can now be resolved.

@paldepind
paldepind merged commit bf2cfab into github:mainMay 27, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

RustPull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@paldepind@hvitved@github-advanced-security@BoomchainLabs