Extensions: fix lowering of implicit conversion on receiver in pattern-based scenarios #78685
Conversation
jcouv
added
Area-Compilers
Feature - Extension Everything
|
|
||
| mixableArguments.Free(); | ||
| escapeValues.Free(); | ||
| return valid; | ||
|
|
||
| void inferDeclarationExpressionValEscape() | ||
| void inferDeclarationExpressionValEscape(ImmutableArray<BoundExpression> argsOpt, SafeContext localScopeDepth, ArrayBuilder<EscapeValue> escapeValues) |
There was a problem hiding this comment.
📝 I was debugging in this area as part of this PR and noticed we're doing some captures
| Debug.Assert(Conversions.IsValidExtensionMethodThisArgConversion(this.Compilation.Conversions.ClassifyConversionFromType(receiver.Type, extensionParameter.Type, isChecked: false, ref discardedUseSiteInfo))); | ||
| #endif | ||
|
|
||
| return this.Convert(extensionParameter.Type, receiver); |
There was a problem hiding this comment.
📝 the problem was that this produces a BoundConversion. That's okay (can be handled by the emit layer) for simple conversions, but for more complex conversions we need to produce lowered nodes (see MakeConversionNode).
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
pattern-based scenarios
jcouv
force-pushed
the
extensions-conversions2
branch
from
71f0783 to
2109823
Compare
| Debug.Assert(Conversions.IsValidExtensionMethodThisArgConversion(this._compilation.Conversions.ClassifyConversionFromType(receiver.Type, extensionParameter.Type, isChecked: false, ref discardedUseSiteInfo))); | ||
| #endif | ||
|
|
||
| return MakeConversionNode(receiver, extensionParameter.Type, @checked: false, acceptFailingConversion: false, markAsChecked: true); |
There was a problem hiding this comment.
I do not see any corresponding changes in binding, therefore, I am not certain whether we really can pass true here. The contract is, we can pass true only if this conversion was actually checked during binding. This is achieved by calling CreateConversion helper and actually attempting to create BoundConversion node (there are plenty examples for this). In addition, it is preferred to not drop the created BoundConversion node, but preserve it in the tree and use it during rewrite in order to actually produce its lowered form. There is a small set of cases when we do not preserve the node given too much complexity of the bound node structure, but we still "check" conversion during binding for those cases.
| Debug.Assert(Conversions.IsValidExtensionMethodThisArgConversion(this._compilation.Conversions.ClassifyConversionFromType(receiver.Type, extensionParameter.Type, isChecked: false, ref discardedUseSiteInfo))); | ||
| #endif | ||
|
|
||
| return MakeConversionNode(receiver, extensionParameter.Type, @checked: false, acceptFailingConversion: false, markAsChecked: true); |
There was a problem hiding this comment.
@checked: false
This looks suspicious.
This is a follow-up on #78480, which didn't properly handle the lowering of some more complex implicit conversions (implicit span or implicit tuple conversions).
Relates to test plan #76130