-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Rust: Extract SelfParam
s from crate graph
#19369
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
base: main
Are you sure you want to change the base?
Conversation
ffbe31b
to
6df5a1e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the function emitting logic to extract the self parameter separately from the other parameters in Rust functions. Key changes include:
- Introducing a new variable to hold function_data via db.function_data(function).
- Refactoring the parameter iteration to use .enumerate() with filter_map to extract the self parameter when present.
- Updating the generated ParamList to include the extracted self parameter.
Files not reviewed (4)
- rust/ql/lib/codeql/rust/frameworks/stdlib/Clone.qll: Language not supported
- rust/ql/test/library-tests/dataflow/modeled/inline-flow.expected: Language not supported
- rust/ql/test/library-tests/type-inference/type-inference.ql: Language not supported
- rust/ql/test/query-tests/security/CWE-089/CONSISTENCY/PathResolutionConsistency.expected: Language not supported
pat: None, | ||
}) | ||
|
||
if idx == 0 && function_data.has_self_param() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting the self parameter logic into a separate helper function to improve clarity and reduce complexity in the parameter iteration block.
Copilot uses AI. Check for mistakes.
I'm not sure if it's related or orthogonal or already fixed in this PR, but FWIW it seems to me that functions from dependencies also have one additional non-self parameter. When This just bit me as I added an arity check in method resolution and lost a bunch of results. |
lifetime: None, | ||
name: None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the name
be simply self
? Is it possible to populate the lifetime
field, or is it not important?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I'm wondering whether we should populate the name
and lifetime
fields.
Written with the help from Copilot chat.
DCA looks great; the
Missing call targets
metric goes down for all projects, without significant analysis slowdown.