Merge pull request #20142 from joefarebrother/python-qual-subclass-shadow

Python: Modernise Superclass attribute shadows subclass method query
2025-12-16 16:53:25 +01:00 · 2025-08-28 13:40:26 +01:00
parent 919ed3cff0 45910b9505
commit 7ef2b01119
13 changed files with 174 additions and 108 deletions
--- a/python/ql/integration-tests/query-suite/python-code-quality-extended.qls.expected
+++ b/python/ql/integration-tests/query-suite/python-code-quality-extended.qls.expected
@@ -8,7 +8,7 @@ ql/python/ql/src/Classes/InitCallsSubclass/InitCallsSubclassMethod.ql
 ql/python/ql/src/Classes/MissingCallToDel.ql
 ql/python/ql/src/Classes/MissingCallToInit.ql
 ql/python/ql/src/Classes/MutatingDescriptor.ql
-ql/python/ql/src/Classes/SubclassShadowing.ql
+ql/python/ql/src/Classes/SubclassShadowing/SubclassShadowing.ql
 ql/python/ql/src/Classes/SuperclassDelCalledMultipleTimes.ql
 ql/python/ql/src/Classes/SuperclassInitCalledMultipleTimes.ql
 ql/python/ql/src/Classes/WrongNameForArgumentInClassInstantiation.ql
--- a/python/ql/integration-tests/query-suite/python-code-quality.qls.expected
+++ b/python/ql/integration-tests/query-suite/python-code-quality.qls.expected
@@ -8,7 +8,7 @@ ql/python/ql/src/Classes/InitCallsSubclass/InitCallsSubclassMethod.ql
 ql/python/ql/src/Classes/MissingCallToDel.ql
 ql/python/ql/src/Classes/MissingCallToInit.ql
 ql/python/ql/src/Classes/MutatingDescriptor.ql
-ql/python/ql/src/Classes/SubclassShadowing.ql
+ql/python/ql/src/Classes/SubclassShadowing/SubclassShadowing.ql
 ql/python/ql/src/Classes/SuperclassDelCalledMultipleTimes.ql
 ql/python/ql/src/Classes/SuperclassInitCalledMultipleTimes.ql
 ql/python/ql/src/Classes/WrongNameForArgumentInClassInstantiation.ql
--- a/python/ql/integration-tests/query-suite/python-security-and-quality.qls.expected
+++ b/python/ql/integration-tests/query-suite/python-security-and-quality.qls.expected
@@ -11,7 +11,7 @@ ql/python/ql/src/Classes/MutatingDescriptor.ql
 ql/python/ql/src/Classes/OverwritingAttributeInSuperClass.ql
 ql/python/ql/src/Classes/PropertyInOldStyleClass.ql
 ql/python/ql/src/Classes/SlotsInOldStyleClass.ql
-ql/python/ql/src/Classes/SubclassShadowing.ql
+ql/python/ql/src/Classes/SubclassShadowing/SubclassShadowing.ql
 ql/python/ql/src/Classes/SuperInOldStyleClass.ql
 ql/python/ql/src/Classes/SuperclassDelCalledMultipleTimes.ql
 ql/python/ql/src/Classes/SuperclassInitCalledMultipleTimes.ql
