Syntax API for FileScopedNamespaces

[RikkiGibson]

Background and Motivation

The file scoped namespaces feature #49000 introduces new syntax nodes. Those syntax nodes are naturally part of the public API.

Proposed API

namespace Microsoft.CodeAnalysis.CSharp.Syntax
{
+    public abstract partial class BaseNamespaceDeclarationSyntax : MemberDeclarationSyntax
+    {
+        public abstract SyntaxToken NamespaceKeyword { get; }
+        public abstract NameSyntax Name { get; }
+        public abstract SyntaxList<ExternAliasDirectiveSyntax> Externs { get; }
+        public abstract SyntaxList<UsingDirectiveSyntax> Usings { get; }
+        public abstract SyntaxList<MemberDeclarationSyntax> Members { get; }
+    }

-    public sealed partial class NamespaceDeclarationSyntax : MemberDeclarationSyntax { }
+    public sealed partial class NamespaceDeclarationSyntax : BaseNamespaceDeclarationSyntax
    {
-        public SyntaxToken NamespaceKeyword { get; }
+        public override SyntaxToken NamespaceKeyword { get; }
        // same change for the rest of the abstract members of `BaseNamespaceDeclarationSyntax`...
    }

+    public sealed partial class FileScopedNamespaceDeclarationSyntax : BaseNamespaceDeclarationSyntax
+    {
+        public override SyntaxToken NamespaceKeyword { get; }
        // same change for the rest of the abstract members of `BaseNamespaceDeclarationSyntax`...

+        public SyntaxToken SemicolonToken { get; }
+    }
}

namespace Microsoft.CodeAnalysis.CSharp
{
    public enum SyntaxKind
    {
        ...,
        NamespaceDeclaration,
+        FileScopedNamespaceDeclaration
    }
}

Usage Examples

public class MyVisitor : CSharpSyntaxVisitor
{
    public override void VisitNamespaceDeclaration(NamespaceDeclarationSyntax @namespace)
    {
        VisitBaseNamespaceDeclaration(@namespace);
    }

    public override void VisitFileScopedNamespaceDeclaration(FileScopedNamespaceDeclarationSyntax fileScopedNamespace)
    {
        VisitBaseNamespaceDeclaration(fileScopedNamespace);
    }

    private void VisitBaseNamespaceDeclaration(BaseNamespaceDeclarationSyntax baseNamespace)
    {
        foreach (var member in baseNamespace.Members)
        {
            DoSomethingWith(member);
        }
    }
}

static class SyntaxNodeExtensions
{
    public static IEnumerable<MemberDeclarationSyntax> GetMembers(this SyntaxNode? node)
    {
        switch (node)
        {
            case CompilationUnitSyntax compilation:
                return compilation.Members;
            case BaseNamespaceDeclarationSyntax @namespace:
                return @namespace.Members;
            case TypeDeclarationSyntax type:
                return type.Members;
            case EnumDeclarationSyntax @enum:
                return @enum.Members;
        }

        return SpecializedCollections.EmptyEnumerable<MemberDeclarationSyntax>();
    }
}

Alternative Designs

We might be able to pack the new syntax into the existing NamespaceDeclarationSyntax. However, to do this while maintaining invariants is a bit clumsy, and it more or less violates our guidelines around using the same node to represent two different things.

namespace Microsoft.CodeAnalysis.CSharp.Syntax
{
    public sealed partial class NamespaceDeclarationSyntax : MemberDeclarationSyntax
    {
        public SyntaxToken NamespaceKeyword { get; }
        public NameSyntax Name { get; }
+        public SyntaxToken LeadingSemicolonToken { get; }
        public SyntaxToken OpenBraceToken { get; }
        public SyntaxList<ExternAliasDirectiveSyntax> Externs { get; }
        public SyntaxList<UsingDirectiveSyntax> Usings { get; }
        public SyntaxList<MemberDeclarationSyntax> Members { get; }
        public SyntaxToken CloseBraceToken { get; }
        public SyntaxToken SemicolonToken { get; }
    }
}

