Merge pull request #18446 from michaelnebel/csharp/implicittostring2

C#: Adding synthetic implicit ToString calls in binary- and string interpolation expressions.

csharp/extractor/Semmle.Extraction.CSharp/CodeAnalysisExtensions/SymbolExtensions.cs

@@ -532,6 +532,12 @@ attribute.AttributeClass is INamedTypeSymbol nt &&
             return isInline;
         }
 
+        /// <summary>
+        /// Returns true if this type implements `System.IFormattable`.
+        /// </summary>
+        public static bool ImplementsIFormattable(this ITypeSymbol type) =>
+            type.AllInterfaces.Any(i => i.Name == "IFormattable" && i.ContainingNamespace.ToString() == "System");
+
         /// <summary>
         /// Holds if this type is of the form <code>System.ReadOnlySpan&lt;byte&gt;</code>.
         /// </summary>

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

@@ -129,7 +129,7 @@ public static void CreateDeferred(Context cx, ExpressionSyntax node, IExpression
                 cx.PopulateLater(() => Create(cx, node, parent, child));
         }
 
-        private static bool ContainsPattern(SyntaxNode node) =>
+        protected static bool ContainsPattern(SyntaxNode node) =>
             node is PatternSyntax || node is VariableDesignationSyntax || node.ChildNodes().Any(ContainsPattern);
 
         /// <summary>

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

@@ -129,7 +129,13 @@ public Location Location
 
         public ExprKind Kind { get; set; } = ExprKind.UNKNOWN;
 
-        public bool IsCompilerGenerated { get; set; }
+        public bool IsCompilerGenerated { get; init; }
+
+        /// <summary>
+        /// Whether the expression should have a compiler generated `ToString` call added,
+        /// if there is no suitable implicit cast.
+        /// </summary>
+        public bool ImplicitToString { get; private set; }
 
         public ExpressionNodeInfo SetParent(IExpressionParentEntity parent, int child)
         {
@@ -157,6 +163,12 @@ public ExpressionNodeInfo SetNode(ExpressionSyntax node)
             return this;
         }
 
+        public ExpressionNodeInfo SetImplicitToString(bool value)
+        {
+            ImplicitToString = value;
+            return this;
+        }
+
         private SymbolInfo cachedSymbolInfo;
 
         public SymbolInfo SymbolInfo

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

@@ -14,11 +14,35 @@ private Binary(ExpressionNodeInfo info)
 
         public static Expression Create(ExpressionNodeInfo info) => new Binary(info).TryPopulate();
 
+        private Expression CreateChild(Context cx, ExpressionSyntax node, int child)
+        {
+            // If this is a "+" expression we might need to wrap the child expressions
+            // in ToString calls
+            return Kind == ExprKind.ADD
+                ? ImplicitToString.Create(cx, node, this, child)
+                : Create(cx, node, this, child);
+        }
+
+        /// <summary>
+        /// Creates an expression from a syntax node.
+        /// Inserts type conversion as required.
+        /// Population is deferred to avoid overflowing the stack.
+        /// </summary>
+        private void CreateDeferred(Context cx, ExpressionSyntax node, int child)
+        {
+            if (ContainsPattern(node))
+                // Expressions with patterns should be created right away, as they may introduce
+                // local variables referenced in `LocalVariable::GetAlreadyCreated()`
+                CreateChild(cx, node, child);
+            else
+                cx.PopulateLater(() => CreateChild(cx, node, child));
+        }
+
         protected override void PopulateExpression(TextWriter trapFile)
         {
             OperatorCall(trapFile, Syntax);
-            CreateDeferred(Context, Syntax.Left, this, 0);
-            CreateDeferred(Context, Syntax.Right, this, 1);
+            CreateDeferred(Context, Syntax.Left, 0);
+            CreateDeferred(Context, Syntax.Right, 1);
         }
 
         private static ExprKind GetKind(Context cx, BinaryExpressionSyntax node)

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

@@ -156,6 +156,12 @@ convertedType.Symbol is IPointerTypeSymbol &&
                 return new ImplicitCast(info);
             }
 
+            if (info.ImplicitToString)
+            {
+                // x -> x.ToString() in "abc" + x
+                return ImplicitToString.Wrap(info);
+            }
+
             // Default: Just create the expression without a conversion.
             return Factory.Create(info);
         }

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

