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

EnC - Support top level statements #54102

Merged
merged 30 commits into from
Jul 9, 2021
Merged

EnC - Support top level statements #54102

merged 30 commits into from
Jul 9, 2021

Conversation

davidwengier
Copy link
Contributor

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
@davidwengier davidwengier requested a review from tmat June 15, 2021 04:28
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

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
Contributor Author

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

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
@@ -801,6 +801,11 @@ internal static TextSpan GetEnvelope(SyntaxNode declaration)
return TextSpan.FromBounds(firstSpan.Start, lastSpan.End);
}

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

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
Contributor Author

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.

@davidwengier
Copy link
Contributor Author

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

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

@davidwengier
Copy link
Contributor Author

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

@RikkiGibson RikkiGibson self-assigned this Jul 7, 2021
@cston
Copy link
Member

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
Copy link
Member

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
Contributor Author

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

IsParentKind is an extension that handles nulls happily

@davidwengier
Copy link
Contributor Author

Thanks for the thorough review @cston!

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
Contributor

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
Contributor Author

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/Compilers/CSharp/Test/Semantic/FlowAnalysis/RegionAnalysisTests.cs Outdated Show resolved Hide resolved
src/EditorFeatures/CSharpTest/EditAndContinue/ActiveStatementTests.cs Outdated Show resolved Hide resolved
src/EditorFeatures/CSharpTest/EditAndContinue/TopLevelEditingTests.cs Show resolved Hide resolved
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
Contributor

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
@@ -310,6 +312,17 @@ private static Label ClassifyStatementSyntax(SyntaxKind kind, SyntaxNode? node,
// 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
Contributor

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
Contributor Author

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.

src/Features/CSharp/Portable/EditAndContinue/SyntaxUtilities.cs Show resolved Hide resolved
@RikkiGibson
Copy link
Contributor

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

@Youssef1313 Youssef1313 Youssef1313 left review comments

@AlekseyTs AlekseyTs AlekseyTs left review comments

@tmat tmat tmat approved these changes

@cston cston cston approved these changes

@RikkiGibson RikkiGibson RikkiGibson approved these changes

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