Avoid stack overflow due to deep recursion on long chain of calls. #67913

AlekseyTs · 2023-04-21T11:49:42Z

Fixes #67912.

Fixes dotnet#67912.

AlekseyTs · 2023-04-21T11:50:30Z

@dotnet/roslyn-compiler Please review

AlekseyTs · 2023-04-21T17:54:25Z

@dotnet/roslyn-compiler Please review

AlekseyTs · 2023-04-24T15:12:43Z

@dotnet/roslyn-compiler Please review

AlekseyTs · 2023-04-25T14:17:45Z

@dotnet/roslyn-compiler Please review

jcouv · 2023-04-25T15:59:10Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+                    node = receiver2;
+                }
+
+                TakeIncrementalSnapshot(node); // Visit does this before visiting each node


nit: consider moving this snapshot just before first push (calls.Push(node); on line 5719), so that for each receiver we do snapshot&push together.
nit: then the logic on expressionIsRead could be moved here (just before visitAndCheckReceiver) since the snapshot&push doesn't care about that.
(I understand that you maintained exactly the original ordering, so those are just nits to consider)

jcouv · 2023-04-25T16:02:29Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+            }
+            else
+            {
+                TypeWithState receiverType = visitAndCheckReceiver(node);


Why don't we need to update/track the expressionIsRead value when dealing with a single invocation? #Resolved

Why don't we need to update/track the expressionIsRead value when dealing with a single invocation?

Because the calling Visit does that. Above we are setting it for nested calls.

jcouv · 2023-04-25T16:29:52Z

src/Compilers/CSharp/Portable/Lowering/DiagnosticsPass_ExpressionTrees.cs

+                }
+
+                CheckReceiverIfField(node.ReceiverOpt);
+                this.Visit(node.ReceiverOpt);


The ordering for these two calls (CheckReceiverIfField(node.ReceiverOpt) and this.Visit(node.ReceiverOpt)) seems a bit different than in the original code. I'm not sure if it matters...
Is that intentional?

In comparison, the order for single invocation case below matches the previous order:

VisitCall

CheckReceiverIfField

CheckReferenceToMethodIfLocalFunction

Visit receiver

Visit arguments #Resolved

As far as I understand there is no order dependency between Visit and Check methods.

jcouv · 2023-04-26T01:15:38Z

src/Compilers/CSharp/Portable/Binder/ExpressionVariableFinder.cs

+                invocations.Push(node);
+
+                node = nested;
+                if (receiverIsInvocation(node, out nested))


It looks like we'll push at most two nodes into the invocations queue. Should this be a while loop instead? #Closed

Should this be a while loop instead?

Yes, this is a typo.

src/Compilers/CSharp/Portable/Binder/ExpressionVariableFinder.cs

jcouv · 2023-04-26T02:57:42Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+
+                while (node.ReceiverOpt is BoundCall receiver2)
+                {
+                    TakeIncrementalSnapshot(node); // Visit does this before visiting each node


I have a general concern about the repetition that occurs in this PR: If we make a change to a method that is referenced by a "// Method does this before visiting each node", I'm concerned that we will not catch all these other places that should also be updated.
I don't have a great solution for this, but maybe it would be useful to add cref comments from those methods back to all the places that duplicate some of the logic.

I guess there's also the broader question of whether the gain is worth all this additional complexity in the codebase. I'm curious what the team thinks (@dotnet/roslyn-compiler for thoughts).

I have a general concern about the repetition that occurs in this PR: If we make a change to a method that is referenced by a "// Method does this before visiting each node", I'm concerned that we will not catch all these other places that should also be updated.

We already have to face the same for visiting binary expressions

jcouv · 2023-04-26T03:01:20Z

src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs

+                    Debug.Assert(node.Expression.Kind() is SyntaxKind.SimpleMemberAccessExpression);
+                    var memberAccess = (MemberAccessExpressionSyntax)node.Expression;
+                    analyzedArguments.Clear();
+                    VerifyUnchecked(nested, diagnostics, result);


I didn't follow where this came from. May be worth a comment #Resolved

I didn't follow where this came from. May be worth a comment

Added a comment

jcouv

LGTM Thanks (iteration 4) but would like to get team's opinion

src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs

AlekseyTs · 2023-04-26T04:18:10Z

@dotnet/roslyn-compiler For the second review

AlekseyTs · 2023-04-26T19:34:29Z

@dotnet/roslyn-compiler For the second review

AlekseyTs · 2023-04-28T12:58:05Z

@dotnet/roslyn-compiler For the second review

jaredpar · 2023-04-21T16:03:00Z

src/Compilers/CSharp/Portable/Binder/ExpressionVariableFinder.cs

+                invocations.Push(node);
+
+                node = nested;
+                if (receiverIsInvocation(node, out nested))


Shouldn't this be while not if? #Resolved

Shouldn't this be while not if?

Yes, the typo has been fixed already

jaredpar · 2023-04-21T16:04:57Z

src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs

@@ -240,9 +241,49 @@ public override void VisitInvocationExpression(InvocationExpressionSyntax node)
                return;
            }