@@ -0,0 +1,59 @@
+using System.Linq;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+using Semmle.Extraction.CSharp.Util;
+using Semmle.Extraction.Kinds;
+
+
+namespace Semmle.Extraction.CSharp.Entities.Expressions
+{
+    internal sealed class ImplicitToString : Expression
+    {
+        /// <summary>
+        /// Gets the `ToString` method for the given type.
+        /// </summary>
+        private static IMethodSymbol? GetToStringMethod(ITypeSymbol? type)
+        {
+            return type?
+                .GetMembers()
+                .OfType<IMethodSymbol>()
+                .Where(method =>
+                    method.GetName() == "ToString" &&
+                    method.Parameters.Length == 0
+                )
+                .FirstOrDefault();
+        }
+
+        private ImplicitToString(ExpressionNodeInfo info, IMethodSymbol toString) : base(new ExpressionInfo(info.Context, AnnotatedTypeSymbol.CreateNotAnnotated(toString.ReturnType), info.Location, ExprKind.METHOD_INVOCATION, info.Parent, info.Child, isCompilerGenerated: true, info.ExprValue))
+        {
+            Factory.Create(info.SetParent(this, -1));
+
+            var target = Method.Create(Context, toString);
+            Context.TrapWriter.Writer.expr_call(this, target);
+        }
+
+        private static bool IsStringType(AnnotatedTypeSymbol? type) =>
+            type.HasValue && type.Value.Symbol?.SpecialType == SpecialType.System_String;
+
+        /// <summary>
+        /// Creates a new expression, adding a compiler generated `ToString` call if required.
+        /// </summary>
+        public static Expression Create(Context cx, ExpressionSyntax node, Expression parent, int child)
+        {
+            var info = new ExpressionNodeInfo(cx, node, parent, child);
+            return CreateFromNode(info.SetImplicitToString(IsStringType(parent.Type) && !IsStringType(info.Type)));
+        }
+
+        /// <summary>
+        /// Wraps the resulting expression in a `ToString` call, if a suitable `ToString` method is available.
+        /// </summary>
+        public static Expression Wrap(ExpressionNodeInfo info)
+        {
+            if (GetToStringMethod(info.Type?.Symbol) is IMethodSymbol toString)
+            {
+                return new ImplicitToString(info, toString);
+            }
+            return Factory.Create(info);
+        }
+    }
+}

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

@@ -1,4 +1,5 @@
 using System.IO;
