Implement parsing for Global Using Directive. #51426

AlekseyTs · 2021-02-23T22:29:08Z

Test plan: #51307

CyrusNajmabadi · 2021-02-23T23:48:58Z

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

+                            break;
+
+                        case SyntaxKind.IdentifierToken:
+                            if (this.CurrentToken.ContextualKind != SyntaxKind.GlobalKeyword || this.PeekToken(1).Kind != SyntaxKind.UsingKeyword)


fwiw, i think tihs would be much clearer if hte if was flipped and the condition was just if (CurrentToken == Global && PeekToken(1) == Using). #WontFix

fwiw, i think tihs would be much clearer if hte if was flipped and the condition was just if (CurrentToken == Global && PeekToken(1) == Using).

I prefer to structure the code in this case section and in the previous section as close as possible.

In reply to: 581474923 [](ancestors = 581474923)

AlekseyTs · 2021-02-24T03:47:04Z

/azp run

azure-pipelines · 2021-02-24T03:47:22Z

Azure Pipelines successfully started running 2 pipeline(s).

jcouv · 2021-02-24T06:31:51Z

src/Compilers/CSharp/Portable/Errors/MessageID.cs

@@ -324,6 +325,7 @@ internal static LanguageVersion RequiredVersion(this MessageID feature)
            {
                // C# preview features.
                case MessageID.IDS_FeatureMixedDeclarationsAndExpressionsInDeconstruction:
+                case MessageID.IDS_FeatureUsingGlobal:


IDS_FeatureUsingGlobal [](start = 31, length = 22)

nit: "GlobalUsing" #Closed

jcouv · 2021-02-24T06:31:55Z

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

            if (staticToken != null)
            {
                usingDirective = CheckFeatureAvailability(usingDirective, MessageID.IDS_FeatureUsingStatic);
            }

+            if (globalToken != null)
+            {
+                usingDirective = CheckFeatureAvailability(usingDirective, MessageID.IDS_FeatureUsingGlobal);


nit: why put the diagnostic on the whole directive? I expected we'd check the LangVer when we eat the token above (line 756) and put the diagnostic on globalToken (it's a little more specific) #ByDesign

nit: why put the diagnostic on the whole directive? I expected we'd check the LangVer when we eat the token above (line 756) and put the diagnostic on globalToken (it's a little more specific)

I am following the pattern already established for the static keyword.

In reply to: 581664800 [](ancestors = 581664800)

jcouv · 2021-02-24T06:32:00Z

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

+                var @using = this.ParseUsingDirective();
+                if (seen > NamespaceParts.Usings)
+                {
+                    @using = this.AddError(@using, ErrorCode.ERR_UsingAfterElements);


If I recall correctly, adding an order error during parsing (as opposed to binding) has impact on incremental parsing that we generally want to avoid.
On the other hand, it looks like we already do it for top-level statements.
https://github.com/dotnet/roslyn/blob/master/docs/compilers/Design/Parser.md#do-not-use-ambient-context-to-direct-the-behavior-of-the-parser #Closed

If I recall correctly, adding an order error during parsing (as opposed to binding) has impact on incremental parsing that we generally want to avoid.

this is true. however, in this case, this is the right thing to do. We effectively have no place to put this using-directive as we're now past the location in the CompliationUnitSyntax where we could store them. So we instead have to break this down and attach to whatever real construct we find.

If we had made CompilationUnitSyntax just have a list of generic member nodes, we could have potentially kept the using directive here and error'ed later. However, that would have come with the negative of not having a nice strongly typed API for CompilationUnit that explicitly breaks out the different types of children. #Closed

If I recall correctly, adding an order error during parsing (as opposed to binding) has impact on incremental parsing that we generally want to avoid.

This is a preexisting behavior of the parser. No change is introduced here. The code is simply extracted into a local function and reused.

In reply to: 581664827 [](ancestors = 581664827)

Makes sense. Thanks

In reply to: 581718358 [](ancestors = 581718358)

jcouv

LGTM Thanks (iteration 1)

Youssef1313 · 2021-02-24T08:32:45Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -6600,4 +6600,13 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
  <data name="ERR_FunctionPointerTypesInAttributeNotSupported" xml:space="preserve">
    <value>Using a function pointer type in a 'typeof' in an attribute is not supported.</value>
  </data>
+  <data name="IDS_FeatureUsingGlobal" xml:space="preserve">
+    <value>global using directive</value>


Should this have a comment for localization team? #WontFix

Should this have a comment for localization team?

Historically we do not do this.

In reply to: 581753713 [](ancestors = 581753713)

@AlekseyTs There are few resources that add similar comments for loc team:

roslyn/src/Compilers/CSharp/Portable/CSharpResources.resx

Line 6519 in 2ddaf1c

<comment>UnmanagedCallersOnly is not localizable.</comment>

This results in better translations. Was there a reason to not do this? (I see the resources I'm referring to are very very recent) #Resolved

Was there a reason to not do this?

In my opinion there was no a good reason to add those comments.

In reply to: 582073782 [](ancestors = 582073782)

AlekseyTs · 2021-02-24T15:35:53Z

@dotnet/roslyn-compiler Please review, need a second sign-off.

Youssef1313 · 2021-02-24T15:59:15Z

src/Compilers/CSharp/Portable/Declarations/RootSingleNamespaceDeclaration.cs

@@ -53,6 +57,14 @@ public bool HasAssemblyAttributes
            }
        }

+        public override bool HasGlobalUsings


Does SingleNamespaceDeclarationEx need to override this as well? I'm not sure what SingleNamespaceDeclarationEx is used for, so just noting about it. #ByDesign

