Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions src/System.Web.Mvc/ExpressionHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ public static string GetExpressionText(LambdaExpression expression)

if (!IsSingleArgumentIndexer(methodExpression))
{
// Unsupported
break;
}

Expand All @@ -59,7 +60,18 @@ public static string GetExpressionText(LambdaExpression expression)
else if (part.NodeType == ExpressionType.MemberAccess)
{
MemberExpression 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("__"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we need a compat switch to allow this? Seems like a super corner case though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably Razor doesn't stick a CompilerGeneratedAttribute or something along those lines on the member that we could use to infer this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're already dealing with an infrequent case. Imaging a scenario where this fix causes a problem is pretty extreme. I don't think the slight change is worth a compat switch. But, I'll ask around…

Roslyn does apply [CompilerGenerated] to its generated classes but not to fields in those classes. So, that attribute doesn't help distinguish <>c__DisplayClass3_0.collection (which is fine) from <>c__DisplayClass5_1,CS$<>8__locals1 (which isn't). and __ is what docs say. (See the last paragraph in this section of the language spec for example.)

{
// 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)
Expand All @@ -69,15 +81,19 @@ public static string GetExpressionText(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();
Expand Down
77 changes: 76 additions & 1 deletion test/System.Web.Mvc.Test/Test/ExpressionHelperTest.cs
Original file line number Diff line number Diff line change
@@ -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.Collections.Generic;
using System.Linq.Expressions;
using Microsoft.TestCommon;

Expand Down Expand Up @@ -78,9 +79,83 @@ public void LambdaBasedExpressionTextTests()
"The expression compiler was unable to evaluate the indexer expression '(s.Length - 4)' because it references the model parameter 's' which is unavailable.");
}

public static TheoryDataSet<LambdaExpression, string> ComplicatedLambdaExpressions
{
get
{
var collection = new List<DummyModelContainer>();
var index = 20;
var data = new TheoryDataSet<LambdaExpression, string>
{

{
Lambda((List<DummyModelContainer> m) => collection[10].Model.FirstName),
"collection[10].Model.FirstName"
},
{
Lambda((List<DummyModelContainer> m) => m[10].Model.FirstName),
"[10].Model.FirstName"
},
{
Lambda((List<DummyModelContainer> m) => collection[index].Model.FirstName),
"collection[20].Model.FirstName"
},
{
Lambda((List<DummyModelContainer> m) => m[index].Model.FirstName),
"[20].Model.FirstName"
},
};

return data;
}
}

[Theory]
[PropertyData("ComplicatedLambdaExpressions")]
public void GetExpressionText_WithComplicatedLambdaExpressions_ReturnsExpectedText(
LambdaExpression expression,
string expectedText)
{
// Arrange & Act
var result = ExpressionHelper.GetExpressionText(expression);

// Assert
Assert.Equal(expectedText, result);
}

[Fact]
public void GetExpressionText_WithinALoop_ReturnsExpectedText()
{
// Arrange 0
var collection = new List<DummyModelContainer>();

for (var i = 0; i < 2; i++)
{
// Arrange 1
var expectedText = string.Format("collection[{0}].Model.FirstName", i);

// Act 1
var result = ExpressionHelper.GetExpressionText(Lambda(
(List<DummyModelContainer> m) => collection[i].Model.FirstName));

// Assert 1
Assert.Equal(expectedText, result);

// Arrange 2
expectedText = string.Format("[{0}].Model.FirstName", i);

// Act 2
result = ExpressionHelper.GetExpressionText(Lambda(
(List<DummyModelContainer> m) => m[i].Model.FirstName));

// Assert 2
Assert.Equal(expectedText, result);
}
}

// Helpers

private LambdaExpression Lambda<T1, T2>(Expression<Func<T1, T2>> expression)
private static LambdaExpression Lambda<T1, T2>(Expression<Func<T1, T2>> expression)
{
return expression;
}
Expand Down