--- a/python/ql/src/Classes/SubclassShadowing.py
+++ b/python/ql/src/Classes/SubclassShadowing.py
@@ -1,17 +0,0 @@
-class Mammal(object):
-
-    def __init__(self, milk = 0):
-        self.milk = milk
-
-
-class Cow(Mammal):
-
-    def __init__(self):
-        Mammal.__init__(self)
-
-    def milk(self):
-        return "Milk"
-
-#Cow().milk() will raise an error as Cow().milk is the 'milk' attribute
-#set in Mammal.__init__, not the 'milk' method defined on Cow.
-
--- a/python/ql/src/Classes/SubclassShadowing.qhelp
+++ b/python/ql/src/Classes/SubclassShadowing.qhelp
@@ -1,27 +0,0 @@
-<!DOCTYPE qhelp PUBLIC
-  "-//Semmle//qhelp//EN"
-  "qhelp.dtd">
-<qhelp>
-<overview>
-<p> Subclass shadowing occurs when an instance attribute of a superclass has the
-the same name as a method of a subclass, or vice-versa.
-The semantics of Python attribute look-up mean that the instance attribute of
-the superclass hides the method in the subclass.
-</p>
-
-</overview>
-<recommendation>
-
-<p>Rename the method in the subclass or rename the attribute in the superclass.</p>
-
-</recommendation>
-<example>
-<p>The following code includes an example of subclass shadowing. When you call <code>Cow().milk()</code> 
-an error is raised because <code>Cow().milk</code> is interpreted as the 'milk' attribute set in 
-<code>Mammal.__init__</code>, not the 'milk' method defined within <code>Cow</code>. This can be fixed 
-by changing the name of either the 'milk' attribute or the 'milk' method.</p>
-
-<sample src="SubclassShadowing.py" />
-
-</example>
-</qhelp>
--- a/python/ql/src/Classes/SubclassShadowing.ql
+++ b/python/ql/src/Classes/SubclassShadowing.ql
@@ -1,47 +0,0 @@
-/**
- * @name Superclass attribute shadows subclass method
- * @description Defining an attribute in a superclass method with a name that matches a subclass
- *              method, hides the method in the subclass.
- * @kind problem
- * @problem.severity error
- * @tags quality
- *       reliability
- *       correctness
- * @sub-severity low
- * @precision high
- * @id py/attribute-shadows-method
- */
-
-/*
- * Determine if a class defines a method that is shadowed by an attribute
- *   defined in a super-class
- */
-
-/* Need to find attributes defined in superclass (only in __init__?) */
-import python
-
-predicate shadowed_by_super_class(
-  ClassObject c, ClassObject supercls, Assign assign, FunctionObject f
-) {
-  c.getASuperType() = supercls and
-  c.declaredAttribute(_) = f and
-  exists(FunctionObject init, Attribute attr |
-    supercls.declaredAttribute("__init__") = init and
-    attr = assign.getATarget() and
-    attr.getObject().(Name).getId() = "self" and
-    attr.getName() = f.getName() and
-    assign.getScope() = init.getOrigin().(FunctionExpr).getInnerScope()
-  ) and
-  /*
-   * It's OK if the super class defines the method as well.
-   * We assume that the original method must have been defined for a reason.
-   */
-
-  not supercls.hasAttribute(f.getName())
-}
-
-from ClassObject c, ClassObject supercls, Assign assign, FunctionObject shadowed
-where shadowed_by_super_class(c, supercls, assign, shadowed)
-select shadowed.getOrigin(),
-  "Method " + shadowed.getName() + " is shadowed by an $@ in super class '" + supercls.getName() +
-    "'.", assign, "attribute"
--- a/python/ql/src/Classes/SubclassShadowing/SubclassShadowing.qhelp
+++ b/python/ql/src/Classes/SubclassShadowing/SubclassShadowing.qhelp
@@ -0,0 +1,39 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+When an object has an attribute that shares its name with a method on the object's class (or another class attribute), the instance attribute is 
+prioritized during attribute lookup, shadowing the method.  
+
+If a method on a subclass is shadowed by an attribute on a superclass in this way, this may lead to unexpected results or errors, as this
+shadowing behavior is nonlocal and may be unintended.
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+Ensure method names on subclasses don't conflict with attribute names on superclasses, and rename one.  
+If the shadowing behavior is intended, ensure this is explicit in the superclass.
+</p>
+
+</recommendation>
+<example>
+<p>
+In the following example, the <code>_foo</code> attribute of class <code>A</code> shadows the method <code>_foo</code> of class <code>B</code>. 
+Calls to <code>B()._foo()</code> will result in a <code>TypeError</code>, as <code>3</code> will be called instead. 
+</p>
+
+<sample src="examples/SubclassShadowingBad.py" />
+
+<p>
+In the following example, the behavior of the <code>default</code> attribute being shadowed to allow for customization during initialization is
+intended in within the superclass <code>A</code>. Overriding <code>default</code> in the subclass <code>B</code> is then OK.
+</p>
+
+<sample src="examples/SubclassShadowingGood.py" />
+
+</example>
+</qhelp>
--- a/python/ql/src/Classes/SubclassShadowing/SubclassShadowing.ql
+++ b/python/ql/src/Classes/SubclassShadowing/SubclassShadowing.ql
@@ -0,0 +1,71 @@
+/**
+ * @name Superclass attribute shadows subclass method
+ * @description Defining an attribute in a superclass method with a name that matches a subclass
+ *              method, hides the method in the subclass.
+ * @kind problem
+ * @problem.severity error
+ * @tags quality
+ *       reliability
+ *       correctness
+ * @sub-severity low
+ * @precision high
+ * @id py/attribute-shadows-method
+ */
+
+import python
+import semmle.python.ApiGraphs
+import semmle.python.dataflow.new.internal.DataFlowDispatch
+
+predicate isSettableProperty(Function prop) {
+  isProperty(prop) and
+  exists(Function setter |
+    setter.getScope() = prop.getScope() and
+    setter.getName() = prop.getName() and
+    isSetter(setter)
+  )
+}
+
+predicate isSetter(Function f) {
+  exists(DataFlow::AttrRead attr |
+    f.getADecorator() = attr.asExpr() and
+    attr.getAttributeName() = "setter"
+  )
+}
+
+predicate isProperty(Function prop) {
+  prop.getADecorator() = API::builtin("property").asSource().asExpr()
+}
+
+predicate shadowedBySuperclass(
+  Class cls, Class superclass, DataFlow::AttrWrite write, Function shadowed
+) {
+  getADirectSuperclass+(cls) = superclass and
+  shadowed = cls.getAMethod() and
+  exists(Function init |
+    init = superclass.getInitMethod() and
+    DataFlow::parameterNode(init.getArg(0)).(DataFlow::LocalSourceNode).flowsTo(write.getObject()) and
+    write.getAttributeName() = shadowed.getName()
+  ) and
+  // Allow cases in which the super class defines the method as well.
+  // We assume that the original method must have been defined for a reason.
+  not exists(Function superShadowed |
+    superShadowed = superclass.getAMethod() and
+    superShadowed.getName() = shadowed.getName()
+  ) and
+  // Allow properties if they have setters, as the write in the superclass will call the setter.
+  not isSettableProperty(shadowed) and
+  not isSetter(shadowed)
+}
+
+from Class cls, Class superclass, DataFlow::AttrWrite write, Function shadowed, string extra
+where
+  shadowedBySuperclass(cls, superclass, write, shadowed) and
+  (
+    if isProperty(shadowed)
+    then
+      // it's not a setter, so it's a read-only property
+      extra = " (read-only property may cause an error if written to in the superclass)"
+    else extra = ""
+  )
+select shadowed, "This method is shadowed by $@ in superclass $@." + extra, write,
+  "attribute " + write.getAttributeName(), superclass, superclass.getName()
--- a/python/ql/src/Classes/SubclassShadowing/examples/SubclassShadowingBad.py
+++ b/python/ql/src/Classes/SubclassShadowing/examples/SubclassShadowingBad.py
@@ -0,0 +1,9 @@
+class A:
+    def __init__(self):
+        self._foo = 3
+
+class B(A):
+    # BAD: _foo is shadowed by attribute A._foo
+    def _foo(self):
+        return 2
+
--- a/python/ql/src/Classes/SubclassShadowing/examples/SubclassShadowingGood.py
+++ b/python/ql/src/Classes/SubclassShadowing/examples/SubclassShadowingGood.py
@@ -0,0 +1,15 @@
+class A:
+    def __init__(self, default_func=None):
+        if default_func is not None:
+            self.default = default_func 
+
+    # GOOD: The shadowing behavior is explicitly intended in the superclass.
+    def default(self):
+        return []
+    
+class B(A):
+    
+    # Subclasses may override the method `default`, which will still be shadowed by the attribute `default` if it is set.
+    # As this is part of the expected behavior of the superclass, this is fine. 
+    def default(self):
+        return {}
--- a/python/ql/test/query-tests/Classes/subclass-shadowing/SubclassShadowing.expected
+++ b/python/ql/test/query-tests/Classes/subclass-shadowing/SubclassShadowing.expected
@@ -1 +1,2 @@
-| subclass_shadowing.py:10:5:10:21 | FunctionExpr | Method shadow is shadowed by an $@ in super class 'Base'. | subclass_shadowing.py:6:9:6:23 | AssignStmt | attribute |
+| subclass_shadowing.py:11:5:11:21 | Function shadow | This method is shadowed by $@ in superclass $@. | subclass_shadowing.py:7:9:7:19 | ControlFlowNode for Attribute | attribute shadow | subclass_shadowing.py:4:1:4:11 | Class Base | Base |
+| subclass_shadowing.py:41:5:41:18 | Function foo | This method is shadowed by $@ in superclass $@. (read-only property may cause an error if written to in the superclass) | subclass_shadowing.py:35:9:35:16 | ControlFlowNode for Attribute | attribute foo | subclass_shadowing.py:33:1:33:12 | Class Base3 | Base3 |
--- a/python/ql/test/query-tests/Classes/subclass-shadowing/SubclassShadowing.qlref
+++ b/python/ql/test/query-tests/Classes/subclass-shadowing/SubclassShadowing.qlref
@@ -1 +1,2 @@
-Classes/SubclassShadowing.ql
+query: Classes/SubclassShadowing/SubclassShadowing.ql
+postprocess: utils/test/InlineExpectationsTestQuery.ql
--- a/python/ql/test/query-tests/Classes/subclass-shadowing/subclass_shadowing.py
+++ b/python/ql/test/query-tests/Classes/subclass-shadowing/subclass_shadowing.py
@@ -1,30 +1,51 @@
 #Subclass shadowing

