VB: Do not use overload resolution for expression tree conversion #75490
Conversation
dotnet-issue-labeler
bot
added
the
untriaged
| @@ -32,7 +22,6 @@ Namespace Microsoft.CodeAnalysis.VisualBasic | |||
| BinaryOperatorKind.Divide, | |||
| BinaryOperatorKind.Modulo, | |||
| BinaryOperatorKind.IntegerDivide, | |||
| BinaryOperatorKind.Concatenate, | |||
There was a problem hiding this comment.
nit: If it's the same situation as the Like, consider moving this case below and adding to the comment (line 44) #Resolved
|
@dotnet/roslyn-compiler Please review. |
| End Function | ||
|
|
||
| ' Emit a Default node for a specific type | ||
| Private Function [Default](type As TypeSymbol) As BoundExpression | ||
| Return ConvertRuntimeHelperToExpressionTree("Default", _factory.[Typeof](type, _factory.WellKnownType(WellKnownType.System_Type))) | ||
| Return _factory.Convert( |
There was a problem hiding this comment.
Could you provide some context on why some calls to ConvertRuntimeHelperToExpressionTree are now wrapped in a Convert but not all? #Closed
There was a problem hiding this comment.
Could you provide some context on why some calls to
ConvertRuntimeHelperToExpressionTreeare now wrapped in aConvertbut not all?
Normally, Visit does this conversion. However, we are not getting here due to a Visit invocation and this conversion was applied during overload resolution, which we no longer perform. Therefore, the conversion is applied explicitly because types should match exactly.
There was a problem hiding this comment.
From offline discussion, ConvertRuntimeHelperToExpressionTree that are returned from a Visit... method are already converted, we only need to apply the conversion explicitly for helper/factory methods.
FWIW, I didn't observe any break in CodeGenExprLambda.vb (on either net472 or net9.0) or in Build.cmd -testCompilerOnly -test when removing Convert in Default or New.
There was a problem hiding this comment.
I removed this conversion, apparently changes elsewhere made it unnecessary
| Return ConvertRuntimeHelperToExpressionTree("Call", factoryArgs) | ||
| Return _factory.Convert( | ||
| _expressionType, | ||
| ConvertRuntimeHelperToExpressionTree( |
There was a problem hiding this comment.
nit: The indentation is off (two arguments should be on same level like below) #Resolved
| End Function | ||
|
|
||
| Private Function InitWithParameterlessValueTypeConstructor(type As TypeSymbol) As BoundExpression | ||
| ' The "New" overload without a methodInfo automatically generates the parameterless constructor for us. | ||
| Debug.Assert(type.IsValueType) | ||
| Return ConvertRuntimeHelperToExpressionTree("New", _factory.[Typeof](type, _factory.WellKnownType(WellKnownType.System_Type))) | ||
| Return ConvertRuntimeHelperToExpressionTree(WellKnownMember.System_Linq_Expressions_Expression__New_Type, _factory.[Typeof](type, _factory.WellKnownType(WellKnownType.System_Type))) |
There was a problem hiding this comment.
It is surprising that we don't need Convert here since InitWithParameterlessValueTypeConstructor and [Default] are used in the same place in the same way. #Closed
There was a problem hiding this comment.
It is surprising that we don't need
Converthere sinceInitWithParameterlessValueTypeConstructorand[Default]are used in the same place in the same way.
I was adding conversions on as needed basis, when tests were failing. I also wasn't changing all ConvertRuntimeHelperToExpressionTree calls at the same time, so the conversions were necessary to pass tests at some point. It looks like the need to add conversions for some call sites got eliminated when all call sites were adjusted. I removed those. Thanks.
|
@dotnet/roslyn-compiler For the second review |
|
@dotnet/roslyn-compiler For the second review |
2 similar comments
|
@dotnet/roslyn-compiler For the second review |
|
@dotnet/roslyn-compiler For the second review |
| ElseIf opKind = BinaryOperatorKind.Concatenate Then | ||
| helper = DirectCast(_factory.SpecialMember(SpecialMember.System_String__ConcatStringString), MethodSymbol) | ||
|
|
||
| If helper Is Nothing Then ' Don't know how to do 'BinaryOperatorKind.Power' without the method |
There was a problem hiding this comment.
This comment and the one above look inaccurate. We might not be doing 'Power' operation.
I think the comment accurately reflects the motivation for adding the check. It is an error to fail to find the helper for any operator. The question is what the rewriter should do to recover. The error has been reported, but the rewriter needs to produce a bound node to return. It used to be that the rewriter would continue as though the helper is not needed and would produce a node of that shape, there are expression tree factories to do that (despite the fact that that wouldn't accurately reflect semantics of the operator). In order to maintain that recovery strategy, we would have to make use of a "Power" factory method that is never used in a success scenario, which, in my opinion, would lead to a confusion of anyone who would want to figure out what is a minimal set of factories required by VB. The new error recovery approach is going to work well for all operators, but the primary motivation is to exclude usage of a "Power" factory method that would never be needed in a success scenario.
Related to #74275.