Clean up and open table cloning logic

Fixes dotnet#30982
Fixes dotnet#32664

src/EFCore.Relational/Properties/RelationalStrings.resx

@@ -1134,9 +1134,6 @@
   <data name="SubqueryOverComplexTypesNotSupported" xml:space="preserve">
     <value>The query requires a subquery over complex type '{complexType}'. Subqueries over complex types are currently unsupported.</value>
   </data>
-  <data name="TableExpressionBaseWithoutCloningLogic" xml:space="preserve">
-    <value>Expression type '{expressionType}' does not implement proper cloning logic. Every expression derived from '{tableExpressionBase}' must implement '{clonableTableExpressionBase}' interface or have it's cloning logic added to the '{cloningExpressionVisitor}' inside '{selectExpression}'.</value>
-  </data>
   <data name="TableNotMappedEntityType" xml:space="preserve">
     <value>The entity type '{entityType}' is not mapped to the store object '{table}'.</value>
   </data>

src/EFCore.Relational/Query/Internal/TpcTablesExpression.cs

@@ -108,6 +108,20 @@ protected override Expression VisitChildren(ExpressionVisitor visitor)
     protected override TableExpressionBase CreateWithAnnotations(IEnumerable<IAnnotation> annotations)
         => new TpcTablesExpression(Alias, EntityType, SelectExpressions, annotations);
 
+    /// <inheritdoc />
+    public override TableExpressionBase Clone(ExpressionVisitor cloningExpressionVisitor)
+    {
+        // Deep clone
+        var subSelectExpressions = SelectExpressions.Select(cloningExpressionVisitor.Visit).ToList<SelectExpression>();
+        var newTpcTable = new TpcTablesExpression(Alias, EntityType, subSelectExpressions);
+        foreach (var annotation in GetAnnotations())
+        {
+            newTpcTable.AddAnnotation(annotation.Name, annotation.Value);
+        }
+
+        return newTpcTable;
+    }
+
     /// <summary>
     ///     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

src/EFCore.Relational/Query/SqlExpressions/FromSqlExpression.cs

@@ -14,7 +14,7 @@ namespace Microsoft.EntityFrameworkCore.Query.SqlExpressions;
 ///         not used in application code.
 ///     </para>
 /// </summary>
