Change trimming workaround for Linq.Expressions usage of custom operators

[vitek-karas]

When we annotated the System.Linq.Expressions for trimming we ran into a hard to solve problem with custom operators. The library will access custom operator methods via reflection in various places without a good way to track this via annotations or other means.
One of the goals was to make it possible to trim any expression tree construction code generated by the compiler. Unfortunately the compiler uses the overloads which do not specify the custom operator method as a parameter and instead it relies on code in the Expression class to find the custom operator on the target type (via reflection). For example for expression -x the compiler will generate a call to Expression.Negate(Expression.Parameter("x")).

We discussed a possibility of changing the compiler with the compiler team. The outcome is that the compiler team doesn't want to make this change in the compiler (to call the overloads which take MethodInfo - which would look like Expression.Negate(Expression.Parameter("x"), methodInfo)).

In .NET 6 and 7 the trimmer implemented a workaround for this. If the app uses System.Linq.Expressions.Expression type it will preserve all custom operators on all preserved types (there are some additional optimizations, but they're not interesting for this discussion). Due to this we were able to suppress the trim analysis warning generated in the library. See

runtime/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/UnaryExpression.cs

Lines 355 to 357 in 9d6396d

	[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2072:UnrecognizedReflectionPattern",
	Justification = "The trimmer doesn't remove operators when System.Linq.Expressions is used. See https://github.com/mono/linker/pull/2125.")]
	private static UnaryExpression? GetUserDefinedUnaryOperator(ExpressionType unaryType, string name, Expression operand)

The proposal is to change this workaround to recognize the method Expression.GetUserDefinedUnaryOperatorOrThrow as intrinsic (or basically known method) for the trimmer. So instead of relying on presence of the Expression type, make it more specific and only preserve custom operators on preserved types when the method Expression.GetUserDefinedOperatorOrThrow is actually called by the application. In addition, all the calls to this method pass a constant string as the name of the operator method, so the trimmer could only preserve the custom operators which are actually potentially used (by collecting the constant strings from all callsites).

The advantages of this change:

