Change "data" to "record" by agocke · Pull Request #44841 · dotnet/roslyn

agocke · 2020-06-03T22:35:38Z

Resolves #44676

Adds a new "record" declaration kind and enables when parsing as C#
preview. If C# preview is not enabled, parsing is unchanged.

This PR builds on #44812. Only the last commit is relevant.

This PR takes out the parsing of primary constructors for classes and structs. In retrospect this may have been a mistake since it may have been more work dealing with the broken parsing than semantic errors and fix-ups, but there should be no bad effects aside from diagnostic quality.

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

gafter · 2020-06-03T23:09:05Z

Is there a preferred order for reviewing these 544 commits?

agocke · 2020-06-03T23:24:11Z

@gafter

This PR builds on #44812. Only the last commit is relevant.

agocke · 2020-06-03T23:24:57Z

I'll reset this PR shortly, when the other one is merged.

RikkiGibson · 2020-06-04T00:11:23Z

src/Compilers/CSharp/Test/Syntax/Parsing/RecordParsing.cs

Are there any tests that show the parsing "variants" inside namespaces and/or types? It would be nice if we also get a FeatureInPreview diagnostic there. I'm guessing parsing will just let it through and binding will fail to find a type named 'record'.

I feel like if it's not too difficult to achieve we should try to make sure the workflow of "install VS preview, pop open a C# file in my existing project, and just start using the preview feature" will work. Even something that just notices when a method that returns an ErrorType with the identifier 'record' and adds an additional FeatureInPreview diagnostic seems adequate.

Also, is there any chance that syntax nodes will be reused when changing the LangVersion in an open project? I'm guessing no because today you have to re-parse to get the right diagnostics, but figured it was worth checking.

It would certainly be useful to get the "FeatureInPreview" diagnostic but I'm going to say that's out-of-scope for this PR.

Also, is there any chance that syntax nodes will be reused when changing the LangVersion in an open project?

Nope, changing the parse options always requires reparsing every syntax tree from scratch.

AlekseyTs · 2020-06-04T04:47:34Z

@agocke Could you re-base this PR?

Adds a new "record" declaration kind and enables when parsing as C# preview. If C# preview is not enabled, parsing is unchanged.

agocke · 2020-06-04T05:04:54Z

Done

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

src/Compilers/CSharp/Portable/Symbols/Source/SourceConstructorSymbolBase.cs

src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordConstructor.cs

src/Compilers/CSharp/Portable/Symbols/Source/SourceConstructorSymbolBase.cs

jcouv · 2020-06-04T05:29:19Z

I'll get started on IDE integration starting from this PR. Not sure all the impact yet, but I expect significant.

AlekseyTs · 2020-06-04T15:46:28Z

    public static SyntaxKind GetTypeDeclarationKind(SyntaxKind kind)

Should this method be adjusted? #Closed

Refers to: src/Compilers/CSharp/Portable/Syntax/SyntaxKindFacts.cs:788 in fdb60ac. [](commit_id = fdb60ac, deletion_comment = False)

AlekseyTs · 2020-06-04T15:49:05Z

            // contextual keywords

Should this section be adjusted? #Closed

Refers to: src/Compilers/CSharp/Portable/Syntax/SyntaxKindFacts.cs:1585 in fdb60ac. [](commit_id = fdb60ac, deletion_comment = False)

AlekseyTs · 2020-06-04T15:51:04Z

        for (int i = (int)SyntaxKind.YieldKeyword; i <= (int)SyntaxKind.InitKeyword; i++)

Should this be adjusted?

Refers to: src/Compilers/CSharp/Portable/Syntax/SyntaxKindFacts.cs:1074 in fdb60ac. [](commit_id = fdb60ac, deletion_comment = False)

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

AlekseyTs · 2020-06-04T15:57:02Z

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

                case SyntaxKind.InterfaceDeclaration:
                case SyntaxKind.DelegateDeclaration:
                case SyntaxKind.EnumDeclaration:
