Simplify representation of AttributeLists/Modifiers on all MemberDecls. by CyrusNajmabadi · Pull Request #32999 · dotnet/roslyn

CyrusNajmabadi · 2019-01-31T06:20:08Z

The current (i.e. prior to this PR) way of representing attributes/modifiers is to specify which nodes they can go on in an adhoc manner. Some member decls specify they take attributes+modifiers, some only attributes, and some take none at all. While this is accurate as per the grammar, it makes for some ungainly APIs and complex use cases.

For example, our parser detects if you write attributes/modifiers for a namespace, but then has no place to put it. Similarly, lots of code in later layers has to write specialized code to get at modifiers/attributelists.

--

This PR attempts to simplify things (taking a page from TypeScript). After this PR, we simply define all member decls as being able to have attributes/modifiers. This means that if we run into attributes/modifiers that aren't grammatically legal, we still have a place to put them. And, later layers can more easily get/change/add these properties easily.

--

This also ties into the goal of pulling errors out of hte parser, since they affect incremental parsing, as well as making it more difficult for later phases to understand what is going on.

--

Todo:

tests.

CyrusNajmabadi · 2019-01-31T06:20:22Z

Tagging @jcouv @gafter

CyrusNajmabadi · 2019-01-31T06:20:57Z

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

+            if (node.Modifiers.Count > 0)
+            {
+                diagnostics.Add(ErrorCode.ERR_BadModifiersOnNamespace, node.Modifiers[0].GetLocation());
+            }


this moved from LanguageParser to here. This is how we've dealt with other Namespace errors that were previously syntax errors, that we wanted to move out of hte parser.

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

CyrusNajmabadi · 2019-01-31T06:22:52Z

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

-                    if (attributes.Count > 0)
-                    {
-                        namespaceDecl = AddLeadingSkippedSyntax(namespaceDecl, attributes.ToListNode());
-                    }


no need for skipped syntax (which sucks). We can totally represent this expected sort of case simply in the syntax model.

CyrusNajmabadi · 2019-01-31T06:24:21Z

src/Compilers/CSharp/Portable/PublicAPI.Shipped.txt

 abstract Microsoft.CodeAnalysis.CSharp.Syntax.AnonymousFunctionExpressionSyntax.Body.get -> Microsoft.CodeAnalysis.CSharp.CSharpSyntaxNode
 abstract Microsoft.CodeAnalysis.CSharp.Syntax.BaseArgumentListSyntax.Arguments.get -> Microsoft.CodeAnalysis.SeparatedSyntaxList<Microsoft.CodeAnalysis.CSharp.Syntax.ArgumentSyntax>
 abstract Microsoft.CodeAnalysis.CSharp.Syntax.BaseCrefParameterListSyntax.Parameters.get -> Microsoft.CodeAnalysis.SeparatedSyntaxList<Microsoft.CodeAnalysis.CSharp.Syntax.CrefParameterSyntax>
-abstract Microsoft.CodeAnalysis.CSharp.Syntax.BaseFieldDeclarationSyntax.AttributeLists.get -> Microsoft.CodeAnalysis.SyntaxList<Microsoft.CodeAnalysis.CSharp.Syntax.AttributeListSyntax>


@jcouv I'm not sure if htis is the right way to deal with publicapi.shipped.

Here's what happened. types like BaseFieldDeclarationSyntax had 'AttributeLists'. But this abstract method moved down into MemberDeclaration. To ensure that this member still exists in BaseFieldDeclarationSyntax i added an abstract-override. So you'll see that abstract-override added below. This should be source/binary compatible. But it does require updating these files accordingly.

CyrusNajmabadi · 2019-01-31T06:25:12Z

src/Compilers/CSharp/Portable/PublicAPI.Unshipped.txt

 override Microsoft.CodeAnalysis.CSharp.Syntax.NullableDirectiveTriviaSyntax.IsActive.get -> bool
 override Microsoft.CodeAnalysis.CSharp.Syntax.RangeExpressionSyntax.Accept(Microsoft.CodeAnalysis.CSharp.CSharpSyntaxVisitor visitor) -> void
 override Microsoft.CodeAnalysis.CSharp.Syntax.RangeExpressionSyntax.Accept<TResult>(Microsoft.CodeAnalysis.CSharp.CSharpSyntaxVisitor<TResult> visitor) -> TResult
