From 312a56886b2b3aba66f7dad92475afa7d7ff6764 Mon Sep 17 00:00:00 2001 From: tmat Date: Sat, 24 Jun 2023 18:53:44 -0700 Subject: [PATCH 1/7] Improve sequence point placement in primary constructors and records --- .../Compiler/MethodBodySynthesizer.cs | 1 + .../Instrumentation/DebugInfoInjector.cs | 87 ++++-- .../DebugInfoInjector_SequencePoints.cs | 27 +- .../Records/SynthesizedRecordCopyCtor.cs | 18 ++ .../CSharp/Test/Emit/PDB/PDBLambdaTests.cs | 8 +- .../CSharp/Test/Emit/PDB/PDBTests.cs | 252 ++++++++++++++++++ .../Core/Portable/CodeGen/ILBuilder.cs | 4 +- .../EditAndContinue/BreakpointSpansTests.cs | 154 ++++++++++- .../EditAndContinue/BreakpointSpans.cs | 82 +++++- 9 files changed, 586 insertions(+), 47 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Compiler/MethodBodySynthesizer.cs b/src/Compilers/CSharp/Portable/Compiler/MethodBodySynthesizer.cs index ab95aec4a2fc1..36c2bf96ef216 100644 --- a/src/Compilers/CSharp/Portable/Compiler/MethodBodySynthesizer.cs +++ b/src/Compilers/CSharp/Portable/Compiler/MethodBodySynthesizer.cs @@ -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, diff --git a/src/Compilers/CSharp/Portable/Lowering/Instrumentation/DebugInfoInjector.cs b/src/Compilers/CSharp/Portable/Lowering/Instrumentation/DebugInfoInjector.cs index 29dbfd4a3326d..eca22779e4dbf 100644 --- a/src/Compilers/CSharp/Portable/Lowering/Instrumentation/DebugInfoInjector.cs +++ b/src/Compilers/CSharp/Portable/Lowering/Instrumentation/DebugInfoInjector.cs @@ -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)); + + case ConstructorInitializerSyntax ctorInit: + // Explicit constructor initializer: + Debug.Assert(ctorInit.Parent is ConstructorDeclarationSyntax); + return new BoundSequencePointWithSpan(ctorInit, rewritten, createSpanForConstructorInitializer((ConstructorDeclarationSyntax)ctorInit.Parent)); + + case TypeDeclarationSyntax typeDecl: + // Primary constructor with implicit base initializer: + // class [|C(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(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)|] { ... } + return CreateSpan(constructorSyntax.Modifiers, constructorSyntax.Identifier, constructorSyntax.ParameterList.CloseParenToken); + } } public override BoundStatement InstrumentFieldOrPropertyInitializer(BoundStatement original, BoundStatement rewritten) @@ -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: { } } }); @@ -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) { @@ -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) + { + // Record property getter sequence point + return new BoundSequencePointWithSpan(parameterSyntax, rewritten, CreateSpan(parameterSyntax)); + } + return new BoundSequencePoint(original.Syntax, rewritten); } diff --git a/src/Compilers/CSharp/Portable/Lowering/Instrumentation/DebugInfoInjector_SequencePoints.cs b/src/Compilers/CSharp/Portable/Lowering/Instrumentation/DebugInfoInjector_SequencePoints.cs index 575615d34a2b5..bed18cb6964c4 100644 --- a/src/Compilers/CSharp/Portable/Lowering/Instrumentation/DebugInfoInjector_SequencePoints.cs +++ b/src/Compilers/CSharp/Portable/Lowering/Instrumentation/DebugInfoInjector_SequencePoints.cs @@ -45,29 +45,10 @@ internal static BoundStatement AddSequencePoint(UsingStatementSyntax usingSyntax return new BoundSequencePointWithSpan(usingSyntax, rewrittenStatement, span); } - private 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)|] { ... } - 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) { diff --git a/src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordCopyCtor.cs b/src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordCopyCtor.cs index 47baaace29767..b48ce17b0db22 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordCopyCtor.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordCopyCtor.cs @@ -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 { @@ -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). + // + // 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(int P, int Q)|] // implicit base constructor initializer span + // record C(int P, int Q) : [|B(...)|] // explicit base constructor initializer span + // record [|C|](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 attributes) diff --git a/src/Compilers/CSharp/Test/Emit/PDB/PDBLambdaTests.cs b/src/Compilers/CSharp/Test/Emit/PDB/PDBLambdaTests.cs index 0c623da3363ab..71b4b6d1c16d1 100644 --- a/src/Compilers/CSharp/Test/Emit/PDB/PDBLambdaTests.cs +++ b/src/Compilers/CSharp/Test/Emit/PDB/PDBLambdaTests.cs @@ -1912,7 +1912,7 @@ public static void M(int a) } [Fact] - public void Record_1() + public void LiftedPrimaryParameter_Record() { var source = WithWindowsLineBreaks(@" using System; @@ -1949,7 +1949,7 @@ record D(int X)