Python: Modernize special method query #18956
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tausbn
force-pushed
the
tausbn/python-more-special-method-query-refactoring
branch
2 times, most recently
from
5b1e08a to
f63c7cf
Compare
Moves a bunch of `owner.declaredAttribute(name) = f` instances to the top level, in the process greatly cleaning up the code. The behaviour should be the unchanged. Having done this, there's only one place where we depend on points-to, and that's in the remaining `declaredAttribute` call. This should greatly simplify the move away from points to.
Adds a new boolean parameter `is_unused_default` that indicates whether the given result is one where a parameter to a special method has a default value (which will never be used when invoked in the normal way). These results are somewhat less useful (because the special method _might_ be invoked directly, in which case the default value would still be relevant), but it seemed like a shame to simply remove the code, so instead I opted to disable it in this way.
Instances of this include - Bodies that contain just a docstring (common in Zope interfaces) - Bodies that do nothing but raise an exception.
tausbn
force-pushed
the
tausbn/python-more-special-method-query-refactoring
branch
from
f63c7cf to
f3353dc
Compare
Technically we still depend on points-to in that we still mention `PythonFunctionValue` and `ClassValue` in the query. However, we immediately move to working with the corresponding `Function` and `Class` AST nodes, and so we're not really using points-to. (The reason for doing things this way is that otherwise the `.toString()` for all of the alerts would change, which would make the diff hard to interpret. This way, it should be fairly simple to see which changes are actually relevant.) We do lose some precision when moving away from points-to, and this is reflected in the changes in the `.expected` file. In particular we no longer do complicated tracking of values, but rather look at the syntactic structure of the classes in question. This causes us to lose out on some results where a special method is defined elsewhere, and causes a single FP where a special method initially has the wrong signature, but is subsequently overwritten with a function with the correct signature. We also lose out on results having to do with default values, as these are now disabled. Finally, it was necessary to add special handling of methods marked with the `staticmethod` decorator, as these expect to receive fewer arguments. This was motivated by a MRVA run, where e.g. sympy showed a lot of examples along the lines of ``` @staticmethod def __abs__(): return ... ```
Also adds `quality` to the list of tags for the query.
tausbn
changed the title
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes the special method query by refactoring it to remove reliance on the points-to API and reducing false positives.
- Updated test cases in om_test.py to return concrete values instead of placeholder pass statements.
- Added a new test class with a static abs method.
- Updated the change notes to document the query modernization and adjustments to default parameter handling.
Reviewed Changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| python/ql/test/query-tests/Functions/general/om_test.py | Updated test cases for special methods to return concrete values, aligning with the modernized query behavior. |
| python/ql/src/change-notes/2025-03-20-modernize-special-method-wrong-signature-query.md | Documented the refactoring and reduced false positives in the query. |
Files not reviewed (2)
- python/ql/src/Functions/SignatureSpecialMethods.ql: Language not supported
- python/ql/test/query-tests/Functions/general/SignatureSpecialMethods.expected: Language not supported
Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more
joefarebrother
approved these changes
Mar 21, 2025
Contributor
joefarebrother
left a comment
joefarebrother
left a comment
There was a problem hiding this comment.
Looks good 👍
Not sure about keeping around the unused_default handling in a disabled form; personally I would remove it but that's up to you.
Contributor
Author
Yeah, I'm still not entirely sure. I think I'll keep it around for now. Removing it later on should be trivial. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(Best reviewed commit-by-commit.)
Refactors the
py/special-method-wrong-signaturequery to no longer rely on the points-to API.Additionally, a few changes were made to reduce the number of false positives: