Fix to #17236 - Improve readability of expressions in client evaluati…

…on exception

Adding a new switch to ExpressionPrinter that creates simplified output for Print and verbose (i.e. what we had before this change) for PrintDebug.
Simplified output means:
- displaying extension methods as extension methods rather than regular static methods,
- simplifying display of Navigation object,
- not showing if lambda body references parameter that is out of scope.

Also fixed minor display bugs in expression printer.

Also some changes in testing:
- added unit tests for ExpressionPrinter,
- DRY the "translation failed" tests,
- "translation failed" tests no longer verify expression in the exception message and instead just make sure that right exception is thrown.

src/EFCore/Metadata/Internal/NavigationExtensions.cs

@@ -1,6 +1,7 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
 using System.Runtime.CompilerServices;
 using System.Text;
 using JetBrains.Annotations;
@@ -17,6 +18,20 @@ namespace Microsoft.EntityFrameworkCore.Metadata.Internal
     /// </summary>
     public static class NavigationExtensions
     {
+        /// <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
+        ///     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>
+        [Obsolete]
+        public static string ToDebugString(
+            [NotNull] this INavigation navigation,
+            bool singleLine,
+            bool includeIndexes,
+            [NotNull] string indent)
+            => ToDebugString(navigation, singleLine, includeIndexes, indent, detailed: true);
+
         /// <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
@@ -27,12 +42,20 @@ public static string ToDebugString(
             [NotNull] this INavigation navigation,
             bool singleLine = true,
             bool includeIndexes = false,
-            [NotNull] string indent = "")
+            [NotNull] string indent = "",
+            bool detailed = true)
         {
             var builder = new StringBuilder();
 
             builder.Append(indent);
 
+            if (!detailed)
+            {
+                builder.Append($"Navigation: {navigation.DeclaringEntityType.DisplayName()}.{navigation.Name}");
+
+                return builder.ToString();
+            }
+
             if (singleLine)
             {
                 builder.Append("Navigation: ").Append(navigation.DeclaringEntityType.DisplayName()).Append(".");
@@ -80,10 +103,7 @@ public static string ToDebugString(
                 builder.Append(" ").Append(indexes.StoreGenerationIndex);
             }
 
-            if (!singleLine)
-            {
-                builder.Append(navigation.AnnotationsToDebugString(indent + "  "));
-            }
+            builder.Append(navigation.AnnotationsToDebugString(indent + "  "));
 
             return builder.ToString();
         }

src/EFCore/Query/ExpressionPrinter.cs

@@ -7,6 +7,7 @@
 using System.Linq;
 using System.Linq.Expressions;
 using System.Reflection;
+using System.Runtime.CompilerServices;
 using JetBrains.Annotations;
 using Microsoft.EntityFrameworkCore.Diagnostics;
 using Microsoft.EntityFrameworkCore.Infrastructure;
@@ -428,7 +429,10 @@ protected override Expression VisitLabel(LabelExpression labelExpression)
 
         protected override Expression VisitLambda<T>(Expression<T> lambdaExpression)
         {
-            _stringBuilder.Append("(");
+            if (lambdaExpression.Parameters.Count != 1)
+            {
+                _stringBuilder.Append("(");
+            }
 
             foreach (var parameter in lambdaExpression.Parameters)
             {
@@ -447,7 +451,12 @@ protected override Expression VisitLambda<T>(Expression<T> lambdaExpression)
                 }
             }
 
-            _stringBuilder.Append(") => ");
+            if (lambdaExpression.Parameters.Count != 1)
+            {
+                _stringBuilder.Append(")");
+            }
+
+            _stringBuilder.Append(" => ");
 
             Visit(lambdaExpression.Body);
 
@@ -464,7 +473,8 @@ protected override Expression VisitMember(MemberExpression memberExpression)
         {
             if (memberExpression.Expression != null)
             {
-                if (memberExpression.Expression.NodeType == ExpressionType.Convert)
+                if (memberExpression.Expression.NodeType == ExpressionType.Convert
+                    || memberExpression.Expression is BinaryExpression)
                 {
                     _stringBuilder.Append("(");
                     Visit(memberExpression.Expression);
@@ -534,34 +544,52 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp
                 _stringBuilder.Append(".");
             }
 
-            _stringBuilder.Append(methodCallExpression.Method.Name);
-            if (methodCallExpression.Method.IsGenericMethod)
+            var methodArguments = methodCallExpression.Arguments.ToList();
+
+            // TODO: issue #18413
+            var simplifiedMethod = GenerateUniqueParameterIds
+                && methodCallExpression.Arguments.Count > 0
+                && methodCallExpression.Method.IsDefined(typeof(ExtensionAttribute), inherit: false);
+
+            if (simplifiedMethod)
+            {
+                Visit(methodArguments[0]);
+                _stringBuilder.IncrementIndent();
+                _stringBuilder.AppendLine();
+                _stringBuilder.Append($".{methodCallExpression.Method.Name}");
+                methodArguments = methodArguments.Skip(1).ToList();
+            }
+            else
             {
-                _stringBuilder.Append("<");
-                var first = true;
-                foreach (var genericArgument in methodCallExpression.Method.GetGenericArguments())
+                _stringBuilder.Append(methodCallExpression.Method.Name);
+                if (methodCallExpression.Method.IsGenericMethod)
                 {
-                    if (!first)
+                    _stringBuilder.Append("<");
+                    var first = true;
+                    foreach (var genericArgument in methodCallExpression.Method.GetGenericArguments())
                     {
-                        _stringBuilder.Append(", ");
+                        if (!first)
+                        {
+                            _stringBuilder.Append(", ");
+                        }
+
+                        _stringBuilder.Append(genericArgument.ShortDisplayName());
+                        first = false;
                     }
 
-                    _stringBuilder.Append(genericArgument.ShortDisplayName());
-                    first = false;
+                    _stringBuilder.Append(">");
                 }
-
-                _stringBuilder.Append(">");
             }
 
             _stringBuilder.Append("(");
 
             var isSimpleMethodOrProperty = _simpleMethods.Contains(methodCallExpression.Method.Name)
-                                           || methodCallExpression.Arguments.Count < 2
+                                           || methodArguments.Count < 2
                                            || methodCallExpression.Method.IsEFPropertyMethod();
 
             var appendAction = isSimpleMethodOrProperty ? (Action<string>)Append : AppendLine;
 
-            if (methodCallExpression.Arguments.Count > 0)
+            if (methodArguments.Count > 0)
             {
                 appendAction("");
 
@@ -577,9 +605,9 @@ var argumentNames
                     indent = _stringBuilder.Indent();
                 }
 
-                for (var i = 0; i < methodCallExpression.Arguments.Count; i++)
+                for (var i = 0; i < methodArguments.Count; i++)
                 {
-                    var argument = methodCallExpression.Arguments[i];
+                    var argument = methodArguments[i];
 
                     if (!isSimpleMethodOrProperty)
                     {
@@ -588,7 +616,7 @@ var argumentNames
 
                     Visit(argument);
 
-                    if (i < methodCallExpression.Arguments.Count - 1)
+                    if (i < methodArguments.Count - 1)
                     {
                         appendAction(", ");
                     }
@@ -602,6 +630,11 @@ var argumentNames
 
             Append(")");
 
+            if (simplifiedMethod)
+            {
+                _stringBuilder.DecrementIndent();
+            }
+
             return methodCallExpression;
         }
 
@@ -712,9 +745,17 @@ protected override Expression VisitParameter(ParameterExpression parameterExpres
             }
             else
             {
-                Append("(Unhandled parameter: ");
-                Append(parameterExpression.Name);
-                Append(")");
+                // TODO: issue #18413
+                if (GenerateUniqueParameterIds)
+                {
+                    Append("(Unhandled parameter: ");
+                    Append(parameterExpression.Name);
+                    Append(")");
+                }
+                else
+                {
+                    Append(parameterExpression.Name);
+                }
             }
 
             if (GenerateUniqueParameterIds)
@@ -829,7 +870,9 @@ protected override Expression VisitExtension(Expression extensionExpression)
         {
             if (extensionExpression is IPrintableExpression printable)
             {
+                _stringBuilder.Append("(");
                 printable.Print(this);
+                _stringBuilder.Append(")");
             }
             else
             {
@@ -840,7 +883,10 @@ protected override Expression VisitExtension(Expression extensionExpression)
         }
 
         private void VisitArguments(
-            IReadOnlyList<Expression> arguments, Action<string> appendAction, string lastSeparator = "", bool areConnected = false)
+            IReadOnlyList<Expression> arguments,
+            Action<string> appendAction,
+            string lastSeparator = "",
+            bool areConnected = false)
         {
             for (var i = 0; i < arguments.Count; i++)
             {

src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.Expressions.cs

@@ -189,8 +189,7 @@ public virtual void MarkAsOptional()
 
             public virtual void Print(ExpressionPrinter expressionPrinter)
             {
-                expressionPrinter.Append(nameof(EntityReference));
-                expressionPrinter.Append(EntityType.DisplayName());
+                expressionPrinter.Append($"{nameof(EntityReference)}: {EntityType.DisplayName()}");
                 if (IsOptional)
                 {
                     expressionPrinter.Append("[Optional]");

src/EFCore/Query/MaterializeCollectionNavigationExpression.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Linq.Expressions;
 using Microsoft.EntityFrameworkCore.Metadata;
+using Microsoft.EntityFrameworkCore.Metadata.Internal;
 
 namespace Microsoft.EntityFrameworkCore.Query
 {
@@ -31,9 +32,13 @@ public virtual MaterializeCollectionNavigationExpression Update(Expression subqu
 
         public virtual void Print(ExpressionPrinter expressionPrinter)
         {
-            expressionPrinter.Append($"MaterializeCollectionNavigation({Navigation}, ");
-            expressionPrinter.Visit(Subquery);
-            expressionPrinter.Append(")");
+            expressionPrinter.AppendLine("MaterializeCollectionNavigation(");
+            using (expressionPrinter.Indent())
+            {
+                expressionPrinter.AppendLine($"navigation: {Navigation.ToDebugString(detailed: false)},");
+                expressionPrinter.Append("subquery: ");
+                expressionPrinter.Visit(Subquery);
+            }
         }
     }
 }