Skip to content

Extensions: fix lowering of implicit conversion on receiver in pattern-based scenarios #78685

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 4, 2025
Merged

Extensions: fix lowering of implicit conversion on receiver in pattern-based scenarios #78685

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2109823
Extensions: fix lowering of implicit conversion on receiver in
jcouv May 21, 2025
105a630
Tweak test
jcouv May 23, 2025
c200978
Address feedback
jcouv Jun 3, 2025
e4933cb
Update tests
jcouv Jun 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3026,13 +3026,13 @@ private bool CheckInvocationArgMixingWithUpdatedRules(
}
}

inferDeclarationExpressionValEscape();
inferDeclarationExpressionValEscape(argsOpt, localScopeDepth, escapeValues);

mixableArguments.Free();
escapeValues.Free();
return valid;

void inferDeclarationExpressionValEscape()
void inferDeclarationExpressionValEscape(ImmutableArray<BoundExpression> argsOpt, SafeContext localScopeDepth, ArrayBuilder<EscapeValue> escapeValues)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 I was debugging in this area as part of this PR and noticed we're doing some captures

{
// find the widest scope that arguments could safely escape to.
// use this scope as the inferred STE of declaration expressions.
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.PatternLocalRewriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ protected BoundExpression LowerEvaluation(BoundDagEvaluation evaluation)
PropertySymbol property = p.Property;
var outputTemp = new BoundDagTemp(p.Syntax, property.Type, p);
BoundExpression output = _tempAllocator.GetTemp(outputTemp);
input = _factory.ConvertReceiverForExtensionMemberIfNeeded(property, input);
input = _localRewriter.ConvertReceiverForExtensionMemberIfNeeded(property, input);
return _factory.AssignmentExpression(output, _localRewriter.MakePropertyAccess(_factory.Syntax, input, property, LookupResultKind.Viable, property.Type, isLeftOfAssignment: false));
}

Expand Down Expand Up @@ -191,7 +191,7 @@ void addArg(RefKind refKind, BoundExpression expression)
addArg(RefKind.Out, _tempAllocator.GetTemp(outputTemp));
}

receiver = _factory.ConvertReceiverForExtensionMemberIfNeeded(method, receiver);
receiver = _localRewriter.ConvertReceiverForExtensionMemberIfNeeded(method, receiver);
return _factory.Call(receiver, method, refKindBuilder.ToImmutableAndFree(), argBuilder.ToImmutableAndFree());
}

Expand Down
18 changes: 18 additions & 0 deletions src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1136,6 +1136,24 @@ private CompoundUseSiteInfo<AssemblySymbol> GetNewCompoundUseSiteInfo()
return new CompoundUseSiteInfo<AssemblySymbol>(_diagnostics, _compilation.Assembly);
}

private BoundExpression ConvertReceiverForExtensionMemberIfNeeded(Symbol member, BoundExpression receiver)
{
if (member.GetIsNewExtensionMember())
{
Debug.Assert(!member.IsStatic);
ParameterSymbol? extensionParameter = member.ContainingType.ExtensionParameter;
Debug.Assert(extensionParameter is not null);
#if DEBUG
var discardedUseSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded;
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);
Copy link
Contributor

@AlekseyTs AlekseyTs May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

markAsChecked: true

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. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whenever initial binding produces the nodes for an extension property access, we go through ResolveExtensionMemberAccessIfResultIsNonMethod/GetExtensionMemberAccess which does CheckAndConvertExtensionReceiver on the receiver.
So I believe markAsChecked: true is correct.
For the broader issue of not recreating conversions during lowering, let's discuss offline.

Copy link
Contributor

@AlekseyTs AlekseyTs Jun 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to suggest the following:

  1. Instead of assuming that each caller performed binding in a certain way, we should request each consumer of ConvertReceiverForExtensionMemberIfNeeded to assert that by passing true to markAsChecked parameter (which I suggest to add).
  2. For each call site of ConvertReceiverForExtensionMemberIfNeeded let's try to add a test that makes the fact that ```CheckAndConvertExtensionReceiver `` was called observable. Perhaps by observing a diagnostic that otherwise wouldn't be reported.

Copy link
Contributor

@AlekseyTs AlekseyTs May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@checked: false

This looks suspicious. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only implicit conversions are allowed on the receiver of the extension member. Implicit conversions don't care about checked/unchecked (only certain operators and explicit conversions care)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider capturing this line of thinking in a comment.

}

