Rust: Extract SelfParams from crate graph#19369
Conversation
hvitved
force-pushed
the
rust/crate-graph-self-param
branch
from
ffbe31b to
6df5a1e
Compare
There was a problem hiding this comment.
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.
Consider extracting the self parameter logic into a separate helper function to improve clarity and reduce complexity in the parameter iteration block.
|
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.
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.
I decided to not populate the name, since we also don't do that for non-self parameters. Lifetimes are currently not important.
aibaars
left a comment
aibaars
left a comment
There was a problem hiding this comment.
Looks good to me. I'm wondering whether we should populate the name and lifetime fields.
That should be resolved by this PR as well. |
Nice! Thanks. |
Written with the help from Copilot chat.
DCA looks great; the
Missing call targetsmetric goes down for all projects, without significant analysis slowdown.