Adjust records behavior around IOperations and analyzer actions. by AlekseyTs · Pull Request #46308 · dotnet/roslyn

AlekseyTs · 2020-07-24T17:40:23Z

No description provided.

AlekseyTs · 2020-07-24T20:32:29Z

@mavasani, @cston, @jcouv, @RikkiGibson, @333fred Please review.

AlekseyTs · 2020-07-27T15:14:03Z

@mavasani, @cston, @jcouv, @RikkiGibson, @333fred Please review.

AlekseyTs · 2020-07-27T18:54:10Z

@mavasani, @cston, @jcouv, @RikkiGibson, @333fred Please review.

mavasani · 2020-07-27T21:54:24Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs

+            {
+                context.RegisterSymbolStartAction(Handle1, SymbolKind.Method);
+                context.RegisterSymbolStartAction(Handle1, SymbolKind.Property);
+                context.RegisterSymbolStartAction(Handle1, SymbolKind.Parameter);


Can we add a context.RegisterSymbolStartAction(..., SymbolKind.NamedType) with a nested RegisterSymbolEndAction and then verify that all the FireCountX are 0 at the start of the former callback and 1 at the start of latter callback? This should verify correct ordering of callbacks. #Resolved

mavasani · 2020-07-27T21:55:45Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs

+
+            public override void Initialize(AnalysisContext context)
+            {
+                context.RegisterSymbolAction(Handle, SymbolKind.Method);


Should we also add context.RegisterSymbolAction(Handle, SymbolKind.NamedType); to verify callbacks for the record type itself? #Resolved

Should we also add context.RegisterSymbolAction(Handle, SymbolKind.NamedType); to verify callbacks for the record type itself?

I can certainly add one, I don't think there is anything interesting about the type itself, the PR is was mostly focused on record members.

In reply to: 461193457 [](ancestors = 461193457)

mavasani · 2020-07-27T22:09:41Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs

                                Debug.Assert(operationBlock.Parent.IsImplicit);
                                Debug.Assert(operationBlock.Parent.Parent is IConstructorBodyOperation ctorBody &&
                                    ctorBody.Initializer == operationBlock.Parent);
+


Should we add the below assert?

Suggested change

Debug.Assert(!operationBlock.Parent.Parent.IsImplicit);

``` #Resolved

mavasani · 2020-07-27T22:16:06Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs

    }
 }";
            comp = CreateCompilation(sourceB, references: new[] { refA }, parseOptions: TestOptions.RegularPreview, options: TestOptions.ReleaseExe);
-            comp.VerifyDiagnostics();


Was there a reason to remove this call? #Resolved

Was there a reason to remove this call?

It was determined redundant in one of the previous PRs due to .VerifyDiagnostics() below.

In reply to: 461201750 [](ancestors = 461201750)

mavasani · 2020-07-27T22:25:39Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs

+                        declaredNode, declaredSymbol, executableCodeBlocks, (codeBlocks) => codeBlocks.SelectMany(
+                            cb =>
+                            {
+                                var filter = semanticModel.GetSyntaxNodesToAnalyzeFilter(cb, declaredSymbol);


Is the below existing invocation to this method redundant now?

roslyn/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs

Lines 2695 to 2697 in 507c81a

Func<SyntaxNode, bool> additionalFilterOpt = semanticModel.GetSyntaxNodesToAnalyzeFilter(declaredNode, declaredSymbol);

bool shouldAddNode(SyntaxNode node) => (descendantDeclsToSkipOpt == null || !descendantDeclsToSkipOpt.Contains(node)) && (additionalFilterOpt is null || additionalFilterOpt(node));

#Resolved

Is the below existing invocation to this method redundant now?

roslyn/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs

Lines 2695 to 2697 in 507c81a

Func<SyntaxNode, bool> additionalFilterOpt = semanticModel.GetSyntaxNodesToAnalyzeFilter(declaredNode, declaredSymbol);

bool shouldAddNode(SyntaxNode node) => (descendantDeclsToSkipOpt == null || !descendantDeclsToSkipOpt.Contains(node)) && (additionalFilterOpt is null || additionalFilterOpt(node));

No, that place iterates through nodes itself

In reply to: 461205407 [](ancestors = 461205407)

mavasani

Overall LGTM. Only relevant open question is if we can remove the existing invocation to GetSyntaxNodesToAnalyzeFilter in the analyzer driver as we are now invoking it directly within the core analyzer executor.

AlekseyTs · 2020-07-27T22:34:16Z

Only relevant open question is if we can remove the existing invocation to GetSyntaxNodesToAnalyzeFilter in the analyzer driver as we are now invoking it directly within the core analyzer executor.

Responded on that thread.

In reply to: 456180326 [](ancestors = 456180326)

jcouv

LGTM Thanks (iteration 2)

AlekseyTs · 2020-07-27T23:45:07Z

@cston, @RikkiGibson, @333fred Please review, need a second sign-off.

AlekseyTs · 2020-07-28T15:33:35Z

@cston, @RikkiGibson, @333fred Please review, need a second sign-off.

cston · 2020-07-28T18:42:44Z

src/Compilers/CSharp/Portable/Compilation/SyntaxTreeSemanticModel.cs

                    model = (accessor.Body != null || accessor.ExpressionBody != null) ? GetOrAddModel(node) : null;
                    break;
-
+                case RecordDeclarationSyntax { ParameterList: { }, PrimaryConstructorBaseType: { } } recordDeclaration when TryGetSynthesizedRecordConstructor(recordDeclaration) is SynthesizedRecordConstructor ctor:


ctor [](start = 210, length = 4)

Consider omitting the variable name so it's clear we're not expecting to use the value. #Pending

Consider omitting the variable name so it's clear we're not expecting to use the value.

Sometimes it is useful during debugging

In reply to: 461794242 [](ancestors = 461794242)

cston · 2020-07-28T19:28:24Z

src/Compilers/CSharp/Portable/Compilation/SyntaxTreeSemanticModel.cs

+                                           return node == baseType.ArgumentList;
+                                       }
+
+                                       return true;


return true [](start = 39, length = 11)

Is this correct given the comment above ("Accept only nodes ...")? #Resolved

Is this correct given the comment above ("Accept only nodes ...")?

I think yes, this filter is called in the process of descending down the tree. If it returns false for parent node, it is never called for a child node.

In reply to: 461820251 [](ancestors = 461820251)

cston · 2020-07-28T21:01:47Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs

+            var recordDeclarations = tree.GetRoot().DescendantNodes().OfType<RecordDeclarationSyntax>().Skip(1).ToArray();
+
+            Assert.Equal("C", recordDeclarations[0].Identifier.ValueText);
+            Assert.Null(model.GetOperation(recordDeclarations[0]));


model.GetOperation(recordDeclarations[0]) [](start = 24, length = 41)

Why does GetOperation() return null? #Resolved

Why does GetOperation() return null?

There is no user code for this primary constructor, no code blocks to fire analyzers on, we never create a member semantic model for a primary constructor like that, etc. Technically, we could produce an "empty" IConstructorBodyOperation node, but then we would have to do something with analyzers to make sure an action for this node would fire. Unfortunately, analyzer driver is able to do that only when there is a "real" node for an initializer or a body (see GetOperationsToAnalyze in AnalyzerDriver.cs), we have neither of them here. I decided to not deal with this complexity right now, when we don't really have a real scenario to enable.

In reply to: 461876729 [](ancestors = 461876729)

333fred

Sorry for the late review. This LGTM

Adjust records behavior around IOperations and analyzer actions.

10ab522

AlekseyTs added Area-Analyzers Area-Compilers Feature - Records Records labels Jul 24, 2020

AlekseyTs requested review from 333fred, RikkiGibson, cston, jcouv and mavasani July 24, 2020 17:40

AlekseyTs requested a review from a team as a code owner July 24, 2020 17:40

mavasani reviewed Jul 27, 2020

View reviewed changes

mavasani approved these changes Jul 27, 2020

View reviewed changes

PR feedback

a970598

jcouv approved these changes Jul 27, 2020

View reviewed changes

jcouv self-assigned this Jul 27, 2020

Adjust test expectations

86b31ef

cston reviewed Jul 28, 2020

View reviewed changes

cston approved these changes Jul 28, 2020

View reviewed changes

AlekseyTs merged commit 5d1ba27 into dotnet:master Jul 28, 2020

ghost added this to the Next milestone Jul 28, 2020

333fred reviewed Jul 30, 2020

View reviewed changes

RikkiGibson modified the milestones: Next, 16.8.P2 Aug 11, 2020

Youssef1313 mentioned this pull request May 4, 2021

SyntaxNodeAction is invoked twice for the same syntax node in case of positional records #53136

Closed

AlekseyTs mentioned this pull request Jan 6, 2022

Avoid duplicate syntax node callbacks for record declarations #58482

Merged


	Debug.Assert(!operationBlock.Parent.Parent.IsImplicit);
	``` #Resolved

	Func<SyntaxNode, bool> additionalFilterOpt = semanticModel.GetSyntaxNodesToAnalyzeFilter(declaredNode, declaredSymbol);

	bool shouldAddNode(SyntaxNode node) => (descendantDeclsToSkipOpt == null \|\| !descendantDeclsToSkipOpt.Contains(node)) && (additionalFilterOpt is null \|\| additionalFilterOpt(node));

Conversation

AlekseyTs commented Jul 24, 2020

Uh oh!

AlekseyTs commented Jul 24, 2020

Uh oh!

AlekseyTs commented Jul 27, 2020

Uh oh!

AlekseyTs commented Jul 27, 2020

Uh oh!

mavasani Jul 27, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mavasani Jul 27, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

mavasani Jul 27, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mavasani Jul 27, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

mavasani Jul 27, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

mavasani left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jul 27, 2020

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jul 27, 2020

Uh oh!

AlekseyTs commented Jul 28, 2020

Uh oh!

cston Jul 28, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 28, 2020

Choose a reason for hiding this comment

Uh oh!

cston Jul 28, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 28, 2020

Choose a reason for hiding this comment

Uh oh!

cston Jul 28, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 28, 2020

Choose a reason for hiding this comment

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

mavasani Jul 27, 2020 •

edited by AlekseyTs

Loading

mavasani Jul 27, 2020 •

edited by AlekseyTs

Loading

mavasani Jul 27, 2020 •

edited by AlekseyTs

Loading

mavasani Jul 27, 2020 •

edited by AlekseyTs

Loading

mavasani Jul 27, 2020 •

edited by AlekseyTs

Loading

cston Jul 28, 2020 •

edited by AlekseyTs

Loading

cston Jul 28, 2020 •

edited by AlekseyTs

Loading

cston Jul 28, 2020 •

edited by AlekseyTs

Loading