Merge pull request #5619 from tamasvajk/feature/fix-default-argument-value-extraction

C# Improve default argument value extraction

csharp/change-notes/2021-04-09-default-argument-values.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* The extractor has been improved to store default argument values for parameters that are extracted from referenced assemblies.

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

@@ -164,6 +164,39 @@ namespace Semmle.Extraction.CSharp.Entities
             }
         }
 
+        /// <summary>
+        /// Creates a generated expression for a default argument value.
+        /// </summary>
+        public static Expression? CreateGenerated(Context cx, IParameterSymbol parameter, IExpressionParentEntity parent,
+            int childIndex, Extraction.Entities.Location location)
+        {
+            if (!parameter.HasExplicitDefaultValue)
+            {
+                return null;
+            }
+
+            var defaultValue = parameter.ExplicitDefaultValue;
+
+            if (parameter.Type is INamedTypeSymbol nt && nt.EnumUnderlyingType is not null)
+            {
+                // = (MyEnum)1, = MyEnum.Value1, = default(MyEnum), = new MyEnum()
+                // we're generating a (MyEnum)value cast expression:
+                defaultValue ??= 0;
+                Action<Expression, int> createChild = (parent, index) => Literal.CreateGenerated(cx, parent, index, nt.EnumUnderlyingType, defaultValue, location);
+                return Cast.CreateGenerated(cx, parent, childIndex, parameter.Type, defaultValue, createChild, location);
+            }
+
+            if (defaultValue is null)
+            {
+                // = null, = default, = default(T), = new MyStruct()
+                // we're generating a default expression:
+                return Default.CreateGenerated(cx, parent, childIndex, location, parameter.Type.IsReferenceType ? ValueAsString(null) : null);
+            }
+
+            // const literal:
+            return Literal.CreateGenerated(cx, parent, childIndex, parameter.Type, defaultValue, location);
+        }
+
         /// <summary>
         /// Adapt the operator kind depending on whether it's a dynamic call or a user-operator call.
         /// </summary>

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

@@ -14,5 +14,20 @@ namespace Semmle.Extraction.CSharp.Entities.Expressions
         {
             TypeAccess.Create(Context, Syntax.Type, this, 0);
         }
+
+        public static Expression CreateGenerated(Context cx, IExpressionParentEntity parent, int childIndex, Extraction.Entities.Location location, string? value)
+        {
+            var info = new ExpressionInfo(
+                cx,
+                null,
+                location,
+                ExprKind.DEFAULT,
+                parent,
+                childIndex,
+                true,
+                value);
+
+            return new Expression(info);
+        }
     }
 }

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

@@ -4,6 +4,7 @@ using System.Linq;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Semmle.Extraction.Entities;
 using System.IO;
+using System;
 
 namespace Semmle.Extraction.CSharp.Entities
 {
@@ -124,6 +125,17 @@ namespace Semmle.Extraction.CSharp.Entities
                 trapFile.param_location(this, Context.CreateLocation());
             }
 
+            if (Symbol.HasExplicitDefaultValue && Context.Defines(Symbol))
+            {
+                var defaultValueSyntax = GetDefaultValueFromSyntax(Symbol);
+
+                Action defaultValueExpressionCreation = defaultValueSyntax is not null
+                    ? () => Expression.Create(Context, defaultValueSyntax.Value, this, 0)
+                    : () => Expression.CreateGenerated(Context, Symbol, this, 0, Location);
+
+                Context.PopulateLater(defaultValueExpressionCreation);
+            }
+
             if (!IsSourceDeclaration || !Symbol.FromSource())
                 return;
 
@@ -139,36 +151,28 @@ namespace Semmle.Extraction.CSharp.Entities
                     TypeMention.Create(Context, syntax.Type!, this, type);
                 }
             }
+        }
 
