Breaking change and possible Bug in SyntaxFactory.ParseCompilationUnit with the new GlobalStatementSyntax

[Herrmel]

Version Used: Microsoft.CodeAnalysis.CSharp.Workspaces 3.8.0-3.final and 3.7.0

Steps to Reproduce:

1. Create .Net Core 3.1 or .Net 5 RC1 Console-Project
2. Use following example Code. This example contains a simple AdHocWorkspace with a single document where i try to parse and insert a new MethodDeclaration after an existing PropertyDeclaration. I check and output the document

Code

using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Text;
using System;
using System.Linq;

namespace CompilerTest
{
    class Program
    {
        private static readonly MetadataReference CorlibReference = MetadataReference.CreateFromFile(typeof(object).Assembly.Location);
        private static readonly MetadataReference SystemCoreReference = MetadataReference.CreateFromFile(typeof(Enumerable).Assembly.Location);
        private static readonly MetadataReference CSharpSymbolsReference = MetadataReference.CreateFromFile(typeof(CSharpCompilation).Assembly.Location);
        private static readonly MetadataReference CodeAnalysisReference = MetadataReference.CreateFromFile(typeof(Compilation).Assembly.Location);

        private const string methodText = @"#region SetValue

        private void SetValue(double value)
        {
//whatever
        }

#endregion SetValue
";

        private const string source = @"namespace Fis.Test{

    public class TestItem {

        public double Value { get; set; }

    }
}";

        private const string expectedSource = @"namespace Fis.Test{

    public class TestItem {

        public double Value { get; set; }
#region SetValue

        private void SetValue(double value)
        {
//whatever
        }

#endregion SetValue

    }
}";


        static void Main()
        {
            var projectId = ProjectId.CreateNewId(debugName: "TestProjekt");
            Solution solution;
            using (var workspace = new AdhocWorkspace())
            {
                solution = workspace
                .CurrentSolution
                .AddProject(projectId, "TestProjekt", "TestProjekt", LanguageNames.CSharp)
                .AddMetadataReference(projectId, CorlibReference)
                .AddMetadataReference(projectId, SystemCoreReference)
                .AddMetadataReference(projectId, CSharpSymbolsReference)
                .AddMetadataReference(projectId, CodeAnalysisReference);
            }
            var documentId = DocumentId.CreateNewId(projectId, debugName: "Test.cs");
            solution = solution.AddDocument(documentId, "Test.cs", SourceText.From(source));
            var doc=solution.GetProject(projectId).GetDocument(documentId);
            WriteStatus();

            //Modification
            var newNodes = SyntaxFactory.ParseCompilationUnit(methodText).ChildNodes();
            SyntaxNode root = doc.GetSyntaxRootAsync().Result;
            doc= doc.WithSyntaxRoot
            (
                root.InsertNodesAfter
                (
                    root.DescendantNodes().OfType<PropertyDeclarationSyntax>().First(),
                    newNodes
                )
            );

            WriteStatus();

            void WriteStatus()
            {
                var diagnostics=doc.GetSemanticModelAsync().Result.GetDiagnostics();
                Console.WriteLine($"Diagnostics: {diagnostics.Length}\n{string.Join('\n', diagnostics)}");
                Console.WriteLine();
                Console.WriteLine(doc.GetTextAsync().Result);
                Console.WriteLine();
                Console.WriteLine();
            }
        }
    }
}

3. Run Project And see output

Output

Diagnostics: 0

namespace Fis.Test{

    public class TestItem {

        public double Value { get; set; }

    }
}


Diagnostics: 1
Test.cs(8,9): error CS0106: Der "private"-Modifizierer ist für dieses Element nicht gültig.

namespace Fis.Test{

    public class TestItem {

        public double Value { get; set; }
#region SetValue

        private void SetValue(double value)
        {
//whatever
        }

    }
}

Expected Behavior:
No Diagnostics from each SyntaxTree

Actual Behavior:
After Modification of Document with the parsed MethodDeclarationSyntax the Diagnostic CS0106 (private Modifier for this element is not allowed) is reported.

