Do not use overload resolution for expression tree conversion #75427
Conversation
dotnet-issue-labeler
bot
added
the
untriaged
|
|
||
| private BoundExpression ExprFactory(string name, ImmutableArray<TypeSymbol> typeArgs, params BoundExpression[] arguments) | ||
| { | ||
| return _bound.StaticCall(_ignoreAccessibility ? BinderFlags.IgnoreAccessibility : BinderFlags.None, ExpressionType, name, disallowExpandedNonArrayParams: true, typeArgs, arguments); |
There was a problem hiding this comment.
It looks like we no longer use _ignoreAccessibility. Is that an unintended behavior change? Also consider removing the unused field. #Resolved
There was a problem hiding this comment.
It looks like we no longer use
_ignoreAccessibility. Is that an unintended behavior change?
Definitions of well known types and members must be accessible. I am not sure why this field was used. I assume that, if there were specific scenarios in mind, there would be tests that would fail. BTW, the ExpressionType in which the factory methods are, was aways a well known type.
Also consider removing the unused field.
Will do.
| { | ||
| if (_binder is null || _binder.Flags != flags) | ||
| { | ||
| _binder = new SyntheticBinderImpl(this).WithFlags(flags); |
There was a problem hiding this comment.
Can we remove SyntheticBinderImpl class now? #Resolved
There was a problem hiding this comment.
Can we remove
SyntheticBinderImplclass now?
Yes
| @@ -337,6 +337,23 @@ internal enum WellKnownType | |||
|
|
|||
| System_Runtime_CompilerServices_ParamCollectionAttribute, | |||
|
|
|||
| System_Linq_Expressions_BinaryExpression, | |||
There was a problem hiding this comment.
Because we use well-known types now, there might be some behavior changes, right? For example, having ambiguous BinaryExpressions (so we won't resolve the well-known type for it), we now won't be able to bind Expression.Binary whereas previously we could. I'm not sure it matters though. #Resolved
There was a problem hiding this comment.
Because we use well-known types now, there might be some behavior changes, right? For example, having ambiguous
BinaryExpressions (so we won't resolve the well-known type for it), we now won't be able to bindExpression.Binarywhereas previously we could.
That is correct.
There was a problem hiding this comment.
This only happens when customers define a their own types in the System namespace though. That is indeed a break but it's generally one that we are okay with. Defining types in the System namespace, particularly when they clash with the ones in corelib is not recommended for this very reason
Related to #74275.