Do not include compiler-generated names in expression names

- #2890
- add lots of `ExpressionHelper` tests using `IdFor()` and `NameFor()` (which are thin veneers)

Author: dougbu

src/Microsoft.AspNet.Mvc.ViewFeatures/Rendering/Expressions/ExpressionHelper.cs

@@ -31,9 +31,9 @@ public static string GetExpressionText([NotNull] LambdaExpression expression)
                 if (part.NodeType == ExpressionType.Call)
                 {
                     var methodExpression = (MethodCallExpression)part;
-
                     if (!IsSingleArgumentIndexer(methodExpression))
                     {
+                        // Unsupported.
                         break;
                     }
 
@@ -58,7 +58,18 @@ public static string GetExpressionText([NotNull] LambdaExpression expression)
                 else if (part.NodeType == ExpressionType.MemberAccess)
                 {
                     var memberExpressionPart = (MemberExpression)part;
-                    nameParts.Push("." + memberExpressionPart.Member.Name);
+                    var name = memberExpressionPart.Member.Name;
+
+                    // If identifier contains "__", it is "reserved for use by the implementation" and likely compiler-
+                    // or Razor-generated e.g. the name of a field in a delegate's generated class.
+                    if (name.Contains("__"))
+                    {
+                        // Exit loop. Should have the entire name because previous MemberAccess has same name as the
+                        // leftmost expression node (a variable).
+                        break;
+                    }
+
+                    nameParts.Push("." + name);
                     part = memberExpressionPart.Expression;
                 }
                 else if (part.NodeType == ExpressionType.Parameter)
@@ -67,15 +78,19 @@ public static string GetExpressionText([NotNull] LambdaExpression expression)
                     // string onto the stack and stop evaluating. The extra empty string makes sure that
                     // we don't accidentally cut off too much of m => m.Model.
                     nameParts.Push(string.Empty);
-                    part = null;
+
+                    // Exit loop. Have the entire name because the parameter cannot be used as an indexer; always the
+                    // leftmost expression node.
+                    break;
                 }
                 else
                 {
+                    // Unsupported.
                     break;
                 }
             }
 
