+
+
+When an object has an attribute that shares the same name 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.
+
+
+
+
+
+
+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.
+
+
+
+
+
+In the following example, the _foo attribute of class A shadows the method _foo of class B.
+Calls to B()._foo() will result in a TypeError, as 3 will be called instead.
+
+
+
+
+
+In the following example, the behavior of the default attribute being shadowed to allow for customization during initialization is
+intended in within the superclass A. Overriding default in the subclass B is then OK.
+
+
+
+
+
+
diff --git a/python/ql/src/Classes/SubclassShadowing/SubclassShadowing.ql b/python/ql/src/Classes/SubclassShadowing/SubclassShadowing.ql
new file mode 100644
index 000000000000..39a320f75ac6
--- /dev/null
+++ 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()
diff --git a/python/ql/src/Classes/SubclassShadowing/examples/SubclassShadowingBad.py b/python/ql/src/Classes/SubclassShadowing/examples/SubclassShadowingBad.py
new file mode 100644
index 000000000000..00a221760b4c
--- /dev/null
+++ 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
+
diff --git a/python/ql/src/Classes/SubclassShadowing/examples/SubclassShadowingGood.py b/python/ql/src/Classes/SubclassShadowing/examples/SubclassShadowingGood.py
new file mode 100644
index 000000000000..8fca041176ca
--- /dev/null
+++ 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 {}
\ No newline at end of file
diff --git a/python/ql/test/query-tests/Classes/subclass-shadowing/SubclassShadowing.expected b/python/ql/test/query-tests/Classes/subclass-shadowing/SubclassShadowing.expected
index caad71a9a31f..5f5513ae9906 100644
--- 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 |
diff --git a/python/ql/test/query-tests/Classes/subclass-shadowing/SubclassShadowing.qlref b/python/ql/test/query-tests/Classes/subclass-shadowing/SubclassShadowing.qlref
index 5fed3f9f8fc6..5205014a3d55 100644
--- 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
\ No newline at end of file
diff --git a/python/ql/test/query-tests/Classes/subclass-shadowing/subclass_shadowing.py b/python/ql/test/query-tests/Classes/subclass-shadowing/subclass_shadowing.py
index 98e7f992e84e..b9fcd975eb33 100644
--- 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
\ No newline at end of file