-            if (Symbol.HasExplicitDefaultValue && Context.Defines(Symbol))
+        private static EqualsValueClauseSyntax? GetDefaultValueFromSyntax(IParameterSymbol symbol)
+        {
+            // This is a slight bug in the dbscheme
+            // We should really define param_default(param, string)
+            // And use parameter child #0 to encode the default expression.
+            var defaultValue = GetParameterDefaultValue(symbol);
+            if (defaultValue is null)
             {
-                // This is a slight bug in the dbscheme
-                // We should really define param_default(param, string)
-                // And use parameter child #0 to encode the default expression.
-                var defaultValue = GetParameterDefaultValue(Symbol);
-                if (defaultValue is null)
+                // In case this parameter belongs to an accessor of an indexer, we need
+                // to get the default value from the corresponding parameter belonging
+                // to the indexer itself
+                if (symbol.ContainingSymbol is IMethodSymbol method)
                 {
-                    // In case this parameter belongs to an accessor of an indexer, we need
-                    // to get the default value from the corresponding parameter belonging
-                    // to the indexer itself
-                    var method = (IMethodSymbol)Symbol.ContainingSymbol;
-                    if (method is not null)
-                    {
-                        var i = method.Parameters.IndexOf(Symbol);
-                        var indexer = (IPropertySymbol?)method.AssociatedSymbol;
-                        if (indexer is not null)
-                            defaultValue = GetParameterDefaultValue(indexer.Parameters[i]);
-                    }
-                }
-
-                if (defaultValue is not null)
-                {
-                    Context.PopulateLater(() =>
-                    {
-                        Expression.Create(Context, defaultValue.Value, this, 0);
-                    });
+                    var i = method.Parameters.IndexOf(symbol);
+                    if (method.AssociatedSymbol is IPropertySymbol indexer)
+                        defaultValue = GetParameterDefaultValue(indexer.Parameters[i]);
                 }
             }
+
+            return defaultValue;
         }
 
         public override bool IsSourceDeclaration => Symbol.IsSourceDeclaration();

csharp/ql/src/semmle/code/csharp/Element.qll