• More precise detection of the case where we need to preserve custom operators on types -> smaller apps which don't need the custom operators
• More precise detection of custom operators which need to be preserved -> smaller apps even if they do need some custom operators
• Potential for additional analysis improvements in the trimmer to eventually make the code fully analyzable and remove the warning suppression in the libraries (it's unclear what the complexity of that would be, but this change makes it possible).
• There's also reduced complexity of the implementation of this behavior in the trimmer mainly because it has rich infrastructure for detection and handling of intrinsic method calls. The current implementation relying on type existence is "special"

The disadvantages:

• Effectively making Expression.GetUserDefinedUnaryOperatorOrThrow a "semi-public" API - meaning we should not change it in any way (if we do we break tools, so possible but complex).

@MichalStrehovsky (the original author of this idea), @sbomer, @agocke, @eerhardt , @stephentoub

[ghost]

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

Issue Details

---

When we annotated the System.Linq.Expressions for trimming we ran into a hard to solve problem with custom operators. The library will access custom operator methods via reflection in various places without a good way to track this via annotations or other means.
One of the goals was to make it possible to trim any expression tree construction code generated by the compiler. Unfortunately the compiler uses the overloads which do not specify the custom operator method as a parameter and instead it relies on code in the Expression class to find the custom operator on the target type (via reflection). For example for expression -x the compiler will generate a call to Expression.Negate(Expression.Parameter("x")).

We discussed a possibility of changing the compiler with the compiler team. The outcome is that the compiler team doesn't want to make this change in the compiler (to call the overloads which take MethodInfo - which would look like Expression.Negate(Expression.Parameter("x"), methodInfo)).

In .NET 6 and 7 the trimmer implemented a workaround for this. If the app uses System.Linq.Expressions.Expression type it will preserve all custom operators on all preserved types (there are some additional optimizations, but they're not interesting for this discussion). Due to this we were able to suppress the trim analysis warning generated in the library. See

runtime/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/UnaryExpression.cs

Lines 355 to 357 in 9d6396d

	[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2072:UnrecognizedReflectionPattern",
	Justification = "The trimmer doesn't remove operators when System.Linq.Expressions is used. See https://github.com/mono/linker/pull/2125.")]
	private static UnaryExpression? GetUserDefinedUnaryOperator(ExpressionType unaryType, string name, Expression operand)

The proposal is to change this workaround to recognize the method Expression.GetUserDefinedUnaryOperatorOrThrow as intrinsic (or basically known method) for the trimmer. So instead of relying on presence of the Expression type, make it more specific and only preserve custom operators on preserved types when the method Expression.GetUserDefinedOperatorOrThrow is actually called by the application. In addition, all the calls to this method pass a constant string as the name of the operator method, so the trimmer could only preserve the custom operators which are actually potentially used (by collecting the constant strings from all callsites).

The advantages of this change:

• More precise detection of the case where we need to preserve custom operators on types -> smaller apps which don't need the custom operators
• More precise detection of custom operators which need to be preserved -> smaller apps even if they do need some custom operators
• Potential for additional analysis improvements in the trimmer to eventually make the code fully analyzable and remove the warning suppression in the libraries (it's unclear what the complexity of that would be, but this change makes it possible).
• There's also reduced complexity of the implementation of this behavior in the trimmer mainly because it has rich infrastructure for detection and handling of intrinsic method calls. The current implementation relying on type existence is "special"

The disadvantages:

• Effectively making Expression.GetUserDefinedUnaryOperatorOrThrow a "semi-public" API - meaning we should not change it in any way (if we do we break tools, so possible but complex).

@MichalStrehovsky (the original author of this idea), @sbomer, @agocke, @eerhardt , @stephentoub

Author:	vitek-karas
Assignees:	-
Labels:	area-System.Linq.Expressions, linkable-framework
Milestone:	-

[vitek-karas]

The question is basically: Is it OK to hardcode a dependency on the Expression.GetUserDefinedUnaryOperatorOrThrow method into the trimmer tools?

[MichalStrehovsky]

My justification for why it would be okay to hardcode the dependency is in the snippet Vitek quoted below: The trimmer doesn't remove operators when System.Linq.Expressions is used.. The suppression in S.L.Expression already hardcodes a dependency on trimming's internal implementation detail for this. Therefore it should be fine for this dependency to go both ways and turn the suppression into a two way contract that can be reasoned about.

It's a heuristic in the trimming logic right now that is hard to replicate in AOT (there's no "a type definition for System.Linq.Expressions.Expression was generated" that would be a reliable indicator).

We tried to avoid having the S.L.Expression side of the contract in dotnet/linker#1821 but that didn't work out. So I would at least make lemonade out of that lemon.

[stephentoub]

The disadvantages: Effectively making Expression.GetUserDefinedUnaryOperatorOrThrow a "semi-public" API - meaning we should not change it in any way (if we do we break tools, so possible but complex).

What if we actually made it public? Is this the API we'd want, would it fully solve the problem, does exposing it publicly expose implementation details we wouldn't want exposed, etc? It's just easier to avoid breaking things when we can rely on all of our normal mechanisms to do so.

[vitek-karas]

What if we actually made it public? Is this the API we'd want, would it fully solve the problem, does exposing it publicly expose implementation details we wouldn't want exposed, etc? It's just easier to avoid breaking things when we can rely on all of our normal mechanisms to do so.

I don't think we would want to make fully public API - in the sense that we document it and so on. We could make it public but not include it in the ref assembly. I can't remember if we use this approach elsewhere or if it's not something we want to do in general.

If we were to make it into a fully public feature, then we would probably want something more general - so move the method to a different class and rename it. I personally don't see much value in doing so though - I'm no aware of any other use case for this functionality outside of Linq.Expressions. (The other library which probably has similar logic in it is Microsoft.CSharp, but that one will never be trim compatible, and solving the trim compatibility just for unary operators would basically not help at all)

I would like to hear from the owners of the Linq.Expressions as well.

[jaredpar]

We discussed a possibility of changing the compiler with the compiler team. The outcome is that the compiler team doesn't want to make this change in the compiler (to call the overloads which take MethodInfo - which would look like Expression.Negate(Expression.Parameter("x"), methodInfo)).

Where did this discussion happen?

The only recollection I have on this is a few releases ago where we were asked to change codegen around Expression creation to avoid reflection paths. My memory of that isn't that we said "no" but that we said "not a priority at the moment". Having trouble though tracking down the exact issues where we had this discussion.

[vitek-karas]

Where did this discussion happen?

The only recollection I have on this is a few releases ago where we were asked to change codegen around Expression creation to avoid reflection paths. My memory of that isn't that we said "no" but that we said "not a priority at the moment". Having trouble though tracking down the exact issues where we had this discussion.

That was the discussion - it happened in .NET 6 timeframe when we were annotating Expressions. I can't find the issue either (I do remember that it happened somewhere though).

[eerhardt]

Where did this discussion happen?

dotnet/linker#1821 (comment)

[jaredpar]

The conclusion from that discussion was:

Believe this is relatively doable in the compiler. There are a few choke points in the expression rewriter that we could leverage to call the new methods. Probably the biggest issue is validating the assumption that whenever there is a user defined operator that we do indeed pass a MethodInfo along with it.

Whenever making expression tree changes though, even if it's intended to be a no-op, we need to involve the EF Core team. They have the heaviest dependency on expression trees and can validate if the new structure does / doesn't work for them.

Probably a small / medium feature in the compiler.

I don't know how this is translating to compiler isn't going to do this. It just wasn't at the right priority level two releases ago. I'd rather we put this into the prioritization bucket then push forward old decisions without discussion.

[agocke]

My recollection actually matches Jared's. At the time we needed something that would work in the near term and was relatively cheap to implement. The linker approach seemed simple at the time and we didn't need to go through API review, which I think was perceived to be the biggest cost of this change.

I think it's definitely worth revisiting this now, at the beginning of .NET 8, to see if we'd prefer to redouble our efforts to move to a trimmable-by-design LINQ design.

[vitek-karas]

My apologies @jaredpar - obviously my memory was wrong.
It would be great if the compiler could help us here.

What do we think about supporting binaries compiled with the older version? I think the tooling still needs to support it - probably for a rather long time. The proposal in this issue would simplify such support (and make it more precise).

[agocke]

What do we think about supporting binaries compiled with the older version?

Not really sure how important this is. Maybe something I would want to find out with customer response, or try to scan the IL from popular NuGet packages.

[jaredpar]

@vitek-karas

No problem. It took me a good 30 minutes to page in my memories as it was 😄

What do we think about supporting binaries compiled with the older version?

The other parts of the trimming work I'm involved in I believe only work when you move to a new target framework. Or at least use a new compiler / SDK. That would seem to indicate that older binaries weren't as important. But I also realize I that I'm only seeing a smaller piece of the puzzle here.

[vitek-karas]

My main concern is libraries (NuGets) which are not affected by the SDK version used to build the app. The proposal here would work in that case as well (Since it's tied to a TFM/Framework assembly and that will work just fine). But if we implement a compiler solution, it will not solve it for already compiled binaries.
On top of that, quite a few NuGets may not be built with latest SDKs, mainly because they target older TFMs anyway.

But I agree it's likely not a big problem.

[jaredpar]

Had to re-read the original issue and it reminded me this is tied to new APIs. So it's not just a new SDK, it's a new TFM + SDK. That is indeed a bigger hurdle than I was thinking.

[jkotas]

quite a few NuGets may not be built with latest SDKs, mainly because they target older TFMs anyway

If a NuGet is not trim compatible as is, it is fine to require recompilation with latest SDK and switching to new trim-compatible API if it is the right way to fix the trimming issues.

If it is straightforward to make a new trimming-related feature work downlevel (e.g. it is just a new custom attribute annotation), it makes sense to allow it work downlevel. If it is not, the feature should be available in latest SDK/runtime only and people have to update to get it. We have been applying this principle in number of situations, for example many of the source generators that are required to fix trimming issues are not available downlevel.

[eerhardt]

Hopefully we can solve this with a compiler change to make Linq.Expression code more trimming/AOT compatible.

But if we can't, and are still looking for feedback on the original proposal here, note that we will need to make more than just Expression.GetUserDefinedUnaryOperatorOrThrow as intrinsic. That only handles unary operators. We would also need to make binary, coercion, and boolean operators as intrinsic as well:

runtime/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/BinaryExpression.cs

Lines 713 to 715 in 7bbee52

	[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2072:UnrecognizedReflectionPattern",
	Justification = "The trimmer doesn't remove operators when System.Linq.Expressions is used. See https://github.com/mono/linker/pull/2125.")]
	private static MethodInfo? GetUserDefinedBinaryOperator(ExpressionType binaryType, Type leftType, Type rightType, string name)

runtime/src/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/TypeUtils.cs

Lines 626 to 628 in 7bbee52

	[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2075:UnrecognizedReflectionPattern",
	Justification = "The trimmer doesn't remove operators when System.Linq.Expressions is used. See https://github.com/mono/linker/pull/2125.")]
	public static MethodInfo? GetUserDefinedCoercionMethod(Type convertFrom, Type convertToType)

runtime/src/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/TypeUtils.cs

Lines 833 to 835 in 7bbee52

	[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2067:UnrecognizedReflectionPattern",
	Justification = "The trimmer doesn't remove operators when System.Linq.Expressions is used. See https://github.com/mono/linker/pull/2125.")]
	public static MethodInfo? GetBooleanOperator(Type type, string name)