SyntaxEditor.SetModifiers may not properly format the result when multiple modifiers are added at once

[BhaaLseN]

(I apologize in advance for not using the issue template; but selecting the code line and using "Open Issue" didn't suggest it. Also, I'm not sure if it is actually tied to any particular Visual Studio version since it happens in my own Analyzer using either Microsoft.CodeAnalysis.CSharp.Workspaces 2.4.0 or 2.8.2. In case it does matter, this is on Visual Studio Enterprise 2017 15.7.0)

I tried writing a CodeFix to make certain fields static and readonly; so I borrowed the following snippet:

roslyn/src/Features/Core/Portable/MakeFieldReadonly/AbstractMakeFieldReadonlyCodeFixProvider.cs

Line 64 in ef41d39

editor.SetModifiers(fieldDeclarators.Key, editor.Generator.GetModifiers(fieldDeclarators.Key) | DeclarationModifiers.ReadOnly);

Whenever this code snippet is run (at least in my CodeFix), the resulting code is incorrectly formatted. I don't know if and when this happens during the built-in "Make read only" CodeFix, but when I copied that snippet into one of my own analyzers, the space being between readonly and the type name is missing (only in Visual Studio though when trying it on my target project; it works correctly in the Unit Test which is even more interresting).

Hence I'm assuming that the built-in CodeFix would fail in that case, just like my own does.

My CodeFix doesn't have those fancy base classes that do the work for me, so the method looks a little more involved (but should basically do just the same):

private Task<Document> MakeStaticReadonly(Document document, SyntaxNode root, SyntaxNode fieldDecl, CancellationToken cancellationToken)
{
    var syntaxEditor = new SyntaxEditor(root, document.Project.Solution.Workspace);
    syntaxEditor.SetModifiers(fieldDecl, syntaxEditor.Generator.GetModifiers(fieldDecl).WithIsReadOnly(true).WithIsStatic(true));
    var newRoot = syntaxEditor.GetChangedRoot();
    return Task.FromResult(document.WithSyntaxRoot(newRoot));
}    

When invoked on a static field

class Test
{
    private static object _field = default;
}

instead of the expected

class Test
{
    private static readonly object _field = default;
}

the result is the less compily (is that a word?) with a missing space

class Test
{
    private static readonlyobject _field = default;
}

If I use the else block (modified for single result) in my own CodeFix, the space is applied correctly:

private async Task<Document> MakeStaticReadonlyAsync(Document document, SyntaxNode root, VariableDeclaratorSyntax declarator, CancellationToken cancellationToken)
{
    var editor = new SyntaxEditor(root, document.Project.Solution.Workspace);
    var model = await document.GetSemanticModelAsync().ConfigureAwait(false);
    var generator = editor.Generator;

    var symbol = (IFieldSymbol)model.GetDeclaredSymbol(declarator);
    var fieldDeclaration = declarator.FirstAncestorOrSelf<FieldDeclarationSyntax>();
    var modifiers = generator.GetModifiers(fieldDeclaration).WithIsReadOnly(true).WithIsStatic(true);

    var newDeclaration = generator.FieldDeclaration(
            symbol.Name,
            generator.TypeExpression(symbol.Type),
            Accessibility.Private,
            modifiers,
            declarator.Initializer?.Value)
        .WithAdditionalAnnotations(Formatter.Annotation);

    editor.InsertAfter(fieldDeclaration, newDeclaration);
    editor.RemoveNode(fieldDeclaration);

    var newRoot = editor.GetChangedRoot();
    return document.WithSyntaxRoot(newRoot);
}

The result is always correct if I run the Unit Test that is created with the "Analyzer with CodeFix" template, but it isn't when run inside Visual Studio on "real" code.

I'm not sure what the proper fix for this would be (or if this is actually a real bug in roslyn code vs. me missing something while copying the code over to my CodeFix); the main apparent difference is that the loop applies .WithAdditionalAnnotations(Formatter.Annotation) to the newly generated nodes (other than the fact the whole expression is rebuilt from scratch instead of updated).

My CodeFix works just fine when the target field has no modifiers at all and both are added at the same time. Adding static also works (probably because my Code Style settings/.editorconfig specify that static should come before readonly), but adding just readonly does not.

Please let me know if theres any additional infomation I (sh|c)ould provide (other than a sample solution; which will probably have to wait until the weekend or so).

[jmarolf]

@BhaaLseN a quick glance looks like editor.SetModifiers is not correctly setting the formatter annotation when you add multiple modifiers at once.

[BhaaLseN]

Thats interresting. I'll see if I can find that piece of code myself and create a pull request later. Gave up after an hour, ended up in generated code all the time so no idea where to tackle this one.
It is still interresting that this behavior is not caught by a Unit Test; they simply run just fine.

[CyrusNajmabadi]

Fixed with: #76054