Merge pull request #6217 from hvitved/csharp/dataflow/csv-override-fix

C#: Fix CSV overrides logic

Author: hvitved

csharp/ql/src/semmle/code/csharp/dataflow/ExternalFlow.qll

@@ -325,33 +325,49 @@ private predicate elementSpec(
   summaryModel(namespace, type, subtypes, name, signature, ext, _, _, _)
 }
 
+private predicate elementSpec(
+  string namespace, string type, boolean subtypes, string name, string signature, string ext,
+  UnboundValueOrRefType t
+) {
+  elementSpec(namespace, type, subtypes, name, signature, ext) and
+  t.hasQualifiedName(namespace, type)
+}
+
 private class UnboundValueOrRefType extends ValueOrRefType {
   UnboundValueOrRefType() { this.isUnboundDeclaration() }
 
-  UnboundValueOrRefType getASubTypeUnbound() { result = this.getASubType().getUnboundDeclaration() }
+  UnboundValueOrRefType getASubTypeUnbound() {
+    exists(Type t |
+      result.getABaseType() = t and
+      this = t.getUnboundDeclaration()
+    )
+  }
 }
 
-bindingset[namespace, type, subtypes]
-private UnboundValueOrRefType interpretType(string namespace, string type, boolean subtypes) {
-  exists(UnboundValueOrRefType t |
-    t.hasQualifiedName(namespace, type) and
-    if subtypes = true then result = t.getASubTypeUnbound*() else result = t
-  )
-}
+private class UnboundCallable extends Callable, Virtualizable {
+  UnboundCallable() { this.isUnboundDeclaration() }
 
-private Member interpretMember(
-  string namespace, string type, boolean subtypes, string name, string signature
-) {
-  elementSpec(namespace, type, subtypes, name, signature, _) and
-  exists(UnboundValueOrRefType t |
-    t = interpretType(namespace, type, subtypes) and
-    result.getDeclaringType() = t and
-    result.hasName(name)
-  )
+  predicate overridesOrImplementsUnbound(UnboundCallable that) {
+    exists(Callable c |
+      this.overridesOrImplementsOrEquals(c) and
+      this != c and
+      that = c.getUnboundDeclaration()
+    )
+  }
 }
 
 private class InterpretedCallable extends Callable {
-  InterpretedCallable() { this = interpretMember(_, _, _, _, _) }
+  InterpretedCallable() {
+    exists(UnboundValueOrRefType t, boolean subtypes, string name |
+      elementSpec(_, _, subtypes, name, _, _, t) and
+      this.hasName(name)
+    |
+      this.getDeclaringType() = t
+      or
+      subtypes = true and
+      this.getDeclaringType() = t.getASubTypeUnbound+()
+    )
+  }
 }
 
 private string paramsStringPartA(InterpretedCallable c, int i) {
@@ -388,10 +404,13 @@ private string paramsString(InterpretedCallable c) {
 private Element interpretElement0(
   string namespace, string type, boolean subtypes, string name, string signature
 ) {
-  elementSpec(namespace, type, subtypes, name, signature, _) and
-  exists(UnboundValueOrRefType t | t = interpretType(namespace, type, subtypes) |
+  exists(UnboundValueOrRefType t | elementSpec(namespace, type, subtypes, name, signature, _, t) |
     exists(Member m |
-      result = m and
+      (
+        result = m
+        or
+        subtypes = true and result.(UnboundCallable).overridesOrImplementsUnbound(m)
+      ) and
       m.getDeclaringType() = t and
       m.hasName(name)
     |
@@ -400,6 +419,12 @@ private Element interpretElement0(
       paramsString(m) = signature
     )
     or
+    (
+      result = t
+      or
+      subtypes = true and
+      result = t.(UnboundValueOrRefType).getASubTypeUnbound+()
+    ) and
     result = t and
     name = "" and
     signature = ""

csharp/ql/test/library-tests/dataflow/external-models/Steps.cs

@@ -40,6 +40,8 @@ void Foo()
             var gen = new Generic<int>();
             gen.StepGeneric(0);
             gen.StepGeneric2(false);
+
+            new Sub().StepOverride("string");
         }
 
         object StepArgRes(object x) { return null; }
@@ -74,5 +76,15 @@ class Generic<T>
 
             public T StepGeneric2<S>(S s) => throw null;
         }
+
+        class Base<T>
+        {
+            public virtual T StepOverride(T t) => throw null;
+        }
+
+        class Sub : Base<string>
+        {
+            public override string StepOverride(string i) => throw null;
+        }
     }
 }

csharp/ql/test/library-tests/dataflow/external-models/steps.expected

@@ -10,11 +10,12 @@ summaryThroughStep
 | Steps.cs:26:13:26:31 | this access | Steps.cs:26:25:26:30 | [post] access to local variable argOut | false |
 | Steps.cs:41:29:41:29 | 0 | Steps.cs:41:13:41:30 | call to method StepGeneric | true |
 | Steps.cs:42:30:42:34 | false | Steps.cs:42:13:42:35 | call to method StepGeneric2 | true |
+| Steps.cs:44:36:44:43 | "string" | Steps.cs:44:13:44:44 | call to method StepOverride | true |
 summaryGetterStep
-| Steps.cs:28:13:28:16 | this access | Steps.cs:28:13:28:34 | call to method StepFieldGetter | Steps.cs:55:13:55:17 | field Field |
-| Steps.cs:32:13:32:16 | this access | Steps.cs:32:13:32:37 | call to method StepPropertyGetter | Steps.cs:61:13:61:20 | property Property |
+| Steps.cs:28:13:28:16 | this access | Steps.cs:28:13:28:34 | call to method StepFieldGetter | Steps.cs:57:13:57:17 | field Field |
+| Steps.cs:32:13:32:16 | this access | Steps.cs:32:13:32:37 | call to method StepPropertyGetter | Steps.cs:63:13:63:20 | property Property |
 | Steps.cs:36:13:36:16 | this access | Steps.cs:36:13:36:36 | call to method StepElementGetter | file://:0:0:0:0 | element |
 summarySetterStep
-| Steps.cs:30:34:30:34 | 0 | Steps.cs:30:13:30:16 | [post] this access | Steps.cs:55:13:55:17 | field Field |
-| Steps.cs:34:37:34:37 | 0 | Steps.cs:34:13:34:16 | [post] this access | Steps.cs:61:13:61:20 | property Property |
+| Steps.cs:30:34:30:34 | 0 | Steps.cs:30:13:30:16 | [post] this access | Steps.cs:57:13:57:17 | field Field |
+| Steps.cs:34:37:34:37 | 0 | Steps.cs:34:13:34:16 | [post] this access | Steps.cs:63:13:63:20 | property Property |
 | Steps.cs:38:36:38:36 | 0 | Steps.cs:38:13:38:16 | [post] this access | file://:0:0:0:0 | element |

csharp/ql/test/library-tests/dataflow/external-models/steps.ql

@@ -21,7 +21,8 @@ class SummaryModelTest extends SummaryModelCsv {
         "My.Qltest;C;false;StepElementGetter;();;Element of Argument[-1];ReturnValue;value",
         "My.Qltest;C;false;StepElementSetter;(System.Int32);;Argument[0];Element of Argument[-1];value",
         "My.Qltest;C+Generic<>;false;StepGeneric;(T);;Argument[0];ReturnValue;value",
-        "My.Qltest;C+Generic<>;false;StepGeneric2;(S);;Argument[0];ReturnValue;value"
+        "My.Qltest;C+Generic<>;false;StepGeneric2;(S);;Argument[0];ReturnValue;value",
+        "My.Qltest;C+Base<>;true;StepOverride;(T);;Argument[0];ReturnValue;value"
       ]
   }
 }