I suspect this is due to addition of TopLevelStatements. This probably introduced the following change in SyntaxFactory.ParseCompilationUnit after v3.6.0: when a method-sourcetext is parsed it now results in a GlobalStatementSyntax with a LocalFunctionDeclaration and local functions do not allow modifiers.

The addition of the GlobalStamentSyntax itself to the Testclass does seem to work, but does it even make sense to allow a GlobalStamentSyntax to be inserted in 'ClassDeclarationSyntax.Members'? From what I read I guess that the statement is intendet to be parsed as a LocalFunctionDeclaration. Do I have to use 'SyntaxFactory.ParseMemberDeclaration' from now on? Maybe you could use the modifier keyword to indicate that the parsed Text is not a GlobalStatement? The main advantage of 'SyntaxFactory.ParseCompilationUnit' or 'CSharpSyntaxTree.ParseText' was that it could parse multiple declarations at once if needed for my Codefixes. I also noticed the outputtext of the whole tree with the inserted GlobalStamentSyntax is missing the #endregion for some reason but does not report an error for it.

I did not notice this issue happening in Visual Studio or Visual Studio Preview (yet?) but my AnalyzerUnitTests failed for the Codefixes that used this approach since v3.7. I guess my UnitTests Payed off^^

[CyrusNajmabadi]

This is by design (for a few reasons). First, we do not absolutely guarantee parsing output from version to version. Some construccts will change parsing depending on the versions you're using. For example, record Foo(int i) { } would previously parse as a method/function. Now it will parse as a record. On top of this, we absolutely will change parsing around constructs that were previously not legal and now are legal. In your case you have:

#region SetValue

        private void SetValue(double value)
        {
//whatever
        }

#endregion SetValue

this was never legal code in the past. So the parser just did its best and just produced some concrete syntax tree encoding that (using all the error recovery logic we baked in). That code is still not semantically legal (though it is syntactically legal). Our parser produces the new tree it thinks best represents this here (a local-function).

Possible workaround coming up.

[CyrusNajmabadi]

Do I have to use 'SyntaxFactory.ParseMemberDeclaration' from now on?

Yes. That is the recommended way to handle this. Or, just synthesize the node you want directly instead of using 'ParseXXX' functions. Then you have 100% control and don't have to worry about the parsing functions changing.

[CyrusNajmabadi]

Maybe you could use the modifier keyword to indicate that the parsed Text is not a GlobalStatement?

We could, but that could potentially have other negative implications. We'd probably want this to be a Global-Function so that other functionality (including IDE functionality) works.

[CyrusNajmabadi]

I also noticed the outputtext of the whole tree with the inserted GlobalStamentSyntax is missing the #endregion for some reason but does not report an error for it.

You're manually generating a tree. That won't have any diagnostics in it execpt the ones you put in it, or which are in the nodes you're inserting into the tree. In htis case, you're only inserting the member, and not the trivia that will be on the EOF token in the original compilation unit you parsed.

[Herrmel]

Ok thank you for your answers @CyrusNajmabadi. Just one thing left unanswered I think. Does it make sense to allow adding a GlobalStamentSyntax to ClassDeclarationSyntax.Members? I would expect to get an exception there since TopLevelStatements cannot be declared inside a class or am I wrong there?

[CyrusNajmabadi]

I would expect to get an exception there since TopLevelStatements cannot be declared inside a class or am I wrong there?

As above, when manually generating, general parsing validation is not done. In this case, GlobalStatementSyntax is-a MemberDeclarationSyntax. So it would be allowed in ClassDeclarationSyntax.Members.

[CyrusNajmabadi]

A good way to think about is that:

1. The parser takes in text and produces a tree, while also validating hte syntactic shape of things.
2. the syntax factory is effectively a pure-data, non-validating, factory. It doesn't know the syntactic rules of C#. It just lets you build trees however you want

:)

[Herrmel]

Okay, thank you very much for the explanation 😃