Skip to content

Extensions: address instrumentation follow-ups #79557

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 5 commits into from
Jul 31, 2025
Merged

Extensions: address instrumentation follow-ups #79557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bc99cda
Extensions: address instrumentation follow-ups
Jul 18, 2025
8251ca5
Use ParameterId for index
Jul 23, 2025
184aa0d
Adjust arguments for local state tracing. Add tests for CodeCoverageI…
Jul 23, 2025
9616ffe
Address feedback
Jul 31, 2025
30b3a28
Address feedback
Jul 31, 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
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -727,7 +727,7 @@
<!-- Instrumentation info attached to BoundBlock -->
<Node Name="BoundBlockInstrumentation" Base="BoundNode">
<!-- Local variables with scope that cover the entire block including the instrumentation prologue and epilogue. -->
<Field Name="Locals" Type="OneOrMany&lt;LocalSymbol&gt;" Null="disallow"/>
<Field Name="Locals" Type="ImmutableArray&lt;LocalSymbol&gt;" Null="disallow"/>
Copy link
Member Author

@jcouv jcouv Jul 23, 2025

Choose a reason for hiding this comment

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

📝 The purpose of this change is to get the locals rewritten (see VisitLocals in VisitBlockInstrumentation). Otherwise, we end up with non-rewritten locals in the emit stage (ie. not found in LocalSlotManager)

<!-- Optional prologue emitted in front of any instructions of the block, so that it always executes. -->
<Field Name="Prologue" Type="BoundStatement?" Null="allow"/>
<!--
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3593,7 +3593,7 @@ private void EmitParameterIdExpression(BoundParameterId node)

if (node.HoistedField is null)
{
_builder.EmitIntConstant(node.Parameter.Ordinal); // Tracked by https://github.com/dotnet/roslyn/issues/78963 : Follow up
_builder.EmitIntConstant(node.Parameter.Ordinal);
}
else
{
Expand Down
8 changes: 0 additions & 8 deletions src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2305,14 +2305,6 @@ private void DeclareVariables(ImmutableArray<LocalSymbol> locals)
}
}

private void DeclareVariables(OneOrMany<LocalSymbol> locals)
Copy link
Contributor

@AlekseyTs AlekseyTs Jul 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

DeclareVariables

I am assuming the place that called this method silently calls the method above. #Closed

Copy link
Member Author

@jcouv jcouv Jul 31, 2025

Choose a reason for hiding this comment

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

Correct

{
foreach (var symbol in locals)
{
DeclareVariable(symbol);
}
}

private void DeclareVariable(LocalSymbol symbol)
{
var initiallyAssigned =
Expand Down
5 changes: 3 additions & 2 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6660,14 +6660,15 @@ static ImmutableArray<ParameterSymbol> getParameters(ImmutableArray<ParameterSym
}
}

