Maintain the same parse option when add symbol to destination#53001
Maintain the same parse option when add symbol to destination#53001Cosifne merged 7 commits intodotnet:release/dev16.11from
Conversation
c6164c4 to
a16677f
Compare
This reverts commit a16677f.
src/Workspaces/CSharp/Portable/CodeGeneration/ConstructorGenerator.cs
Outdated
Show resolved
Hide resolved
|
Can you calrify how this fixes things? the destination syntax tree should have valid options for us to use. If it doesn't, i think we need to track back waht caused that. |
@CyrusNajmabadi I am testing this simple fix to see if this would cause any error. : ) And it does. So I am trying to another way to fix this |
This seems like teh bug :) |
| { | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| currentDestination = UpdateDestination(availableIndices, options, currentDestination, member, cancellationToken); | ||
| currentDestination = UpdateDestination(availableIndices, options, currentDestination, member, destination.SyntaxTree.Options, cancellationToken); |
There was a problem hiding this comment.
When the symbol is passed to Property/Field/Method Generator, the generated syntax node doesn't have parent.
So its parse definition would be https://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/CSharpParseOptions.cs,21
And later the default option is translated to the latest C# version https://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/LanguageVersion.cs,426
Therefore when this loop is being called, its parsing option may change after the first iteration. (As this is the reason for this bug, C# 5 becomes C# 9 at this point)
The best way I feel here is to first generate all the syntax nodes for members using the same parse option, and then doing the insertion.
This would need to refactor a lot of code in the CodeGenerationService and the method/field/.. generator.
before I do this
tag @CyrusNajmabadi to see if this is the right way to fix this
There was a problem hiding this comment.
The best way I feel here is to first generate all the syntax nodes for members using the same parse option, and then doing the insertion.
I'm not opposed to that. However, I feel like something is still wrong here:
And later the default option is translated to the latest C# version
There should be some way to avoid this. Maybe @jasonmalinowski knows? It seems patently weird that we fork nodes and trees, but end up changing the options here. Would prefer we avoid that, or force the options to be what we started with.
There was a problem hiding this comment.
I'm fine with us exploring both options in parallel btw.
There was a problem hiding this comment.
So the problem is a node created in isolation of a tree doesn't have any options. We create a tree on the fly when asked, but since there's no options it has no choice but to just use the default ones.
The alternative here would be in the path where we have a node and want the option to be remembered would be to explicitly call CSharpSyntaxTree.Create passing in the random node that we created with the option, and then grab the new red node root back from that. That means later calls to node.Tree.Options will have what you want.
This reverts commit 0d48d62.
fbccba7 to
1f0ac0f
Compare
Issue:
#51779
Reason:
When adding a symbol to a syntax node, the newly generated syntax node won't have a parent node, and therefore, it will use the default parse option. And later, we always think the default option as C# 9.
Fix:
After the destination node is generated, regenerate the syntax tree use the correct parse option. (Thanks Jason's help to point the right way to fix this)