-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Python: Modernize 4 queries for missing/multiple calls to init/del methods #19932
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?
Python: Modernize 4 queries for missing/multiple calls to init/del methods #19932
Conversation
QHelp previews: python/ql/src/Classes/CallsToInitDel/MissingCallToDel.qhelpMissing call to superclass
|
3a1ccc1
to
e8a65b8
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.
I think overall this looks really solid, but I must admit I didn't have time to fully digest the new implementation. (However, as I'm going away on PTO, I figured it was best to give you my interim review regardless.)
I'm slightly worried about the many not exists(...)
constructions in the modelling (as these may result in large joins). Make sure the performance testing is run with the tuple-counting option enabled.
Other than that, if the performance looks good, I would be happy to merge this.
Function getASuperCallTargetFromClass(Class mroBase, Class cls, string name) { | ||
exists(Function target | | ||
target = findFunctionAccordingToMroKnownStartingClass(cls, mroBase, name) and | ||
( | ||
result = target | ||
or | ||
result = getASuperCallTargetFromCall(mroBase, target, _, name) | ||
) | ||
) | ||
} |
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.
This MRO business is a bit awkward. I wonder if we could clean it up by creating a new IPA type representing "the MRO starting at a particular base class", and thus avoid having to thread mroBase
through everything (which might mean we could use the built-in transitive closure operators).
This is just some musing on my part -- not necessarily an immediately actionable suggestion.
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.
That could be a useful component of a public-facing dataflow dispatch / call graph resolution API
78235f2
to
7dad89f
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.
One minor comment, otherwise this looks good to me. 👍
I assume DCA was uneventful?
locationBefore(call1.getLocation(), call2.getLocation()) and | ||
calledMulti = getASuperCallTargetFromCall(cls, meth, call1, name) and | ||
calledMulti = getASuperCallTargetFromCall(cls, meth, call2, name) and |
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.
Hmm... I guess if you have
As an alternative, we could consider rank
ing the superclass method calls by location, and picking the first two items in this ranking. That way, we only ever produce two method calls.
As this query makes use of an Option type with a location, a pattern I've also identified as useful in another quality query, it has been extracted into a Shared Library PR (#20280), that I intend to refactor a little to include once it's merged. It seems like the previous DCA run had a FreeCAD failure that I'm not certain the cause of. That has historically been indicative of a performance concern in uncommon scenarios. I will start a new run on an updated branch to check whether it reproduces. |
… change alerts to alert on the class and provide clearer information, using optional location links.
…ke much sense (__init__ is still called anyway)
b33a1c2
to
89bc6e6
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 modernizes 4 queries related to missing/multiple calls to __init__
/__del__
methods in Python class hierarchies. The queries are moved to a new directory structure and rewritten to use DataFlowDispatch methods instead of the legacy pointsTo approach, improving precision and descriptiveness of alerts.
Key changes:
- Migrates from legacy libraries to modern dataflow analysis for better precision
- Reorganizes query locations from
Classes/
toClasses/CallsToInitDel/
directory - Enhances alert messages with more descriptive information and additional context about method calls
Reviewed Changes
Copilot reviewed 48 out of 48 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
python/ql/test/query-tests/Classes/multiple/multiple-init/multiple_init.py |
Updated test cases with more concrete examples and better alerts |
python/ql/test/query-tests/Classes/missing-init/missing_init.py |
Enhanced test coverage with realistic scenarios and improved alert annotations |
python/ql/src/Classes/CallsToInitDel/SuperclassInitCalledMultipleTimes.ql |
New modernized query for detecting multiple init calls using dataflow analysis |
python/ql/src/Classes/CallsToInitDel/MethodCallOrder.qll |
New library providing core logic for method call analysis with MRO support |
python/ql/src/Classes/MethodCallOrder.qll |
Deprecated the old library in favor of the new implementation |
Various .expected files |
Updated to reflect new query output format and improved precision |
Modernizes py/missing-call-to-init, py/missing-call-to-del, py/multiple-calls-to-init,, and py/multiplecalls-to-del.
Uses DataFlowDispatch methods rather than pointsTo.
Improves precision (to reduce FPs) in resolving relevant calls as precisely as possible using known MRO information, improves descriptiveness of alert messages by including more possible relevant locations, and improves documentation.