private static ImmutableArray<RefKind> GetArgumentRefKinds(ImmutableArray<RefKind> argumentRefKindsOpt, bool isNewExtension,
internal static ImmutableArray<RefKind> GetArgumentRefKinds(ImmutableArray<RefKind> argumentRefKindsOpt, bool adjustForNewExtension,
MethodSymbol method, int argumentCount)
{
if (!isNewExtension)
if (!adjustForNewExtension)
{
return argumentRefKindsOpt;
}

Debug.Assert(method.GetIsNewExtensionMember());
RefKind receiverRefKind = GetExtensionReceiverRefKind(method);

if (argumentRefKindsOpt.IsDefault)
Expand Down
19 changes: 14 additions & 5 deletions src/Compilers/CSharp/Portable/Generated/BoundNodes.xml.Generated.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 10 additions & 4 deletions src/Compilers/CSharp/Portable/Lowering/ExtensionMethodBodyRewriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ namespace Microsoft.CodeAnalysis.CSharp
/// </summary>
internal sealed class ExtensionMethodBodyRewriter : BoundTreeToDifferentEnclosingContextRewriter
{
private readonly SourceExtensionImplementationMethodSymbol _implementationMethod;

/// <summary>
/// Maps parameters and local functions from original enclosing context to corresponding rewritten symbols for rewritten context.
/// </summary>
Expand All @@ -37,7 +35,6 @@ public ExtensionMethodBodyRewriter(MethodSymbol sourceMethod, SourceExtensionImp
Debug.Assert(implementationMethod is not null);
Debug.Assert(sourceMethod == (object)implementationMethod.UnderlyingMethod);

_implementationMethod = implementationMethod;
_symbolMap = ImmutableDictionary<Symbol, Symbol>.Empty.WithComparers(ReferenceEqualityComparer.Instance, ReferenceEqualityComparer.Instance);

bool haveExtraParameter = sourceMethod.ParameterCount != implementationMethod.ParameterCount;
Expand Down Expand Up @@ -97,6 +94,10 @@ public override ParameterSymbol VisitParameterSymbol(ParameterSymbol symbol)
var rewritten = new RewrittenLambdaOrLocalFunctionSymbol(node.Symbol, _rewrittenContainingMethod);

var savedState = EnterMethod(node.Symbol, rewritten);

// BoundMethodDefIndex in instrumentation will refer to the lambda method symbol, so we need to map it.
_symbolMap = _symbolMap.Add(node.Symbol, rewritten);
Copy link
Contributor

@AlekseyTs AlekseyTs Jul 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

_symbolMap

Should the map be restored at the end? #Closed

Copy link
Member Author

@jcouv jcouv Jul 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't see harm in leaving it in scope
I'll adjust. Feels cleaner not to leave


BoundBlock body = (BoundBlock)this.Visit(node.Body);
(_rewrittenContainingMethod, _symbolMap) = savedState;

Expand Down Expand Up @@ -150,7 +151,7 @@ protected override ImmutableArray<MethodSymbol> VisitDeclaredLocalFunctions(Immu
switch (symbol?.MethodKind)
{
case MethodKind.LambdaMethod:
throw ExceptionUtilities.Unreachable();
return (MethodSymbol)_symbolMap[symbol];

case MethodKind.LocalFunction:
if (symbol.IsDefinition)
Expand Down Expand Up @@ -208,5 +209,10 @@ public override BoundNode VisitUnaryOperator(BoundUnaryOperator node)
{
return ExtensionMethodReferenceRewriter.VisitBinaryOperatorData(this, node);
}

public override BoundNode? VisitMethodDefIndex(BoundMethodDefIndex node)
{
return ExtensionMethodReferenceRewriter.VisitMethodDefIndex(this, node);
}
}
}
10 changes: 7 additions & 3 deletions src/Compilers/CSharp/Portable/Lowering/ExtensionMethodReferenceRewriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,9 +194,13 @@ method.OriginalDefinition is ErrorMethodSymbol ||

public override BoundNode? VisitMethodDefIndex(BoundMethodDefIndex node)
{
MethodSymbol method = node.Method;
Debug.Assert(method.IsDefinition); // Tracked by https://github.com/dotnet/roslyn/issues/78962 : From the code coverage and other instrumentations perspective, should we remap the index to the implementation symbol?
TypeSymbol? type = this.VisitType(node.Type);
return VisitMethodDefIndex(this, node);
}

public static BoundNode VisitMethodDefIndex(BoundTreeRewriter rewriter, BoundMethodDefIndex node)
{
MethodSymbol method = VisitMethodSymbolWithExtensionRewrite(rewriter, node.Method);
TypeSymbol? type = rewriter.VisitType(node.Type);
return node.Update(method, type);
}

Expand Down
25 changes: 22 additions & 3 deletions src/Compilers/CSharp/Portable/Lowering/Instrumentation/LocalStateTracingInstrumenter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ public override void InstrumentBlock(BoundBlock original, LocalRewriter rewriter

var prologueBuilder = ArrayBuilder<BoundStatement>.GetInstance(_factory.CurrentFunction.ParameterCount);

foreach (var parameter in _factory.CurrentFunction.Parameters)
foreach (var parameter in _factory.CurrentFunction.GetParametersIncludingExtensionParameter(skipExtensionIfStatic: true))
{
if (parameter.RefKind == RefKind.Out || parameter.IsDiscard)
{
Expand All @@ -312,8 +312,12 @@ public override void InstrumentBlock(BoundBlock original, LocalRewriter rewriter
var parameterLogger = GetLocalOrParameterStoreLogger(parameter.Type, parameter, refAssignmentSourceIsLocal: null, _factory.Syntax);
if (parameterLogger != null)
{
int ordinal = parameter.ContainingSymbol.GetIsNewExtensionMember()
? SourceExtensionImplementationMethodSymbol.GetImplementationParameterOrdinal(parameter)
: parameter.Ordinal;

prologueBuilder.Add(_factory.ExpressionStatement(_factory.Call(receiver: _factory.Local(_scope.ContextVariable), parameterLogger,
MakeStoreLoggerArguments(parameterLogger.Parameters[0], parameter, parameter.Type, _factory.Parameter(parameter), refAssignmentSourceIndex: null, _factory.Literal((ushort)parameter.Ordinal)))));
MakeStoreLoggerArguments(parameterLogger.Parameters[0], parameter, parameter.Type, _factory.Parameter(parameter), refAssignmentSourceIndex: null, _factory.Literal((ushort)ordinal)))));
}
}

Expand Down Expand Up @@ -440,6 +444,7 @@ private ImmutableArray<BoundExpression> MakeStoreLoggerArguments(
BoundExpression? refAssignmentSourceIndex,
BoundExpression index)
{
Debug.Assert(index is BoundParameterId or BoundLocalId or BoundLiteral);
if (refAssignmentSourceIndex != null)
{
return ImmutableArray.Create(_factory.Sequence(new[] { value }, refAssignmentSourceIndex), index);
Expand Down Expand Up @@ -541,7 +546,20 @@ public override void InstrumentCatchBlock(
}

public override BoundExpression InstrumentCall(BoundCall original, BoundExpression rewritten)
=> InstrumentCall(base.InstrumentCall(original, rewritten), original.Arguments, original.ArgumentRefKindsOpt);
{
ImmutableArray<BoundExpression> arguments = original.Arguments;
MethodSymbol method = original.Method;
bool adjustForNewExtension = method.GetIsNewExtensionMember() && !method.IsStatic;
ImmutableArray<RefKind> argumentRefKindsOpt = NullableWalker.GetArgumentRefKinds(original.ArgumentRefKindsOpt, adjustForNewExtension, method, arguments.Length);

if (adjustForNewExtension)
{
Debug.Assert(original.ReceiverOpt is not null);
arguments = [original.ReceiverOpt, .. arguments];
}

return InstrumentCall(base.InstrumentCall(original, rewritten), arguments, argumentRefKindsOpt);
}

public override BoundExpression InstrumentObjectCreationExpression(BoundObjectCreationExpression original, BoundExpression rewritten)
=> InstrumentCall(base.InstrumentObjectCreationExpression(original, rewritten), original.Arguments, original.ArgumentRefKindsOpt);
Expand Down Expand Up @@ -572,6 +590,7 @@ private BoundExpression InstrumentCall(BoundExpression invocation, ImmutableArra
builder.Add(invocation);
}

// Record outbound assignments
for (int i = 0; i < arguments.Length; i++)
{
if (refKinds[i] is not (RefKind.Ref or RefKind.Out))
Expand Down
4 changes: 2 additions & 2 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 @@ -522,7 +522,7 @@ public BoundBlockInstrumentation CombineInstrumentation(BoundBlockInstrumentatio
(epilogue != null) ? Concat(innerInstrumentation.Epilogue, epilogue) : innerInstrumentation.Epilogue)
: new BoundBlockInstrumentation(
Syntax,
OneOrMany.OneOrNone(local),
(local != null) ? [local] : [],
prologue,
epilogue);
}
Expand Down Expand Up @@ -550,7 +550,7 @@ public BoundStatement Instrument(BoundStatement statement, BoundBlockInstrumenta
statements.Add(statement);
}

return Block(instrumentation.Locals.ToImmutable(), statements.ToImmutableAndClear());
return Block(instrumentation.Locals, statements.ToImmutableAndClear());
}

public BoundReturnStatement Return(BoundExpression? expression = null)
Expand Down
33 changes: 18 additions & 15 deletions ...Compilers/CSharp/Portable/Symbols/Extensions/SourceExtensionImplementationMethodSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,6 @@ protected override ImmutableArray<ParameterSymbol> MakeParameters()

if (!_originalMethod.IsStatic)
{
// Tracked by https://github.com/dotnet/roslyn/issues/78962 : Need to confirm if this rewrite going to break LocalStateTracingInstrumenter
// Specifically BoundParameterId, etc.
parameters.Add(new ExtensionMetadataMethodParameterSymbol(this, ((SourceNamedTypeSymbol)_originalMethod.ContainingType).ExtensionParameter!));
}

Expand All @@ -138,6 +136,23 @@ protected override ImmutableArray<ParameterSymbol> MakeParameters()
return parameters.ToImmutableAndFree();
}

internal static int GetImplementationParameterOrdinal(ParameterSymbol underlyingParameter)
{
if (underlyingParameter.ContainingSymbol is NamedTypeSymbol)
{
return 0;
}

var ordinal = underlyingParameter.Ordinal;

if (underlyingParameter.ContainingSymbol.IsStatic)
{
return ordinal;
}

return ordinal + 1;
}

internal override bool TryGetThisParameter(out ParameterSymbol? thisParameter)
{
thisParameter = null;
Expand Down Expand Up @@ -181,19 +196,7 @@ public override int Ordinal
{
get
{
if (this._underlyingParameter.ContainingSymbol is NamedTypeSymbol)
{
return 0;
}

var ordinal = this._underlyingParameter.Ordinal;

if (this._underlyingParameter.ContainingSymbol.IsStatic)
{
return ordinal;
}

return ordinal + 1;
return GetImplementationParameterOrdinal(this._underlyingParameter);
}
}

Expand Down
Loading
Loading