C#: Fix a bug in ThrowingCallable

hvitved · hvitved · commit c887dc89dcc5 · 2018-12-07T10:56:11.000+01:00
A method such as

```
void M()
{
    throw new Exception();
}
```

was incorrectly not categorized as a `ThrowingCallable`, that is, a callable
that always throws an exception upon invocation.
diff --git a/csharp/ql/src/semmle/code/csharp/controlflow/ControlFlowGraph.qll b/csharp/ql/src/semmle/code/csharp/controlflow/ControlFlowGraph.qll
@@ -1871,28 +1871,32 @@ module ControlFlow {
           }
         }
 
+        private predicate directlyThrows(ThrowElement te, ThrowCompletion c) {
+          c.getExceptionClass() = te.getThrownExceptionType() and
+          // For stub implementations, there may exist proper implementations that are not seen
+          // during compilation, so we conservatively rule those out
+          not isStub(te)
+        }
+
         private ControlFlowElement getAThrowingElement(ThrowCompletion c) {
           c = result.(ThrowingCall).getACompletion()
           or
-          result = any(ThrowElement te |
-            c.getExceptionClass() = te.getThrownExceptionType() and
-            // For stub implementations, there may exist proper implementations that are not seen
-            // during compilation, so we conservatively rule those out
-            not isStub(te)
-          )
+          directlyThrows(result, c)
           or
           result = getAThrowingStmt(c)
         }
 
         private Stmt getAThrowingStmt(ThrowCompletion c) {
+          directlyThrows(result, c)
+          or
           result.(ExprStmt).getExpr() = getAThrowingElement(c)
           or
           result.(BlockStmt).getFirstStmt() = getAThrowingStmt(c)
           or
           exists(IfStmt ifStmt, ThrowCompletion c1, ThrowCompletion c2 |
             result = ifStmt and
-            ifStmt.getThen() = getAThrowingElement(c1) and
-            ifStmt.getElse() = getAThrowingElement(c2) |
+            ifStmt.getThen() = getAThrowingStmt(c1) and
+            ifStmt.getElse() = getAThrowingStmt(c2) |
             c = c1
             or
             c = c2
diff --git a/csharp/ql/test/library-tests/controlflow/graph/BasicBlock.expected b/csharp/ql/test/library-tests/controlflow/graph/BasicBlock.expected
@@ -187,7 +187,7 @@
 | ExitMethods.cs:43:9:46:9 | [exception: Exception] catch (...) {...} | ExitMethods.cs:43:9:46:9 | [exception: Exception] catch (...) {...} | 1 |
 | ExitMethods.cs:44:9:46:9 | {...} | ExitMethods.cs:45:13:45:19 | return ...; | 2 |
 | ExitMethods.cs:47:9:50:9 | [exception: Exception] catch (...) {...} | ExitMethods.cs:49:13:49:19 | return ...; | 3 |
-| ExitMethods.cs:53:10:53:11 | enter M7 | ExitMethods.cs:53:10:53:11 | exit M7 | 6 |
+| ExitMethods.cs:53:10:53:11 | enter M7 | ExitMethods.cs:53:10:53:11 | exit M7 | 5 |
 | ExitMethods.cs:59:10:59:11 | enter M8 | ExitMethods.cs:59:10:59:11 | exit M8 | 5 |
 | ExitMethods.cs:65:17:65:26 | enter ErrorMaybe | ExitMethods.cs:67:13:67:13 | access to parameter b | 4 |
 | ExitMethods.cs:65:17:65:26 | exit ErrorMaybe | ExitMethods.cs:65:17:65:26 | exit ErrorMaybe | 1 |
diff --git a/csharp/ql/test/library-tests/controlflow/graph/Dominance.expected b/csharp/ql/test/library-tests/controlflow/graph/Dominance.expected
@@ -735,11 +735,10 @@
 | post | ExitMethods.cs:45:13:45:19 | return ...; | ExitMethods.cs:44:9:46:9 | {...} |
 | post | ExitMethods.cs:48:9:50:9 | {...} | ExitMethods.cs:47:9:50:9 | [exception: Exception] catch (...) {...} |
 | post | ExitMethods.cs:49:13:49:19 | return ...; | ExitMethods.cs:48:9:50:9 | {...} |
-| post | ExitMethods.cs:53:10:53:11 | exit M7 | ExitMethods.cs:56:9:56:15 | return ...; |
+| post | ExitMethods.cs:53:10:53:11 | exit M7 | ExitMethods.cs:55:9:55:22 | call to method ErrorAlways2 |
 | post | ExitMethods.cs:54:5:57:5 | {...} | ExitMethods.cs:53:10:53:11 | enter M7 |
 | post | ExitMethods.cs:55:9:55:22 | call to method ErrorAlways2 | ExitMethods.cs:55:9:55:23 | ...; |
 | post | ExitMethods.cs:55:9:55:23 | ...; | ExitMethods.cs:54:5:57:5 | {...} |
-| post | ExitMethods.cs:56:9:56:15 | return ...; | ExitMethods.cs:55:9:55:22 | call to method ErrorAlways2 |
 | post | ExitMethods.cs:59:10:59:11 | exit M8 | ExitMethods.cs:61:9:61:22 | call to method ErrorAlways3 |
 | post | ExitMethods.cs:60:5:63:5 | {...} | ExitMethods.cs:59:10:59:11 | enter M8 |
 | post | ExitMethods.cs:61:9:61:22 | call to method ErrorAlways3 | ExitMethods.cs:61:9:61:23 | ...; |
@@ -2959,9 +2958,8 @@
 | pre | ExitMethods.cs:48:9:50:9 | {...} | ExitMethods.cs:49:13:49:19 | return ...; |
 | pre | ExitMethods.cs:53:10:53:11 | enter M7 | ExitMethods.cs:54:5:57:5 | {...} |
 | pre | ExitMethods.cs:54:5:57:5 | {...} | ExitMethods.cs:55:9:55:23 | ...; |
-| pre | ExitMethods.cs:55:9:55:22 | call to method ErrorAlways2 | ExitMethods.cs:56:9:56:15 | return ...; |
+| pre | ExitMethods.cs:55:9:55:22 | call to method ErrorAlways2 | ExitMethods.cs:53:10:53:11 | exit M7 |
 | pre | ExitMethods.cs:55:9:55:23 | ...; | ExitMethods.cs:55:9:55:22 | call to method ErrorAlways2 |
-| pre | ExitMethods.cs:56:9:56:15 | return ...; | ExitMethods.cs:53:10:53:11 | exit M7 |
 | pre | ExitMethods.cs:59:10:59:11 | enter M8 | ExitMethods.cs:60:5:63:5 | {...} |
 | pre | ExitMethods.cs:60:5:63:5 | {...} | ExitMethods.cs:61:9:61:23 | ...; |
 | pre | ExitMethods.cs:61:9:61:22 | call to method ErrorAlways3 | ExitMethods.cs:59:10:59:11 | exit M8 |
diff --git a/csharp/ql/test/library-tests/controlflow/graph/ElementGraph.expected b/csharp/ql/test/library-tests/controlflow/graph/ElementGraph.expected
@@ -562,7 +562,6 @@
 | ExitMethods.cs:47:9:50:9 | catch (...) {...} | ExitMethods.cs:48:9:50:9 | {...} | semmle.label | match |
 | ExitMethods.cs:48:9:50:9 | {...} | ExitMethods.cs:49:13:49:19 | return ...; | semmle.label | successor |
 | ExitMethods.cs:54:5:57:5 | {...} | ExitMethods.cs:55:9:55:23 | ...; | semmle.label | successor |
-| ExitMethods.cs:55:9:55:22 | call to method ErrorAlways2 | ExitMethods.cs:56:9:56:15 | return ...; | semmle.label | successor |
 | ExitMethods.cs:55:9:55:23 | ...; | ExitMethods.cs:55:9:55:22 | call to method ErrorAlways2 | semmle.label | successor |
 | ExitMethods.cs:60:5:63:5 | {...} | ExitMethods.cs:61:9:61:23 | ...; | semmle.label | successor |
 | ExitMethods.cs:61:9:61:23 | ...; | ExitMethods.cs:61:9:61:22 | call to method ErrorAlways3 | semmle.label | successor |
diff --git a/csharp/ql/test/library-tests/controlflow/graph/ExitElement.expected b/csharp/ql/test/library-tests/controlflow/graph/ExitElement.expected
@@ -788,9 +788,10 @@
 | ExitMethods.cs:47:9:50:9 | catch (...) {...} | ExitMethods.cs:49:13:49:19 | return ...; | return |
 | ExitMethods.cs:48:9:50:9 | {...} | ExitMethods.cs:49:13:49:19 | return ...; | return |
 | ExitMethods.cs:49:13:49:19 | return ...; | ExitMethods.cs:49:13:49:19 | return ...; | return |
+| ExitMethods.cs:54:5:57:5 | {...} | ExitMethods.cs:55:9:55:22 | call to method ErrorAlways2 | throw(Exception) |
 | ExitMethods.cs:54:5:57:5 | {...} | ExitMethods.cs:56:9:56:15 | return ...; | return |
-| ExitMethods.cs:55:9:55:22 | call to method ErrorAlways2 | ExitMethods.cs:55:9:55:22 | call to method ErrorAlways2 | normal |
-| ExitMethods.cs:55:9:55:23 | ...; | ExitMethods.cs:55:9:55:22 | call to method ErrorAlways2 | normal |
+| ExitMethods.cs:55:9:55:22 | call to method ErrorAlways2 | ExitMethods.cs:55:9:55:22 | call to method ErrorAlways2 | throw(Exception) |
+| ExitMethods.cs:55:9:55:23 | ...; | ExitMethods.cs:55:9:55:22 | call to method ErrorAlways2 | throw(Exception) |
 | ExitMethods.cs:56:9:56:15 | return ...; | ExitMethods.cs:56:9:56:15 | return ...; | return |
 | ExitMethods.cs:60:5:63:5 | {...} | ExitMethods.cs:61:9:61:22 | call to method ErrorAlways3 | throw(Exception) |
 | ExitMethods.cs:60:5:63:5 | {...} | ExitMethods.cs:62:9:62:15 | return ...; | return |
diff --git a/csharp/ql/test/library-tests/controlflow/graph/NodeGraph.expected b/csharp/ql/test/library-tests/controlflow/graph/NodeGraph.expected
@@ -859,9 +859,8 @@
 | ExitMethods.cs:49:13:49:19 | return ...; | ExitMethods.cs:37:10:37:11 | exit M6 | semmle.label | return |
 | ExitMethods.cs:53:10:53:11 | enter M7 | ExitMethods.cs:54:5:57:5 | {...} | semmle.label | successor |
 | ExitMethods.cs:54:5:57:5 | {...} | ExitMethods.cs:55:9:55:23 | ...; | semmle.label | successor |
-| ExitMethods.cs:55:9:55:22 | call to method ErrorAlways2 | ExitMethods.cs:56:9:56:15 | return ...; | semmle.label | successor |
+| ExitMethods.cs:55:9:55:22 | call to method ErrorAlways2 | ExitMethods.cs:53:10:53:11 | exit M7 | semmle.label | exception(Exception) |
 | ExitMethods.cs:55:9:55:23 | ...; | ExitMethods.cs:55:9:55:22 | call to method ErrorAlways2 | semmle.label | successor |
-| ExitMethods.cs:56:9:56:15 | return ...; | ExitMethods.cs:53:10:53:11 | exit M7 | semmle.label | return |
 | ExitMethods.cs:59:10:59:11 | enter M8 | ExitMethods.cs:60:5:63:5 | {...} | semmle.label | successor |
 | ExitMethods.cs:60:5:63:5 | {...} | ExitMethods.cs:61:9:61:23 | ...; | semmle.label | successor |
 | ExitMethods.cs:61:9:61:22 | call to method ErrorAlways3 | ExitMethods.cs:59:10:59:11 | exit M8 | semmle.label | exception(Exception) |
