Skip to content
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

Jump to bottom

Improve sequence point placement in primary constructors and records #68765

Merged
merged 7 commits into from
Jul 19, 2023
Merged

Improve sequence point placement in primary constructors and records #68765

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
312a568
Improve sequence point placement in primary constructors and records
tmat Jun 25, 2023
9110372
Fixes
tmat Jun 26, 2023
97f9a42
Fix
tmat Jun 26, 2023
9949c81
Undo
tmat Jun 26, 2023
a670b35
Update src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/Synt…
tmat Jul 10, 2023
5131fe1
Feedback
tmat Jul 18, 2023
9e9887f
Feedback
tmat Jul 18, 2023
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
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Compiler/MethodBodySynthesizer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ internal static BoundBlock ConstructAutoPropertyAccessorBody(SourceMemberMethodS
else
{
Debug.Assert(accessor.MethodKind == MethodKind.PropertySet);

var parameter = accessor.Parameters[0];
statement = new BoundExpressionStatement(
accessor.SyntaxNode,
Expand Down
87 changes: 71 additions & 16 deletions src/Compilers/CSharp/Portable/Lowering/Instrumentation/DebugInfoInjector.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,21 +58,65 @@ public override BoundStatement InstrumentExpressionStatement(BoundExpressionStat

if (original.IsConstructorInitializer())
{
switch (original.Syntax.Kind())
switch (original.Syntax)
{
case SyntaxKind.ConstructorDeclaration:
// This is an implicit constructor initializer.
var decl = (ConstructorDeclarationSyntax)original.Syntax;
return new BoundSequencePointWithSpan(decl, rewritten, CreateSpanForConstructorInitializer(decl));
case SyntaxKind.BaseConstructorInitializer:
case SyntaxKind.ThisConstructorInitializer:
var init = (ConstructorInitializerSyntax)original.Syntax;
Debug.Assert(init.Parent is object);
return new BoundSequencePointWithSpan(init, rewritten, CreateSpanForConstructorInitializer((ConstructorDeclarationSyntax)init.Parent));
case ConstructorDeclarationSyntax ctorDecl:
// Implicit constructor initializer:
return new BoundSequencePointWithSpan(ctorDecl, rewritten, createSpanForConstructorInitializer(ctorDecl));
tmat marked this conversation as resolved.
Show resolved Hide resolved

case ConstructorInitializerSyntax ctorInit:
// Explicit constructor initializer:
Debug.Assert(ctorInit.Parent is ConstructorDeclarationSyntax);
tmat marked this conversation as resolved.
Show resolved Hide resolved
return new BoundSequencePointWithSpan(ctorInit, rewritten, createSpanForConstructorInitializer((ConstructorDeclarationSyntax)ctorInit.Parent));

case TypeDeclarationSyntax typeDecl:
// Primary constructor with implicit base initializer:
// class [|C<T>(int X, int Y)|] : B;
Debug.Assert(typeDecl.ParameterList != null);
Debug.Assert(typeDecl.BaseList?.Types.Any(t => t is PrimaryConstructorBaseTypeSyntax { ArgumentList: not null }) != true);

return new BoundSequencePointWithSpan(typeDecl, rewritten, TextSpan.FromBounds(typeDecl.Identifier.SpanStart, typeDecl.ParameterList.Span.End));

case PrimaryConstructorBaseTypeSyntax baseInit:
// Explicit base initializer:
// class C<T>(int X, int Y) : [|B(...)|];
return new BoundSequencePointWithSpan(baseInit, rewritten, baseInit.Span);

default:
throw ExceptionUtilities.UnexpectedValue(original.Syntax.Kind());
}
}
else if (original.Syntax is ParameterSyntax parameterSyntax)
{
// Record property setter sequence point.
return new BoundSequencePointWithSpan(parameterSyntax, rewritten, CreateSpan(parameterSyntax));
}

return AddSequencePoint(rewritten);

static TextSpan createSpanForConstructorInitializer(ConstructorDeclarationSyntax constructorSyntax)
{
if (constructorSyntax.Initializer != null)
{
// [SomeAttribute] public MyCtorName(params int[] values): [|base()|] { ... }
var start = constructorSyntax.Initializer.ThisOrBaseKeyword.SpanStart;
var end = constructorSyntax.Initializer.ArgumentList.CloseParenToken.Span.End;
return TextSpan.FromBounds(start, end);
}

if (constructorSyntax.Modifiers.Any(SyntaxKind.StaticKeyword))
{
Debug.Assert(constructorSyntax.Body != null);

// [SomeAttribute] static MyCtorName(...) [|{|] ... }
var start = constructorSyntax.Body.OpenBraceToken.SpanStart;
var end = constructorSyntax.Body.OpenBraceToken.Span.End;
return TextSpan.FromBounds(start, end);
}

// [SomeAttribute] [|public MyCtorName(params int[] values)|] { ... }
tmat marked this conversation as resolved.
Show resolved Hide resolved
return CreateSpan(constructorSyntax.Modifiers, constructorSyntax.Identifier, constructorSyntax.ParameterList.CloseParenToken);
}
}

public override BoundStatement InstrumentFieldOrPropertyInitializer(BoundStatement original, BoundStatement rewritten)
Expand All @@ -94,7 +138,8 @@ private static BoundStatement InstrumentFieldOrPropertyInitializer(BoundStatemen
if (syntax.IsKind(SyntaxKind.Parameter))
{
// This is an initialization of a generated property based on record parameter.
return AddSequencePoint(rewritten);
// Do not add sequence point - the initialization is trivial and there is no value stepping over every backing field assignment.
return rewritten;
}

Debug.Assert(syntax is { Parent: { Parent: { } } });
Expand Down Expand Up @@ -163,14 +208,18 @@ public override void InstrumentBlock(BoundBlock original, LocalRewriter rewriter
}
else if (original == rewriter.CurrentMethodBody)
{
if (previousPrologue != null)
// Add hidden sequence point at the start of
// 1) a prologue added by instrumentation.
// 2) a synthesized main entry point for top-level code.
// 3) synthesized body of primary and copy constructors of a record type that has at least one parameter.
// This hidden sequence point covers assignments of parameters/original fields to the current instance backing fields.
// These assignments do not have their own sequence points.
if (previousPrologue != null ||
rewriter.Factory.TopLevelMethod is SynthesizedSimpleProgramEntryPointSymbol ||
original.Syntax is RecordDeclarationSyntax { ParameterList.Parameters.Count: > 0 })
{
prologue = BoundSequencePoint.CreateHidden(previousPrologue);
}
else if (rewriter.Factory.TopLevelMethod is SynthesizedSimpleProgramEntryPointSymbol)
{
prologue = BoundSequencePoint.CreateHidden();
}

if (previousEpilogue != null)
{
Expand Down Expand Up @@ -364,6 +413,12 @@ public override BoundStatement InstrumentReturnStatement(BoundReturnStatement or
return new BoundSequencePointWithSpan(original.Syntax, rewritten, ((BlockSyntax)original.Syntax).CloseBraceToken.Span);
}

if (original.Syntax is ParameterSyntax parameterSyntax)
tmat marked this conversation as resolved.
Show resolved Hide resolved
{
// Record property getter sequence point
return new BoundSequencePointWithSpan(parameterSyntax, rewritten, CreateSpan(parameterSyntax));
}

return new BoundSequencePoint(original.Syntax, rewritten);
}

Expand Down
27 changes: 4 additions & 23 deletions src/Compilers/CSharp/Portable/Lowering/Instrumentation/DebugInfoInjector_SequencePoints.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,29 +45,10 @@ internal static BoundStatement AddSequencePoint(UsingStatementSyntax usingSyntax
return new BoundSequencePointWithSpan(usingSyntax, rewrittenStatement, span);
}

private static TextSpan CreateSpanForConstructorInitializer(ConstructorDeclarationSyntax constructorSyntax)
tmat marked this conversation as resolved.
Show resolved Hide resolved
{
if (constructorSyntax.Initializer != null)
{
// [SomeAttribute] public MyCtorName(params int[] values): [|base()|] { ... }
var start = constructorSyntax.Initializer.ThisOrBaseKeyword.SpanStart;
var end = constructorSyntax.Initializer.ArgumentList.CloseParenToken.Span.End;
return TextSpan.FromBounds(start, end);
}

if (constructorSyntax.Modifiers.Any(SyntaxKind.StaticKeyword))
{
Debug.Assert(constructorSyntax.Body != null);

// [SomeAttribute] static MyCtorName(...) [|{|] ... }
var start = constructorSyntax.Body.OpenBraceToken.SpanStart;
var end = constructorSyntax.Body.OpenBraceToken.Span.End;
return TextSpan.FromBounds(start, end);
}

// [SomeAttribute] [|public MyCtorName(params int[] values)|] { ... }
return CreateSpan(constructorSyntax.Modifiers, constructorSyntax.Identifier, constructorSyntax.ParameterList.CloseParenToken);
}
private static TextSpan CreateSpan(ParameterSyntax parameter)
// exclude attributes and default value:
// [A] [|in T p|] = default
=> CreateSpan(parameter.Modifiers, parameter.Type, parameter.Identifier);

private static TextSpan CreateSpan(SyntaxTokenList startOpt, SyntaxNodeOrToken startFallbackOpt, SyntaxNodeOrToken endOpt)
{
Expand Down
18 changes: 18 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordCopyCtor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
using System.Collections.Immutable;
using System.Diagnostics;
using Microsoft.CodeAnalysis.CSharp.Emit;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Text;

namespace Microsoft.CodeAnalysis.CSharp.Symbols
{
Expand Down Expand Up @@ -53,6 +55,22 @@ internal override void GenerateMethodBodyStatements(SyntheticBoundNodeFactory F,
statements.Add(F.Assignment(F.Field(F.This(), field), F.Field(param, field)));
}
}

// Add a sequence point at the end of the copy-constructor, so that a breakpoint placed on the record constructor
// can be hit whenever a new instance of the record is created (either via a primary constructor or vie a copy constructor).
tmat marked this conversation as resolved.
Show resolved Hide resolved
//
// If the record doesn't have base initializer the span overlaps the span of the sequence point generated for primary
// constructor implicit base initializer:
//
// record [|C<T>(int P, int Q)|] // implicit base constructor initializer span
// record C<T>(int P, int Q) : [|B(...)|] // explicit base constructor initializer span
// record [|C<T>|](int P, int Q) // copy-constructor span
//
// The copy-constructor span does not include the parameter list since the parameters are not in scope.
var recordDeclaration = (RecordDeclarationSyntax)F.Syntax;
statements.Add(new BoundSequencePointWithSpan(recordDeclaration, statementOpt: null,
(recordDeclaration.TypeParameterList == null) ? recordDeclaration.Identifier.Span :
TextSpan.FromBounds(recordDeclaration.Identifier.Span.Start, recordDeclaration.TypeParameterList.Span.End)));
}

internal override void AddSynthesizedAttributes(PEModuleBuilder moduleBuilder, ref ArrayBuilder<SynthesizedAttributeData> attributes)
Expand Down
36 changes: 30 additions & 6 deletions src/Compilers/CSharp/Test/Emit/PDB/PDBLambdaTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1912,7 +1912,7 @@ public static void M(int a)
}

[Fact]
public void Record_1()
public void LiftedPrimaryParameter_Record()
{
var source = WithWindowsLineBreaks(@"
using System;
Expand Down Expand Up @@ -1947,9 +1947,9 @@ record D(int X)
</customDebugInfo>
<sequencePoints>
<entry offset=""0x0"" hidden=""true"" document=""1"" />
<entry offset=""0xd"" startLine=""3"" startColumn=""10"" endLine=""3"" endColumn=""15"" document=""1"" />
<entry offset=""0xd"" hidden=""true"" document=""1"" />
<entry offset=""0x19"" startLine=""5"" startColumn=""34"" endLine=""5"" endColumn=""74"" document=""1"" />
<entry offset=""0x31"" startLine=""3"" startColumn=""1"" endLine=""6"" endColumn=""2"" document=""1"" />
<entry offset=""0x31"" startLine=""3"" startColumn=""8"" endLine=""3"" endColumn=""16"" document=""1"" />
</sequencePoints>
<scope startOffset=""0x0"" endOffset=""0x39"">
<namespace name=""System"" />
Expand All @@ -1976,6 +1976,15 @@ record D(int X)
<entry offset=""0x0"" startLine=""5"" startColumn=""25"" endLine=""5"" endColumn=""29"" document=""1"" />
</sequencePoints>
</method>
<method containingType=""D"" name="".ctor"" parameterNames=""original"">
<customDebugInfo>
<forward declaringType=""D"" methodName="".ctor"" parameterNames=""X"" />
</customDebugInfo>
<sequencePoints>
<entry offset=""0x0"" hidden=""true"" document=""1"" />
<entry offset=""0x1f"" startLine=""3"" startColumn=""8"" endLine=""3"" endColumn=""9"" document=""1"" />
</sequencePoints>
</method>
<method containingType=""D+&lt;&gt;c__DisplayClass0_0"" name=""&lt;.ctor&gt;b__0"" parameterNames=""a"">
<customDebugInfo>
<forward declaringType=""D"" methodName="".ctor"" parameterNames=""X"" />
Expand All @@ -1989,7 +1998,7 @@ record D(int X)
}

[Fact]
public void Record_2()
public void PrimaryBaseInitializer()
{
var source = WithWindowsLineBreaks(@"
using System;
Expand Down Expand Up @@ -2028,8 +2037,8 @@ static int F(int x, out int p)
</using>
</customDebugInfo>
<sequencePoints>
<entry offset=""0x0"" startLine=""3"" startColumn=""10"" endLine=""3"" endColumn=""15"" document=""1"" />
<entry offset=""0x7"" startLine=""3"" startColumn=""1"" endLine=""9"" endColumn=""2"" document=""1"" />
<entry offset=""0x0"" hidden=""true"" document=""1"" />
<entry offset=""0x7"" startLine=""3"" startColumn=""8"" endLine=""3"" endColumn=""16"" document=""1"" />
</sequencePoints>
<scope startOffset=""0x0"" endOffset=""0xf"">
<namespace name=""System"" />
Expand All @@ -2055,6 +2064,15 @@ static int F(int x, out int p)
<entry offset=""0x9"" startLine=""8"" startColumn=""5"" endLine=""8"" endColumn=""6"" document=""1"" />
</sequencePoints>
</method>
<method containingType=""C"" name="".ctor"" parameterNames=""original"">
<customDebugInfo>
<forward declaringType=""C"" methodName="".ctor"" parameterNames=""X"" />
</customDebugInfo>
<sequencePoints>
<entry offset=""0x0"" hidden=""true"" document=""1"" />
<entry offset=""0x13"" startLine=""3"" startColumn=""8"" endLine=""3"" endColumn=""9"" document=""1"" />
</sequencePoints>
</method>
<method containingType=""D"" name="".ctor"" parameterNames=""X"">
<customDebugInfo>
<forward declaringType=""C"" methodName="".ctor"" parameterNames=""X"" />
Expand Down Expand Up @@ -2089,6 +2107,12 @@ static int F(int x, out int p)
<entry offset=""0xa"" startLine=""17"" startColumn=""5"" endLine=""17"" endColumn=""6"" document=""1"" />
</sequencePoints>
</method>
<method containingType=""D"" name="".ctor"" parameterNames=""original"">
<sequencePoints>
<entry offset=""0x0"" hidden=""true"" document=""1"" />
<entry offset=""0x8"" startLine=""11"" startColumn=""8"" endLine=""11"" endColumn=""9"" document=""1"" />
</sequencePoints>
</method>
<method containingType=""D+&lt;&gt;c__DisplayClass0_0"" name=""&lt;.ctor&gt;b__0"">
<customDebugInfo>
<forward declaringType=""C"" methodName="".ctor"" parameterNames=""X"" />
Expand Down
Loading