Simplify parameter names (#35200)

Closes #35113
Fixes #35196

Author: roji

src/EFCore.Cosmos/Query/Internal/CosmosSqlTranslatingExpressionVisitor.cs

@@ -24,7 +24,7 @@ public class CosmosSqlTranslatingExpressionVisitor(
     QueryableMethodTranslatingExpressionVisitor queryableMethodTranslatingExpressionVisitor)
     : ExpressionVisitor
 {
-    private const string RuntimeParameterPrefix = QueryCompilationContext.QueryParameterPrefix + "entity_equality_";
+    private const string RuntimeParameterPrefix = "entity_equality_";
 
     private static readonly MethodInfo ParameterValueExtractorMethod =
         typeof(CosmosSqlTranslatingExpressionVisitor).GetTypeInfo().GetDeclaredMethod(nameof(ParameterValueExtractor))!;
@@ -1040,9 +1040,7 @@ private bool TryRewriteContainsEntity(Expression source, Expression item, [NotNu
                     QueryCompilationContext.QueryContextParameter
                 );
 
-                var newParameterName =
-                    $"{RuntimeParameterPrefix}"
-                    + $"{sqlParameterExpression.Name[QueryCompilationContext.QueryParameterPrefix.Length..]}_{property.Name}";
+                var newParameterName = $"{RuntimeParameterPrefix}{sqlParameterExpression.Name}_{property.Name}";
 
                 rewrittenSource = queryCompilationContext.RegisterRuntimeParameter(newParameterName, lambda);
                 break;
@@ -1165,9 +1163,7 @@ private Expression CreatePropertyAccessExpression(Expression target, IProperty p
                         Expression.Constant(property, typeof(IProperty))),
                     QueryCompilationContext.QueryContextParameter);
 
-                var newParameterName =
-                    $"{RuntimeParameterPrefix}"
-                    + $"{sqlParameterExpression.Name[QueryCompilationContext.QueryParameterPrefix.Length..]}_{property.Name}";
+                var newParameterName = $"{RuntimeParameterPrefix}{sqlParameterExpression.Name}_{property.Name}";
 
                 return queryCompilationContext.RegisterRuntimeParameter(newParameterName, lambda);
 

src/EFCore.InMemory/Query/Internal/InMemoryExpressionTranslatingExpressionVisitor.cs

@@ -18,7 +18,7 @@ namespace Microsoft.EntityFrameworkCore.InMemory.Query.Internal;
 /// </summary>
 public class InMemoryExpressionTranslatingExpressionVisitor : ExpressionVisitor
 {
-    private const string RuntimeParameterPrefix = QueryCompilationContext.QueryParameterPrefix + "entity_equality_";
+    private const string RuntimeParameterPrefix = "entity_equality_";
 
     private static readonly List<MethodInfo> SingleResultMethodInfos =
     [
@@ -1298,9 +1298,7 @@ when methodCallExpression.Method.GetGenericMethodDefinition() == GetParameterVal
                     QueryCompilationContext.QueryContextParameter
                 );
 
-                var newParameterName =
-                    $"{RuntimeParameterPrefix}"
-                    + $"{parameterName[QueryCompilationContext.QueryParameterPrefix.Length..]}_{property.Name}";
+                var newParameterName = $"{RuntimeParameterPrefix}{parameterName}_{property.Name}";
 
                 rewrittenSource = _queryCompilationContext.RegisterRuntimeParameter(newParameterName, lambda);
                 break;
@@ -1442,9 +1440,7 @@ when methodCallExpression.Method.GetGenericMethodDefinition() == GetParameterVal
                         Expression.Constant(property, typeof(IProperty))),
                     QueryCompilationContext.QueryContextParameter);
 
-                var newParameterName =
-                    $"{RuntimeParameterPrefix}"
-                    + $"{parameterName[QueryCompilationContext.QueryParameterPrefix.Length..]}_{property.Name}";
+                var newParameterName = $"{RuntimeParameterPrefix}{parameterName}_{property.Name}";
 
                 return _queryCompilationContext.RegisterRuntimeParameter(newParameterName, lambda);
 

src/EFCore.Relational/Query/Internal/FromSqlParameterExpandingExpressionVisitor.cs renamed to src/EFCore.Relational/Query/Internal/RelationalParameterProcessor.cs

@@ -1,27 +1,40 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-using System.Diagnostics.CodeAnalysis;
 using Microsoft.EntityFrameworkCore.Query.SqlExpressions;
 using Microsoft.EntityFrameworkCore.Storage.Internal;
 
 namespace Microsoft.EntityFrameworkCore.Query.Internal;
 
 /// <summary>
+///     This visitor processes the parameters of <see cref="FromSqlExpression" />, expanding them and creating the appropriate
+///     <see cref="IRelationalParameter" /> for them, and ensures parameter names are unique across the SQL tree.
+/// </summary>
+/// <remarks>
 ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
 ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
 ///     any release. You should only use it directly in your code with extreme caution and knowing that
 ///     doing so can result in application failures when updating to a new Entity Framework Core release.
-/// </summary>
-public class FromSqlParameterExpandingExpressionVisitor : ExpressionVisitor
+/// </remarks>
+public class RelationalParameterProcessor : ExpressionVisitor
 {
     private readonly IDictionary<FromSqlExpression, Expression> _visitedFromSqlExpressions
         = new Dictionary<FromSqlExpression, Expression>(ReferenceEqualityComparer.Instance);
 
     private readonly ISqlExpressionFactory _sqlExpressionFactory;
     private readonly IRelationalTypeMappingSource _typeMappingSource;
+    private readonly ISqlGenerationHelper _sqlGenerationHelper;
     private readonly IParameterNameGeneratorFactory _parameterNameGeneratorFactory;
 
+    /// <summary>
+    ///     Contains parameter names seen so far, for uniquification. These parameter names have already gone through
+    ///     <see cref="ISqlGenerationHelper.GenerateParameterName(string)"/> (i.e. they're prefixed), since
+    ///     <see cref="DbParameter.ParameterName" /> can be prefixed or not.
+    /// </summary>
+    private readonly HashSet<string> _prefixedParameterNames = new();
+
+    private readonly Dictionary<string, SqlParameterExpression> _sqlParameters = new();
+
     private IReadOnlyDictionary<string, object?> _parametersValues;
     private ParameterNameGenerator _parameterNameGenerator;
     private bool _canCache;
@@ -32,14 +45,15 @@ private readonly IDictionary<FromSqlExpression, Expression> _visitedFromSqlExpre
     ///     any release. You should only use it directly in your code with extreme caution and knowing that
     ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
-    public FromSqlParameterExpandingExpressionVisitor(
+    public RelationalParameterProcessor(
         RelationalParameterBasedSqlProcessorDependencies dependencies)
     {
         Dependencies = dependencies;
 
         _sqlExpressionFactory = dependencies.SqlExpressionFactory;
         _typeMappingSource = dependencies.TypeMappingSource;
         _parameterNameGeneratorFactory = dependencies.ParameterNameGeneratorFactory;
+        _sqlGenerationHelper = dependencies.SqlGenerationHelper;
         _parametersValues = default!;
         _parameterNameGenerator = default!;
     }
@@ -61,6 +75,8 @@ public virtual Expression Expand(
         out bool canCache)
     {
         _visitedFromSqlExpressions.Clear();
+        _prefixedParameterNames.Clear();
+        _sqlParameters.Clear();
         _parameterNameGenerator = _parameterNameGeneratorFactory.Create();
         _parametersValues = parameterValues;
         _canCache = true;
@@ -77,19 +93,55 @@ public virtual Expression Expand(
     ///     any release. You should only use it directly in your code with extreme caution and knowing that
     ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
-    [return: NotNullIfNotNull(nameof(expression))]
-    public override Expression? Visit(Expression? expression)
-    {
-        if (expression is not FromSqlExpression fromSql)
+    protected override Expression VisitExtension(Expression expression)
+        => expression switch
         {
-            return base.Visit(expression);
-        }
+            FromSqlExpression fromSql
+                => _visitedFromSqlExpressions.TryGetValue(fromSql, out var visitedFromSql)
+                    ? visitedFromSql
+                    : _visitedFromSqlExpressions[fromSql] = VisitFromSql(fromSql),
+
+            SqlParameterExpression parameter => VisitSqlParameter(parameter),
+
+            _ => base.VisitExtension(expression)
+        };
 
-        if (_visitedFromSqlExpressions.TryGetValue(fromSql, out var visitedFromSql))
+    private SqlParameterExpression VisitSqlParameter(SqlParameterExpression parameter)
+    {
+        var typeMapping = parameter.TypeMapping!;
+
+        // Try to see if a parameter already exists - if so, just integrate the same placeholder into the SQL instead of sending the same
+        // data twice.
+        // Note that if the type mapping differs, we do send the same data twice (e.g. the same string may be sent once as Unicode, once as
+        // non-Unicode).
+        // TODO: Note that we perform Equals comparison on the value converter. We should be able to do reference comparison, but for
+        // that we need to ensure that there's only ever one type mapping instance (i.e. no type mappings are ever instantiated out of the
+        // type mapping source). See #30677.
+        if (_sqlParameters.TryGetValue(parameter.InvariantName, out var existingParameter)
+            && existingParameter is { TypeMapping: RelationalTypeMapping existingTypeMapping }
+            && string.Equals(existingTypeMapping.StoreType, typeMapping.StoreType, StringComparison.OrdinalIgnoreCase)
+            && (existingTypeMapping.Converter is null && typeMapping.Converter is null
+                || existingTypeMapping.Converter is not null && existingTypeMapping.Converter.Equals(typeMapping.Converter)))
         {
-            return visitedFromSql;
+            return parameter;
         }
 
+        var uniquifiedName = UniquifyParameterName(parameter.Name);
+        var newParameter = uniquifiedName == parameter.Name
+            ? parameter
+            : new SqlParameterExpression(
+                parameter.InvariantName,
+                uniquifiedName,
+                parameter.Type,
+                parameter.IsNullable,
+                parameter.ShouldBeConstantized,
+                parameter.TypeMapping);
+
+        return _sqlParameters[newParameter.InvariantName] = newParameter;
+    }
+
+    private FromSqlExpression VisitFromSql(FromSqlExpression fromSql)
+    {
         switch (fromSql.Arguments)
         {
             case QueryParameterExpression queryParameter:
@@ -101,22 +153,14 @@ public virtual Expression Expand(
                 // ReSharper disable once ForCanBeConvertedToForeach
                 for (var i = 0; i < parameterValues.Length; i++)
                 {
-                    var parameterName = _parameterNameGenerator.GenerateNext();
                     if (parameterValues[i] is DbParameter dbParameter)
                     {
-                        if (string.IsNullOrEmpty(dbParameter.ParameterName))
-                        {
-                            dbParameter.ParameterName = parameterName;
-                        }
-                        else
-                        {
-                            parameterName = dbParameter.ParameterName;
-                        }
-
-                        subParameters.Add(new RawRelationalParameter(parameterName, dbParameter));
+                        ProcessDbParameter(dbParameter);
+                        subParameters.Add(new RawRelationalParameter(dbParameter.ParameterName, dbParameter));
                     }
                     else
                     {
+                        var parameterName = GenerateNewParameterName();
                         subParameters.Add(
                             new TypeMappedRelationalParameter(
                                 parameterName,
@@ -126,18 +170,18 @@ public virtual Expression Expand(
                     }
                 }
 
-                return _visitedFromSqlExpressions[fromSql] = fromSql.Update(
-                    Expression.Constant(new CompositeRelationalParameter(queryParameter.Name!, subParameters)));
+                return fromSql.Update(Expression.Constant(new CompositeRelationalParameter(queryParameter.Name, subParameters)));
 
             case ConstantExpression { Value: object?[] existingValues }:
             {
                 var constantValues = new object?[existingValues.Length];
+
                 for (var i = 0; i < existingValues.Length; i++)
                 {
                     constantValues[i] = ProcessConstantValue(existingValues[i]);
                 }
 
-                return _visitedFromSqlExpressions[fromSql] = fromSql.Update(Expression.Constant(constantValues, typeof(object[])));
+                return fromSql.Update(Expression.Constant(constantValues, typeof(object[])));
             }
 
             case NewArrayExpression { Expressions: var expressions }:
@@ -154,35 +198,60 @@ public virtual Expression Expand(
                     constantValues[i] = ProcessConstantValue(existingValue);
                 }
 
-                return _visitedFromSqlExpressions[fromSql] = fromSql.Update(Expression.Constant(constantValues, typeof(object[])));
+                return fromSql.Update(Expression.Constant(constantValues, typeof(object[])));
             }
 
             default:
-                Check.DebugFail("FromSql.Arguments must be Constant/QueryParameterExpression");
-                return null;
+                throw new UnreachableException("FromSql.Arguments must be Constant/QueryParameterExpression");
         }
 
         object ProcessConstantValue(object? existingConstantValue)
         {
             if (existingConstantValue is DbParameter dbParameter)
             {
-                var parameterName = _parameterNameGenerator.GenerateNext();
-                if (string.IsNullOrEmpty(dbParameter.ParameterName))
-                {
-                    dbParameter.ParameterName = parameterName;
-                }
-                else
-                {
-                    parameterName = dbParameter.ParameterName;
-                }
-
-                return new RawRelationalParameter(parameterName, dbParameter);
+                ProcessDbParameter(dbParameter);
+                return new RawRelationalParameter(dbParameter.ParameterName, dbParameter);
             }
 
             return _sqlExpressionFactory.Constant(
                 existingConstantValue,
                 existingConstantValue?.GetType() ?? typeof(object),
                 _typeMappingSource.GetMappingForValue(existingConstantValue));
         }
+
+        void ProcessDbParameter(DbParameter dbParameter)
+        {
+            dbParameter.ParameterName = string.IsNullOrEmpty(dbParameter.ParameterName)
+                ? GenerateNewParameterName()
+                : UniquifyParameterName(dbParameter.ParameterName);
+        }
+    }
+
+    private string GenerateNewParameterName()
+    {
+        string name, prefixedName;
+        do
+        {
+            name = _parameterNameGenerator.GenerateNext();
+            prefixedName = _sqlGenerationHelper.GenerateParameterName(name);
+        }
+        while (_prefixedParameterNames.Contains(prefixedName));
+
+        _prefixedParameterNames.Add(prefixedName);
+        return name;
+    }
+
+    private string UniquifyParameterName(string originalName)
+    {
+        var parameterName = originalName;
+        var prefixedName = _sqlGenerationHelper.GenerateParameterName(originalName);
+        for (var j = 0; _prefixedParameterNames.Contains(prefixedName); j++)
+        {
+            parameterName = originalName + j;
+            prefixedName = _sqlGenerationHelper.GenerateParameterName(parameterName);
+        }
+
+        _prefixedParameterNames.Add(prefixedName);
+        return parameterName;
     }
 }