Does SingleNamespaceDeclarationEx need to override this as well? I'm not sure what SingleNamespaceDeclarationEx is used for, so just noting about it.

Namespace declarations are not allowed to have Global Using directives. For the purpose of error recovery all usings there will be treated as non-global, therefore, there are no Global Usings there.

In reply to: 582082881 [](ancestors = 582082881)

Youssef1313 · 2021-02-24T16:06:17Z

src/Compilers/CSharp/Test/Syntax/Generated/Syntax.Test.xml.Generated.cs

@@ -10106,7 +10107,7 @@ private static ExternAliasDirectiveSyntax GenerateExternAliasDirective()
            => SyntaxFactory.ExternAliasDirective(SyntaxFactory.Token(SyntaxKind.ExternKeyword), SyntaxFactory.Token(SyntaxKind.AliasKeyword), SyntaxFactory.Identifier("Identifier"), SyntaxFactory.Token(SyntaxKind.SemicolonToken));

        private static UsingDirectiveSyntax GenerateUsingDirective()
-            => SyntaxFactory.UsingDirective(SyntaxFactory.Token(SyntaxKind.UsingKeyword), default(SyntaxToken), default(NameEqualsSyntax), GenerateIdentifierName(), SyntaxFactory.Token(SyntaxKind.SemicolonToken));
+            => SyntaxFactory.UsingDirective(default(SyntaxToken), SyntaxFactory.Token(SyntaxKind.UsingKeyword), default(SyntaxToken), default(NameEqualsSyntax), GenerateIdentifierName(), SyntaxFactory.Token(SyntaxKind.SemicolonToken));


nit: the old overload is now not tested. #Resolved

nit: the old overload is now not tested.

I find existing usage of that overload in the solution sufficient to provide coverage.

In reply to: 582088799 [](ancestors = 582088799)

Implement parsing for Global Using Directive.

2ddaf1c

AlekseyTs added the Area-Compilers label Feb 23, 2021

AlekseyTs requested a review from a team as a code owner February 23, 2021 22:29

CyrusNajmabadi reviewed Feb 23, 2021

View reviewed changes

jcouv reviewed Feb 24, 2021

View reviewed changes

jcouv approved these changes Feb 24, 2021

View reviewed changes

jcouv self-assigned this Feb 24, 2021

jcouv added this to the C# 10 milestone Feb 24, 2021

Youssef1313 reviewed Feb 24, 2021

View reviewed changes

Rename feature id.

fe204bb

Youssef1313 reviewed Feb 24, 2021

View reviewed changes

cston approved these changes Feb 24, 2021

View reviewed changes

AlekseyTs merged commit b0019ba into dotnet:features/GlobalUsingDirective Feb 24, 2021

Youssef1313 mentioned this pull request Mar 26, 2021

Feature: CallerArgumentExpressionAttribute #51952

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement parsing for Global Using Directive. #51426

Implement parsing for Global Using Directive. #51426

AlekseyTs commented Feb 23, 2021 •

edited by jcouv

Loading

CyrusNajmabadi Feb 23, 2021 •

edited by AlekseyTs

Loading

AlekseyTs Feb 24, 2021

AlekseyTs commented Feb 24, 2021

azure-pipelines bot commented Feb 24, 2021

jcouv Feb 24, 2021 •

edited

Loading

jcouv Feb 24, 2021 •

edited by AlekseyTs

Loading

AlekseyTs Feb 24, 2021

jcouv Feb 24, 2021 •

edited

Loading

CyrusNajmabadi Feb 24, 2021 •

edited by jcouv

Loading

AlekseyTs Feb 24, 2021

jcouv Feb 24, 2021

jcouv left a comment

Youssef1313 Feb 24, 2021 •

edited by AlekseyTs

Loading

AlekseyTs Feb 24, 2021

Youssef1313 Feb 24, 2021 •

edited by AlekseyTs

Loading

AlekseyTs Feb 24, 2021

AlekseyTs commented Feb 24, 2021

Youssef1313 Feb 24, 2021 •

edited by AlekseyTs

Loading

AlekseyTs Feb 24, 2021

Youssef1313 Feb 24, 2021 •

edited by AlekseyTs

Loading

AlekseyTs Feb 24, 2021

Implement parsing for Global Using Directive. #51426

Implement parsing for Global Using Directive. #51426

Conversation

AlekseyTs commented Feb 23, 2021 • edited by jcouv Loading

CyrusNajmabadi Feb 23, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Feb 24, 2021

azure-pipelines bot commented Feb 24, 2021

jcouv Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv Feb 24, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi Feb 24, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

Youssef1313 Feb 24, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youssef1313 Feb 24, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Feb 24, 2021

Youssef1313 Feb 24, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youssef1313 Feb 24, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Feb 23, 2021 •

edited by jcouv

Loading

CyrusNajmabadi Feb 23, 2021 •

edited by AlekseyTs

Loading

jcouv Feb 24, 2021 •

edited

Loading

jcouv Feb 24, 2021 •

edited by AlekseyTs

Loading

jcouv Feb 24, 2021 •

edited

Loading

CyrusNajmabadi Feb 24, 2021 •

edited by jcouv

Loading

Youssef1313 Feb 24, 2021 •

edited by AlekseyTs

Loading

Youssef1313 Feb 24, 2021 •

edited by AlekseyTs

Loading

Youssef1313 Feb 24, 2021 •

edited by AlekseyTs

Loading

Youssef1313 Feb 24, 2021 •

edited by AlekseyTs

Loading