+override abstract Microsoft.CodeAnalysis.CSharp.Syntax.BaseFieldDeclarationSyntax.AttributeLists.get -> Microsoft.CodeAnalysis.SyntaxList<Microsoft.CodeAnalysis.CSharp.Syntax.AttributeListSyntax>


here are the abstract overrides corresponding to the members that were removed.

CyrusNajmabadi · 2019-01-31T06:27:20Z

src/Compilers/CSharp/Portable/PublicAPI.Unshipped.txt

@@ -1,4 +1,18 @@
 *REMOVED*static Microsoft.CodeAnalysis.CSharp.LanguageVersionFacts.TryParse(this string version, out Microsoft.CodeAnalysis.CSharp.LanguageVersion result) -> bool
+*REMOVED*abstract Microsoft.CodeAnalysis.CSharp.Syntax.BaseFieldDeclarationSyntax.AttributeLists.get -> Microsoft.CodeAnalysis.SyntaxList<Microsoft.CodeAnalysis.CSharp.Syntax.AttributeListSyntax>


@jcouv I'm not sure if htis is the right way to deal with publicapi.shipped.

Here's what happened. types like BaseFieldDeclarationSyntax had 'AttributeLists'. But this abstract method moved down into MemberDeclaration. To ensure that this member still exists in BaseFieldDeclarationSyntax i added an abstract-override. So you'll see that abstract-override added below. This should be source/binary compatible. But it does require updating these files accordingly.

src/Compilers/CSharp/Portable/Syntax/BaseFieldDeclarationSyntax.cs

src/Compilers/CSharp/Portable/Syntax/EnumMemberDeclarationSyntax.cs

CyrusNajmabadi · 2019-02-04T22:42:11Z

@jcouv @gafter Can you ptal? Thanks!

CyrusNajmabadi · 2019-02-18T08:22:38Z

@jcouv @gafter Can you ptal? Thanks!

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

jcouv

Change LGTM modulo some test suggestions (iteration 34). Pinged Neal to confirm the desire to make this change.

gafter

CyrusNajmabadi · 2019-05-29T00:02:13Z

Thanks for the reviews. Now that i know this works for you guys, i'll beef up the tests :)

CyrusNajmabadi · 2019-05-29T00:11:34Z

src/Features/CSharp/Portable/Completion/KeywordRecommenders/AssemblyKeywordRecommender.cs

-            // [$$
-            // namespace Goo {
-            return skippedTokensTriviaSyntax is SkippedTokensTriviaSyntax;
+            return false;


added bonus. IDE doesn't have to try to infer what's happening from error recovery.

jcouv · 2019-05-31T20:36:40Z

I'm marking as "personal" for now, to make room in our query. Ping when tests are added and ready for another look. Thanks! #Closed

jcouv · 2019-06-03T03:04:00Z

@CyrusNajmabadi This PR looks good and is just missing a couple of tests when you get a chance. Thanks! #Closed

CyrusNajmabadi · 2019-06-03T03:19:18Z

@jcouv Yup! I've been a little under the weather. I'll try to get to this in hte next week. Thanks!

CyrusNajmabadi · 2019-06-07T20:08:22Z

@jcouv added a few tiny tests to just hit the new scenarios.

CyrusNajmabadi · 2019-06-08T01:05:20Z

@jcouv ? :)

CyrusNajmabadi · 2019-06-08T21:06:15Z

@jcouv I'm champing at the bit for this to go in :D

jcouv

LGTM Thanks (iteration 42)

CyrusNajmabadi · 2019-06-08T22:40:19Z

Awesome, thanks!

