Unified: Resolve accessor accesses in type inference

Author: hvitved

unified/ql/lib/codeql/unified/internal/NameBindingPluginSwift.qll

@@ -88,10 +88,16 @@ class NameBindingPluginSwift extends NameBindingPlugin {
     or
     not m instanceof GeneratedConstructor and
     result =
-      concat(int i, Parameter p |
-        p = m.(Callable).getParameter(i)
+      concat(int i, Parameter p, string name |
+        p = m.(Callable).getParameter(i) and
+        (
+          name = p.getExternalNameNode().getValue()
+          or
+          not exists(p.getExternalNameNode()) and
+          name = "_"
+        )
       |
-        p.getExternalNameNode().getValue(), "," order by i
+        name, "," order by i
       )
   }
 

unified/ql/lib/codeql/unified/internal/dataflow/CallGraph.qll

@@ -7,26 +7,9 @@ private import codeql.unified.internal.ExprPositions
 DataFlowCallable viableCallable(DataFlowCall c) {
   result.asSourceCallable() = T::resolveCallTarget(c.asExplicitCall())
   or
-  result.asSourceCallable() = resolveAccessor(c.asGetterCall()).(GetterDeclaration)
+  result.asSourceCallable() = T::resolveAccessorAccess(c.asGetterCall()).(GetterDeclaration)
   or
-  result.asSourceCallable() = resolveAccessor(c.asSetterCall()).(SetterDeclaration)
+  result.asSourceCallable() = T::resolveAccessorAccess(c.asSetterCall()).(SetterDeclaration)
   or
   result.asSummarizedCallable().matchesCall(c.asExplicitCall())
 }
-
-bindingset[access]
-pragma[inline_late]
-private N::NamespaceNode resolveBase(MemberAccessExpr access) {
-  result.isInstanceMemberNamespace(T::inferType(access.getBase()).asClass())
-}
-
-private AccessorDeclaration resolveAccessor(Expr access) {
-  result.getNameNode() = N::getStaticBindingTargetFromRef(access)
-  or
-  not exists(N::getStaticBindingTargetFromRef(access)) and
-  exists(MemberAccessExpr m |
-    access = m and
-    // TODO: Make type inference own accessor-resolution. It also needs it internally.
-    resolveBase(m).getMember(m.getMemberName()).isMember(result)
-  )
-}

unified/ql/lib/codeql/unified/internal/dataflow/DataFlowCall.qll

@@ -2,19 +2,15 @@ private import unified
 private import AllDataFlow
 private import codeql.unified.internal.ExprPositions
 private import codeql.unified.internal.NameBinding as N
+private import codeql.unified.internal.typeinference.TypeInference as T
 
 private newtype TDataFlowCall =
   TExplicitCall(CallExpr call) {
     not isInBindingContext(call, _) // ignore constructor patterns
   } or
   TGetterCall(Expr expr) {
     hasResultValue(expr) and
-    (
-      N::getStaticBindingTargetFromRef(expr) = any(GetterDeclaration g).getNameNode()
-      or
-      // Assume a qualified member accesses might resolve to a getter.
-      expr instanceof MemberAccessExpr
-    )
+    T::resolveAccessorAccess(expr) instanceof GetterDeclaration
   } or
   TSetterCall(Expr expr) {
     (
@@ -24,12 +20,7 @@ private newtype TDataFlowCall =
       hasPostUpdateAfterCallOrAssign(expr, _) and
       mayWriteBackViaSetterCall(expr)
     ) and
-    (
-      N::getStaticBindingTargetFromRef(expr) = any(SetterDeclaration g).getNameNode()
-      or
-      // Assume a qualified member accesses might resolve to a setter.
-      expr instanceof MemberAccessExpr
-    )
+    T::resolveAccessorAccess(expr) instanceof SetterDeclaration
   } or
   TSummaryCall(SummarizedCallable callable, SummaryNode callee) {
     FlowSummaryOutput::summaryCallbackRange(callable, callee)

unified/ql/lib/codeql/unified/internal/dataflow/DataFlowPluginSwift.qll

@@ -13,11 +13,12 @@ private class SwiftDataFlowPlugin extends DataFlowPlugin {
     contents.asSingleton().asNamedMember() = "some.0"
   }
 
-  // Swift writes back mutated values of computed properties via their setter
+  // Swift writes back mutated values of computed value-type properties via their setter
   bindingset[expr]
   override predicate mayWriteBackViaSetterCall(Expr expr) {
-    isInWriteBackPosition(expr)
-    // TODO: Check that 'expr' has a value type. Reference types don't cause writebacks.
+    isInWriteBackPosition(expr) and
+    // conservative value-type check
+    not forex(T::Type t | t = T::inferType(expr) | t.asClass().hasModifier("class"))
   }
 
   override predicate exprStep(Expr expr1, Expr expr2, boolean mustFlow) {

unified/ql/lib/codeql/unified/internal/typeinference/TypeInference.qll

@@ -123,6 +123,8 @@ module Input3 implements InputSig3 {
   predicate cacheRevRef() {
     (exists(resolveFieldAccess(_)) implies any())
     or
+    (exists(resolveAccessorAccess(_)) implies any())
+    or
     (exists(resolveCallTarget(_)) implies any())
   }
 
@@ -252,20 +254,38 @@ module Input3 implements InputSig3 {
     override Location getLocation() { result = decl.getLocation() }
   }
 
-  class Field extends AstVariableDeclaration {
+  final class Field = FieldImpl;
+
+  abstract private class FieldImpl extends DeclarationImpl {
+    abstract AstNode getAstNode();
+  }
+
+  private class VariableDeclarationField extends FieldImpl, AstVariableDeclaration {
     ClassLikeDeclaration cls;
 
-    Field() { decl = cls.getAMember() }
+    VariableDeclarationField() { decl = cls.getAMember() }
+
+    override AstNode getAstNode() { result = this.getDecl() }
 
     override TypeMention getDeclaringType() { result = getClassLikeDeclarationTypeMention(cls) }
   }
 
-  private class EnumField_ extends Field {
+  private class EnumField_ extends VariableDeclarationField {
     override EnumField decl;
 
     override TypeMention getType() { result = decl.getEnum().getNameNode() }
   }
 
+  private class AccessorField extends FieldImpl, Callable {
+    private AccessorDeclaration accessor;
+
+    AccessorField() { accessor = this.getCallable() }
+
+    override TypeMention getType() { result = accessor.getReturnType() }
+
+    override AstNode getAstNode() { result = accessor }
+  }
+
   abstract class FieldAccess extends Expr {
     abstract Expr getReceiver();
 
@@ -275,7 +295,7 @@ module Input3 implements InputSig3 {
   pragma[nomagic]
   private predicate lookupMember0(MemberAccessExpr mae, ClassLikeDeclaration cls, string name) {
     not exists(getStaticBindingTargetFromRef(mae)) and // if a static member can be resolved, use it
-    cls = inferType(mae.getBase()).(ClassLikeDeclarationType).getClassLikeDeclaration() and
+    cls = inferType(mae.getBase()).asClass() and
     name = mae.getMemberName()
   }
 
@@ -303,7 +323,7 @@ module Input3 implements InputSig3 {
       // also mutual recursion
       exists(TupleType tuple, string name |
         this.isTupleProjection(tuple, name) and
-        result.getDecl() = tuple.getField(name)
+        result.getAstNode() = tuple.getField(name)
       )
       or
       // no mutual recursion; can be resolved directly with static name binding
@@ -325,6 +345,18 @@ module Input3 implements InputSig3 {
     override Field getField() { result.getNameNode() = this.getTarget() }
   }
 
+  private class LocalAccessorFieldAccess extends FieldAccess, LocalNameAccess {
+    AccessorDeclaration target;
+
+    LocalAccessorFieldAccess() {
+      target = getStaticBindingTargetFromIdentifier(this).getDeclaration()
+    }
+
+    override Expr getReceiver() { none() }
+
+    override Field getField() { result.getAstNode() = target }
+  }
+
   Type inferFieldAccessReceiverType(FieldAccess fa, TypePath path) {
     // instance field with explicit receiver
     result = inferType(fa.getReceiver(), path)
@@ -506,17 +538,21 @@ module Input3 implements InputSig3 {
     }
 
     Callable getTargetImpl(boolean isFunctionExprInvoke) {
-      // mutual recursion; call with explicit receiver: `obj.foo(42)`
-      result = lookupInstanceMember(this.getCallee()) and
-      isFunctionExprInvoke = false
-      or
-      // mutual recursion; call to a function expression: `callback(42)`
-      result.asCallable() = this.getInvokeTarget(_) and
-      isFunctionExprInvoke = true
-      or
-      // no mutual recursion; can be resolved directly with static name binding
-      result = this.getTargetViaStaticNameBinding() and
-      isFunctionExprInvoke = false
+      (
+        // mutual recursion; call with explicit receiver: `obj.foo(42)`
+        result = lookupInstanceMember(this.getCallee()) and
+        isFunctionExprInvoke = false
+        or
+        // mutual recursion; call to a function expression: `callback(42)`
+        result.asCallable() = this.getInvokeTarget(_) and
+        isFunctionExprInvoke = true
+        or
+        // no mutual recursion; can be resolved directly with static name binding
+        result = this.getTargetViaStaticNameBinding() and
+        isFunctionExprInvoke = false
+      ) and
+      // handled as a field access
+      not result.asCallable() instanceof AccessorDeclaration
     }
 
     Callable getTarget(InvocationResolutionContext c) {
@@ -835,7 +871,23 @@ module CachedStage = M3::CachedStage;
 cached
 VariableDeclaration resolveFieldAccess(Expr access) {
   CachedStage::ref() and
-  result = access.(Input3::FieldAccess).getField().getDecl()
+  result = access.(Input3::FieldAccess).getField().getAstNode()
+}
+
+/**
+ * Gets the accessor that is accessed by the given expression.
+ *
+ * Note that we do not attempt to distinguish between different kinds of
+ * accessors; all accessors belonging to the same declaration will be
+ * returned (for example both the getter and setter of a property).
+ *
+ * `access` is either a `MemberAccessExpr` or an `Identifier` (when
+ * the field is accessed with an implicit receiver).
+ */
+cached
+AccessorDeclaration resolveAccessorAccess(Expr access) {
+  CachedStage::ref() and
+  result = access.(Input3::FieldAccess).getField().getAstNode()
 }
 
 /**

unified/ql/lib/codeql/unified/internal/typeinference/TypeMention.qll

@@ -24,7 +24,11 @@ abstract private class TypeMentionImpl extends AstNode {
 final class ExprTypeMention = ExprTypeMentionImpl;
 
 abstract private class ExprTypeMentionImpl extends TypeMentionImpl, Expr {
-  ExprTypeMentionImpl() { isInTypeContext(this) and not this instanceof GenericTypeExpr }
+  ExprTypeMentionImpl() {
+    isInTypeContext(this) and
+    not this instanceof GenericTypeExpr and
+    not this instanceof ModifiedTypeExpr
+  }
 
   pragma[nomagic]
   abstract TypePath getTypeArgumentPath(int i);
@@ -191,6 +195,14 @@ private class ClassLikeDeclarationTypeMention extends TypeMentionImpl, Identifie
   }
 }
 
+private class ModifiedTypeExprMention extends TypeMentionImpl instanceof ModifiedTypeExpr {
+  private TypeMention base;
+
+  ModifiedTypeExprMention() { base = ModifiedTypeExpr.super.getType() }
+
+  override Type getTypeAt(TypePath path) { result = base.getTypeAt(path) }
+}
+
 TypeMention getClassLikeDeclarationTypeMention(ClassLikeDeclaration c) {
   result = c.getNameNode()
   or

unified/ql/test/library-tests/dataflow/accessors_instance.swift

@@ -131,11 +131,11 @@ func t11() {
     class C {
         var p: (String, String) {
             get { ("safe", "safe") }
-            set { sink(newValue.0) }  // $ hasValueFlow=t11.1 $ MISSING: hasValueFlow=t11.2
+            set { sink(newValue.0) }  // $ hasValueFlow=t11.1 hasValueFlow=t11.2
         }
         var q: C {
             get { C() }
-            set { sink(newValue.p.0) }  // $ SPURIOUS: hasValueFlow=t11.2
+            set { sink(newValue.p.0) }
         }
     }
     let c = C()
@@ -151,9 +151,9 @@ func t12() {
     class C {
         var s: S {
             get { S() }
-            set { sink(newValue.field) }  // $ MISSING: hasValueFlow=t12.1
+            set { sink(newValue.field) }  // $ hasValueFlow=t12.1
         }
     }
     let c = C()
-    c.s.update()  // MISSING: the call to 'update' is not resolved
+    c.s.update()
 }

unified/ql/test/library-tests/dataflow/accessors_local.swift

@@ -100,9 +100,7 @@ func t12() {
     }
     var p: S {
         get { S() }
-        set { sink(newValue.field) }  // $ MISSING: hasValueFlow=t12.1
+        set { sink(newValue.field) }  // $ hasValueFlow=t12.1
     }
-    // TODO: test fails because the call to 'mutate' is not yet resolved
-    // At the time of writing, the test passes if the call graph is manually patched
     p.mutate()
 }