Merge pull request #22909 from hvitved/unified/type-inference-resolve-accessors

Unified: Resolve accessor accesses in type inference

Author: hvitved

shared/typeinference/codeql/typeinference/internal/TypeInference.qll

@@ -3683,6 +3683,16 @@ module Make1<LocationSig Location, InputSig1<Location> Input1> {
       pragma[nomagic]
       Type inferType(AstNode n) { result = inferType(n, TypePath::nil()) }
 
+      /** Gets the unique non-pseudo type inferred at `path` for `n`, if any. */
+      Type inferTypeUnique(AstNode n, TypePath path) {
+        result = unique(Type t | t = inferType(n, path) and not t instanceof PseudoType | t)
+      }
+
+      /**
+       * Gets the unique non-pseudo type inferred at the root for `n`, if any.
+       */
+      Type inferTypeUnique(AstNode n) { result = inferTypeUnique(n, TypePath::nil()) }
+
       /**
        * The cached stage of this module.
        *

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

@@ -7,17 +7,23 @@ private import AllDataFlow
 private import codeql.unified.internal.ExprPositions
 private import codeql.unified.internal.typeinference.TypeInference as T
 
+private predicate isReferenceType(T::Type t) {
+  // todo: type parameters with type constraints
+  t.asClass().hasModifier(["actor", "class"])
+}
+
 private class SwiftDataFlowPlugin extends DataFlowPlugin {
   // Note: For now we assume all code is Swift, but in the future we must restrict these rules to Swift-files
   override predicate bypassContentSet(ContentSet contents) {
     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.
+    exists(boolean direct | isInWriteBackPosition(expr, direct) |
+      if direct = false then not isReferenceType(T::inferTypeUnique(expr)) else any()
+    )
   }
 
   override predicate exprStep(Expr expr1, Expr expr2, boolean mustFlow) {
@@ -100,25 +106,29 @@ private class SwiftDataFlowPlugin extends DataFlowPlugin {
  * Holds if `expr` is target of an assignment or 'inout' argument,
  * or the base of such an expression, e.g. `x` in `x.f1.f2 = foo()`
  */
-private predicate isInWriteBackPosition(Expr expr) {
-  isInAssignmentContext(expr, _)
+private predicate isInWriteBackPosition(Expr expr, boolean direct) {
+  isInAssignmentContext(expr, _) and
+  direct = false
   or
   // Arguments to an 'inout' parameter must be prefixed with '&'
   exists(UnaryExpr unary |
     unary.getOperator().(PrefixOperator).getValue() = "&" and
     unary.getOperand() = expr
     // TODO: support custom operators with an 'inout' parameter, which lack the '&' indicator
-  )
+  ) and
+  direct = true
   or
   // The receiver of a call to a 'mutating' function counts as an 'inout' argument
   exists(CallExpr call, Callable target |
     target = T::resolveCallTarget(call) and
     target.hasModifier("mutating") and
     expr = call.getCallee().(MemberAccessExpr).getBase()
-  )
+  ) and
+  direct = true
   or
   exists(MemberAccessExpr member |
-    isInWriteBackPosition(member) and
-    expr = member.getBase()
+    isInWriteBackPosition(member, _) and
+    expr = member.getBase() and
+    direct = false
   )
 }

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) {
@@ -820,6 +856,10 @@ predicate inferType = M3::inferType/1;
 
 predicate inferType = M3::inferType/2;
 
+predicate inferTypeUnique = M3::inferTypeUnique/1;
+
+predicate inferTypeUnique = M3::inferTypeUnique/2;
+
 predicate inferTypeCertain = M3::inferTypeCertain/2;
 
 module Consistency = M3::Consistency;
@@ -835,7 +875,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,18 @@ func t11() {
     class C {
         var p: (String, String) {
             get { ("safe", "safe") }
-            set { sink(newValue.0) }  // $ hasValueFlow=t11.1
+            set { sink(newValue.0) }  // $ hasValueFlow=t11.1 hasValueFlow=t11.2
+        }
+        var q: C {
+            get { C() }
+            set { sink(newValue.p.0) }
         }
     }
     let c = C()
+    // `p` has value type, so this calls `p`'s setter
     c.p.0 = source("t11.1")
+    // `q` has reference type, so this calls `q`'s getter followed by `p`'s setter
+    c.q.p = (source("t11.2"), source("t11.3"))
 }
 
 func t12() {
@@ -146,9 +153,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()
 }