Skip to content

EnC - Support top level statements#54102

Merged
davidwengier merged 30 commits intodotnet:mainfrom
davidwengier:EnCTopLevelStatements
Jul 9, 2021
Merged

EnC - Support top level statements#54102
davidwengier merged 30 commits intodotnet:mainfrom
davidwengier:EnCTopLevelStatements

Conversation

@davidwengier
Copy link
Member

@davidwengier davidwengier commented Jun 15, 2021

Fixes #44196

I tried to add a test for each type of thing I could of that might be important, but let me know if there is something I missed. This turned out to be generally straight forward, though it would have been lovely if there was a single GlobalStatementBlock syntax node under a compilation unit that had everything it except using statements :)

@davidwengier davidwengier requested review from a team as code owners June 15, 2021 04:28
@ghost ghost added the Area-Compilers label Jun 15, 2021
@davidwengier davidwengier requested a review from tmat June 15, 2021 04:28
Youssef1313
Youssef1313 reviewed Jun 15, 2021
View reviewed changes
Youssef1313
Youssef1313 reviewed Jun 15, 2021
View reviewed changes
src/Features/CSharp/Portable/EditAndContinue/SyntaxComparer.cs
Comment on lines 54 to +56
CompilationUnit,

GlobalStatement,
Copy link
Member

@Youssef1313 Youssef1313 Jun 15, 2021

Choose a reason for hiding this comment

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

I'm confused by this and the comment on the enum that says:

All descendants of a node whose kind is listed here will be ignored regardless of their labels.

Isn't everything a descendant of CompilationUnit, and GlobalStatement is even a direct child?

Copy link
Member Author

@davidwengier davidwengier Jun 15, 2021

Choose a reason for hiding this comment

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

Yes.. as is NamespaceDeclaration, ExternalAliasDirective and UsingDirective. I'm honestly not sure what that last assumption really means - whether nodes are descended into is controlled by the isLeaf output parameter on the GetLabel method, not by the enum directly. Perhaps it doesn't apply any more?

@tmat ?

Copy link
Member

@tmat tmat Jun 23, 2021

Choose a reason for hiding this comment

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

I don't remember. Probably stale comment.

Youssef1313
Youssef1313 reviewed Jun 16, 2021
View reviewed changes
src/Features/CSharp/Portable/EditAndContinue/BreakpointSpans.cs

