-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Python: Modernize Unexpected Raise In Special Method query #20120
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 Unexpected Raise In Special Method query #20120
Conversation
…e the exception is not a simple identifier.
QHelp previews: python/ql/src/Functions/IncorrectRaiseInSpecialMethod.qhelpNon-standard exception raised in special methodUser-defined classes interact with the Python virtual machine via special methods (also called "magic methods"). For example, for a class to support addition it must implement the Since the virtual machine calls these special methods for common expressions, users of the class will expect these operations to raise standard exceptions. For example, users would expect that the expression Therefore, if a method is unable to perform the expected operation then its response should conform to the standard protocol, described below.
RecommendationIf the method is intended to be abstract, and thus always raise an exception, then declare it so using the ExampleIn the following example, the class A:
def __init__(self, a):
self.a = a
def __add__(self, other):
# BAD: Should return NotImplemented instead of raising
if not isinstance(other,A):
raise TypeError(f"Cannot add A to {other.__class__}")
return A(self.a + other.a)
class B:
def __init__(self, a):
self.a = a
def __add__(self, other):
# GOOD: Returning NotImplemented allows for the operation to fallback to other implementations to allow other classes to support adding to B.
if not isinstance(other,B):
return NotImplemented
return B(self.a + other.a)
In the following example, the class C:
def __getitem__(self, idx):
if self.idx < 0:
# BAD: Should raise a KeyError or IndexError instead.
raise ValueError("Invalid index")
return self.lookup(idx)
In the following example, the class class D:
def __hash__(self):
# BAD: Use `__hash__ = None` instead.
raise NotImplementedError(f"{self.__class__} is unhashable.") References
|
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 the py/unexpected-raise-in-special-method
CodeQL query by replacing the pointsTo
analysis with more modern dataflow techniques, enabling detection of conditionally raised exceptions, and reducing false positives. The precision has been changed from very-high
to high
to reflect the expanded scope.
- Replaces the legacy
pointsTo
analysis with modern API graphs and dataflow analysis - Extends detection to cases where exceptions are raised conditionally, not just always
- Improves the accuracy of exception type detection and reduces false positives
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
python/ql/src/Functions/IncorrectRaiseInSpecialMethod.ql |
Complete rewrite using modern CodeQL libraries and expanded logic for conditional exception detection |
python/ql/test/query-tests/Functions/IncorrectRaiseInSpcialMethod/test.py |
New comprehensive test file with various special method scenarios |
python/ql/src/Functions/examples/IncorrectRaiseInSpecialMethod*.py |
Updated example files demonstrating correct and incorrect patterns |
python/ql/src/Functions/IncorrectRaiseInSpecialMethod.qhelp |
Updated documentation with clearer explanations and new examples |
python/ql/src/change-notes/2025-07-25-unexpected-raise-special-method.md |
Release notes documenting the changes |
Comments suppressed due to low confidence (1)
python/ql/test/query-tests/Functions/IncorrectRaiseInSpcialMethod/test.py:3
- The variable
self.idx
should likely beidx
(the parameter). Usingself.idx
assumes the object has anidx
attribute, which is not defined in this class.
def __add__(self, other): # No alert - Always allow NotImplementedError
@@ -0,0 +1,66 @@ | |||
class A: |
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.
The directory name contains a typo: 'IncorrectRaiseInSpcialMethod' should be 'IncorrectRaiseInSpecialMethod' (missing 'e' in 'Special').
Copilot uses AI. Check for mistakes.
class D: | ||
def __hash__(self): | ||
# BAD: Use `__hash__ = None` instead. | ||
raise NotImplementedError(f"{self.__type__} is unhashable.") |
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.
The attribute __type__
should be __class__
or type(self)
. __type__
is not a standard Python attribute and will raise an AttributeError.
raise NotImplementedError(f"{self.__type__} is unhashable.") | |
raise NotImplementedError(f"{self.__class__} is unhashable.") |
Copilot uses AI. Check for mistakes.
def __add__(self, other): | ||
# BAD: Should return NotImplemented instead of raising | ||
if not isinstance(other,A): | ||
raise TypeError(f"Cannot add A to {other.__type__}") |
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.
The attribute __type__
should be __class__
or type(other)
. __type__
is not a standard Python attribute and will raise an AttributeError.
raise TypeError(f"Cannot add A to {other.__type__}") | |
raise TypeError(f"Cannot add A to {type(other).__name__}") |
Copilot uses AI. Check for mistakes.
Replaces
pointsTo
, enables results for cases in which the exception is not always raised, reduces some FPs.