Merge pull request #2710 from calumgrant/cs/short-circuit-out

C#: Remove false positive in cs/non-short-circuit

change-notes/1.24/analysis-csharp.md

@@ -18,6 +18,7 @@ The following changes in version 1.24 affect C# analysis in all applications.
 | **Query**                    | **Expected impact**    | **Change**                        |
 |------------------------------|------------------------|-----------------------------------|
 | Useless assignment to local variable (`cs/useless-assignment-to-local`) | Fewer false positive results | Results have been removed when the variable is named `_` in a `foreach` statement. | 
+| Potentially dangerous use of non-short-circuit logic (`cs/non-short-circuit`) | Fewer false positive results | Results have been removed when the expression contains an `out` parameter. |
 | Dereferenced variable may be null (`cs/dereferenced-value-may-be-null`) | More results | Results are reported from parameters with a default value of `null`. |
 
 ## Removal of old queries

csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.ql

@@ -27,7 +27,8 @@ class DangerousExpression extends Expr {
       e instanceof MethodCall
       or
       e instanceof ArrayAccess
-    )
+    ) and
+    not exists(Expr e | this = e.getParent*() | e.(Call).getTarget().getAParameter().isOutOrRef())
   }
 }
 

csharp/ql/test/query-tests/Likely Bugs/DangerousNonShortCircuitLogic/DangerousNonShortCircuitLogic.cs

@@ -20,6 +20,9 @@ class Test
         var b = true;
         b &= c.Method(); // GOOD
         b |= c[0]; // GOOD
+
+        if (c == null | c.Method(out _)) ; // GOOD
+        if (c == null | (c.Method() | c.Method(out _))) ; // GOOD
     }
 
     class C
@@ -28,6 +31,7 @@ class Test
         public string Property { get; set; }
         public bool this[int i] { get { return false; } set { } }
         public bool Method() { return false; }
+        public bool Method(out int x) { x = 0; return false; }
     }
 }