if (declaration is CompilationUnitSyntax unit && unit.ContainsTopLevelStatements())
{
return TextSpan.FromBounds(unit.Members[0].SpanStart, unit.Members.OfType<GlobalStatementSyntax>().Last().Span.End);
Copy link
Member

@Youssef1313 Youssef1313 Jun 16, 2021

Choose a reason for hiding this comment

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

Just noticed that local functions are parsed as "GlobalStatementSyntax" (I have no idea whether they should be included here or not).

Copy link
Member Author

@davidwengier davidwengier Jun 16, 2021

Choose a reason for hiding this comment

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

Not sure what you mean by this comment. Yes, any top level statement is contained within a GlobalStatementSyntax, including local functions. This bit of code is returning the span for the source of the top level statements, so includes local functions, just like the span for a regular method would include local functions.

tmat
tmat reviewed Jun 23, 2021
View reviewed changes
@davidwengier
Copy link
Member Author

davidwengier commented Jul 5, 2021

Done with review pass (commit 24). It looks like CI is failing.

Thanks, that was my bad. Have updated the condition.

@davidwengier
Copy link
Member Author

davidwengier commented Jul 7, 2021

Ping @dotnet/roslyn-compiler for reviews, as feedback has been addressed.

@RikkiGibson RikkiGibson self-assigned this Jul 7, 2021
cston
cston reviewed Jul 7, 2021
View reviewed changes
cston
cston reviewed Jul 7, 2021
View reviewed changes
cston
cston reviewed Jul 7, 2021
View reviewed changes
@cston
Copy link
Contributor

cston commented Jul 7, 2021 •
edited
Loading 

                                        var newContatiningSymbol = containingSymbolKey.Resolve(newCompilation, ignoreAssemblyKey: true, cancellationToken).Symbol;

Typo (although not part of this change).

In reply to: 875805160

In reply to: 875805160

Refers to: src/Features/Core/Portable/EditAndContinue/AbstractEditAndContinueAnalyzer.cs:2594 in 458e9fe. [](commit_id = 458e9fe, deletion_comment = False)

cston
cston reviewed Jul 7, 2021
View reviewed changes
@cston
Copy link
Contributor

cston commented Jul 7, 2021 

        => node.Parent.IsParentKind(SyntaxKind.PropertyDeclaration, SyntaxKind.IndexerDeclaration, SyntaxKind.EventDeclaration) ? node.Parent.Parent : null;

Do we need to allow for node.Parent is null here too?

Refers to: src/Features/CSharp/Portable/EditAndContinue/CSharpEditAndContinueAnalyzer.cs:1144 in 458e9fe. [](commit_id = 458e9fe, deletion_comment = False)

@davidwengier
Copy link
Member Author

davidwengier commented Jul 7, 2021

Do we need to allow for node.Parent is null here too?

IsParentKind is an extension that handles nulls happily

@davidwengier
Copy link
Member Author

davidwengier commented Jul 8, 2021

Thanks for the thorough review @cston!

Youssef1313
Youssef1313 reviewed Jul 8, 2021
View reviewed changes
RikkiGibson
RikkiGibson approved these changes Jul 8, 2021
View reviewed changes
src/Compilers/CSharp/Test/Emit/Emit/EditAndContinue/EditAndContinueTests.cs

var generation0 = EmitBaseline.CreateInitialBaseline(
md0,
EmptyLocalsProvider);
Copy link
Member

@RikkiGibson RikkiGibson Jul 8, 2021

Choose a reason for hiding this comment

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

I was confused about why GetEncMethodDebugInfo is needed in the new test in EditAndContinueClosureTests but not here. Is it because the method we're editing doesn't have any locals?

Copy link
Member Author

@davidwengier davidwengier Jul 9, 2021

Choose a reason for hiding this comment

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

Not to pretend I understand it and didn't just copy an existing test, but yes GetEncMethodDebugInfo reads the lambda map and locals map.

src/Features/CSharp/Portable/EditAndContinue/CSharpEditAndContinueAnalyzer.cs
if (node is CompilationUnitSyntax unit)
{
// When deleting something from a compilation unit we just report diagnostics for the last global statement
return unit.Members.OfType<GlobalStatementSyntax>().LastOrDefault()?.Span ?? default;
Copy link
Member

@RikkiGibson RikkiGibson Jul 8, 2021

Choose a reason for hiding this comment

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

Should there be some kind of assertion here that a global statement is present (since this method is called GetGlobalStatementDiagnosticSpan)?

src/Features/CSharp/Portable/EditAndContinue/SyntaxComparer.cs
// Expressions are ignored but they may contain nodes that should be matched by tree comparer.
// (e.g. lambdas, declaration expressions). Descending to these nodes is handled in EnumerateChildren.

case SyntaxKind.NamespaceDeclaration:
Copy link
Member

@RikkiGibson RikkiGibson Jul 8, 2021

Choose a reason for hiding this comment

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

This will have to be updated when I merge this change into the features/FileScopedNamespaces branch. Just curious, what is the consequence of failing to update this? Is it in perf or correctness?

Copy link
Member Author

@davidwengier davidwengier Jul 9, 2021

Choose a reason for hiding this comment

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

There is a checklist for EnC for new language features that Tomas put together, I'll forward it to you.

In general, if a node isn't classified in these switch statements it means its invisible to EnC, so for example for file scoped namespaces we wouldn't report a rude edit if one was changed or added.

@RikkiGibson
Copy link
Member

RikkiGibson commented Jul 8, 2021

LGTM. I don't consider any of my comments blocking except maybe #54102 (comment).

@davidwengier davidwengier enabled auto-merge (squash) July 9, 2021 04:31
@davidwengier davidwengier merged commit 56ef4f1 into dotnet:main Jul 9, 2021
@ghost ghost added this to the Next milestone Jul 9, 2021
@davidwengier davidwengier deleted the EnCTopLevelStatements branch July 9, 2021 05:46
@allisonchou allisonchou modified the milestones: Next, 17.0.P3 Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tmat tmat tmat approved these changes

@AlekseyTs AlekseyTs AlekseyTs left review comments

@RikkiGibson RikkiGibson RikkiGibson approved these changes

+2 more reviewers

@Youssef1313 Youssef1313 Youssef1313 left review comments

@cston cston cston approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@RikkiGibson RikkiGibson

Labels

Area-Compilers Interactive-EnC

Projects

None yet

Milestone

17.0.P3

Development

Successfully merging this pull request may close these issues.

A change in a top-level statement or an addition of a new top-level statement is not picked up by ENC

7 participants

@davidwengier @tmat @AlekseyTs @cston @RikkiGibson @Youssef1313 @allisonchou