Merge pull request #7469 from hvitved/csharp/promote-adhoc-consistency-checks

C#: Promote existing ad-hoc consistency checks to consistency queries

Author: hvitved

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImplCommon.qll

@@ -659,4 +659,15 @@ module Consistency {
     not phiHasInputFromBlock(_, def, _) and
     not uncertainWriteDefinitionInput(_, def)
   }
+
+  query predicate notDominatedByDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) {
+    exists(BasicBlock bbDef, int iDef | def.definesAt(v, bbDef, iDef) |
+      ssaDefReachesReadWithinBlock(v, def, bb, i) and
+      (bb != bbDef or i < iDef)
+      or
+      ssaDefReachesRead(v, def, bb, i) and
+      not ssaDefReachesReadWithinBlock(v, def, bb, i) and
+      not def.definesAt(v, getImmediateBasicBlockDominator*(bb), _)
+    )
+  }
 }

csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs

@@ -113,11 +113,7 @@ protected override void ExtractInitializers(TextWriter trapFile)
 
             trapFile.expr_call(init, target);
 
-            var child = 0;
-            foreach (var arg in initializer.ArgumentList.Arguments)
-            {
-                Expression.Create(Context, arg.Expression, init, child++);
-            }
+            init.PopulateArguments(trapFile, initializer.ArgumentList, 0);
         }
 
         private ConstructorDeclarationSyntax? Syntax

csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs

@@ -4,6 +4,7 @@
 using Semmle.Extraction.CSharp.Entities.Expressions;
 using Semmle.Extraction.Kinds;
 using System;
+using System.Collections.Generic;
 using System.IO;
 using System.Linq;
 
@@ -324,7 +325,12 @@ public void MakeConditional(TextWriter trapFile)
 
         public void PopulateArguments(TextWriter trapFile, BaseArgumentListSyntax args, int child)
         {
-            foreach (var arg in args.Arguments)
+            PopulateArguments(trapFile, args.Arguments, child);
+        }
+
+        public void PopulateArguments(TextWriter trapFile, IEnumerable<ArgumentSyntax> args, int child)
+        {
+            foreach (var arg in args)
                 PopulateArgument(trapFile, arg, child++);
         }
 

csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Initializer.cs

@@ -105,12 +105,7 @@ protected override void PopulateExpression(TextWriter trapFile)
                     if (assignment.Left is ImplicitElementAccessSyntax iea)
                     {
                         // An array/indexer initializer of the form `[...] = ...`
-
-                        var indexChild = 0;
-                        foreach (var arg in iea.ArgumentList.Arguments)
-                        {
-                            Expression.Create(Context, arg.Expression, access, indexChild++);
-                        }
+                        access.PopulateArguments(trapFile, iea.ArgumentList.Arguments, 0);
                     }
                 }
                 else

csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Tuple.cs

@@ -15,11 +15,7 @@ private Tuple(ExpressionNodeInfo info) : base(info.SetKind(ExprKind.TUPLE))
 
         protected override void PopulateExpression(TextWriter trapFile)
         {
-            var child = 0;
-            foreach (var argument in Syntax.Arguments.Select(a => a.Expression))
-            {
-                Expression.Create(Context, argument, this, child++);
-            }
+            PopulateArguments(trapFile, Syntax.Arguments, 0);
         }
     }
 }

csharp/ql/consistency-queries/AstConsistency.qll

@@ -0,0 +1,19 @@
+import csharp
+
+query predicate missingLocation(Element e) {
+  (
+    e instanceof Declaration or
+    e instanceof Expr or
+    e instanceof Stmt
+  ) and
+  not e instanceof ImplicitAccessorParameter and
+  not e instanceof NullType and
+  not e instanceof Parameter and // Bug in Roslyn - params occasionally lack locations
+  not e.(Operator).getDeclaringType() instanceof IntType and // Roslyn quirk
+  not e instanceof Constructor and
+  not e instanceof ArrayType and
+  not e instanceof UnknownType and
+  not e instanceof ArglistType and
+  not exists(TupleType t | e = t or e = t.getAField()) and
+  not exists(e.getLocation())
+}

csharp/ql/consistency-queries/GenericsConsistency.qll

@@ -0,0 +1,9 @@
+import csharp
+
+query predicate missingUnbound(ConstructedGeneric g) { not exists(g.getUnboundGeneric()) }
+
+query predicate missingArgs(ConstructedGeneric g) { g.getNumberOfTypeArguments() = 0 }
+
+query predicate inconsistentArgCount(ConstructedGeneric g) {
+  g.getUnboundGeneric().getNumberOfTypeParameters() != g.getNumberOfTypeArguments()
+}