+                case SyntaxKind.RecordDeclaration:


case SyntaxKind.RecordDeclaration: [](start = 16, length = 34)

Please make sure we have targeted tests for all modified APIs in this file.

I looked around and didn't find any tests for these APIs, it usually looks like we rely on the parsing tests to validate that contextual tokens are properly lexed. Do you want me to add new tests?

Do you want me to add new tests?

At least add tests for those that you missed and existing tests didn't catch the fact, or add a note to Test Plan to follow up

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

src/Compilers/CSharp/Test/Symbol/Symbols/Source/RecordTests.cs

src/Compilers/CSharp/Test/Syntax/Parsing/RecordParsing.cs

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

AlekseyTs · 2020-06-04T16:35:48Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceConstructorSymbolBase.cs

            DiagnosticBag diagnostics)
            : base(containingType, syntax.GetReference(), ImmutableArray.Create(location), SyntaxFacts.HasYieldOperations(syntax))
        {
-            Debug.Assert(syntax.IsKind(SyntaxKind.ConstructorDeclaration) || syntax.IsKind(SyntaxKind.ClassDeclaration) || syntax.IsKind(SyntaxKind.StructDeclaration));


SyntaxKind.ClassDeclaration [](start = 91, length = 27)

Please make sure that all the places in the compiler that check for SyntaxKind.ClassDeclaration or ClassDeclarationSyntax are properly adjusted to handle records as well. #Closed

I went through all of them and found a bunch more places. I don't want to delay this PR to add tests for all the places, so I added https://github.com/dotnet/roslyn/projects/57#card-39516963 to follow-up

RikkiGibson · 2020-06-04T16:55:23Z

release_64 failure appears to be a stack size regression.

AlekseyTs · 2020-06-04T16:58:12Z

Done with review pass (iteration 2) #Closed

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

jcouv · 2020-06-04T17:51:57Z

src/Compilers/CSharp/Portable/Symbols/NamespaceOrTypeSymbol.cs

            SyntaxKind kind,
            CSharpSyntaxNode syntax)
        {
-            Debug.Assert(


Why remove the assertion? #Closed

ToDeclarationKind one line below will fire an exception in all instances if the assert isn't followed, so this just feels like duplication #Closed

jcouv · 2020-06-04T17:54:19Z

src/Compilers/CSharp/Test/Symbol/Symbols/Source/RecordTests.cs

        }

-        [Fact]
+        [Fact(Skip = "record struct")]


nit: link to issue would be better. Or add note to test plan to unskip all tests.

jcouv · 2020-06-04T17:55:16Z

src/Compilers/CSharp/Test/Symbol/Symbols/Source/RecordTests.cs

+  IL_0016:  call       ""object..ctor()""
+  IL_001b:  ret
+}");
+        }


Let's keep a test for data class and maybe even data record (both errors)

jcouv · 2020-06-04T17:56:32Z

src/Compilers/CSharp/Test/Syntax/Parsing/RecordParsing.cs

        }

+        [Fact]
+        public void InvalidBase()


InvalidBase [](start = 20, length = 11)

nit: strange test name for this scenario

docs/compilers/CSharp/Compiler Breaking Changes - post VS2019.md

AlekseyTs

LGTM (iteration 7)

jcouv

LGTM Thanks (iteration 7)

jasonmalinowski

Signing off on the (trivial) IDE portion. Compiler portion not reviewed.

RikkiGibson · 2020-06-04T21:02:10Z

It appears that DeeplyNestedGeneric is still failing.

agocke · 2020-06-04T21:32:15Z

I'm gonna temporarily reduce the required depth here to get this in. We can fix it up as a bug.

ghost

Auto-approval

agocke requested review from a team as code owners June 3, 2020 22:35

agocke requested a review from a team June 3, 2020 22:35

agocke added the Area-Compilers label Jun 3, 2020

jaredpar reviewed Jun 3, 2020

View reviewed changes

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

RikkiGibson reviewed Jun 4, 2020

View reviewed changes

Change data class to "record"

