Fix directive placement when removing nodes using syntax API. #76275
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
CyrusNajmabadi
changed the title
|
@dotnet/roslyn-compiler ptal. |
| var expectedText = """ | ||
| class C | ||
| { | ||
|
|
There was a problem hiding this comment.
unfortunate blank line. i'm considering chnaging that in this PR to not happen either. LMK if you'd prefer that.
There was a problem hiding this comment.
i looked into changing this and it was a wash. Undesirable lines like this were kept, while desirable lines (like those between methods) were removed. So i'm keeping this for now.
There was a problem hiding this comment.
probably a bunch more heuristics would be needed in order to make it a net improvement.
| var expectedText = """ | ||
| class C | ||
| { | ||
|
|
There was a problem hiding this comment.
probably a bunch more heuristics would be needed in order to make it a net improvement.
src/EditorFeatures/CSharpTest/CodeActions/MoveType/MoveTypeTests.MoveToNewFile.cs
Outdated
Show resolved
Hide resolved
|
@dotnet/roslyn-compiler for second pair of eyes. thanks! |
|
@333fred naming handled. Ok signing off? |
|
@ToddGrun ptal |
| @@ -290,16 +340,6 @@ private void AddPartialModifiersToTypeChain( | |||
| documentEditor.RemovePrimaryConstructor(node); | |||
| } | |||
| } | |||
|
|
|||
| documentEditor.ReplaceNode(State.TypeNode, | |||
There was a problem hiding this comment.
this was moved to a helper method. it is still called in both the callers that called this method. just at different places now.
| documentEditor.RemoveNode(directive); | ||
| } | ||
|
|
||
| RemoveLeadingBlankLinesFromMovedType(documentEditor); |
There was a problem hiding this comment.
We want ot call this after we remove all the appropriate directives from the node so that the spacing is correct after that happens.
| @@ -18,6 +18,7 @@ | |||
| using Microsoft.CodeAnalysis.Text; | |||
| using Roslyn.Utilities; | |||
| using Microsoft.CodeAnalysis.CSharp.Extensions.ContextQuery; | |||
| using System.Collections.Immutable; | |||
There was a problem hiding this comment.
doen in followup pr.
| """ | ||
| public partial class Goo | ||
| { | ||
| #region Region |
There was a problem hiding this comment.
Just to make sure I'm reading this correctly, the directive is expected to be in both places?
There was a problem hiding this comment.
it's a stretch to say 'expected'. But at the very least we want it to be correct.
Fixes #19613
When removing a node, a standard flag is passed in saying "keep directives around if removing them would lead to an unbalanced directive state (like removing a
#regionwhen the corresponding#endregionstays around). This is an intuitive default as it means users generally don't have to think about pp trivia when removing nodes (since often the PP trivia is actually spread over many physical nodes, even though it looks to surround one 'virtual' node).However, the logic for actually doing this was not great. When the api detected a PP directive to keep, it would grab only it and keep only that in the resultant tree. This means that a PP-directive that was indented, would become NOT indented, leading to ill formatted code.
For directives like
#ifthis was rarely an issue as VS-formatting and the .Net ecosystem almost always has those left-aligned on column 1. However, for#regiondirectives, this was suboptimal as it meant the directive would go from being indented, to left-aligned.The fix is to have the removal logic try to keep leading whitespace before the PP directive, to ensure that it stays at the same location, even when the rest of the trivia around it is removed