Follow up to universal System.Linq.Expressions #62229

MichalStrehovsky · 2021-12-01T07:42:45Z

Most of this is test changes so that we run more tests when the expression IL compiler is disabled. There is one product change that we don't need to take, but felt like an improvement. I'll put comments inline in a bit.

ghost · 2021-12-01T07:43:02Z

Tagging subscribers to this area: @cston
See info in area-owners.md if you want to be subscribed.

Issue Details

Most of this is test changes so that we run more tests when the expression IL compiler is disabled. There is one product change that we don't need to take, but felt like an improvement. I'll put comments inline in a bit.

Author:	MichalStrehovsky
Assignees:	-
Labels:	`area-System.Linq.Expressions`
Milestone:	-

MichalStrehovsky

I've validated that we run the same number of tests in default CoreCLR configuration.

I've also validated that the test fully passes when I artificially disable IL expression compiler on CoreCLR.

MichalStrehovsky · 2021-12-01T07:45:07Z

src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/Compiler/DelegateHelpers.cs

@@ -109,7 +109,7 @@ private static bool IsByRef(DynamicMetaObject mo)

        private static Type MakeNewCustomDelegate(Type[] types)
        {
-            if (LambdaExpression.CanCompileToIL)
+            if (RuntimeFeature.IsDynamicCodeSupported)


PRODUCT CHANGE: the existing setting was mechanically propagated from FEATURE_COMPILE, but I think here we should be more nuanced between "IL expression compiler is disabled" and "the platform supports reflection emit".

This codepath is not related to compiling IL expressions and we can allow it if emit is otherwise supported. It doesn't feel like it should be lumped into IL expression compiler not being enabled.

If we don't take this change, we need to disable a couple more tests on IL compiler being unavailable.

MichalStrehovsky · 2021-12-01T07:47:41Z

src/libraries/System.Linq.Expressions/tests/Dynamic/InvokeMemberBindingTests.cs

@@ -54,7 +54,7 @@ public static IEnumerable<object[]> ObjectArguments()
            yield return new[] {new object()};
        }

-        [ActiveIssue("https://github.com/dotnet/runtime/issues/55070", typeof(PlatformDetection), nameof(PlatformDetection.IsLinqExpressionsBuiltWithIsInterpretingOnly))]
+        [ActiveIssue("https://github.com/dotnet/runtime/issues/55070", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]


This test is passing with CoreCLR and IL compiler disabled, so whatever this is, it's iDevice-specific. This test was disabled in #54970 and not part of the mechanical refactoring.

MichalStrehovsky · 2021-12-01T07:49:29Z

src/libraries/System.Linq.Expressions/tests/Dynamic/InvokeMemberBindingTests.cs

@@ -335,8 +331,7 @@ private class ManyOverloads
                int arg10) => 11;
        }

-        // We're not testing compilation, but we do need Reflection.Emit for the test
-        [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotLinqExpressionsBuiltWithIsInterpretingOnly))]
+        [Fact]


This was probably hitting some .NET Native specific issues with dynamic keyword. I don't see it using Ref.Emit anywhere so leaving like this.

MichalStrehovsky · 2021-12-01T07:50:03Z

src/libraries/System.Linq.Expressions/tests/Dynamic/InvokeMemberBindingTests.cs

@@ -380,8 +375,7 @@ public static IEnumerable<object[]> SameNameObjectPairs()
            return testObjects.SelectMany(i => testObjects.Select(j => new[] { i, j }));
        }

-        // We're not testing compilation, but we do need Reflection.Emit for the test
-        [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotLinqExpressionsBuiltWithIsInterpretingOnly))]
+        [Theory]


Also don't see anything ref.emitting. Was probably some .NET Native issue related to dynamic.

MichalStrehovsky · 2021-12-01T07:51:08Z

src/libraries/System.Linq.Expressions/tests/IndexExpression/IndexExpressionTests.cs