-public class FromSqlExpression : TableExpressionBase, ITableBasedExpression, IClonableTableExpressionBase
+public class FromSqlExpression : TableExpressionBase, ITableBasedExpression
 {
     /// <summary>
     ///     Creates a new instance of the <see cref="FromSqlExpression" /> class.
@@ -106,7 +106,7 @@ protected override Expression VisitChildren(ExpressionVisitor visitor)
         => this;
 
     /// <inheritdoc />
-    public virtual TableExpressionBase Clone()
+    public override TableExpressionBase Clone(ExpressionVisitor cloningVisitor)
         => new FromSqlExpression(Alias, Table, Sql, Arguments, GetAnnotations());
 
     /// <inheritdoc />

src/EFCore.Relational/Query/SqlExpressions/JoinExpressionBase.cs

@@ -46,6 +46,12 @@ protected JoinExpressionBase(TableExpressionBase table, bool prunable, IEnumerab
     /// <returns>This expression if no children changed, or an expression with the updated children.</returns>
     public abstract JoinExpressionBase Update(TableExpressionBase table);
 
+    /// <inheritdoc />
+    public override TableExpressionBase Clone(ExpressionVisitor cloningExpressionVisitor)
+        // Joins necessary contain other TableExpressionBase, which will get cloned; this will cause our VisitChildren to create a new
+        // copy of this JoinExpressionBase by calling Update.
+        => (TableExpressionBase)VisitChildren(cloningExpressionVisitor);
+
     /// <inheritdoc />
     public override bool Equals(object? obj)
         => obj != null

src/EFCore.Relational/Query/SqlExpressions/SelectExpression.Helper.cs

@@ -753,136 +753,12 @@ private sealed class CloningExpressionVisitor : ExpressionVisitor
     {
         [return: NotNullIfNotNull("expression")]
         public override Expression? Visit(Expression? expression)
-        {
-            switch (expression)
+            => expression switch
             {
-                case SelectExpression selectExpression:
-                {
-                    var newProjectionMappings = new Dictionary<ProjectionMember, Expression>(selectExpression._projectionMapping.Count);
-                    foreach (var (projectionMember, value) in selectExpression._projectionMapping)
-                    {
-                        newProjectionMappings[projectionMember] = Visit(value);
-                    }
-
-                    var newProjections = selectExpression._projection.Select(Visit).ToList<ProjectionExpression>();
-
-                    var newTables = selectExpression._tables.Select(Visit).ToList<TableExpressionBase>();
-                    var tpcTablesMap = selectExpression._tables.Select(UnwrapJoinExpression).Zip(newTables.Select(UnwrapJoinExpression))
-                        .Where(e => e.First is TpcTablesExpression)
-                        .ToDictionary(e => (TpcTablesExpression)e.First, e => (TpcTablesExpression)e.Second);
-
-                    // Since we are cloning we need to generate new table references
-                    // In other cases (like VisitChildren), we just reuse the same table references and update the SelectExpression inside it.
-                    // We initially assign old SelectExpression in table references and later update it once we construct clone
-                    var newTableReferences = selectExpression._tableReferences
-                        .Select(e => new TableReferenceExpression(selectExpression, e.Alias)).ToList();
-                    Check.DebugAssert(
-                        newTables.Select(GetAliasFromTableExpressionBase).SequenceEqual(newTableReferences.Select(e => e.Alias)),
-                        "Alias of updated tables must match the old tables.");
-
-                    var predicate = (SqlExpression?)Visit(selectExpression.Predicate);
-                    var newGroupBy = selectExpression._groupBy.Select(Visit)
-                        .Where(e => e is not (SqlConstantExpression or SqlParameterExpression))
-                        .ToList<SqlExpression>();
-                    var havingExpression = (SqlExpression?)Visit(selectExpression.Having);
-                    var newOrderings = selectExpression._orderings.Select(Visit).ToList<OrderingExpression>();
-                    var offset = (SqlExpression?)Visit(selectExpression.Offset);
-                    var limit = (SqlExpression?)Visit(selectExpression.Limit);
-
-                    var newSelectExpression = new SelectExpression(
-                        selectExpression.Alias, newProjections, newTables, newTableReferences, newGroupBy, newOrderings,
-                        selectExpression.GetAnnotations())
-                    {
-                        Predicate = predicate,
-                        Having = havingExpression,
-                        Offset = offset,
-                        Limit = limit,
-                        IsDistinct = selectExpression.IsDistinct,
-                        Tags = selectExpression.Tags,
-                        _usedAliases = selectExpression._usedAliases.ToHashSet(),
-                        _projectionMapping = newProjectionMappings,
-                        _mutable = selectExpression._mutable
-                    };
-
-                    foreach (var kvp in selectExpression._tpcDiscriminatorValues)
-                    {
-                        newSelectExpression._tpcDiscriminatorValues[tpcTablesMap[kvp.Key]] = kvp.Value;
-                    }
-
-                    // Since identifiers are ColumnExpression, they are not visited since they don't contain SelectExpression inside it.
-                    newSelectExpression._identifier.AddRange(selectExpression._identifier);
-                    newSelectExpression._childIdentifiers.AddRange(selectExpression._childIdentifiers);
-
-                    // Remap tableReferences in new select expression
-                    foreach (var tableReference in newTableReferences)
-                    {
-                        tableReference.UpdateTableReference(selectExpression, newSelectExpression);
-                    }
-
-                    // Now that we have SelectExpression, we visit all components and update table references inside columns
-                    newSelectExpression = (SelectExpression)new ColumnExpressionReplacingExpressionVisitor(
-                        selectExpression, newSelectExpression._tableReferences).Visit(newSelectExpression);
-
-                    return newSelectExpression;
-                }
-
-                case TpcTablesExpression tpcTablesExpression:
-                {
-                    // Deep clone
-                    var subSelectExpressions = tpcTablesExpression.SelectExpressions.Select(Visit).ToList<SelectExpression>();
-                    var newTpcTable = new TpcTablesExpression(
-                        tpcTablesExpression.Alias, tpcTablesExpression.EntityType, subSelectExpressions);
-                    foreach (var annotation in tpcTablesExpression.GetAnnotations())
-                    {
-                        newTpcTable.AddAnnotation(annotation.Name, annotation.Value);
-                    }
+                TableExpressionBase table => table.Clone(this),
 
-                    return newTpcTable;
-                }
-
-                case IClonableTableExpressionBase cloneable:
-                    return cloneable.Clone();
-
-                case TableValuedFunctionExpression tableValuedFunctionExpression:
-                {
-                    var newArguments = new SqlExpression[tableValuedFunctionExpression.Arguments.Count];
-                    for (var i = 0; i < newArguments.Length; i++)
-                    {
-                        newArguments[i] = (SqlExpression)Visit(tableValuedFunctionExpression.Arguments[i]);
-                    }
-
-                    var newTableValuedFunctionExpression = tableValuedFunctionExpression.StoreFunction is null
-                        ? new TableValuedFunctionExpression(
-                            tableValuedFunctionExpression.Alias, tableValuedFunctionExpression.Name, newArguments)
-                        : new TableValuedFunctionExpression(
-                            tableValuedFunctionExpression.StoreFunction, newArguments) { Alias = tableValuedFunctionExpression.Alias };
-
-                    foreach (var annotation in tableValuedFunctionExpression.GetAnnotations())
-                    {
-                        newTableValuedFunctionExpression.AddAnnotation(annotation.Name, annotation.Value);
-                    }
-
-                    return newTableValuedFunctionExpression;
-                }
-
-                // join and set operations are fine, because they contain other TableExpressionBases inside, that will get cloned
-                // and therefore set expression's Update function will generate a new instance.
-                case JoinExpressionBase or SetOperationBase:
-                    return base.Visit(expression);
-
-                case TableExpressionBase:
-                    throw new InvalidOperationException(
-                        RelationalStrings.TableExpressionBaseWithoutCloningLogic(
-                            expression.GetType().Name,
-                            nameof(TableExpressionBase),
-                            nameof(IClonableTableExpressionBase),
-                            nameof(CloningExpressionVisitor),
-                            nameof(SelectExpression)));
-
-                default:
-                    return base.Visit(expression);
-            }
-        }
+                _ => base.Visit(expression)
+            };
     }
 
     private sealed class ColumnExpressionReplacingExpressionVisitor : ExpressionVisitor

src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs

@@ -4341,6 +4341,80 @@ public SelectExpression Clone()
         return (SelectExpression)_cloningExpressionVisitor.Visit(this);
     }
 
