[release/6.0] Port stackoverflow fix from Roslyn to SourceGenerator PolyFill #76946
Conversation
ghost
assigned 
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
|
What's the empty csproj being added? |
No clue. Must have been teh command line operations i was performing. |
|
@CyrusNajmabadi is this a direct port of the Roslyn change? Or did you have to make some adjustments? Mainly asking to see if we have some blindspots as this code is not very well tested in runtime. |
CyrusNajmabadi
force-pushed
the
generatorStackOverflow
branch
from
dffbec8 to
17a8cd4
Compare
Definitely had to make some adjustments. Specifically:
That said, it is otherwise identical. These changes are the same ones i needed to make when just doing the original port as well. And the core logic is the same. |
|
@CyrusNajmabadi when this is ready, please add the |
carlossanlop
changed the title
|
I see. Let's make sure that we thoroughly test this manually to make sure things are working as expected. I'll sync with you offline to do that. |
CyrusNajmabadi
added
the
Servicing-consider
Happy to. Where is this template? |
|
@joperezr just added the headers to the description to help. If you need inspiration, here are some past examples. |
|
This change will not be enough for .NET 6. We will also need to service all NuGet packages that use the polyfill approach. I'll push the relevant changes in a bit |
|
Looks like the only 2 packages affected by this are System.Text.Json and Microsoft.Extensions.Logging.Abstractions, and both of them are already inline to get serviced in the next servicing train, so no need for further changes here. |
| localAliases.Length = localAliasCount; | ||
| } | ||
| else if (syntaxHelper.IsAttributeList(node)) | ||
| processCompilationOrNamespaceMembers( |
There was a problem hiding this comment.
What about this recursion? Can't you still have stackoverflow if some generated code has a lot of namespaces nested within each other?
There was a problem hiding this comment.
the goal is not to prevent all recursion, it's to prevent recursion on cases that occur in the wild in reasonable scenarios. So, a user having a highly recursive expression (like a + b+ c+ ...) happens in teh wild and is a problem. We do not see users having insanely deeply recursive namespaces. Roslyn itself does not guard against that here or for other recursive cases.
Note: this algorithm is hard to make 'fully recursive' due to how it pushes/pops state as it enters/exits namespaces. THat's why the namespace portion stays implicitly recursive, but the rest of the code becomes explicitly recursive.
There was a problem hiding this comment.
Yeah, my suggestion wasn't really to not make this recursive or to prevent recursion, but instead to protect against a stack overflow in places were it is likely to happen. If the answer is: we don't expect ever to get a stackoverflow here, then I'm fine with not guarding against it. My main concern is that this code is used by analyzers that automatically run on all projects, so if we ever think it may be possible to hit, we should be defensive. That is just my 2 cents.
|
@carlossanlop are all of the failures here known issues? I've looked at them and none of them seem to be related to what is being changed here. |
|
Lots of network issues. Re-run might be the best course of action. |
joperezr
added
Servicing-approved
|
Approved via email. |
|
CI green, approved by Tactics, signed off. Ready to merge. |
Fixes #76953
Ports dotnet/roslyn#64322 back to the runtime polyfill.
Corresponding 7.0 fix is here: #76954
Customer Impact
Customers using 6.0.x sdk can run into stack-overflows in the Source-Generator helper code code, in both VS and compiler, when compiling code with certain problematic code-constructs. This happens for tehse customers even if they are not using Source-Generators themselves as the overflow occurs in the code that is trying to determine if the generator should run.
This commonly happens when working with, or trying to compile, generated code produced by tools like T4 or Antlr (which commonly generate deeply recursive trees).
Examples of code that causes this are:
In a case like this, the concatenation code is not a balanced tree but instead, effectively, a linear tree like so:
The existing code works by recursing the user's parse tree, which ends up blowing the stack in cases like these.
As this is a stack overflow, it is fairly catastrophic for the user. Absent them changing this code outside of VS, which may not be possible for tool-generated code, the only workaround is to disable these generators (like System.Text.Json). Disabling the generator may be suitable for customers that are not using that generator, but will completely break users who are using it and who do have these code constructs.
Testing
The affected source generators have automation tests which provide some coverage for the main scenarios. Additionally, we performed manual testing, specifically against the reported repro projects, and after the changes on this PR, we have validated that the issue no longer repros.
Risk
Low-Medium. This is a simple port of a change made in roslyn to switch from implicit recursion, to using an explicit stack. This is a refactoring that roslyn is familiar with as stack-overflows are not uncommon for us as users do have these sorts of files, and recursive solutions have often had to be refactored in a similar fashion. The reason why we add Medium to the risk, is only because we don't currently have full automation tests for running against older versions of VS, but we have validated that those work manually.