From d403a953dea3823443cb6aead372f5534e20ca6a Mon Sep 17 00:00:00 2001
From: Smit Patel <smitpatel@users.noreply.github.com>
Date: Fri, 3 Apr 2020 15:27:48 -0700
Subject: [PATCH] Query: Parameterize components of NewExpression when possible

Issue: When evaluating MemberInitExpression/ListInitExpression, we skipped visiting inner NewExpression if other components were not evaluatable.
We did this since if we cannot evaluate NewExpression otherwise due to structure of expression which does not take ParmeterExpression in place of NewExpression.
But existing logic skipped visiting NewExpression altogether, leaving behind closure variable inside NewExpression.
Fix: Visit NewExpression so that it's components are marked for parameterization and explicitly disallow evaluating NewExpression

Resolves #20502
---
 .../ParameterExtractingExpressionVisitor.cs   | 16 +++++++---
 .../NorthwindMiscellaneousQueryTestBase.cs    | 29 +++++++++++++++++++
 ...orthwindMiscellaneousQuerySqlServerTest.cs | 10 +++++++
 3 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/src/EFCore/Query/Internal/ParameterExtractingExpressionVisitor.cs b/src/EFCore/Query/Internal/ParameterExtractingExpressionVisitor.cs
index aa5c1581773..397f96716c3 100644
--- a/src/EFCore/Query/Internal/ParameterExtractingExpressionVisitor.cs
+++ b/src/EFCore/Query/Internal/ParameterExtractingExpressionVisitor.cs
@@ -529,9 +529,13 @@ protected override Expression VisitMemberInit(MemberInitExpression memberInitExp
                 Visit(memberInitExpression.Bindings, VisitMemberBinding);
 
                 // Cannot make parameter for NewExpression if Bindings cannot be evaluated
-                if (_evaluatable)
+                // but we still need to visit inside of it.
+                var bindingsEvaluatable = _evaluatable;
+                Visit(memberInitExpression.NewExpression);
+
+                if (!bindingsEvaluatable)
                 {
-                    Visit(memberInitExpression.NewExpression);
+                    _evaluatableExpressions.Remove(memberInitExpression.NewExpression);
                 }
 
                 return memberInitExpression;
@@ -542,9 +546,13 @@ protected override Expression VisitListInit(ListInitExpression listInitExpressio
                 Visit(listInitExpression.Initializers, VisitElementInit);
 
                 // Cannot make parameter for NewExpression if Initializers cannot be evaluated
-                if (_evaluatable)
+                // but we still need to visit inside of it.
+                var initializersEvaluatable = _evaluatable;
+                Visit(listInitExpression.NewExpression);
+
+                if (!initializersEvaluatable)
                 {
-                    Visit(listInitExpression.NewExpression);
+                    _evaluatableExpressions.Remove(listInitExpression.NewExpression);
                 }
 
                 return listInitExpression;
diff --git a/test/EFCore.Specification.Tests/Query/NorthwindMiscellaneousQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/NorthwindMiscellaneousQueryTestBase.cs
index f766c0458df..5735eeae44f 100644
--- a/test/EFCore.Specification.Tests/Query/NorthwindMiscellaneousQueryTestBase.cs
+++ b/test/EFCore.Specification.Tests/Query/NorthwindMiscellaneousQueryTestBase.cs
@@ -5813,5 +5813,34 @@ public virtual Task Entity_equality_contains_with_list_of_null(bool async)
                 ss => ss.Set<Customer>().Where(c => customers.Contains(c)),
                 entryCount: 1);
         }
+
+        [ConditionalTheory]
+        [MemberData(nameof(IsAsyncData))]
+        public virtual Task MemberInitExpression_NewExpression_is_funcletized_even_when_bindings_are_not_evaluatable(bool async)
+        {
+            var randomString = "random";
+            return AssertQuery(
+                async,
+                ss => ss.Set<Customer>().Where(c => c.CustomerID.StartsWith("A"))
+                    .Select(c => new Dto(randomString)
+                    {
+                        CustomerID = c.CustomerID,
+                        NestedDto = new Dto(randomString)
+                    }),
+                elementSorter: e => e.CustomerID,
+                elementAsserter: (e, a) => Assert.Equal(e.CustomerID, a.CustomerID));
+        }
+
+        private class Dto
+        {
+            public Dto(string value)
+            {
+                Value = value;
+            }
+
+            public string Value { get; }
+            public string CustomerID { get; set; }
+            public Dto NestedDto { get; set; }
+        }
     }
 }
diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs
index 121d690909f..a73af113a20 100644
--- a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs
+++ b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs
@@ -5047,6 +5047,16 @@ FROM [Customers] AS [c]
 WHERE [c].[CustomerID] IN (N'ALFKI')");
         }
 
+        public override async Task MemberInitExpression_NewExpression_is_funcletized_even_when_bindings_are_not_evaluatable(bool async)
+        {
+            await base.MemberInitExpression_NewExpression_is_funcletized_even_when_bindings_are_not_evaluatable(async);
+
+            AssertSql(
+                @"SELECT [c].[CustomerID]
+FROM [Customers] AS [c]
+WHERE [c].[CustomerID] LIKE N'A%'");
+        }
+
         private void AssertSql(params string[] expected)
             => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);