Merge pull request #4484 from tamasvajk/feature/custom-assert-methods

C#: Add support for custom assert methods (DoesNotReturnIfAttribute)

Author: tamasvajk

csharp/change-notes/2020-10-21-CodeAnalysis-attributes-in-assert.md

@@ -0,0 +1,4 @@
+lgtm,codescanning
+* Assertion methods are now taking into account methods with parameters that are annotated
+with `System.Diagnostics.CodeAnalysis.DoesNotReturnIfAttribute`. This change is expected to
+lead to higher precision in any query that relies on control flow graphs.

csharp/ql/src/semmle/code/csharp/commons/Assertions.qll

@@ -9,11 +9,27 @@ private import ControlFlow::BasicBlocks
 
 /** An assertion method. */
 abstract class AssertMethod extends Method {
-  /** Gets the index of the parameter being asserted. */
-  abstract int getAssertionIndex();
+  /**
+   * DEPRECATED: renamed to `getAnAssertionIndex`.
+   *
+   * Gets the index of a parameter being asserted.
+   */
+  deprecated int getAssertionIndex() { result = getAnAssertionIndex() }
+
+  /** Gets the index of a parameter being asserted. */
+  abstract int getAnAssertionIndex();
+
+  /**
+   * DEPRECATED: renamed to `getAnAssertedParameter`.
+   *
+   * Gets a parameter being asserted.
+   */
+  deprecated Parameter getAssertedParameter() { result = getAnAssertedParameter() }
 
-  /** Gets the parameter being asserted. */
-  final Parameter getAssertedParameter() { result = this.getParameter(this.getAssertionIndex()) }
+  /** Gets a parameter being asserted. */
+  final Parameter getAnAssertedParameter() {
+    result = this.getParameter(this.getAnAssertionIndex())
+  }
 
   /** Gets the exception being thrown if the assertion fails, if any. */
   abstract Class getExceptionClass();
@@ -40,8 +56,15 @@ class Assertion extends MethodCall {
   /** Gets the assertion method targeted by this assertion. */
   AssertMethod getAssertMethod() { result = target }
 
-  /** Gets the expression that this assertion pertains to. */
-  Expr getExpr() { result = this.getArgumentForParameter(target.getAssertedParameter()) }
+  /**
+   * DEPRECATED: renamed to `getAnExpr`.
+   *
+   * Gets an expression that this assertion pertains to.
+   */
+  deprecated Expr getExpr() { result = this.getAnExpr() }
+
+  /** Gets an expression that this assertion pertains to. */
+  Expr getAnExpr() { result = this.getArgumentForParameter(target.getAnAssertedParameter()) }
 
   /**
    * Holds if basic block `succ` is immediately dominated by this assertion.
@@ -144,7 +167,7 @@ class FailingAssertion extends Assertion {
   FailingAssertion() {
     exists(AssertMethod am, Expr e |
       am = this.getAssertMethod() and
-      e = this.getExpr()
+      e = this.getAnExpr()
     |
       am instanceof AssertTrueMethod and
       e.getValue() = "false"
@@ -163,7 +186,7 @@ class SystemDiagnosticsDebugAssertTrueMethod extends AssertTrueMethod {
     this = any(SystemDiagnosticsDebugClass c).getAssertMethod()
   }
 
-  override int getAssertionIndex() { result = 0 }
+  override int getAnAssertionIndex() { result = 0 }
 
   override Class getExceptionClass() {
     // A failing assertion generates a message box, see
@@ -186,7 +209,7 @@ class SystemDiagnosticsContractAssertTrueMethod extends AssertTrueMethod {
     )
   }
 
-  override int getAssertionIndex() { result = 0 }
+  override int getAnAssertionIndex() { result = 0 }
 
   override Class getExceptionClass() {
     // A failing assertion generates a message box, see
@@ -195,11 +218,50 @@ class SystemDiagnosticsContractAssertTrueMethod extends AssertTrueMethod {
   }
 }
 
+private predicate isDoesNotReturnIfAttributeParameter(Parameter p, boolean value) {
+  exists(Attribute a | a = p.getAnAttribute() |
+    a.getType() instanceof SystemDiagnosticsCodeAnalysisDoesNotReturnIfAttributeClass and
+    a.getConstructorArgument(0).(BoolLiteral).getBoolValue() = value
+  )
+}
+
+/**
+ * A method with a parameter that is annotated with
+ * `System.Diagnostics.CodeAnalysis.DoesNotReturnIfAttribute(false)`.
+ */
+class SystemDiagnosticsCodeAnalysisDoesNotReturnIfAnnotatedAssertTrueMethod extends AssertTrueMethod {
+  private int i;
+
+  SystemDiagnosticsCodeAnalysisDoesNotReturnIfAnnotatedAssertTrueMethod() {
+    isDoesNotReturnIfAttributeParameter(this.getParameter(i), false)
+  }
+
+  override int getAnAssertionIndex() { result = i }
+
+  override SystemExceptionClass getExceptionClass() { any() }
+}
+
+/**
+ * A method with a parameter that is annotated with
+ * `System.Diagnostics.CodeAnalysis.DoesNotReturnIfAttribute(true)`.
+ */
+class SystemDiagnosticsCodeAnalysisDoesNotReturnIfAnnotatedAssertFalseMethod extends AssertFalseMethod {
+  private int i;
+
+  SystemDiagnosticsCodeAnalysisDoesNotReturnIfAnnotatedAssertFalseMethod() {
+    isDoesNotReturnIfAttributeParameter(this.getParameter(i), true)
+  }
+
+  override int getAnAssertionIndex() { result = i }
+
+  override SystemExceptionClass getExceptionClass() { any() }
+}
+
 /** A Visual Studio assertion method. */
 class VSTestAssertTrueMethod extends AssertTrueMethod {
   VSTestAssertTrueMethod() { this = any(VSTestAssertClass c).getIsTrueMethod() }
 
-  override int getAssertionIndex() { result = 0 }
+  override int getAnAssertionIndex() { result = 0 }
 
   override AssertFailedExceptionClass getExceptionClass() { any() }
 }
@@ -208,7 +270,7 @@ class VSTestAssertTrueMethod extends AssertTrueMethod {
 class VSTestAssertFalseMethod extends AssertFalseMethod {
   VSTestAssertFalseMethod() { this = any(VSTestAssertClass c).getIsFalseMethod() }
 
-  override int getAssertionIndex() { result = 0 }
+  override int getAnAssertionIndex() { result = 0 }
 
   override AssertFailedExceptionClass getExceptionClass() { any() }
 }
@@ -217,7 +279,7 @@ class VSTestAssertFalseMethod extends AssertFalseMethod {
 class VSTestAssertNullMethod extends AssertNullMethod {
   VSTestAssertNullMethod() { this = any(VSTestAssertClass c).getIsNullMethod() }
 
-  override int getAssertionIndex() { result = 0 }
+  override int getAnAssertionIndex() { result = 0 }
 
   override AssertFailedExceptionClass getExceptionClass() { any() }
 }
@@ -226,14 +288,14 @@ class VSTestAssertNullMethod extends AssertNullMethod {
 class VSTestAssertNonNullMethod extends AssertNonNullMethod {
   VSTestAssertNonNullMethod() { this = any(VSTestAssertClass c).getIsNotNullMethod() }
 
-  override int getAssertionIndex() { result = 0 }
+  override int getAnAssertionIndex() { result = 0 }
 
   override AssertFailedExceptionClass getExceptionClass() { any() }
 }
 
 /** An NUnit assertion method. */
 abstract class NUnitAssertMethod extends AssertMethod {
-  override int getAssertionIndex() { result = 0 }
+  override int getAnAssertionIndex() { result = 0 }
 
   override AssertionExceptionClass getExceptionClass() { any() }
 }
@@ -292,11 +354,11 @@ class ForwarderAssertMethod extends AssertMethod {
     strictcount(AssignableDefinition def | def.getTarget() = p) = 1 and
     forex(ControlFlowElement body | body = this.getBody() |
       bodyAsserts(this, body, a) and
-      a.getExpr() = p.getAnAccess()
+      a.getAnExpr() = p.getAnAccess()
     )
   }
 
-  override int getAssertionIndex() { result = p.getPosition() }
+  override int getAnAssertionIndex() { result = p.getPosition() }
 
   override Class getExceptionClass() {
     result = this.getUnderlyingAssertMethod().getExceptionClass()
@@ -345,4 +407,4 @@ class ForwarderAssertNonNullMethod extends ForwarderAssertMethod, AssertNonNullM
 }
 
 /** Holds if expression `e` appears in an assertion. */
-predicate isExprInAssertion(Expr e) { e = any(Assertion a).getExpr().getAChildExpr*() }
+predicate isExprInAssertion(Expr e) { e = any(Assertion a).getAnExpr().getAChildExpr*() }

csharp/ql/src/semmle/code/csharp/controlflow/internal/Completion.qll

@@ -391,7 +391,7 @@ private predicate invalidCastCandidate(CastExpr ce) {
 }
 
 private predicate assertion(Assertion a, AssertMethod am, Expr e) {
-  e = a.getExpr() and
+  e = a.getAnExpr() and
   am = a.getAssertMethod()
 }
 

csharp/ql/src/semmle/code/csharp/controlflow/internal/Splitting.qll

@@ -445,7 +445,7 @@ module AssertionSplitting {
 
     override predicate hasEntry(ControlFlowElement pred, ControlFlowElement succ, Completion c) {
       exists(AssertMethod m |
-        pred = last(a.getExpr(), c) and
+        pred = last(a.getAnExpr(), c) and
         succ = succ(pred, c) and
         this.getAssertion() = a and
         m = a.getAssertMethod()

csharp/ql/src/semmle/code/csharp/frameworks/system/Diagnostics.qll

@@ -11,11 +11,26 @@ class SystemDiagnosticsNamespace extends Namespace {
   }
 }
 
+/** The `System.Diagnostics.CodeAnalysis` namespace. */
+class SystemDiagnosticsCodeAnalysisNamespace extends Namespace {
+  SystemDiagnosticsCodeAnalysisNamespace() {
+    this.getParentNamespace() instanceof SystemDiagnosticsNamespace and
+    this.hasName("CodeAnalysis")
+  }
+}
+
 /** A class in the `System.Diagnostics` namespace. */
 class SystemDiagnosticsClass extends Class {
   SystemDiagnosticsClass() { this.getNamespace() instanceof SystemDiagnosticsNamespace }
 }
 
+/** A class in the `System.Diagnostics.CodeAnalysis` namespace. */
+class SystemDiagnosticsCodeAnalysisClass extends Class {
+  SystemDiagnosticsCodeAnalysisClass() {
+    this.getNamespace() instanceof SystemDiagnosticsCodeAnalysisNamespace
+  }
+}
+
 /** The `System.Diagnostics.Debug` class. */
 class SystemDiagnosticsDebugClass extends SystemDiagnosticsClass {
   SystemDiagnosticsDebugClass() {
@@ -57,3 +72,10 @@ class SystemDiagnosticsProcessClass extends SystemDiagnosticsClass {
     result.getReturnType() instanceof SystemDiagnosticsProcessClass
   }
 }
+
+/** The `System.Diagnostics.CodeAnalysis.DoesNotReturnIfAttribute` class. */
+class SystemDiagnosticsCodeAnalysisDoesNotReturnIfAttributeClass extends SystemDiagnosticsCodeAnalysisClass {
+  SystemDiagnosticsCodeAnalysisDoesNotReturnIfAttributeClass() {
+    this.hasName("DoesNotReturnIfAttribute")
+  }
+}

csharp/ql/test/library-tests/commons/Assertions/Assertions.ql

@@ -2,21 +2,21 @@ import csharp
 import semmle.code.csharp.commons.Assertions
 
 query predicate assertTrue(Assertion a, Expr e) {
-  a.getExpr() = e and
+  a.getAnExpr() = e and
   a.getTarget() instanceof AssertTrueMethod
 }
 
 query predicate assertFalse(Assertion a, Expr e) {
-  a.getExpr() = e and
+  a.getAnExpr() = e and
   a.getTarget() instanceof AssertFalseMethod
 }
 
 query predicate assertNull(Assertion a, Expr e) {
-  a.getExpr() = e and
+  a.getAnExpr() = e and
   a.getTarget() instanceof AssertNullMethod
 }
 
 query predicate assertNonNull(Assertion a, Expr e) {
-  a.getExpr() = e and
+  a.getAnExpr() = e and
   a.getTarget() instanceof AssertNonNullMethod
 }

csharp/ql/test/library-tests/frameworks/test/Assertions.expected

@@ -1,5 +1,8 @@
 assertTrue
 | ../../../resources/stubs/Microsoft.VisualStudio.TestTools.UnitTesting.cs:9:28:9:33 | IsTrue | ../../../resources/stubs/Microsoft.VisualStudio.TestTools.UnitTesting.cs:9:40:9:40 | b |
+| DoesNotReturnIf.cs:8:22:8:31 | AssertTrue | DoesNotReturnIf.cs:8:95:8:103 | condition |
+| DoesNotReturnIf.cs:16:22:16:32 | AssertTrue2 | DoesNotReturnIf.cs:17:75:17:84 | condition1 |
+| DoesNotReturnIf.cs:16:22:16:32 | AssertTrue2 | DoesNotReturnIf.cs:18:75:18:84 | condition2 |
 | nunit.cs:28:21:28:24 | True | nunit.cs:28:31:28:39 | condition |
 | nunit.cs:29:21:29:24 | True | nunit.cs:29:31:29:39 | condition |
 | nunit.cs:31:21:31:26 | IsTrue | nunit.cs:31:33:31:41 | condition |
@@ -9,6 +12,7 @@ assertTrue
 | nunit.cs:54:21:54:24 | That | nunit.cs:54:31:54:39 | condition |
 assertFalse
 | ../../../resources/stubs/Microsoft.VisualStudio.TestTools.UnitTesting.cs:10:28:10:34 | IsFalse | ../../../resources/stubs/Microsoft.VisualStudio.TestTools.UnitTesting.cs:10:41:10:41 | b |
+| DoesNotReturnIf.cs:12:22:12:32 | AssertFalse | DoesNotReturnIf.cs:12:95:12:103 | condition |
 | nunit.cs:34:21:34:25 | False | nunit.cs:34:32:34:40 | condition |
 | nunit.cs:35:21:35:25 | False | nunit.cs:35:32:35:40 | condition |
 | nunit.cs:37:21:37:27 | IsFalse | nunit.cs:37:34:37:42 | condition |

csharp/ql/test/library-tests/frameworks/test/Assertions.ql

@@ -2,17 +2,17 @@ import csharp
 import semmle.code.csharp.commons.Assertions
 
 query predicate assertTrue(AssertTrueMethod m, Parameter p) {
-  m.fromSource() and m.fromSource() and p = m.getAssertedParameter()
+  m.fromSource() and m.fromSource() and p = m.getAnAssertedParameter()
 }
 
 query predicate assertFalse(AssertFalseMethod m, Parameter p) {
-  m.fromSource() and m.fromSource() and p = m.getAssertedParameter()
+  m.fromSource() and m.fromSource() and p = m.getAnAssertedParameter()
 }
 
 query predicate assertNull(AssertNullMethod m, Parameter p) {
-  m.fromSource() and m.fromSource() and p = m.getAssertedParameter()
+  m.fromSource() and m.fromSource() and p = m.getAnAssertedParameter()
 }
 
 query predicate assertNonNull(AssertNonNullMethod m, Parameter p) {
-  m.fromSource() and m.fromSource() and p = m.getAssertedParameter()
+  m.fromSource() and m.fromSource() and p = m.getAnAssertedParameter()
 }

csharp/ql/test/library-tests/frameworks/test/DoesNotReturnIf.cs

@@ -0,0 +1,23 @@
+using System;
+using System.Collections.Generic;
+
+namespace DoesNotReturnIfTests
+{
+    class MyTestSuite
+    {
+        private void AssertTrue([System.Diagnostics.CodeAnalysis.DoesNotReturnIf(false)] bool condition)
+        {
+        }
+
+        private void AssertFalse([System.Diagnostics.CodeAnalysis.DoesNotReturnIf(true)] bool condition)
+        {
+        }
+
+        private void AssertTrue2(
+            [System.Diagnostics.CodeAnalysis.DoesNotReturnIf(false)] bool condition1,
+            [System.Diagnostics.CodeAnalysis.DoesNotReturnIf(false)] bool condition2,
+            bool nonCondition)
+        {
+        }
+    }
+}