+using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.CSharp;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Semmle.Extraction.Kinds;
@@ -20,7 +21,15 @@ protected override void PopulateExpression(TextWriter trapFile)
                 {
                     case SyntaxKind.Interpolation:
                         var interpolation = (InterpolationSyntax)c;
-                        Create(Context, interpolation.Expression, this, child++);
+                        var exp = interpolation.Expression;
+                        if (Context.GetTypeInfo(exp).Type is ITypeSymbol type && !type.ImplementsIFormattable())
+                        {
+                            ImplicitToString.Create(Context, exp, this, child++);
+                        }
+                        else
+                        {
+                            Create(Context, exp, this, child++);
+                        }
                         break;
                     case SyntaxKind.InterpolatedStringText:
                         // Create a string literal

csharp/ql/examples/snippets/ternary_conditional.ql

@@ -11,7 +11,7 @@ import csharp
 
 from ConditionalExpr e
 where
-  e.getThen().stripImplicitCasts() != e.getElse().stripImplicitCasts() and
+  e.getThen().stripImplicit() != e.getElse().stripImplicit() and
   not e.getThen().getType() instanceof NullType and
   not e.getElse().getType() instanceof NullType
 select e

csharp/ql/lib/change-notes/2025-01-09-implicit-to-string.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Added extractor support for extracting implicit `ToString` calls in binary `+` expressions and string interpolation expressions.

csharp/ql/lib/ext/Microsoft.AspNetCore.Http.model.yml

@@ -0,0 +1,6 @@
+extensions:
+  - addsTo:
+      pack: codeql/csharp-all
+      extensible: summaryModel
+    data:
+      - ["Microsoft.AspNetCore.Http", "PathString", True, "ToString", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]

csharp/ql/lib/semmle/code/csharp/PrintAst.qll

@@ -32,7 +32,9 @@ private predicate shouldPrint(Element e, Location l) {
 }
 
 private predicate isImplicitExpression(ControlFlowElement element) {
-  element.(Expr).isImplicit() and
+  // Include compiler generated cast expressions and `ToString` calls if
+  // they wrap actual source expressions.
+  element.(Expr).stripImplicit().isImplicit() and
   not element instanceof CastExpr and
   not element.(OperatorCall).getTarget() instanceof ImplicitConversionOperator and
   not element instanceof ElementInitializer

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

@@ -43,7 +43,7 @@ predicate isConstantComparison(ComparisonOperation co, boolean b) {
 private module ConstantComparisonOperation {
   private import semmle.code.csharp.commons.ComparisonTest
 
-  private SimpleType convertedType(Expr expr) { result = expr.stripImplicitCasts().getType() }
+  private SimpleType convertedType(Expr expr) { result = expr.stripImplicit().getType() }
 
   private int maxValue(Expr expr) {
     if convertedType(expr) instanceof IntegralType and exists(expr.getValue())

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

@@ -44,11 +44,11 @@ class ImplicitToStringExpr extends Expr {
     )
     or
     exists(AddExpr add, Expr o | o = add.getAnOperand() |
-      o.stripImplicitCasts().getType() instanceof StringType and
-      this = add.getOtherOperand(o)
+      o.stripImplicit().getType() instanceof StringType and
+      this = add.getOtherOperand(o).stripImplicit()
     )
     or
-    this = any(InterpolatedStringExpr ise).getAnInsert()
+    this = any(InterpolatedStringExpr ise).getAnInsert().stripImplicit()
   }
 }
 

csharp/ql/lib/semmle/code/csharp/dataflow/Nullness.qll

@@ -526,7 +526,11 @@ class Dereference extends G::DereferenceableExpr {
         not underlyingType instanceof NullableType
       )
     ) else (
-      this = any(QualifiableExpr qe | not qe.isConditional()).getQualifier() and
+      this =
+        any(QualifiableExpr qe |
+          not qe.isConditional() and
+          not qe.(MethodCall).isImplicit()
+        ).getQualifier() and
       not this instanceof ThisAccess and
       not this instanceof BaseAccess and
       not this instanceof TypeAccess

csharp/ql/lib/semmle/code/csharp/dispatch/Dispatch.qll

@@ -857,7 +857,7 @@ private module Internal {
     private predicate hasDynamicArg(int i, Type argumentType) {
       exists(Expr argument |
         argument = this.getArgument(i) and
-        argument.stripImplicitCasts().getType() instanceof DynamicType and
+        argument.stripImplicit().getType() instanceof DynamicType and
         argumentType = getAPossibleType(argument, _)
       )
     }

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

@@ -281,6 +281,10 @@ class MethodCall extends Call, QualifiableExpr, LateBindableExpr, @method_invoca
       result = this.getArgument(i - 1)
     else result = this.getArgument(i)
   }
+
+  override Expr stripImplicit() {
+    if this.isImplicit() then result = this.getQualifier().stripImplicit() else result = this
+  }
 }
 
 /**

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

@@ -82,10 +82,18 @@ class Expr extends ControlFlowElement, @expr {
   Expr stripCasts() { result = this }
 
   /**
+   * DEPRECATED: Use `stripImplicit` instead.
+   *
    * Gets an expression that is the result of stripping (recursively) all
    * implicit casts from this expression, if any.
    */
-  Expr stripImplicitCasts() { result = this }
+  deprecated Expr stripImplicitCasts() { result = this.stripImplicit() }
+
+  /**
+   * Gets an expression that is the result of stripping (recursively) all
+   * implicit casts and implicit ToString calls from this expression, if any.
+   */
+  Expr stripImplicit() { result = this }
 
   /**
    * Gets the explicit parameter name used to pass this expression as an
@@ -714,8 +722,8 @@ class Cast extends Expr {
 
   override Expr stripCasts() { result = this.getExpr().stripCasts() }
 
-  override Expr stripImplicitCasts() {
-    if this.isImplicit() then result = this.getExpr().stripImplicitCasts() else result = this
+  override Expr stripImplicit() {
+    if this.isImplicit() then result = this.getExpr().stripImplicit() else result = this
   }
 }
 

csharp/ql/src/Bad Practices/VirtualCallInConstructorOrDestructor.ql

@@ -34,7 +34,9 @@ predicate overriddenSealed(RefType t, Virtualizable d) {
 }
 
 predicate virtualAccessWithThisQualifier(Expr e, Member d) {
-  exists(VirtualMethodCall c | c = e and c.getTarget() = d and c.hasThisQualifier())
+  exists(VirtualMethodCall c |
+    c = e and c.getTarget() = d and c.hasThisQualifier() and not c.isImplicit()
+  )
   or
   exists(VirtualMethodAccess c | c = e and c.getTarget() = d and c.hasThisQualifier())
   or

csharp/ql/src/Likely Bugs/Dynamic/BadDynamicCall.ql

@@ -44,11 +44,11 @@ abstract class BadDynamicCall extends DynamicExpr {
       ultimateSsaDef = ssaDef.getAnUltimateDefinition()
     |
       ultimateSsaDef.getADefinition() =
-        any(AssignableDefinition def | source = def.getSource().stripImplicitCasts())
+        any(AssignableDefinition def | source = def.getSource().stripImplicit())
       or
       ultimateSsaDef.getADefinition() =
         any(AssignableDefinitions::ImplicitParameterDefinition p |
-          source = p.getParameter().getAnAssignedValue().stripImplicitCasts()
+          source = p.getParameter().getAnAssignedValue().stripImplicit()
         )
     )
   }

csharp/ql/src/Likely Bugs/ObjectComparison.ql

@@ -28,7 +28,7 @@ class ReferenceEqualityTestOnObject extends EqualityOperation {
     exists(getObjectOperand(this)) and
     // Neither operand is 'null'.
     not this.getAnOperand() instanceof NullLiteral and
-    not exists(Type t | t = this.getAnOperand().stripImplicitCasts().getType() |
+    not exists(Type t | t = this.getAnOperand().stripImplicit().getType() |
       t instanceof NullType or
       t instanceof ValueType
     ) and

csharp/ql/test/library-tests/dataflow/global/GetAnOutNode.expected

@@ -176,6 +176,7 @@
 | GlobalDataFlowStringBuilder.cs:36:21:36:34 | call to method ToString | normal | GlobalDataFlowStringBuilder.cs:36:21:36:34 | call to method ToString |
 | GlobalDataFlowStringBuilder.cs:39:19:39:37 | object creation of type StringBuilder | normal | GlobalDataFlowStringBuilder.cs:39:19:39:37 | object creation of type StringBuilder |
 | GlobalDataFlowStringBuilder.cs:40:9:40:27 | call to method Append | normal | GlobalDataFlowStringBuilder.cs:40:9:40:27 | call to method Append |
+| GlobalDataFlowStringBuilder.cs:40:23:40:24 | call to method ToString | normal | GlobalDataFlowStringBuilder.cs:40:23:40:24 | call to method ToString |
 | GlobalDataFlowStringBuilder.cs:41:21:41:34 | call to method ToString | normal | GlobalDataFlowStringBuilder.cs:41:21:41:34 | call to method ToString |
 | GlobalDataFlowStringBuilder.cs:44:9:44:18 | call to method Clear | normal | GlobalDataFlowStringBuilder.cs:44:9:44:18 | call to method Clear |
 | GlobalDataFlowStringBuilder.cs:45:23:45:35 | call to method ToString | normal | GlobalDataFlowStringBuilder.cs:45:23:45:35 | call to method ToString |

csharp/ql/test/library-tests/dataflow/global/TaintTrackingPath.expected

@@ -514,7 +514,7 @@ edges
 | GlobalDataFlowStringBuilder.cs:24:19:24:26 | (...) ... : AppendInterpolatedStringHandler | GlobalDataFlowStringBuilder.cs:24:9:24:10 | [post] access to parameter sb : StringBuilder | provenance | MaD:16 |
 | GlobalDataFlowStringBuilder.cs:30:31:30:32 | [post] access to local variable sb : StringBuilder | GlobalDataFlowStringBuilder.cs:31:21:31:22 | access to local variable sb : StringBuilder | provenance |  |
 | GlobalDataFlowStringBuilder.cs:30:31:30:32 | [post] access to local variable sb : StringBuilder | GlobalDataFlowStringBuilder.cs:35:20:35:21 | access to local variable sb : StringBuilder | provenance |  |
-| GlobalDataFlowStringBuilder.cs:30:31:30:32 | [post] access to local variable sb : StringBuilder | GlobalDataFlowStringBuilder.cs:40:20:40:26 | (...) ... : AppendInterpolatedStringHandler | provenance |  |
+| GlobalDataFlowStringBuilder.cs:30:31:30:32 | [post] access to local variable sb : StringBuilder | GlobalDataFlowStringBuilder.cs:40:23:40:24 | access to local variable sb : StringBuilder | provenance |  |
 | GlobalDataFlowStringBuilder.cs:30:35:30:48 | "taint source" : String | GlobalDataFlowStringBuilder.cs:17:64:17:64 | s : String | provenance |  |
 | GlobalDataFlowStringBuilder.cs:30:35:30:48 | "taint source" : String | GlobalDataFlowStringBuilder.cs:30:31:30:32 | [post] access to local variable sb : StringBuilder | provenance | MaD:14 |
 | GlobalDataFlowStringBuilder.cs:31:13:31:17 | access to local variable sink0 : String | GlobalDataFlowStringBuilder.cs:32:15:32:19 | access to local variable sink0 | provenance |  |
@@ -527,6 +527,8 @@ edges
 | GlobalDataFlowStringBuilder.cs:36:21:36:34 | call to method ToString : String | GlobalDataFlowStringBuilder.cs:36:13:36:17 | access to local variable sink1 : String | provenance |  |
 | GlobalDataFlowStringBuilder.cs:40:9:40:11 | [post] access to local variable sb2 : StringBuilder | GlobalDataFlowStringBuilder.cs:41:21:41:23 | access to local variable sb2 : StringBuilder | provenance |  |
 | GlobalDataFlowStringBuilder.cs:40:20:40:26 | (...) ... : AppendInterpolatedStringHandler | GlobalDataFlowStringBuilder.cs:40:9:40:11 | [post] access to local variable sb2 : StringBuilder | provenance | MaD:16 |
+| GlobalDataFlowStringBuilder.cs:40:23:40:24 | access to local variable sb : StringBuilder | GlobalDataFlowStringBuilder.cs:40:23:40:24 | call to method ToString : String | provenance | MaD:17 |
+| GlobalDataFlowStringBuilder.cs:40:23:40:24 | call to method ToString : String | GlobalDataFlowStringBuilder.cs:40:20:40:26 | (...) ... : AppendInterpolatedStringHandler | provenance |  |
 | GlobalDataFlowStringBuilder.cs:41:13:41:17 | access to local variable sink2 : String | GlobalDataFlowStringBuilder.cs:42:15:42:19 | access to local variable sink2 | provenance |  |
 | GlobalDataFlowStringBuilder.cs:41:21:41:23 | access to local variable sb2 : StringBuilder | GlobalDataFlowStringBuilder.cs:41:21:41:34 | call to method ToString : String | provenance | MaD:17 |
 | GlobalDataFlowStringBuilder.cs:41:21:41:34 | call to method ToString : String | GlobalDataFlowStringBuilder.cs:41:13:41:17 | access to local variable sink2 : String | provenance |  |
@@ -1046,6 +1048,8 @@ nodes
 | GlobalDataFlowStringBuilder.cs:37:15:37:19 | access to local variable sink1 | semmle.label | access to local variable sink1 |
 | GlobalDataFlowStringBuilder.cs:40:9:40:11 | [post] access to local variable sb2 : StringBuilder | semmle.label | [post] access to local variable sb2 : StringBuilder |
 | GlobalDataFlowStringBuilder.cs:40:20:40:26 | (...) ... : AppendInterpolatedStringHandler | semmle.label | (...) ... : AppendInterpolatedStringHandler |
+| GlobalDataFlowStringBuilder.cs:40:23:40:24 | access to local variable sb : StringBuilder | semmle.label | access to local variable sb : StringBuilder |
+| GlobalDataFlowStringBuilder.cs:40:23:40:24 | call to method ToString : String | semmle.label | call to method ToString : String |
 | GlobalDataFlowStringBuilder.cs:41:13:41:17 | access to local variable sink2 : String | semmle.label | access to local variable sink2 : String |
 | GlobalDataFlowStringBuilder.cs:41:21:41:23 | access to local variable sb2 : StringBuilder | semmle.label | access to local variable sb2 : StringBuilder |
 | GlobalDataFlowStringBuilder.cs:41:21:41:34 | call to method ToString : String | semmle.label | call to method ToString : String |