csharp/ql/consistency-queries/SsaConsistency.ql

@@ -1,5 +1,6 @@
 import csharp
 import semmle.code.csharp.dataflow.internal.SsaImplCommon::Consistency
+import Ssa
 
 class MyRelevantDefinition extends RelevantDefinition, Ssa::Definition {
   override predicate hasLocationInfo(
@@ -8,3 +9,18 @@ class MyRelevantDefinition extends RelevantDefinition, Ssa::Definition {
     this.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
   }
 }
+
+query predicate localDeclWithSsaDef(LocalVariableDeclExpr d) {
+  // Local variables in C# must be initialized before every use, so uninitialized
+  // local variables should not have an SSA definition, as that would imply that
+  // the declaration is live (can reach a use without passing through a definition)
+  exists(ExplicitDefinition def |
+    d = def.getADefinition().(AssignableDefinitions::LocalVariableDefinition).getDeclaration()
+  |
+    not d = any(ForeachStmt fs).getVariableDeclExpr() and
+    not d = any(SpecificCatchClause scc).getVariableDeclExpr() and
+    not d.getVariable().getType() instanceof Struct and
+    not d instanceof PatternExpr and
+    not d.getVariable().isCaptured()
+  )
+}

csharp/ql/lib/semmle/code/cil/internal/SsaImplCommon.qll

@@ -659,4 +659,15 @@ module Consistency {
     not phiHasInputFromBlock(_, def, _) and
     not uncertainWriteDefinitionInput(_, def)
   }
+
+  query predicate notDominatedByDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) {
+    exists(BasicBlock bbDef, int iDef | def.definesAt(v, bbDef, iDef) |
+      ssaDefReachesReadWithinBlock(v, def, bb, i) and
+      (bb != bbDef or i < iDef)
+      or
+      ssaDefReachesRead(v, def, bb, i) and
+      not ssaDefReachesReadWithinBlock(v, def, bb, i) and
+      not def.definesAt(v, getImmediateBasicBlockDominator*(bb), _)
+    )
+  }
 }

csharp/ql/lib/semmle/code/csharp/commons/ConsistencyChecks.qll

@@ -659,4 +659,15 @@ module Consistency {
     not phiHasInputFromBlock(_, def, _) and
     not uncertainWriteDefinitionInput(_, def)
   }
+
+  query predicate notDominatedByDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) {
+    exists(BasicBlock bbDef, int iDef | def.definesAt(v, bbDef, iDef) |
+      ssaDefReachesReadWithinBlock(v, def, bb, i) and
+      (bb != bbDef or i < iDef)
+      or
+      ssaDefReachesRead(v, def, bb, i) and
+      not ssaDefReachesReadWithinBlock(v, def, bb, i) and
+      not def.definesAt(v, getImmediateBasicBlockDominator*(bb), _)
+    )
+  }
 }

csharp/ql/lib/semmle/code/csharp/controlflow/internal/pressa/SsaImplCommon.qll

@@ -659,4 +659,15 @@ module Consistency {
     not phiHasInputFromBlock(_, def, _) and
     not uncertainWriteDefinitionInput(_, def)
   }
+
+  query predicate notDominatedByDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) {
+    exists(BasicBlock bbDef, int iDef | def.definesAt(v, bbDef, iDef) |
+      ssaDefReachesReadWithinBlock(v, def, bb, i) and
+      (bb != bbDef or i < iDef)
+      or
+      ssaDefReachesRead(v, def, bb, i) and
+      not ssaDefReachesReadWithinBlock(v, def, bb, i) and
+      not def.definesAt(v, getImmediateBasicBlockDominator*(bb), _)
+    )
+  }
 }

csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImplCommon.qll

@@ -659,4 +659,15 @@ module Consistency {
     not phiHasInputFromBlock(_, def, _) and
     not uncertainWriteDefinitionInput(_, def)
   }
+
+  query predicate notDominatedByDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) {
+    exists(BasicBlock bbDef, int iDef | def.definesAt(v, bbDef, iDef) |
+      ssaDefReachesReadWithinBlock(v, def, bb, i) and
+      (bb != bbDef or i < iDef)
+      or
+      ssaDefReachesRead(v, def, bb, i) and
+      not ssaDefReachesReadWithinBlock(v, def, bb, i) and
+      not def.definesAt(v, getImmediateBasicBlockDominator*(bb), _)
+    )
+  }
 }

csharp/ql/lib/semmle/code/csharp/dataflow/internal/basessa/SsaImplCommon.qll

@@ -316,6 +316,12 @@ private predicate hasChildPattern(ControlFlowElement pm, Expr child) {
     child = mid.getChildExpr(0) or
     child = mid.getChildExpr(1)
   )
