Draft implementation of file-scoped-namespaces by CyrusNajmabadi · Pull Request #48937 · dotnet/roslyn

CyrusNajmabadi · 2020-10-27T01:17:16Z

Draft impl of championed feature: dotnet/csharplang#137
Speclet at: dotnet/csharplang#4073
Test plan: #49000

To help ground discussion, find issues, and garner feedback to help with the language design process.

The general approach here was to take an approach similar to what we did with TypeScript. Specifically, given the two namespaces:

namespace X; class C { } and
namespace X { class C { } }

We represent these with different concrete types, but a similar abstract parse tree shape. Specifically, both generate a parse tree like so:

CompilationUnit
|
NamespaceDeclaration (or SingleLineNamespaceDeclaration)
|
-- Name
|    |
|    -- X
-- Members
    |
    -- ClassDeclaration
        |
       -- Name
            |
            -- C

NamespaceDeclaration and the new SingleLineNamespaceDeclaration derive from a common BaseNamespaceDeclaration. This type contains the Name, Externs, Usings, Attributes and Members. The two subclasses contain the only syntactic differents (the curlies, or hte semicolon).

By doing this, we allow all code to still presume a certain containment structure for files. For example, in the IDE there is lots of code that will walk upwards in a file looking for the containing namespace declaration. This approach allows all existing code (that doesn't care about curlies) to just move to processing 'BaseNamespaceDeclaration's, isntead of 'NamespaceDeclaration's, and allows anything that currently looks for SyntaxKind.NamespaceDeclaration to just look for that and SyntaxKind.SingleLineNamespaceDeclaration.

--

For my testing approach, i made targeted parsing tests and targeted tests for the specific specialized errors we produce. I then searched the compiler test base for usages of NamespaceDeclarationSyntax, SyntaxKind.NamespaceDeclaration, INamespaceSymbol, and NamespaceSymbol and generally created sibling tests that validated the same behavior when a single-line namespace was used instead.

jcouv · 2021-03-24T21:48:43Z

            // the reported errors should take into consideration whether or not one expects them in the current context.
            bool variableDeclarationsExpected =
                parentKind != SyntaxKind.NamespaceDeclaration &&
+                parentKind != SyntaxKind.SingleLineNamespaceDeclaration &&


Is there a test for this change?

@RikkiGibson to ensure we've addressed this

Adding the tests.

jcouv · 2021-03-24T22:44:42Z

-                var semicolon = this.TryEatToken(SyntaxKind.SemicolonToken);
+                    SyntaxListBuilder initialBadNodes = null;
+                    this.ParseNamespaceBody(ref semicolon, ref body, ref initialBadNodes, SyntaxKind.SingleLineNamespaceDeclaration);
+                    Debug.Assert(initialBadNodes == null); // init bad nodes should have been attached to open brace...


Comment is wrong (would be attached to semicolon). Do we have a test scenario where that happens?

@RikkiGibson to address

See SingleLineDeclarationParsingTests.NamespaceWithExtraSemicolon. Will fix the comment.

jcouv · 2021-03-24T22:48:57Z

+            var comp = CreateCompilation(tree);
+            var model = comp.GetSemanticModel(tree);
+            var errors = comp.GetDiagnostics().ToArray();
+            Assert.Equal(2, errors.Length);


nit: Let's check the diagnostics instead of just the count

I am OK with this because it's taken from the test directly above.

jcouv · 2021-03-24T22:56:11Z

+            Assert.False(classDecl.Identifier.IsMissing);
+            Assert.True(classDecl.OpenBraceToken.IsMissing);
+            Assert.True(classDecl.CloseBraceToken.IsMissing);
+            var ns = root.DescendantNodes().OfType<SingleLineNamespaceDeclarationSyntax>().Single();


Did you intend to check something on ns?

@RikkiGibson to address

It's not that meaningful but I'll add an assert that the semicolon is present.

jcouv

LGTM Thanks (iteration 26) with a couple test suggestions

jcouv · 2021-03-25T03:42:53Z

+                {
+                    diagnostics.Add(ErrorCode.ERR_SingleLineAndNormalNamespace, node.Name.GetLocation());
+                }
+            }