-class Base(object):
+# BAD: `shadow` method shadows attribute
+class Base:

    def __init__(self):
        self.shadow = 4

 class Derived(Base):

-    def shadow(self):
+    def shadow(self): # $ Alert 
        pass
    
    
-#OK if the super class defines the method as well.
-#Since the original method must exist for some reason.
-#See JSONEncoder.default for real example
+# OK: Allow if superclass also shadows its own method, as this is likely intended.
+# Example: stdlib JSONEncoder.default uses this pattern.  
+class Base2:

-class Base2(object):
+    def __init__(self, default=None):
+        if default:
+            self.default = default

-    def __init__(self, shadowy=None):
-        if shadowy:
-            self.shadow = shadowy
-
-    def shadow(self):
+    def default(self):
        pass

 class Derived2(Base2):

-    def shadow(self):
+    def default(self): # No alert
        return 0
+
+# Properties 
+
+class Base3:
+    def __init__(self):
+        self.foo = 1 
+        self.bar = 2 
+
+class Derived3(Base3):
+    # BAD: Write to foo in superclass init raises an error.
+    @property 
+    def foo(self): # $ Alert
+        return 2 
+    
+    # OK: This property has a setter, so the write is OK.
+    @property 
+    def bar(self): # No alert
+        return self._bar 
+    
+    @bar.setter 
+    def bar(self, val):
+        self._bar = val