@@ -464,15 +464,15 @@ public void IndexedPropertyVarArgs()
            AssertExtensions.Throws<ArgumentException>("propertyName", () => Expression.Property(instance, "Item", Expression.Constant(0), Expression.Constant(0), Expression.Constant(0)));
        }

-        [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotLinqExpressionsBuiltWithIsInterpretingOnly))]
+        [Fact]


Don't see ref emitting here and it passes with IL expression compiler disabled, so leaving like this.

MichalStrehovsky · 2021-12-01T07:51:34Z

src/libraries/System.Linq.Expressions/tests/InterpreterTests.cs

-        // IsNotLinqExpressionsBuiltWithIsInterpretingOnly is not directly required,
-        // but this functionality relies on private reflection and that would not work with AOT
-        [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotLinqExpressionsBuiltWithIsInterpretingOnly))]
+        [Fact]


Original motivation sounds .NET Native related.

MichalStrehovsky · 2021-12-01T07:53:38Z

src/libraries/System.Linq.Expressions/tests/New/NewWithByRefParameterTests.cs

@@ -70,7 +70,8 @@ public void CreateByRefAliasingInterpreted()
            CreateByRefAliasing(useInterpreter: true);
        }

-        [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotLinqExpressionsBuiltWithIsInterpretingOnly))]
+        [ActiveIssue("https://github.com/dotnet/runtime/issues/19286", typeof(PlatformDetection), nameof(PlatformDetection.IsLinqExpressionsBuiltWithIsInterpretingOnly))]


Switched this one to ActiveIssue - the preceding test has the motivation for this.

MichalStrehovsky · 2021-12-02T08:18:11Z

@cston do you have any objection to the change in DelegateHelpers.cs?

The behavior change is basically:

In deployment models with expression compiler disabled, but ref emit available, we would newly not throw PNSE, but make things work.
In deployment models with expression compiler disabled, but ref emit unavailable, no change.
In deployment models with ref emit unavilable, PNSE, but from a different codepath (Ref.Emit would throw originally, now we short circuit earlier).
In deployment models with expression compiler enabled and ref.emit available, no change.

Basically, it either makes things work, which sounds like a good thing, or just throws PNSE from a different place.

MichalStrehovsky added 2 commits December 1, 2021 15:03

first

3a956b8

The rest

2790531

ghost added the area-System.Linq.Expressions label Dec 1, 2021

MichalStrehovsky commented Dec 1, 2021

View reviewed changes

stephentoub approved these changes Dec 1, 2021

View reviewed changes

cston approved these changes Dec 3, 2021

View reviewed changes

MichalStrehovsky merged commit 98e3fbb into dotnet:main Dec 7, 2021

MichalStrehovsky deleted the exprfollow branch December 7, 2021 00:20

ghost locked as resolved and limited conversation to collaborators Jan 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Follow up to universal System.Linq.Expressions #62229

Follow up to universal System.Linq.Expressions #62229

Uh oh!

MichalStrehovsky commented Dec 1, 2021

Uh oh!

ghost commented Dec 1, 2021

Uh oh!

MichalStrehovsky left a comment

Uh oh!

MichalStrehovsky Dec 1, 2021

Uh oh!

MichalStrehovsky Dec 1, 2021

Uh oh!

MichalStrehovsky Dec 1, 2021

Uh oh!

MichalStrehovsky Dec 1, 2021

Uh oh!

MichalStrehovsky Dec 1, 2021

Uh oh!

MichalStrehovsky Dec 1, 2021

Uh oh!

MichalStrehovsky Dec 1, 2021

Uh oh!

MichalStrehovsky commented Dec 2, 2021

Uh oh!

Uh oh!

Follow up to universal System.Linq.Expressions #62229

Follow up to universal System.Linq.Expressions #62229

Uh oh!

Conversation

MichalStrehovsky commented Dec 1, 2021

Uh oh!

ghost commented Dec 1, 2021

Uh oh!

MichalStrehovsky left a comment

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky commented Dec 2, 2021

Uh oh!

Uh oh!