Note particularly that this alternative design would require keeping the existing, optional SemicolonToken which trails the closing brace of the namespace, as well as adding a new LeadingSemicolonToken, which precedes the list of externs+usings+members in the namespace.

Risks

If we ship the suggested design, users will have to react by changing their code:

• If they were looking for SyntaxKind.NamespaceDeclaration, they will probably want to look for both SyntaxKind.NamespaceDeclaration and SyntaxKind.FileScopedNamespaceDeclaration
• If they were looking for NamespaceDeclarationSyntax, they will probably want to switch to BaseNamespaceDeclarationSyntax
• If they were inheriting from the CSharpSyntaxVisitor, they will probably want to override both Visit(NamespaceDeclarationSyntax) and Visit(FileScopedNamespaceDeclarationSyntax), likely delegating to the same method for both.

However, there are some cases we've spotted internally, like the normalizer and formatter, where we definitely wouldn't want to treat these nodes the same for determining indent depth, for example.

This design also matches some past changes to the syntax design, such as for RecordDeclaration or ForEachStatement.

[333fred]

@CyrusNajmabadi I assume you're good with this api proposal as well?

[CyrusNajmabadi]

Yes i am.

[tmat]

Why are the common members abstract?

[RikkiGibson]

Why are the common members abstract?

I think this is a convention of syntax generation--for <Field>s underneath <AbstractNode>s in Syntax.xml, we produce only abstract properties. For situations where the subtypes of the abstract node have different numbers of child nodes, it probably helps us generate the implementation per our conventions--calling into GetRed with the right slot and so on.

https://github.com/dotnet/roslyn/blob/5d28e2905a84603cb6818c5ce787710ae9655ccd/src/Compilers/CSharp/Portable/Generated/CSharpSyntaxGenerator/CSharpSyntaxGenerator.SourceGenerator/Syntax.xml.Syntax.Generated.cs#L9302-L9336

https://github.com/dotnet/roslyn/blob/5d28e2905a84603cb6818c5ce787710ae9655ccd/src/Compilers/CSharp/Portable/Generated/CSharpSyntaxGenerator/CSharpSyntaxGenerator.SourceGenerator/Syntax.xml.Syntax.Generated.cs#L9181-L9227

If you ctrl+f for "abstract partial class" in the file linked above you'll find that the abstract classes don't declare fields or concrete properties, only the non-abstract subtypes do. I do not know if it would be possible to sometimes push fields into the parent type and de-virtualize some of the properties, for example, but I think it would be a new and separate thing.

[CyrusNajmabadi]

Yup. This is just an affectation of how the generator works. It might be possible to change this, but there hasn't been a need yet

[chsienki]

Notes

Shape / nesting

Should this really be a nesting node, or an independent node. There has been discussion about this in LDM, and the decision was made to represent it this way, so we should keep as it.

It matches the 'spirit' of the spec, so seems appropriate.

Breaking change?

This will break users of the syntax model, but either way we do it they will have to change code, so it doesn't matter either way. There is some evidence from the FXCop analyzers that handling this kind of base class extraction change is fairly painless, so we think its ok.

[taylorchasewhite]

Has this been released? In preview mode I am debugging and see FileScopeNamespaceDeclarationSyntax, but I cannot reference it or its base class in my code...

[333fred]

This has not been released. The issue will be closed when implemented.

[RikkiGibson]

You'll have to reference a recent 4.0.0 version of the Roslyn packages to use the new types in your code.

[jaredpar]

@RikkiGibson @333fred has this been addressed by API review? Getting very close to the release now and want to make sure we don't let this slip through

[333fred]

@RikkiGibson @333fred has this been addressed by API review? Getting very close to the release now and want to make sure we don't let this slip through

Yes, this has the api-approved label. It's ready to be implemented.

[RikkiGibson]

Since this is just describing the syntax model for the feature as it was implemented, and the feature has been merged, we're all done here.

[jcouv]

@RikkiGibson Can we close this issue?