Content-Length: 445277 | pFad | http://github.com/github/codeql/pull/20120

0F Python: Modernize Unexpected Raise In Special Method query by joefarebrother · Pull Request #20120 · github/codeql · GitHub
Skip to content

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

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

Conversation

joefarebrother
Copy link
Contributor

Replaces pointsTo, enables results for cases in which the exception is not always raised, reduces some FPs.

Copy link
Contributor

github-actions bot commented Jul 24, 2025

QHelp previews:

python/ql/src/Functions/IncorrectRaiseInSpecialMethod.qhelp

Non-standard exception raised in special method

User-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 __add__ and __radd__ special methods. When the expression a + b is evaluated the Python virtual machine will call type(a).__add__(a, b) and if that is not implemented it will call type(b).__radd__(b, a).

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 a.b may raise an AttributeError if the object a does not have an attribute b. If a KeyError were raised instead, then this would be unexpected and may break code that expected an AttributeError, but not a KeyError.

Therefore, if a method is unable to perform the expected operation then its response should conform to the standard protocol, described below.

  • Attribute access, a.b (__getattr__): Raise AttributeError.
  • Arithmetic operations, a + b (__add__): Do not raise an exception, return NotImplemented instead.
  • Indexing, a[b] (__getitem__): Raise KeyError or IndexError.
  • Hashing, hash(a) (__hash__): Should not raise an exception. Use __hash__ = None to indicate that an object is unhashable rather than raising an exception.
  • Equality methods, a == b (__eq__): Never raise an exception, always return True or False.
  • Ordering comparison methods, a < b (__lt__): Raise a TypeError if the objects cannot be ordered.
  • Most others: If the operation is never supported, the method often does not need to be implemented at all; otherwise a TypeError should be raised.

Recommendation

If the method is intended to be abstract, and thus always raise an exception, then declare it so using the @abstractmethod decorator. Otherwise, either remove the method or ensure that the method raises an exception of the correct type.

Example

In the following example, the __add__ method of A raises a TypeError if other is of the wrong type. However, it should return NotImplemented instead of rising an exception, to allow other classes to support adding to A. This is demonstrated in the class B.

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 __getitem__ method of C raises a ValueError, rather than a KeyError or IndexError as expected.

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 __hash__ method of D raises NotImplementedError. This causes D to be incorrectly identified as hashable by isinstance(obj, collections.abc.Hashable); so the correct way to make a class unhashable is to set __hash__ = None.

class D:
    def __hash__(self):
        # BAD: Use `__hash__ = None` instead.
        raise NotImplementedError(f"{self.__class__} is unhashable.")

References

@joefarebrother joefarebrother changed the title [Draft] Python: Modernize Unexpected Raise In Special Method query Python: Modernize Unexpected Raise In Special Method query Jul 25, 2025
@joefarebrother joefarebrother marked this pull request as ready for review July 25, 2025 10:00
@Copilot Copilot AI review requested due to automatic review settings July 25, 2025 10:00
@joefarebrother joefarebrother requested a review from a team as a code owner July 25, 2025 10:00
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

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 be idx (the parameter). Using self.idx assumes the object has an idx attribute, which is not defined in this class.
    def __add__(self, other): # No alert - Always allow NotImplementedError

@@ -0,0 +1,66 @@
class A:
Copy link
Preview

Copilot AI Jul 25, 2025

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.")
Copy link
Preview

Copilot AI Jul 25, 2025

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.

Suggested change
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__}")
Copy link
Preview

Copilot AI Jul 25, 2025

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.

Suggested change
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.

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








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: http://github.com/github/codeql/pull/20120

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy