Modernize attribute shadows subclass, Add cases for properties

python/ql/src/Classes/SubclassShadowing.ql

@@ -17,31 +17,53 @@
  *   defined in a super-class
  */
 
-/* Need to find attributes defined in superclass (only in __init__?) */
 import python
+import semmle.python.ApiGraphs
+import semmle.python.dataflow.new.internal.DataFlowDispatch
 
-predicate shadowed_by_super_class(
+predicate isSettableProperty(Function prop) {
-  ClassObject c, ClassObject supercls, Assign assign, FunctionObject f
+  isProperty(prop) and
-) {
+  exists(Function setter, DataFlow::AttrRead setterRead, FunctionExpr propExpr |
-  c.getASuperType() = supercls and
+    setterRead.asExpr() = setter.getADecorator() and
-  c.declaredAttribute(_) = f and
+    setterRead.getAttributeName() = "setter" and
-  exists(FunctionObject init, Attribute attr |
+    propExpr.getInnerScope() = prop and
-    supercls.declaredAttribute("__init__") = init and
+    DataFlow::exprNode(propExpr).(DataFlow::LocalSourceNode).flowsTo(setterRead.getObject())
-    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
+predicate isProperty(Function prop) {
-where shadowed_by_super_class(c, supercls, assign, shadowed)
+  prop.getADecorator() = API::builtin("property").asSource().asExpr()
-select shadowed.getOrigin(),
+}
-  "Method " + shadowed.getName() + " is shadowed by an $@ in super class '" + supercls.getName() +
+
-    "'.", assign, "attribute"
+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)
+}
+
+from Class cls, Class superclass, DataFlow::AttrWrite write, Function shadowed, string extra
+where
+  shadowedBySuperclass(cls, superclass, write, shadowed) and
+  (
+    if isProperty(shadowed)
+    then
+      not isSettableProperty(shadowed) and
+      extra = " (read-only property may cause an error if written to.)"
+    else extra = ""
+  )
+select shadowed, "This method is shadowed by $@ in superclass $@." + extra, write,
+  "attribute " + write.getAttributeName(), superclass, superclass.getName()

python/ql/test/query-tests/Classes/subclass-shadowing/SubclassShadowing.qlref

@@ -1 +1,2 @@
-Classes/SubclassShadowing.ql
+query: Classes/SubclassShadowing.ql
+postprocess: utils/test/InlineExpectationsTestQuery.ql

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.
+# OK: Allow if superclass also shadows its own method, as this is likely intended.
-#Since the original method must exist for some reason.
+# Example: stdlib JSONEncoder.default uses this pattern.  
-#See JSONEncoder.default for real example
+class Base2:
 
-class Base2(object):
+    def __init__(self, default=None):
+        if default:
+            self.default = default
 
-    def __init__(self, shadowy=None):
+    def default(self):
-        if shadowy:
-            self.shadow = shadowy
-
-    def shadow(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