+    /// <summary>
+    ///     Creates a new object that is a copy of the current instance.
+    /// </summary>
+    /// <param name="cloningExpressionVisitor">The cloning expression for further visitation of nested nodes.</param>
+    /// <returns>A new object that is a copy of this instance.</returns>
+    public override TableExpressionBase Clone(ExpressionVisitor cloningExpressionVisitor)
+    {
+        var newProjectionMappings = new Dictionary<ProjectionMember, Expression>(_projectionMapping.Count);
+        foreach (var (projectionMember, value) in _projectionMapping)
+        {
+            newProjectionMappings[projectionMember] = cloningExpressionVisitor.Visit(value);
+        }
+
+        var newProjections = _projection.Select(cloningExpressionVisitor.Visit).ToList<ProjectionExpression>();
+
+        var newTables = _tables.Select(cloningExpressionVisitor.Visit).ToList<TableExpressionBase>();
+        var tpcTablesMap = _tables.Select(UnwrapJoinExpression).Zip(newTables.Select(UnwrapJoinExpression))
+            .Where(e => e.First is TpcTablesExpression)
+            .ToDictionary(e => (TpcTablesExpression)e.First, e => (TpcTablesExpression)e.Second);
+
+        // Since we are cloning we need to generate new table references
+        // In other cases (like VisitChildren), we just reuse the same table references and update the SelectExpression inside it.
+        // We initially assign old SelectExpression in table references and later update it once we construct clone
+        var newTableReferences = _tableReferences.Select(e => new TableReferenceExpression(this, e.Alias)).ToList();
+        Check.DebugAssert(
+            newTables.Select(GetAliasFromTableExpressionBase).SequenceEqual(newTableReferences.Select(e => e.Alias)),
+            "Alias of updated tables must match the old tables.");
+
+        var predicate = (SqlExpression?)cloningExpressionVisitor.Visit(Predicate);
+        var newGroupBy = _groupBy.Select(cloningExpressionVisitor.Visit)
+            .Where(e => e is not (SqlConstantExpression or SqlParameterExpression))
+            .ToList<SqlExpression>();
+        var havingExpression = (SqlExpression?)cloningExpressionVisitor.Visit(Having);
+        var newOrderings = _orderings.Select(cloningExpressionVisitor.Visit).ToList<OrderingExpression>();
+        var offset = (SqlExpression?)cloningExpressionVisitor.Visit(Offset);
+        var limit = (SqlExpression?)cloningExpressionVisitor.Visit(Limit);
+
+        var newSelectExpression = new SelectExpression(
+            Alias, newProjections, newTables, newTableReferences, newGroupBy, newOrderings,
+            GetAnnotations())
+        {
+            Predicate = predicate,
+            Having = havingExpression,
+            Offset = offset,
+            Limit = limit,
+            IsDistinct = IsDistinct,
+            Tags = Tags,
+            _usedAliases = _usedAliases.ToHashSet(),
+            _projectionMapping = newProjectionMappings,
+            _mutable = _mutable
+        };
+
+        foreach (var kvp in _tpcDiscriminatorValues)
+        {
+            newSelectExpression._tpcDiscriminatorValues[tpcTablesMap[kvp.Key]] = kvp.Value;
+        }
+
+        // Since identifiers are ColumnExpression, they are not visited since they don't contain SelectExpression inside it.
+        newSelectExpression._identifier.AddRange(_identifier);
+        newSelectExpression._childIdentifiers.AddRange(_childIdentifiers);
+
+        // Remap tableReferences in new select expression
+        foreach (var tableReference in newTableReferences)
+        {
+            tableReference.UpdateTableReference(this, newSelectExpression);
+        }
+
+        // Now that we have SelectExpression, we visit all components and update table references inside columns
+        newSelectExpression = (SelectExpression)new ColumnExpressionReplacingExpressionVisitor(
+            this, newSelectExpression._tableReferences).Visit(newSelectExpression);
+
+        return newSelectExpression;
+    }
+
     /// <summary>
     ///     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