-            base.VisitInvocationExpression(node);
+            if (receiverIsInvocation(node, out InvocationExpressionSyntax? nested))


Seems like this is roughly the same code on several derivations of CSharpSyntaxWalker? Did you consider putting a helper directly there that we could access? Probably cost us a Delegate allocation but maybe that's not too much. #Resolved

? Did you consider putting a helper directly there that we could access?

There are only two of them and they are slightly different. I do not think it is worth adding an abstraction across them

jaredpar · 2023-04-21T16:06:18Z

src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs

+                    node = receiver2;
+                }
+
+                VisitReceiver(node);


Why isn't this inside the do loop below? Seems like we miss intermediate receiver calls as written. #Resolved

Why isn't this inside the do loop below? Seems like we miss intermediate receiver calls as written.

The intermediate receiver is the previous call and we already visited all interesting parts of it. It is our intention to not call VisitReceiver for them. I'll add a comment.

src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs

* upstream/main: (2060 commits) implement code folding for anonymous objects Cleanup/perf in creating member maps. (dotnet#67997) Address feedback, clean the use of ILegacyGlobalOptionsWorkspaceService Update Publish.json Semantic snippets: Add inline statement snippets (dotnet#67819) Update VSSDK Build tools version Update doc comment Address feedback Move declaration near reference Address feedback Avoid stack overflow due to deep recursion on long chain of calls. (dotnet#67913) Check ILegacyGlobalOptionsWorkspaceService is null test scout queue add test update editor "Where" clause typo fix (dotnet#68002) Use `Keyword` helper method instead of hardcoding keyword help terms Move the SyntaxTree and SemanticModel action based analyzers to respect context.FilterSpan EnC refactoring: Align wrappers with contracts better (dotnet#67967) Include EA.RazorCompiler in source build (dotnet#67996) ...

The EndToEnd OverflowOnFluentCall test is fixed in #67913, but would be a significant change to backport.

Avoid stack overflow due to deep recursion on long chain of calls.

1f97f7f

Fixes dotnet#67912.

AlekseyTs added the Area-Compilers label Apr 21, 2023

AlekseyTs requested a review from a team as a code owner April 21, 2023 11:49

dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Apr 21, 2023

AlekseyTs added 2 commits April 21, 2023 05:59

Fixup

09a18da

Fixup

3829157

jcouv self-assigned this Apr 25, 2023

jcouv added this to the 17.7 milestone Apr 25, 2023

jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label Apr 25, 2023

jcouv reviewed Apr 25, 2023

View reviewed changes

jcouv reviewed Apr 26, 2023

View reviewed changes

AlekseyTs commented Apr 26, 2023

View reviewed changes

src/Compilers/CSharp/Portable/Binder/ExpressionVariableFinder.cs Outdated Show resolved Hide resolved

Update src/Compilers/CSharp/Portable/Binder/ExpressionVariableFinder.cs

b131a4f

jcouv reviewed Apr 26, 2023

View reviewed changes

jcouv approved these changes Apr 26, 2023

View reviewed changes

AlekseyTs commented Apr 26, 2023

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs Outdated Show resolved Hide resolved

Update src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs

8fecdcf

runfoapp bot mentioned this pull request Apr 26, 2023

BasicArgumentProvider.ImplicitArgumentSwitching is flaky #66533

Closed

jaredpar approved these changes Apr 28, 2023

View reviewed changes

AlekseyTs commented Apr 28, 2023

View reviewed changes

src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs Show resolved Hide resolved

Update src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs

aeb9d49

AlekseyTs enabled auto-merge (squash) April 28, 2023 18:17

AlekseyTs merged commit 53ac77e into dotnet:main Apr 29, 2023

ghost modified the milestones: 17.7, Next Apr 29, 2023

allisonchou mentioned this pull request May 2, 2023

[Automated] PRs inserted in VS build main-33702.166 #68059

Closed

Cosifne modified the milestones: Next, 17.7 P2 May 31, 2023

jjonescz mentioned this pull request Sep 6, 2023

Handle long extension invocation chains #69831

Merged

JoeRobich added a commit that referenced this pull request Jun 11, 2024

Skip failing test.

7cdcef3

The EndToEnd OverflowOnFluentCall test is fixed in #67913, but would be a significant change to backport.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid stack overflow due to deep recursion on long chain of calls. #67913

Avoid stack overflow due to deep recursion on long chain of calls. #67913

AlekseyTs commented Apr 21, 2023

AlekseyTs commented Apr 21, 2023

AlekseyTs commented Apr 21, 2023

AlekseyTs commented Apr 24, 2023

AlekseyTs commented Apr 25, 2023

jcouv Apr 25, 2023

jcouv Apr 25, 2023 •

edited by AlekseyTs

Loading

AlekseyTs Apr 25, 2023

jcouv Apr 25, 2023 •

edited by AlekseyTs

Loading

AlekseyTs Apr 25, 2023

jcouv Apr 26, 2023 •

edited

Loading

AlekseyTs Apr 26, 2023

jcouv Apr 26, 2023

jcouv Apr 26, 2023

AlekseyTs Apr 26, 2023

jcouv Apr 26, 2023 •

edited by AlekseyTs

Loading

AlekseyTs Apr 26, 2023

jcouv left a comment

AlekseyTs commented Apr 26, 2023

AlekseyTs commented Apr 26, 2023

AlekseyTs commented Apr 28, 2023

jaredpar Apr 21, 2023 •

edited by AlekseyTs

Loading

AlekseyTs Apr 28, 2023

jaredpar Apr 21, 2023 •

edited by AlekseyTs

Loading

AlekseyTs Apr 28, 2023

jaredpar Apr 21, 2023 •

edited by AlekseyTs

Loading

AlekseyTs Apr 28, 2023

Avoid stack overflow due to deep recursion on long chain of calls. #67913

Avoid stack overflow due to deep recursion on long chain of calls. #67913

Conversation

AlekseyTs commented Apr 21, 2023

AlekseyTs commented Apr 21, 2023

AlekseyTs commented Apr 21, 2023

AlekseyTs commented Apr 24, 2023

AlekseyTs commented Apr 25, 2023

Choose a reason for hiding this comment

jcouv Apr 25, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Apr 25, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Apr 26, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs commented Apr 26, 2023

AlekseyTs commented Apr 26, 2023

AlekseyTs commented Apr 28, 2023

jaredpar Apr 21, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar Apr 21, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar Apr 21, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Apr 25, 2023 •

edited by AlekseyTs

Loading

jcouv Apr 25, 2023 •

edited by AlekseyTs

Loading

jcouv Apr 26, 2023 •

edited

Loading

jcouv Apr 26, 2023 •

edited by AlekseyTs

Loading

jaredpar Apr 21, 2023 •

edited by AlekseyTs

Loading

jaredpar Apr 21, 2023 •

edited by AlekseyTs

Loading

jaredpar Apr 21, 2023 •

edited by AlekseyTs

Loading