53caa79

Adds a new "record" declaration kind and enables when parsing as C# preview. If C# preview is not enabled, parsing is unchanged.

agocke force-pushed the record-keyword branch from 519b4b0 to 53caa79 Compare June 4, 2020 04:59

AlekseyTs reviewed Jun 4, 2020

View reviewed changes

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

AlekseyTs reviewed Jun 4, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceConstructorSymbolBase.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Jun 4, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordConstructor.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Jun 4, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceConstructorSymbolBase.cs Outdated Show resolved Hide resolved

Respond to PR comments

fdb60ac

AlekseyTs reviewed Jun 4, 2020

View reviewed changes

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

AlekseyTs reviewed Jun 4, 2020

View reviewed changes

src/Compilers/CSharp/Test/Symbol/Symbols/Source/RecordTests.cs Show resolved Hide resolved

AlekseyTs reviewed Jun 4, 2020

View reviewed changes

src/Compilers/CSharp/Test/Syntax/Parsing/RecordParsing.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Jun 4, 2020

View reviewed changes

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

AlekseyTs reviewed Jun 4, 2020

View reviewed changes

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

AlekseyTs reviewed Jun 4, 2020

View reviewed changes

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

AlekseyTs reviewed Jun 4, 2020

View reviewed changes

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

AlekseyTs reviewed Jun 4, 2020

View reviewed changes

agocke added 3 commits June 4, 2020 10:32

Respond to PR comments

5739a4c

Add partial test

c4b63da

Check for record declaration in more places

854bb36

jcouv reviewed Jun 4, 2020

View reviewed changes

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

jcouv reviewed Jun 4, 2020

View reviewed changes

Add breaking change note to the post-2019 breaking changes doc

c912c90

jcouv reviewed Jun 4, 2020

View reviewed changes

AlekseyTs reviewed Jun 4, 2020

View reviewed changes

docs/compilers/CSharp/Compiler Breaking Changes - post VS2019.md Outdated Show resolved Hide resolved

Adjust breaking change scope

3b8cccc

AlekseyTs approved these changes Jun 4, 2020

View reviewed changes

jcouv self-assigned this Jun 4, 2020

jcouv approved these changes Jun 4, 2020

View reviewed changes

agocke added 2 commits June 4, 2020 11:26

Adjust tests from comments

0b767d7

Remove temp variable to pass stack test

4f7a417

jasonmalinowski approved these changes Jun 4, 2020

View reviewed changes

Temporarily decrease stack depth

722719b

agocke mentioned this pull request Jun 4, 2020

Fix stack increase required for parsing records #44863

Closed

agocke added the auto-merge label Jun 4, 2020

ghost approved these changes Jun 4, 2020

View reviewed changes

ghost merged commit 5e8c7ad into dotnet:features/records Jun 4, 2020

jcouv mentioned this pull request Feb 17, 2022

Produce specific error for misplaced 'record' keyword #59622

Merged

This pull request was closed.

Conversation

agocke commented Jun 3, 2020 • edited by RikkiGibson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

gafter commented Jun 3, 2020

Uh oh!

agocke commented Jun 3, 2020

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jun 4, 2020

Uh oh!

agocke commented Jun 4, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jcouv commented Jun 4, 2020

Uh oh!

AlekseyTs commented Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Jun 4, 2020

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlekseyTs Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson commented Jun 4, 2020

Uh oh!

AlekseyTs commented Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agocke Jun 4, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

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!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

agocke commented Jun 3, 2020 •

edited by RikkiGibson

Loading

agocke commented Jun 3, 2020 •

edited

Loading

AlekseyTs commented Jun 4, 2020 •

edited

Loading

AlekseyTs commented Jun 4, 2020 •

edited

Loading

AlekseyTs Jun 4, 2020 •

edited

Loading

AlekseyTs commented Jun 4, 2020 •

edited

Loading

jcouv Jun 4, 2020 •

edited

Loading

agocke Jun 4, 2020 •

edited by jcouv

Loading