-            // If it starts with "model", then strip that away
+            // If parts start with "model", then strip that part away.
             if (nameParts.Count > 0 && string.Equals(nameParts.Peek(), ".model", StringComparison.OrdinalIgnoreCase))
             {
                 nameParts.Pop();

test/Microsoft.AspNet.Mvc.ViewFeatures.Test/Rendering/HtmlHelperNameExtensionsTest.cs

@@ -1,9 +1,13 @@
 // 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.Collections.Generic;
+using System.Linq.Expressions;
 using Microsoft.AspNet.Mvc.ModelBinding;
 using Microsoft.AspNet.Mvc.ModelBinding.Metadata;
 using Microsoft.AspNet.Mvc.Rendering;
+using Microsoft.AspNet.Testing;
 using Moq;
 using Xunit;
 
@@ -14,6 +18,26 @@ namespace Microsoft.AspNet.Mvc.Core
     /// </summary>
     public class HtmlHelperNameExtensionsTest
     {
+        private static List<string> _staticCollection = new List<string>();
+        private static int _staticIndex = 6;
+
+        private List<string> _collection = new List<string>();
+        private int _index = 7;
+        private List<string[]> _nestedCollection = new List<string[]>();
+        private string _string = string.Empty;
+
+        private static List<string> StaticCollection { get; }
+
+        private static int StaticIndex { get; } = 8;
+
+        private List<string> Collection { get; }
+
+        private int Index { get; } = 9;
+
+        private List<string[]> NestedCollection { get; }
+
+        private string StringProperty { get; }
+
         [Fact]
         public void IdAndNameHelpers_ReturnEmptyForModel()
         {
@@ -140,7 +164,7 @@ public void IdAndNameHelpers_DoNotConsultMetadataOrMetadataProvider()
             // Arrange
             var provider = new Mock<IModelMetadataProvider>(MockBehavior.Strict);
             var metadata = new Mock<ModelMetadata>(
-                MockBehavior.Loose, 
+                MockBehavior.Loose,
                 ModelMetadataIdentity.ForType(typeof(DefaultTemplatesUtilities.ObjectTemplateModel)));
             provider
                 .Setup(m => m.GetMetadataForType(typeof(DefaultTemplatesUtilities.ObjectTemplateModel)))
@@ -223,28 +247,106 @@ public void IdForAndNameFor_ReturnEmpty_IfExpressionUnsupported()
             Assert.Empty(nameResult);
         }
 
-        [Fact]
-        public void IdForAndNameFor_ReturnVariableName()
+        // expression, expected name, expected id
+        public static TheoryData StaticExpressionNamesData
+        {
+            get
+            {
+                // Give expressions access to non-static fields and properties.
+                var test = new HtmlHelperNameExtensionsTest();
+                return test.ExpressionNamesData;
+            }
+        }
+
+        // expression, expected name, expected id
+        private TheoryData ExpressionNamesData
+        {
+            get
+            {
+                var collection = new List<string>();
+                var nestedCollection = new List<string[]>();
+                var index1 = 5;
+                var index2 = 23;
+                var unknownKey = "this is a dummy parameter value";
+
+                return new TheoryData<Expression<Func<List<OuterClass>, string>>, string, string>
+                {
+                    { m => unknownKey, "unknownKey", "unknownKey" },
+                    { m => collection[index1], "collection[5]", "collection_5_" },
+                    { m => nestedCollection[index1][23], "nestedCollection[5][23]", "nestedCollection_5__23_" },
+                    { m => nestedCollection[index1][index2], "nestedCollection[5][23]", "nestedCollection_5__23_" },
+                    { m => nestedCollection[_index][Index], "nestedCollection[7][9]", "nestedCollection_7__9_" },
+                    { m => nestedCollection[Index][StaticIndex], "nestedCollection[9][8]", "nestedCollection_9__8_" },
+                    { m => _string, "_string", "zstring" },
+                    { m => _collection[_index], "_collection[7]", "zcollection_7_" },
+                    { m => _nestedCollection[_index][23], "_nestedCollection[7][23]", "znestedCollection_7__23_" },
+                    { m => _nestedCollection[_index][index2], "_nestedCollection[7][23]", "znestedCollection_7__23_" },
+                    { m => _nestedCollection[Index][_staticIndex], "_nestedCollection[9][6]", "znestedCollection_9__6_" },
+                    { m => _nestedCollection[StaticIndex][_index], "_nestedCollection[8][7]", "znestedCollection_8__7_" },
+                    { m => StringProperty, "StringProperty", "StringProperty" },
+                    { m => Collection[Index], "Collection[9]", "Collection_9_" },
+                    { m => NestedCollection[Index][23], "NestedCollection[9][23]", "NestedCollection_9__23_" },
+                    { m => NestedCollection[Index][index2], "NestedCollection[9][23]", "NestedCollection_9__23_" },
+                    { m => NestedCollection[_index][Index], "NestedCollection[7][9]", "NestedCollection_7__9_" },
+                    { m => NestedCollection[Index][StaticIndex], "NestedCollection[9][8]", "NestedCollection_9__8_" },
+                    { m => _staticCollection[_staticIndex], "_staticCollection[6]", "zstaticCollection_6_" },
+                    { m => _staticCollection[Index], "_staticCollection[9]", "zstaticCollection_9_" },
+                    { m => _staticCollection[_index], "_staticCollection[7]", "zstaticCollection_7_" },
+                    { m => StaticCollection[StaticIndex], "StaticCollection[8]", "StaticCollection_8_" },
+                    { m => StaticCollection[_staticIndex], "StaticCollection[6]", "StaticCollection_6_" },
+                    { m => StaticCollection[index1], "StaticCollection[5]", "StaticCollection_5_" },
+                    { m => m[index1].Inner.Name, "[5].Inner.Name", "z5__Inner_Name" },
+                    { m => m[_staticIndex].Inner.Name, "[6].Inner.Name", "z6__Inner_Name" },
+                    { m => m[_index].Inner.Name, "[7].Inner.Name", "z7__Inner_Name" },
+                    { m => m[StaticIndex].Inner.Name, "[8].Inner.Name", "z8__Inner_Name" },
+                    { m => m[Index].Inner.Name, "[9].Inner.Name", "z9__Inner_Name" },
+                };
+            }
+        }
+
+        [Theory]
+        [MemberData(nameof(StaticExpressionNamesData))]
+        public void IdForAndNameFor_ReturnExpectedValues_WithVariablesInExpression(
+            Expression<Func<List<OuterClass>, string>> expression,
+            string expectedName,
+            string expectedId)
         {
             // Arrange
-            var unknownKey = "this is a dummy parameter value";
-            var helper = DefaultTemplatesUtilities.GetHtmlHelper();
+            var model = new List<OuterClass>();
+            var helper = DefaultTemplatesUtilities.GetHtmlHelper(model);
 
             // Act
-            var idResult = helper.IdFor(model => unknownKey);
-            var nameResult = helper.NameFor(model => unknownKey);
+            var idResult = helper.IdFor(expression);
+            var nameResult = helper.NameFor(expression);
 
             // Assert
-            Assert.Equal("unknownKey", idResult);
-            Assert.Equal("unknownKey", nameResult);
+            Assert.Equal(expectedId, idResult);
+            Assert.Equal(expectedName, nameResult);
         }
 
-        private sealed class InnerClass
+        [Fact]
+        public void IdForAndNameFor_Throws_WhenParameterUsedAsIndexer()
+        {
+            // Arrange
+            var collection = new List<string>();
+            var index = 24;
+            var helper = DefaultTemplatesUtilities.GetHtmlHelper(index);
+            var message = "The expression compiler was unable to evaluate the indexer expression 'm' because it " +
+                "references the model parameter 'm' which is unavailable.";
+
+            // Act & Assert
+            ExceptionAssert.Throws<InvalidOperationException>(() => helper.IdFor(m => collection[m]), message);
+            ExceptionAssert.Throws<InvalidOperationException>(() => helper.NameFor(m => collection[m]), message);
+        }
+
+        public sealed class InnerClass
         {
             public int Id { get; set; }
+
+            public string Name { get; set; }
         }
 
-        private sealed class OuterClass
+        public sealed class OuterClass
         {
             public InnerClass Inner { get; set; }
         }