Merge pull request #1377 from hvitved/csharp/useless-upcast

C#: Teach `cs/useless-upcast` about disambiguating constructor calls

change-notes/1.21/analysis-csharp.md

@@ -6,6 +6,7 @@
 |------------------------------|------------------------|-----------------------------------|
 | Class defines a field that uses an ICryptoTransform class in a way that would be unsafe for concurrent threads (`cs/thread-unsafe-icryptotransform-field-in-class`) | Fewer false positive results | The criteria for a result has changed to include nested properties, nested fields and collections. The format of the alert message has changed to highlight the static field. |
 | Constant condition (`cs/constant-condition`) | Fewer false positive results | Results have been removed where the `null` value is in a conditional expression on the left hand side of a null-coalescing expression. For example, in `(a ? b : null) ?? c`, `null` is not considered to be a constant condition. |
+| Useless upcast (`cs/useless-upcast`) | Fewer false positive results | Results have been removed where the upcast is used to disambiguate the target of a constructor call. |
 
 ## Changes to code extraction
 

csharp/ql/src/Language Abuse/UselessUpcast.ql

@@ -65,6 +65,13 @@ int getMaximumArguments(Callable c) {
   result = c.getNumberOfParameters()
 }
 
+private class ConstructorCall extends Call {
+  ConstructorCall() {
+    this instanceof ObjectCreation or
+    this instanceof ConstructorInitializer
+  }
+}
+
 /** An explicit upcast. */
 class ExplicitUpcast extends ExplicitCast {
   ValueOrRefType src;
@@ -140,6 +147,17 @@ class ExplicitUpcast extends ExplicitCast {
     )
   }
 
+  /** Holds if this upcast may be used to disambiguate the target of a constructor call. */
+  pragma[nomagic]
+  private predicate isDisambiguatingConstructorCall(Constructor other, int args) {
+    exists(ConstructorCall cc, Constructor target, ValueOrRefType t | this.isArgument(cc, target) |
+      t = target.getDeclaringType() and
+      t.hasMember(other) and
+      args = cc.getNumberOfArguments() and
+      other != target
+    )
+  }
+
   /** Holds if this upcast may be used to disambiguate the target of a call. */
   private predicate isDisambiguatingCall() {
     exists(Callable other, int args |
@@ -148,6 +166,8 @@ class ExplicitUpcast extends ExplicitCast {
       this.isDisambiguatingExtensionCall(other, args)
       or
       this.isDisambiguatingStaticCall(other, args)
+      or
+      this.isDisambiguatingConstructorCall(other, args)
     |
       args >= getMinimumArguments(other) and
       not args > getMaximumArguments(other)

csharp/ql/test/query-tests/Language Abuse/UselessUpcast/UselessUpcast.cs

@@ -132,3 +132,36 @@ static class StaticMethods
    public static void M1(B _) { }
    public static void M2(B _) { }
 }
+
+class Constructors : I2
+{
+    Constructors(I1 i) { }
+    Constructors(I2 i) { }
+    Constructors(Constructors c) : this((I1)c) { } // GOOD
+    Constructors(Sub s) : this((I2)s) { } // GOOD
+
+    void M(Constructors c)
+    {
+        new Constructors((I1)c); // GOOD
+        new Constructors((I2)c); // GOOD
+    }
+
+    public int Foo() => 0;
+
+    float I2.Foo() => 0;
+
+    class Sub : Constructors
+    {
+        public Sub(Sub s) : base((I1)s) { } // GOOD
+    }
+
+    class SubSub : Sub
+    {
+        SubSub(SubSub ss) : base((Sub)ss) { } // BAD
+
+        void M(SubSub ss)
+        {
+            new Sub((Sub)ss); // BAD
+        }
+    }
+}

csharp/ql/test/query-tests/Language Abuse/UselessUpcast/UselessUpcast.expected

@@ -4,4 +4,6 @@
 | UselessUpcast.cs:85:12:85:15 | (...) ... | There is no need to upcast from $@ to $@ - the conversion can be done implicitly. | UselessUpcast.cs:20:7:20:7 | B | B | UselessUpcast.cs:13:7:13:7 | A | A |
 | UselessUpcast.cs:94:12:94:15 | (...) ... | There is no need to upcast from $@ to $@ - the conversion can be done implicitly. | UselessUpcast.cs:20:7:20:7 | B | B | UselessUpcast.cs:13:7:13:7 | A | A |
 | UselessUpcast.cs:105:16:105:19 | (...) ... | There is no need to upcast from $@ to $@ - the conversion can be done implicitly. | UselessUpcast.cs:27:7:27:7 | C | C | UselessUpcast.cs:20:7:20:7 | B | B |
+| UselessUpcast.cs:160:34:160:40 | (...) ... | There is no need to upcast from $@ to $@ - the conversion can be done implicitly. | UselessUpcast.cs:158:11:158:16 | SubSub | SubSub | UselessUpcast.cs:153:11:153:13 | Sub | Sub |
+| UselessUpcast.cs:164:21:164:27 | (...) ... | There is no need to upcast from $@ to $@ - the conversion can be done implicitly. | UselessUpcast.cs:158:11:158:16 | SubSub | SubSub | UselessUpcast.cs:153:11:153:13 | Sub | Sub |
 | UselessUpcastBad.cs:9:23:9:32 | (...) ... | There is no need to upcast from $@ to $@ - the conversion can be done implicitly. | UselessUpcastBad.cs:4:11:4:13 | Sub | Sub | UselessUpcastBad.cs:3:11:3:15 | Super | Super |