There was a previous parsing change (dotnet#32999) which modified namespace parsing to allow modifiers and attributes on namespaces, to improve error recovery. This PR contained a bug because it didn't move the incremental parsing check to before parsing attributes and modifiers, which should now be included in incremental parsing to prevent changes from being dropped Fixes dotnet#37665, dotnet#37664, dotnet#37663

There was a previous parsing change (#32999) which modified namespace parsing to allow modifiers and attributes on namespaces, to improve error recovery. This PR contained a bug because it didn't move the incremental parsing check to before parsing attributes and modifiers, which should now be included in incremental parsing to prevent changes from being dropped Fixes #37665, #37664, #37663

CyrusNajmabadi added 3 commits January 30, 2019 20:00

handle mods and attributes on a namespace

63f92e9

Support modifiers and attribute lists on all memberdecls

f4918b5

Add diagnostics back

ee498aa

CyrusNajmabadi requested a review from a team as a code owner January 31, 2019 06:20

CyrusNajmabadi commented Jan 31, 2019

View reviewed changes

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Outdated Show resolved Hide resolved

CyrusNajmabadi commented Jan 31, 2019

View reviewed changes

Revert

883ccab

CyrusNajmabadi commented Jan 31, 2019

View reviewed changes

src/Compilers/CSharp/Portable/Syntax/BaseFieldDeclarationSyntax.cs Show resolved Hide resolved

CyrusNajmabadi commented Jan 31, 2019

View reviewed changes

src/Compilers/CSharp/Portable/Syntax/EnumMemberDeclarationSyntax.cs Show resolved Hide resolved

CyrusNajmabadi added 3 commits January 30, 2019 22:34

Null refs

169d861

Add tests.

df527f0

Simplify impl

b781847

CyrusNajmabadi requested a review from a team as a code owner January 31, 2019 07:51

CyrusNajmabadi added 2 commits January 30, 2019 23:52

Simplify impl

a0ba693

Simplify

522c7e6

jinujoseph added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Jan 31, 2019

CyrusNajmabadi added 2 commits January 31, 2019 16:29

Update test

421aa1c

Fix regression

1f6dc14

Merge branch 'master' into namespaceMods

bceb358

jcouv self-assigned this Feb 19, 2019

jcouv added this to the 16.1.P1 milestone Feb 19, 2019

jcouv reviewed Feb 19, 2019

View reviewed changes

src/Compilers/CSharp/Portable/Syntax/Syntax.xml Show resolved Hide resolved

Use switch expression

333bb83

jcouv reviewed May 28, 2019

View reviewed changes

gafter approved these changes May 28, 2019

View reviewed changes

Fixup keyword provider.

b93ad10

CyrusNajmabadi commented May 29, 2019

View reviewed changes

Simplify

9137605

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label May 31, 2019

CyrusNajmabadi added 2 commits June 7, 2019 10:30

Merge remote-tracking branch 'upstream/master' into namespaceMods

b2d86dc

Add asserts for global sttaements

d948c66

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jun 7, 2019

CyrusNajmabadi added 3 commits June 7, 2019 12:40

Add test.

265fb18

Test added.

9c6f229

Add test

3aa112d

Fix tests

46a8193

CyrusNajmabadi force-pushed the namespaceMods branch from 4c10cfe to 46a8193 Compare June 7, 2019 21:44

jcouv approved these changes Jun 8, 2019

View reviewed changes

jcouv merged commit 685348a into dotnet:master Jun 8, 2019

CyrusNajmabadi deleted the namespaceMods branch June 8, 2019 22:40

agocke mentioned this pull request Aug 6, 2019

Fix namespace incremental parsing bug #37770

Merged

		@@ -1,4 +1,18 @@
		REMOVEDstatic Microsoft.CodeAnalysis.CSharp.LanguageVersionFacts.TryParse(this string version, out Microsoft.CodeAnalysis.CSharp.LanguageVersion result) -> bool
		REMOVEDabstract Microsoft.CodeAnalysis.CSharp.Syntax.BaseFieldDeclarationSyntax.AttributeLists.get -> Microsoft.CodeAnalysis.SyntaxList<Microsoft.CodeAnalysis.CSharp.Syntax.AttributeListSyntax>

Conversation

CyrusNajmabadi commented Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Jan 31, 2019

Uh oh!

CyrusNajmabadi Jan 31, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CyrusNajmabadi Jan 31, 2019

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Jan 31, 2019

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Jan 31, 2019

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Jan 31, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

CyrusNajmabadi commented Feb 4, 2019

Uh oh!

CyrusNajmabadi commented Feb 18, 2019

Uh oh!

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

gafter left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented May 29, 2019

Uh oh!

CyrusNajmabadi May 29, 2019

Choose a reason for hiding this comment

Uh oh!

jcouv commented May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Jun 3, 2019

Uh oh!

CyrusNajmabadi commented Jun 7, 2019

Uh oh!

CyrusNajmabadi commented Jun 8, 2019

Uh oh!

CyrusNajmabadi commented Jun 8, 2019

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Jun 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CyrusNajmabadi commented Jan 31, 2019 •

edited

Loading

jcouv commented May 31, 2019 •

edited

Loading

jcouv commented Jun 3, 2019 •

edited

Loading