src/EFCore.Relational/Query/SqlExpressions/SetOperationBase.cs

@@ -87,6 +87,12 @@ public override string? Alias
     /// <returns>This expression if no children changed, or an expression with the updated children.</returns>
     public abstract SetOperationBase Update(SelectExpression source1, SelectExpression source2);
 
+    /// <inheritdoc />
+    public override TableExpressionBase Clone(ExpressionVisitor cloningExpressionVisitor)
+        // Set operations necessary contain other TableExpressionBase, which will get cloned; this will cause our VisitChildren to create a
+        // new copy of this SetOperationBase by calling Update.
+        => (TableExpressionBase)VisitChildren(cloningExpressionVisitor);
+
     /// <inheritdoc />
     public override bool Equals(object? obj)
         => obj != null

src/EFCore.Relational/Query/SqlExpressions/TableExpression.cs

@@ -13,7 +13,7 @@ namespace Microsoft.EntityFrameworkCore.Query.SqlExpressions;
 ///     application or database provider code. If this is a problem for your application or provider, then please file
 ///     an issue at <see href="https://github.com/dotnet/efcore">github.com/dotnet/efcore</see>.
 /// </remarks>
-public sealed class TableExpression : TableExpressionBase, IClonableTableExpressionBase, ITableBasedExpression
+public sealed class TableExpression : TableExpressionBase, ITableBasedExpression
 {
     internal TableExpression(ITableBase table)
         : this(table, annotations: null)
@@ -76,7 +76,7 @@ protected override void Print(ExpressionPrinter expressionPrinter)
     }
 
     /// <inheritdoc />
-    public TableExpressionBase Clone()
+    public override TableExpressionBase Clone(ExpressionVisitor cloningExpressionVisitor)
         => CreateWithAnnotations(GetAnnotations());
 
     /// <inheritdoc />