We should also have a check to disallow top-level statements.

@RikkiGibson Add PROTOTYPE or track elsewehere?

I think this is handled by the existing mechanisms that prevent top-level statements within namespaces. See SingleLineNamespaceWithFollowingStatement in AssemblyAndNamespaceTests.cs

RikkiGibson

LGTM. My expectation is I will make the changes to resolve the comments myself in a follow-up PR.

RikkiGibson · 2021-06-09T22:48:31Z

                externAliasDirectives = compilationUnit.Externs;
            }
-            else if (declarationSyntax.Kind() == SyntaxKind.NamespaceDeclaration)
+            else if (declarationSyntax.Kind() is SyntaxKind.NamespaceDeclaration or SyntaxKind.SingleLineNamespaceDeclaration)


wondering if we should have an extension for this. Depends on how often this check is performed of course.

@RikkiGibson I find 5 or 6 instances of this check so an extension sounds fine. I'd say push a commit if you'd like (you're in charge of the PR ;-)

Not really interested in doing this at this time. Would rather make the adjustment as part of a larger refactoring e.g. to replace Kind() tests with type tests in more places.

RikkiGibson · 2021-06-09T22:50:31Z

            {
                // top-level namespace:
-                if (memberDeclaration.Kind() == SyntaxKind.NamespaceDeclaration)
+                if (memberDeclaration.Kind() == SyntaxKind.NamespaceDeclaration ||


nit: feels like we should use or here instead of calling .Kind() twice

RikkiGibson · 2021-06-09T22:51:55Z


            // a namespace or a type in an explicitly declared namespace:
-            if (memberDeclaration.Kind() == SyntaxKind.NamespaceDeclaration || SyntaxFacts.IsTypeDeclaration(memberDeclaration.Kind()))
+            if (memberDeclaration.Kind() is SyntaxKind.NamespaceDeclaration or SyntaxKind.SingleLineNamespaceDeclaration ||


nit: prefer binary operators on the start of the next line. For example || SyntaxFacts.IsTypeDeclaration(...)

RikkiGibson · 2021-06-10T20:56:37Z


+        [WorkItem(791756, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/791756")]
+        [Fact]
+        public void KindOptionsSingleLineNamespace()


I may end up coming up with some kind of helper that makes it easier to express these tests as [Theory].

Have done this for many of the tests.

RikkiGibson · 2021-06-10T21:12:27Z


        [Fact]
-        public void TypeParameterDeclaringSyntax()
+        public void TypeParameterDeclaringSyntaxSingleLineNamespace()


I didn't understand why the "regular" namespace version of the test was essentially removed here.

Good point. Let's add the original back, or make this a Theory to cover both

RikkiGibson · 2021-06-10T21:13:33Z

                Diagnostic(ErrorCode.ERR_BadModifiersOnNamespace, "[System.Obsolete]").WithLocation(1, 1));
        }
+
+        private static readonly CSharpParseOptions s_previewOptions = CSharpParseOptions.Default.WithLanguageVersion(LanguageVersion.Preview);


nit: Typically we use TestOptions.RegularPreview

Renamed this. I think it could be handy to be able to change this from Preview to Regular in one place. We have done this for other features in the past, IIRC.

RikkiGibson · 2021-06-10T21:47:35Z

@jcouv @chsienki My expectation is that we will try to merge this PR more or less as-is, then submit a follow up which resolves any unresolved comments on this PR. Does that seem reasonable? Or should I maybe just push commits to this PR?

jaredpar · 2021-06-10T22:10:43Z

@RikkiGibson That seems reasonable to me. This PR is already quite large, a bit unweildy (evidenced by the current conflicts), and we are going into a feature branch. I would focus on getting this merged then follow up with a PR that cleans up the items that you noticed.

… into singleLineNamespaces

jcouv

LGTM Thanks (iteration 28)

CyrusNajmabadi added 7 commits October 26, 2020 12:41

Update grammar to support single line namespaces

b4faf36

Add parsing for single line namespaces

76910ee

Add errors for misplaced single-line namespaces

9a8008a

Add tests

1ab99c5

Add symbolic tests

1178616

Update to singleline namespaces

3e84b39

Add tests

095717a

CyrusNajmabadi requested a review from a team as a code owner October 27, 2020 01:17

Dotnet-GitSync-Bot added the Area-Compilers label Oct 27, 2020

CyrusNajmabadi marked this pull request as draft October 27, 2020 01:17

CyrusNajmabadi added 4 commits October 26, 2020 19:24

Add to public API

b2984b4

Fixup test

7a6a27f

Add to public API

91a0190

Get building

a183d9e

alrz reviewed Oct 27, 2020

View reviewed changes

Comment thread src/Compilers/CSharp/Portable/Syntax/Syntax.xml

Comment thread src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

CyrusNajmabadi added 8 commits October 27, 2020 08:39

Change test

1800e91

Change test

e0f5ae4

Change test

ae47f77

Change test

824f093

Change test

144dc48

Change test

7ffeabc

Change test

0388a4c

Change test

35593fa

jcouv reviewed Oct 28, 2020

View reviewed changes

Comment thread src/Compilers/CSharp/Portable/Syntax/Syntax.xml Outdated

jcouv reviewed Oct 28, 2020

View reviewed changes

Comment thread src/Compilers/CSharp/Portable/Binder/BinderFactory.BinderFactoryVisitor.cs

jcouv reviewed Oct 28, 2020

View reviewed changes

Comment thread src/Compilers/CSharp/Portable/Binder/BinderFactory.BinderFactoryVisitor.cs Outdated

jcouv reviewed Oct 28, 2020

View reviewed changes

Comment thread src/Compilers/CSharp/Portable/Binder/Imports.cs Outdated

This comment has been minimized.

Sign in to view

CyrusNajmabadi marked this pull request as ready for review February 24, 2021 18:29

jcouv requested a review from chsienki February 24, 2021 18:48

This comment has been minimized.

Sign in to view

chsienki changed the base branch from master to features/FileScopedNamespaces February 26, 2021 00:56

This comment has been minimized.

Sign in to view

jcouv added this to the C# 10 milestone Mar 23, 2021

jcouv reviewed Mar 24, 2021

View reviewed changes

Comment thread src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

jcouv reviewed Mar 24, 2021

View reviewed changes

Comment thread src/Compilers/CSharp/Portable/CSharpResources.resx

jcouv reviewed Mar 24, 2021

View reviewed changes

jcouv approved these changes Mar 25, 2021

View reviewed changes

jcouv self-assigned this Mar 25, 2021

jcouv reviewed Mar 25, 2021

View reviewed changes

jcouv mentioned this pull request Apr 27, 2021

Demo: single-line namespaces #52972

Merged

CyrusNajmabadi mentioned this pull request Jun 5, 2021

Add "IndentNamespace" formatting option. #24129

Closed

RikkiGibson self-assigned this Jun 7, 2021

RikkiGibson approved these changes Jun 10, 2021

View reviewed changes

RikkiGibson added 3 commits June 14, 2021 13:39

Merge remote-tracking branch 'upstream/features/FileScopedNamespaces'…

7a9f9cf

… into singleLineNamespaces

Fix merge conflicts and test failures

ee7aad3

Revert unrelated file change

455a8aa

jcouv approved these changes Jun 14, 2021

View reviewed changes

Fix assert

dc6b8b1

RikkiGibson merged commit 1a69700 into dotnet:features/FileScopedNamespaces Jun 21, 2021

RikkiGibson mentioned this pull request Jun 21, 2021

File scoped namespaces cleanup #54274

Merged

CyrusNajmabadi deleted the singleLineNamespaces branch July 1, 2022 16:24

Conversation

CyrusNajmabadi commented Oct 27, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RikkiGibson commented Jun 10, 2021

CyrusNajmabadi commented Oct 27, 2020 •

edited by jcouv

Loading