@@ -27,9 +27,6 @@ class Element extends DotNet::Element, @element {
   /** Gets a location of this element, including sources and assemblies. */
   override Location getALocation() { none() }
 
-  /** Holds if this element is from an assembly. */
-  predicate fromLibrary() { this.getFile().fromLibrary() }
-
   /** Gets the parent of this element, if any. */
   Element getParent() { result.getAChild() = this }
 

csharp/ql/src/semmle/code/dotnet/Element.qll

@@ -28,6 +28,9 @@ class Element extends @dotnet_element {
   /** Holds if this element is from source code. */
   predicate fromSource() { this.getFile().fromSource() }
 
+  /** Holds if this element is from an assembly. */
+  predicate fromLibrary() { this.getFile().fromLibrary() }
+
   /**
    * Gets the "language" of this program element, as defined by the extension of the filename.
    * For example, C# has language "cs", and Visual Basic has language "vb".

csharp/ql/test/library-tests/controlflow/graph/Common.qll

@@ -11,9 +11,15 @@ class SourceControlFlowElement extends ControlFlowElement {
 }
 
 class SourceControlFlowNode extends ControlFlow::Node {
-  SourceControlFlowNode() { not this.getLocation().getFile() instanceof StubFile }
+  SourceControlFlowNode() {
+    not this.getLocation().getFile() instanceof StubFile and
+    not this.getLocation().getFile().fromLibrary()
+  }
 }
 
 class SourceBasicBlock extends ControlFlow::BasicBlock {
-  SourceBasicBlock() { not this.getLocation().getFile() instanceof StubFile }
+  SourceBasicBlock() {
+    not this.getLocation().getFile() instanceof StubFile and
+    not this.getLocation().getFile().fromLibrary()
+  }
 }

csharp/ql/test/library-tests/controlflow/splits/SplittingStressTest.ql

@@ -1,6 +1,7 @@
 import csharp
 
 query predicate countSplits(ControlFlowElement cfe, int i) {
+  not cfe.fromLibrary() and
   i = strictcount(ControlFlow::Nodes::ElementNode n | n.getElement() = cfe)
 }
 

csharp/ql/test/library-tests/csharp7.1/DefaultLiterals.ql

@@ -1,4 +1,5 @@
 import csharp
 
 from DefaultValueExpr l
+where l.fromSource()
 select l, l.getValue()

csharp/ql/test/library-tests/csharp7/LocalTaintFlow.ql

@@ -2,5 +2,7 @@ import csharp
 import semmle.code.csharp.dataflow.TaintTracking
 
 from DataFlow::Node pred, DataFlow::Node succ
-where TaintTracking::localTaintStep(pred, succ)
+where
+  TaintTracking::localTaintStep(pred, succ) and
+  not pred.asExpr().fromLibrary()
 select pred, succ

csharp/ql/test/library-tests/dataflow/local/DataFlowStep.ql

@@ -1,5 +1,5 @@
 import csharp
 
 from DataFlow::Node pred, DataFlow::Node succ
-where DataFlow::localFlowStep(pred, succ)
+where not pred.asExpr().fromLibrary() and DataFlow::localFlowStep(pred, succ)
 select pred, succ

csharp/ql/test/library-tests/dataflow/local/TaintTrackingStep.ql

@@ -1,5 +1,7 @@
 import csharp
 
 from DataFlow::Node pred, DataFlow::Node succ
-where TaintTracking::localTaintStep(pred, succ)
+where
+  not pred.asExpr().fromLibrary() and
+  TaintTracking::localTaintStep(pred, succ)
 select pred, succ

csharp/ql/test/library-tests/dataflow/modulusanalysis/ModulusAnalysis.ql

@@ -4,5 +4,7 @@ import semmle.code.csharp.dataflow.ModulusAnalysis
 import semmle.code.csharp.dataflow.Bound
 
 from ControlFlow::Nodes::ExprNode e, Bound b, int delta, int mod
-where exprModulus(e, b, delta, mod)
+where
+  not e.getExpr().fromLibrary() and
+  exprModulus(e, b, delta, mod)
 select e, b.toString(), delta, mod

csharp/ql/test/library-tests/dataflow/signanalysis/SignAnalysis.ql

@@ -18,4 +18,5 @@ string getASignString(ControlFlow::Nodes::ExprNode e) {
 }
 
 from ControlFlow::Nodes::ExprNode e
+where not e.getExpr().fromLibrary()
 select e, strictconcat(string s | s = getASignString(e) | s, " ")

csharp/ql/test/library-tests/dataflow/tuples/DataFlowStep.ql

@@ -1,5 +1,5 @@
 import csharp
 
 from DataFlow::Node pred, DataFlow::Node succ
-where DataFlow::localFlowStep(pred, succ)
+where not pred.asExpr().fromLibrary() and DataFlow::localFlowStep(pred, succ)
 select pred, succ

csharp/ql/test/library-tests/exprorstmtparent/Parameter.ql

@@ -1,4 +1,5 @@
 import csharp
 
 from Parameter p
+where p.fromSource()
 select p, p.getDefaultValue()

csharp/ql/test/library-tests/goto/Goto1.ql

@@ -1,6 +1,7 @@
 import csharp
 
 query predicate edges(ControlFlow::Node node, ControlFlow::Node successor, string attr, string val) {
+  not node.getElement().fromLibrary() and
   exists(ControlFlow::SuccessorType t | successor = node.getASuccessorByType(t) |
     attr = "semmle.label" and
     val = t.toString()

csharp/ql/test/library-tests/parameters/Parameters.cs

@@ -0,0 +1,17 @@
+public class Parameters
+{
+    public void M1(int a, object b, string c) => throw null;
+    public void M2(int a, object b = null, string c = "default string") => throw null;
+    public void M3(int a = 1, object b = null, string c = "null") => throw null;
+    public void M4(int a = default, object b = default) => throw null;
+    public void M5(int a = new int(), object b = default) => throw null;
+    public void M6(MyStruct s1, MyStruct s2 = default(MyStruct), MyStruct s3 = new MyStruct()) => throw null;
+    public void M7(MyEnum e1, MyEnum e2 = default(MyEnum), MyEnum e3 = new MyEnum(), MyEnum e4 = MyEnum.A, MyEnum e5 = (MyEnum)5) => throw null;
+
+    public void M8<T>(T t = default) => throw null;
+    public void M9<T>(T t = default) where T : struct => throw null;
+    public void M10<T>(T t = default) where T : class => throw null;
+
+    public struct MyStruct { }
+    public enum MyEnum { A = 1, B = 2 }
+}

csharp/ql/test/library-tests/parameters/Parameters.cs_

@@ -0,0 +1,17 @@
+public class ParametersDll
+{
+    public void M1(int a, object b, string c) => throw null;
+    public void M2(int a, object b = null, string c = "default string") => throw null;
+    public void M3(int a = 1, object b = null, string c = "null") => throw null;
+    public void M4(int a = default, object b = default) => throw null;
+    public void M5(int a = new int(), object b = default) => throw null;
+    public void M6(MyStruct s1, MyStruct s2 = default(MyStruct), MyStruct s3 = new MyStruct()) => throw null;
+    public void M7(MyEnum e1, MyEnum e2 = default(MyEnum), MyEnum e3 = new MyEnum(), MyEnum e4 = MyEnum.A, MyEnum e5 = (MyEnum)5) => throw null;
+
+    public void M8<T>(T t = default) => throw null;
+    public void M9<T>(T t = default) where T : struct => throw null;
+    public void M10<T>(T t = default) where T : class => throw null;
+
+    public struct MyStruct { }
+    public enum MyEnum { A = 1, B = 2 }
+}

csharp/ql/test/library-tests/parameters/Parameters.expected

@@ -0,0 +1,50 @@
+noDefaultValue
+| Parameters.cs:3:17:3:18 | M1 | Parameters.cs:3:24:3:24 | a | 0 |
+| Parameters.cs:3:17:3:18 | M1 | Parameters.cs:3:34:3:34 | b | 1 |
+| Parameters.cs:3:17:3:18 | M1 | Parameters.cs:3:44:3:44 | c | 2 |
+| Parameters.cs:4:17:4:18 | M2 | Parameters.cs:4:24:4:24 | a | 0 |
+| Parameters.cs:8:17:8:18 | M6 | Parameters.cs:8:29:8:30 | s1 | 0 |
+| Parameters.cs:9:17:9:18 | M7 | Parameters.cs:9:27:9:28 | e1 | 0 |
+| Parameters.dll:0:0:0:0 | M1 | Parameters.dll:0:0:0:0 | a | 0 |
+| Parameters.dll:0:0:0:0 | M1 | Parameters.dll:0:0:0:0 | b | 1 |
+| Parameters.dll:0:0:0:0 | M1 | Parameters.dll:0:0:0:0 | c | 2 |
+| Parameters.dll:0:0:0:0 | M2 | Parameters.dll:0:0:0:0 | a | 0 |
+| Parameters.dll:0:0:0:0 | M6 | Parameters.dll:0:0:0:0 | s1 | 0 |
+| Parameters.dll:0:0:0:0 | M7 | Parameters.dll:0:0:0:0 | e1 | 0 |
+withDefaultValue
+| Parameters.cs:4:17:4:18 | M2 | Parameters.cs:4:34:4:34 | b | 1 | Parameters.cs:4:38:4:41 | null | null |
+| Parameters.cs:4:17:4:18 | M2 | Parameters.cs:4:51:4:51 | c | 2 | Parameters.cs:4:55:4:70 | "default string" | default string |
+| Parameters.cs:5:17:5:18 | M3 | Parameters.cs:5:24:5:24 | a | 0 | Parameters.cs:5:28:5:28 | 1 | 1 |
+| Parameters.cs:5:17:5:18 | M3 | Parameters.cs:5:38:5:38 | b | 1 | Parameters.cs:5:42:5:45 | null | null |
+| Parameters.cs:5:17:5:18 | M3 | Parameters.cs:5:55:5:55 | c | 2 | Parameters.cs:5:59:5:64 | "null" | null |
+| Parameters.cs:6:17:6:18 | M4 | Parameters.cs:6:24:6:24 | a | 0 | Parameters.cs:6:28:6:34 | (...) ... | 0 |
+| Parameters.cs:6:17:6:18 | M4 | Parameters.cs:6:44:6:44 | b | 1 | Parameters.cs:6:48:6:54 | default | null |
+| Parameters.cs:7:17:7:18 | M5 | Parameters.cs:7:24:7:24 | a | 0 | Parameters.cs:7:28:7:36 | object creation of type Int32 | 0 |
+| Parameters.cs:7:17:7:18 | M5 | Parameters.cs:7:46:7:46 | b | 1 | Parameters.cs:7:50:7:56 | default | null |
+| Parameters.cs:8:17:8:18 | M6 | Parameters.cs:8:42:8:43 | s2 | 1 | Parameters.cs:8:47:8:63 | default(...) | - |
+| Parameters.cs:8:17:8:18 | M6 | Parameters.cs:8:75:8:76 | s3 | 2 | Parameters.cs:8:80:8:93 | object creation of type MyStruct | - |
+| Parameters.cs:9:17:9:18 | M7 | Parameters.cs:9:38:9:39 | e2 | 1 | Parameters.cs:9:43:9:57 | default(...) | 0 |
+| Parameters.cs:9:17:9:18 | M7 | Parameters.cs:9:67:9:68 | e3 | 2 | Parameters.cs:9:72:9:83 | object creation of type MyEnum | 0 |
+| Parameters.cs:9:17:9:18 | M7 | Parameters.cs:9:93:9:94 | e4 | 3 | Parameters.cs:9:98:9:105 | access to constant A | 1 |
+| Parameters.cs:9:17:9:18 | M7 | Parameters.cs:9:115:9:116 | e5 | 4 | Parameters.cs:9:120:9:128 | (...) ... | 5 |
+| Parameters.cs:11:17:11:21 | M8 | Parameters.cs:11:25:11:25 | t | 0 | Parameters.cs:11:29:11:35 | (...) ... | - |
+| Parameters.cs:12:17:12:21 | M9 | Parameters.cs:12:25:12:25 | t | 0 | Parameters.cs:12:29:12:35 | (...) ... | - |
+| Parameters.cs:13:17:13:22 | M10 | Parameters.cs:13:26:13:26 | t | 0 | Parameters.cs:13:30:13:36 | (...) ... | null |
+| Parameters.dll:0:0:0:0 | M2 | Parameters.dll:0:0:0:0 | b | 1 | Parameters.dll:0:0:0:0 | default | null |
+| Parameters.dll:0:0:0:0 | M2 | Parameters.dll:0:0:0:0 | c | 2 | Parameters.dll:0:0:0:0 | "default string" | default string |
+| Parameters.dll:0:0:0:0 | M3 | Parameters.dll:0:0:0:0 | a | 0 | Parameters.dll:0:0:0:0 | 1 | 1 |
+| Parameters.dll:0:0:0:0 | M3 | Parameters.dll:0:0:0:0 | b | 1 | Parameters.dll:0:0:0:0 | default | null |
+| Parameters.dll:0:0:0:0 | M3 | Parameters.dll:0:0:0:0 | c | 2 | Parameters.dll:0:0:0:0 | "null" | null |
+| Parameters.dll:0:0:0:0 | M4 | Parameters.dll:0:0:0:0 | a | 0 | Parameters.dll:0:0:0:0 | 0 | 0 |
+| Parameters.dll:0:0:0:0 | M4 | Parameters.dll:0:0:0:0 | b | 1 | Parameters.dll:0:0:0:0 | default | null |
+| Parameters.dll:0:0:0:0 | M5 | Parameters.dll:0:0:0:0 | a | 0 | Parameters.dll:0:0:0:0 | 0 | 0 |
+| Parameters.dll:0:0:0:0 | M5 | Parameters.dll:0:0:0:0 | b | 1 | Parameters.dll:0:0:0:0 | default | null |
+| Parameters.dll:0:0:0:0 | M6 | Parameters.dll:0:0:0:0 | s2 | 1 | Parameters.dll:0:0:0:0 | default | - |
+| Parameters.dll:0:0:0:0 | M6 | Parameters.dll:0:0:0:0 | s3 | 2 | Parameters.dll:0:0:0:0 | default | - |
+| Parameters.dll:0:0:0:0 | M7 | Parameters.dll:0:0:0:0 | e2 | 1 | Parameters.dll:0:0:0:0 | (...) ... | 0 |
+| Parameters.dll:0:0:0:0 | M7 | Parameters.dll:0:0:0:0 | e3 | 2 | Parameters.dll:0:0:0:0 | (...) ... | 0 |
+| Parameters.dll:0:0:0:0 | M7 | Parameters.dll:0:0:0:0 | e4 | 3 | Parameters.dll:0:0:0:0 | (...) ... | 1 |
+| Parameters.dll:0:0:0:0 | M7 | Parameters.dll:0:0:0:0 | e5 | 4 | Parameters.dll:0:0:0:0 | (...) ... | 5 |
+| Parameters.dll:0:0:0:0 | M8 | Parameters.dll:0:0:0:0 | t | 0 | Parameters.dll:0:0:0:0 | default | - |
+| Parameters.dll:0:0:0:0 | M9 | Parameters.dll:0:0:0:0 | t | 0 | Parameters.dll:0:0:0:0 | default | - |
+| Parameters.dll:0:0:0:0 | M10 | Parameters.dll:0:0:0:0 | t | 0 | Parameters.dll:0:0:0:0 | default | null |

csharp/ql/test/library-tests/parameters/Parameters.ql

@@ -0,0 +1,19 @@
+import csharp
+
+private predicate fromTestLocation(Element e) {
+  e.fromSource() or e.getFile().getStem() = "Parameters"
+}
+
+query predicate noDefaultValue(Parameterizable container, Parameter p, int i) {
+  fromTestLocation(container) and
+  not p.hasDefaultValue() and
+  container.getParameter(i) = p
+}
+
+query predicate withDefaultValue(Parameterizable container, Parameter p, int i, Expr e, string value) {
+  fromTestLocation(container) and
+  p.hasDefaultValue() and
+  container.getParameter(i) = p and
+  p.getDefaultValue() = e and
+  if exists(e.getValue()) then value = e.getValue() else value = "-"
+}

csharp/ql/test/library-tests/standalone/controlflow/cfg.ql

@@ -17,4 +17,6 @@ class UnknownLocalVariableDeclExpr extends LocalVariableDeclAndInitExpr {
   override string toString() { result = "(unknown type) " + this.getName() }
 }
 
-query predicate edges(ControlFlow::Node n1, ControlFlow::Node n2) { n2 = n1.getASuccessor() }
+query predicate edges(ControlFlow::Node n1, ControlFlow::Node n2) {
+  not n1.getElement().fromLibrary() and n2 = n1.getASuccessor()
+}