return receiver;
}

#if DEBUG
/// <summary>
/// Note: do not use a static/singleton instance of this type, as it holds state.
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_FixedStatement.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ private BoundStatement InitializeFixedStatementGetPinnable(
}

// .GetPinnable()
callReceiver = factory.ConvertReceiverForExtensionMemberIfNeeded(getPinnableMethod, callReceiver);
callReceiver = this.ConvertReceiverForExtensionMemberIfNeeded(getPinnableMethod, callReceiver);
var getPinnableCall = getPinnableMethod.IsStatic ?
factory.Call(null, getPinnableMethod, callReceiver) :
factory.Call(callReceiver, getPinnableMethod);
Expand Down
2 changes: 1 addition & 1 deletion .../Portable/Lowering/LocalRewriter/LocalRewriter_ObjectOrCollectionInitializerExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -702,7 +702,7 @@ private BoundExpression MakeObjectInitializerMemberAccess(
_compilation.Conversions.HasImplicitConversionToOrImplementsVarianceCompatibleInterface(rewrittenReceiver.Type, memberSymbol.ContainingType, ref discardedUseSiteInfo, out _));
// It is possible there are use site diagnostics from the above, but none that we need report as we aren't generating code for the conversion
#endif
rewrittenReceiver = _factory.ConvertReceiverForExtensionMemberIfNeeded(memberSymbol, rewrittenReceiver);
rewrittenReceiver = this.ConvertReceiverForExtensionMemberIfNeeded(memberSymbol, rewrittenReceiver);
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.ConvertReceiverForExtensionMemberIfNeeded(memberSymbol, rewrittenReceiver);

Is there a test that would fail for this code path without the change? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not able to find one.
We need:

  1. a conversion that is acceptable for the extension parameter: those are identity, boxing, implicit reference, implicit span, or implicit tuple (see IsValidExtensionMethodThisArgConversion)
  2. the conversion needs to be non-trivial (ie. needs lowering): we're left with implicit span or implicit tuple
  3. the receiver type of the property should not be a struct (see logic in CheckPropertyValueKind involving RequiresVariableReceiver): we're left with no options

The ExtensionMemberLookup_ObjectInitializer_Conversion_02 test illustrates the ROS scenario. I'll add a couple similar tests


switch (memberSymbol.Kind)
{
Expand Down
18 changes: 0 additions & 18 deletions src/Compilers/CSharp/Portable/Lowering/SyntheticBoundNodeFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,24 +431,6 @@ public BoundExpression AssignmentExpression(BoundExpression left, BoundExpressio
return AssignmentExpression(Syntax, left, right, isRef: isRef, wasCompilerGenerated: true);
}

public BoundExpression ConvertReceiverForExtensionMemberIfNeeded(Symbol member, BoundExpression receiver)
{
if (member.GetIsNewExtensionMember())
{
Debug.Assert(!member.IsStatic);
ParameterSymbol? extensionParameter = member.ContainingType.ExtensionParameter;
Debug.Assert(extensionParameter is not null);
#if DEBUG
var discardedUseSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded;
Debug.Assert(Conversions.IsValidExtensionMethodThisArgConversion(this.Compilation.Conversions.ClassifyConversionFromType(receiver.Type, extensionParameter.Type, isChecked: false, ref discardedUseSiteInfo)));
#endif

return this.Convert(extensionParameter.Type, receiver);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 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).

}

return receiver;
}

/// <summary>
/// Creates a general assignment that might be instrumented.
/// </summary>
Expand Down
Loading