Do not cache expressions containing method calls

- #5655
- also make `ExpressionTextCache` more robust for defence-in-depth

nits:
- two `null` expression nodes are equal
- declare data properties as `TheoryData<T>`

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ExpressionHelper.cs

@@ -61,11 +61,14 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal
                 switch (part.NodeType)
                 {
                     case ExpressionType.Call:
+                        // Will exit loop if at Method().Property or [i,j].Property. In that case (like [i].Property),
+                        // don't cache and don't remove ".Model" (if that's .Property).
+                        containsIndexers = true;
+                        lastIsModel = false;
+
                         var methodExpression = (MethodCallExpression)part;
                         if (IsSingleArgumentIndexer(methodExpression))
                         {
-                            containsIndexers = true;
-                            lastIsModel = false;
                             length += "[99]".Length;
                             part = methodExpression.Object;
                             segmentCount++;

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ExpressionTextCache.cs

@@ -37,6 +37,11 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal
 
                 while (true)
                 {
+                    if (expression1 == null && expression2 == null)
+                    {
+                        return true;
+                    }
+
                     if (expression1 == null || expression2 == null)
                     {
                         return false;
@@ -47,31 +52,41 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal
                         return false;
                     }
 
-                    if (expression1.NodeType == ExpressionType.MemberAccess)
+                    switch (expression1.NodeType)
                     {
-                        var memberExpression1 = (MemberExpression)expression1;
+                        case ExpressionType.MemberAccess:
-                        var memberName1 = memberExpression1.Member.Name;
+                            var memberExpression1 = (MemberExpression)expression1;
-                        expression1 = memberExpression1.Expression;
+                            var memberName1 = memberExpression1.Member.Name;
+                            expression1 = memberExpression1.Expression;
 
-                        var memberExpression2 = (MemberExpression)expression2;
+                            var memberExpression2 = (MemberExpression)expression2;
-                        var memberName2 = memberExpression2.Member.Name;
+                            var memberName2 = memberExpression2.Member.Name;
-                        expression2 = memberExpression2.Expression;
+                            expression2 = memberExpression2.Expression;
 
-                        // If identifier contains "__", it is "reserved for use by the implementation" and likely compiler-
+                            // If identifier contains "__", it is "reserved for use by the implementation" and likely
-                        // or Razor-generated e.g. the name of a field in a delegate's generated class.
+                            // compiler- or Razor-generated e.g. the name of a field in a delegate's generated class.
-                        if (memberName1.Contains("__") && memberName2.Contains("__"))
+                            if (memberName1.Contains("__") && memberName2.Contains("__"))
-                        {
+                            {
-                            return true;
+                                return true;
-                        }
+                            }
 
-                        if (!string.Equals(memberName1, memberName2, StringComparison.OrdinalIgnoreCase))
+                            if (!string.Equals(memberName1, memberName2, StringComparison.OrdinalIgnoreCase))
-                        {
+                            {
+                                return false;
+                            }
+                            break;
+
+                        case ExpressionType.ArrayIndex:
+                            // Shouldn't be cached. Just in case, ensure indexers are all different.
                             return false;
-                        }
+
-                    }
+                        case ExpressionType.Call:
-                    else
+                            // Shouldn't be cached. Just in case, ensure indexers and other calls are all different.
-                    {
+                            return false;
-                        return true;
+
+                        default:
+                            // Everything else terminates name generation. Haven't found a difference so far...
+                            return true;
                     }
                 }
             }

test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/ExpressionHelperTest.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Linq;
 using System.Linq.Expressions;
 using Xunit;
 
@@ -12,7 +13,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal
     {
         private readonly ExpressionTextCache _expressionTextCache = new ExpressionTextCache();
 
-        public static IEnumerable<object[]> ExpressionAndTexts
+        public static TheoryData<Expression, string> ExpressionAndTexts
         {
             get
             {
@@ -80,11 +81,35 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal
                         (Expression<Func<IList<TestModel>, int>>)(model => model[2].PreferredCategories[i].CategoryId),
                         "[2].PreferredCategories[3].CategoryId"
                     },
+                    {
+                        (Expression<Func<IList<TestModel>, string>>)(model => model.FirstOrDefault().Name),
+                        "Name"
+                    },
+                    {
+                        (Expression<Func<IList<TestModel>, string>>)(model => model.FirstOrDefault().Model),
+                        "Model"
+                    },
+                    {
+                        (Expression<Func<IList<TestModel>, int>>)(model => model.FirstOrDefault().SelectedCategory.CategoryId),
+                        "SelectedCategory.CategoryId"
+                    },
+                    {
+                        (Expression<Func<IList<TestModel>, string>>)(model => model.FirstOrDefault().SelectedCategory.CategoryName.MainCategory),
+                        "SelectedCategory.CategoryName.MainCategory"
+                    },
+                    {
+                        (Expression<Func<IList<TestModel>, int>>)(model => model.FirstOrDefault().PreferredCategories.Count),
+                        "PreferredCategories.Count"
+                    },
+                    {
+                        (Expression<Func<IList<TestModel>, int>>)(model => model.FirstOrDefault().PreferredCategories.FirstOrDefault().CategoryId),
+                        "CategoryId"
+                    },
                 };
             }
         }
 
-        public static IEnumerable<object[]> CachedExpressions
+        public static TheoryData<Expression> CachedExpressions
         {
             get
             {
@@ -104,7 +129,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal
             }
         }
 
-        public static IEnumerable<object[]> IndexerExpressions
+        public static TheoryData<Expression> IndexerExpressions
         {
             get
             {
@@ -123,7 +148,29 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal
             }
         }
 
-        public static IEnumerable<object[]> EquivalentExpressions
+        public static TheoryData<Expression> UnsupportedExpressions
+        {
+            get
+            {
+                var i = 2;
+                var j = 3;
+
+                return new TheoryData<Expression>
+                {
+                    // Indexers that have multiple arguments.
+                    (Expression<Func<TestModel[][], string>>)(model => model[23][3].Name),
+                    (Expression<Func<TestModel[][], string>>)(model => model[i][3].Name),
+                    (Expression<Func<TestModel[][], string>>)(model => model[23][j].Name),
+                    (Expression<Func<TestModel[][], string>>)(model => model[i][j].Name),
+                    // Calls that aren't indexers.
+                    (Expression<Func<IList<TestModel>, string>>)(model => model.FirstOrDefault().Name),
+                    (Expression<Func<IList<TestModel>, string>>)(model => model.FirstOrDefault().SelectedCategory.CategoryName.MainCategory),
+                    (Expression<Func<IList<TestModel>, int>>)(model => model.FirstOrDefault().PreferredCategories.FirstOrDefault().CategoryId),
+                };
+            }
+        }
+
+        public static TheoryData<Expression, Expression> EquivalentExpressions
         {
             get
             {
@@ -167,7 +214,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal
             }
         }
 
-        public static IEnumerable<object[]> NonEquivalentExpressions
+        public static TheoryData<Expression, Expression> NonEquivalentExpressions
         {
             get
             {
@@ -253,7 +300,8 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal
 
         [Theory]
         [MemberData(nameof(IndexerExpressions))]
-        public void GetExpressionText_DoesNotCacheIndexerExpression(LambdaExpression expression)
+        [MemberData(nameof(UnsupportedExpressions))]
+        public void GetExpressionText_DoesNotCacheIndexerOrUnspportedExpression(LambdaExpression expression)
         {
             // Act - 1
             var text1 = ExpressionHelper.GetExpressionText(expression, _expressionTextCache);