+  or
+  exists(Expr mid |
+    hasChildPattern(pm, mid) and
+    mid instanceof @tuple_expr and
+    child = mid.getAChildExpr()
+  )
 }
 
 /**
@@ -420,13 +426,10 @@ class TypeAccessPatternExpr extends TypePatternExpr, TypeAccess {
   override string getAPrimaryQlClass() { result = "TypeAccessPatternExpr" }
 }
 
-/** A pattern that may bind a variable, for example `string s` in `x is string s`. */
-class BindingPatternExpr extends PatternExpr {
-  BindingPatternExpr() {
-    this instanceof LocalVariableDeclExpr or
-    this instanceof @recursive_pattern_expr
-  }
+private class TBindingPatternExpr = @local_var_decl_expr or @recursive_pattern_expr;
 
+/** A pattern that may bind a variable, for example `string s` in `x is string s`. */
+class BindingPatternExpr extends PatternExpr, TBindingPatternExpr {
   /**
    * Gets the local variable declaration of this pattern, if any. For example,
    * `string s` in `string { Length: 5 } s`.

csharp/ql/lib/semmle/code/csharp/exprs/Expr.qll

@@ -24,13 +24,21 @@
 | arguments.cs:45:35:45:38 | null | 0 |
 | arguments.cs:55:21:55:21 | 1 | 0 |
 | arguments.cs:55:24:55:24 | 2 | 0 |
+| arguments.cs:56:10:56:13 | access to property Prop | 0 |
+| arguments.cs:56:16:56:25 | access to indexer | 0 |
 | arguments.cs:56:21:56:21 | 3 | 0 |
 | arguments.cs:56:24:56:24 | 4 | 0 |
+| arguments.cs:56:31:56:31 | 5 | 0 |
+| arguments.cs:56:34:56:34 | 6 | 0 |
 | arguments.cs:59:14:59:14 | 8 | 0 |
 | arguments.cs:59:17:59:17 | 9 | 0 |
 | arguments.cs:60:14:60:15 | 10 | 0 |
 | arguments.cs:60:14:60:15 | 10 | 0 |
 | arguments.cs:60:18:60:19 | 11 | 0 |
 | arguments.cs:60:18:60:19 | 11 | 0 |
+| arguments.cs:61:22:61:23 | 13 | 0 |
+| arguments.cs:61:26:61:27 | 14 | 0 |
+| arguments.cs:62:10:62:13 | access to property Prop | 0 |
+| arguments.cs:62:16:62:27 | access to indexer | 0 |
 | arguments.cs:62:21:62:22 | 15 | 0 |
 | arguments.cs:62:25:62:26 | 16 | 0 |

csharp/ql/test/library-tests/arguments/argumentType.expected

@@ -102,7 +102,7 @@ csharp73.cs:
 #   46|           1: [IntLiteral] 5
 #   49|   5: [InstanceConstructor] ExpressionVariables
 #   49|     3: [ConstructorInitializer] call to constructor ExpressionVariables
-#   49|       0: [LocalVariableDeclExpr] Int32 x
+#   49|       0: [LocalVariableAccess,LocalVariableDeclExpr] Int32 x
 #   50|     4: [BlockStmt] {...}
 #   51|       0: [ExprStmt] ...;
 #   51|         0: [MethodCall] call to method WriteLine

csharp/ql/test/library-tests/csharp7.3/PrintAst.expected

@@ -932,8 +932,8 @@ patterns.cs:
 #   58|         10: [BreakStmt] break;
 #   59|         11: [CaseStmt] case ...:
 #   59|           0: [TupleExpr] (..., ...)
-#   59|             0: [LocalVariableDeclExpr] Int32 x
-#   59|             1: [LocalVariableDeclExpr] Int32 y
+#   59|             0: [VariablePatternExpr] Int32 x
+#   59|             1: [VariablePatternExpr] Int32 y
 #   60|         12: [BreakStmt] break;
 #   61|         13: [DefaultCase] default:
 #   62|         14: [BreakStmt] break;
@@ -1159,8 +1159,8 @@ patterns.cs:
 #  130|               2: [IntLiteral] 2
 #  131|             3: [SwitchCaseExpr] ... => ...
 #  131|               0: [TupleExpr] (..., ...)
-#  131|                 0: [LocalVariableDeclExpr] Int32 x
-#  131|                 1: [DiscardExpr] _
+#  131|                 0: [VariablePatternExpr] Int32 x
+#  131|                 1: [DiscardPatternExpr] _
 #  131|               2: [IntLiteral] 3
 #  134|       2: [TryStmt] try {...} ...
 #  135|         0: [BlockStmt] {...}