Skip to content

Python: Modernize the Signature Mismatch query #20217

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

joefarebrother
Copy link
Contributor

Modernizes the py/inheritance/signature-mismatch query to not use pointsTo, and have more specific alert messages.
The results of the similar py/inheritance/incorrect-overriding-signature (which alert for an example of a call that matches the base method signature without matching the overriding method's signature) are also consolidated into the signature-mismatch query.

Copy link
Contributor

github-actions bot commented Aug 19, 2025

QHelp previews:

python/ql/src/Functions/SignatureOverriddenMethod.qhelp

Signature mismatch in overriding method

When the signature of a method of a base class and a method of a subclass that overrides it don't match, a call to the base class method may not be a valid call to the subclass method, and thus raise an exception if an instance of the subclass is passed instead. If following the Liskov Substitution Principle, in which an instance of a subclass should be usable in every context as though it were an instance of the base class, this behavior breaks the principle.

Recommendation

Ensure that the overriding method in the subclass accepts the same parameters as the base method.

Example

In the following example, Base.runsource takes an optional filename argument. However, the overriding method Sub.runsource does not. This means the run function will fail if passed an instance of Sub.

class Base:
    def runsource(self, source, filename="<input>"):
        ...
    
    
class Sub(Base):
    def runsource(self, source): # BAD: Does not match the signature of overridden method.
        ... 

def run(obj: Base):
    obj.runsource("source", filename="foo.txt")

References

@joefarebrother joefarebrother force-pushed the python-qual-signature-mismatch branch from 8641941 to 43aff2c Compare August 19, 2025 12:33
@joefarebrother joefarebrother marked this pull request as ready for review August 19, 2025 12:34
@joefarebrother joefarebrother requested a review from a team as a code owner August 19, 2025 12:34
@Copilot Copilot AI review requested due to automatic review settings August 19, 2025 12:34
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Modernizes the Python signature mismatch query to provide more precise results without using pointsTo and includes more specific alert messages. The deprecated py/inheritance/incorrect-overriding-signature query results are consolidated into the main signature mismatch query.

  • Removes dependency on pointsTo and implements more precise signature analysis logic
  • Adds detailed error messages describing specific signature mismatches
  • Consolidates two separate queries into one for better maintainability

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
shared/util/codeql/util/Option.qll Adds new LocOption and LocOption2 modules for option types with location information
python/ql/src/Functions/SignatureOverriddenMethod.ql Complete rewrite with new signature analysis logic and detailed error messages
python/ql/src/Functions/SignatureOverriddenMethod.qhelp Updates documentation to reflect new query behavior
python/ql/src/Functions/SignatureOverriddenMethod.py Updates example code to match new query approach
python/ql/src/Functions/IncorrectlyOverriddenMethod.ql Marks query as deprecated
python/ql/test/query-tests/Functions/overriding/test.py Adds extensive test cases for new signature analysis
python/ql/test/query-tests/Functions/overriding/SignatureOverriddenMethod.qlref Updates test configuration to use inline expectations
python/ql/test/query-tests/Functions/overriding/SignatureOverriddenMethod.expected Updates expected results for new query output
python/ql/test/query-tests/Functions/general/SignatureOverriddenMethod.expected Updates expected results
python/ql/test/query-tests/Functions/overriding/WrongNameForArgumentInCall.expected Adds new expected result
python/ql/src/change-notes/2025-08-19-signature-mismatch.md Documents the changes

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

exists(string arg |
// TODO: positional-only args not considered
// e.g. `def foo(x, y, /, z):` has x,y as positional only args, should not be considered as possible kw args
// However, this likely does not create FPs, as we require a 'witness' call to generate an alert.
Copy link
Preview

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

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

This TODO comment indicates incomplete handling of positional-only arguments. While the comment mentions it likely won't create false positives, this limitation should be documented more clearly or addressed, as it could affect the accuracy of signature mismatch detection for functions using positional-only parameters."

Suggested change
// However, this likely does not create FPs, as we require a 'witness' call to generate an alert.
// TODO: Limitation: positional-only arguments (introduced in Python 3.8 with the `/` syntax) are not considered here.
// For example, in `def foo(x, y, /, z):`, `x` and `y` are positional-only and should not be considered as possible keyword arguments.
// This may affect the accuracy of signature mismatch detection for functions using positional-only parameters.
// See: https://docs.python.org/3/glossary.html#term-positional-only-parameter
// However, this likely does not create false positives, as we require a 'witness' call to generate an alert.

Copilot uses AI. Check for mistakes.

or
exists(call.getStarArg())
or
exists(call.getKwargs())
) and
Copy link
Preview

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

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

This TODO comment indicates a significant limitation in the callMatchesSignature predicate where keyword arguments that could match positional parameters are not properly handled. This could lead to false negatives in signature mismatch detection and should be addressed or the limitation should be more prominently documented."

Suggested change
) and
/**
* Determines if the call matches the function's signature, accounting for both positional and keyword arguments.
* This version checks that all required parameters are satisfied by either a positional or keyword argument.
*/
predicate callMatchesSignature(Function func, Call call) {
(
// Check if all required positional parameters (excluding those with defaults) are satisfied
forall(int i |
i >= extraSelfArg(func) and
i < func.getParameterCount() and
not func.getParameter(i).hasDefaultValue()
|
// Satisfied if there is a positional argument for this parameter
i - extraSelfArg(func) < call.getPositionalArgumentCount()
or
// Or if there is a keyword argument matching this parameter's name
exists(string name |
name = func.getParameter(i).getName() and
name = call.getANamedArgumentName()
)
or
// Or if there is a star argument (arbitrary positional arguments)
exists(call.getStarArg())
or
// Or if there is a kwargs argument (arbitrary keyword arguments)
exists(call.getKwargs())
)
) and
// Ensure the total number of positional arguments does not exceed the function's maximum

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant