Produce specific error for misplaced 'record' keyword #59622

jcouv · 2022-02-17T18:09:09Z

CyrusNajmabadi · 2022-02-17T19:09:52Z

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

+                Diagnostic(ErrorCode.ERR_MisplacedRecord, "record").WithLocation(1, 8),
+                // (1,16): error CS1514: { expected
+                // struct record C(int X, int Y);
+                Diagnostic(ErrorCode.ERR_LbraceExpected, "(").WithLocation(1, 16),


this is a pity. consider (if cheap) supporting this as a record, with misplaced struct. Once you see 'record' (even if it's errant) i think we should exercise the record-parsing path. #Closed

the primary reason for this is so that the tree doesn't go off the rails. that helps the entire experience.

I understand, but prefer to leave as-is.
It's not trivial because the keyword is struct (not record) and so we'd have to loosen assertions in constructTypeDeclaration(...) below which I'd rather not.
This will likely resolve when we allow parameter list on type declaration for primary constructors.

That works for me! Thanks :)

CyrusNajmabadi · 2022-02-17T19:12:00Z

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

+        {
+            var text = "class record C(int X, int Y);";
+            UsingTree(text, options: TestOptions.Regular10,
+                // (1,7): error CS9012: Unexpected keyword 'record'. Did you mean 'struct record' or 'class record'?


i'm a bit confused by the error message. It says Did you mean 'struct record' or 'class record'? but the user did write class record. #Resolved

D'oh! Fixed

Youssef1313 · 2022-02-17T21:47:41Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordStructTests.cs

+        [Fact]
+        public void StructNamedRecord()
+        {
+            var source = "struct record { } ";


struct record : IMyInterface? #Closed

Good catch. Thanks

jcouv · 2022-02-23T20:45:17Z

@333fred Could you take another look? We've updated the PR with Cyrus's commit to treat struct record and class record as record declarations (placing struct as leading trivia on record rather than record as trailing trivia on struct).

jcouv · 2022-02-24T17:34:05Z

@333fred @dotnet/roslyn-compiler for review. Thanks

333fred

Done review pass (commit 5)

333fred · 2022-02-24T18:12:08Z

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

@@ -15,6 +15,7 @@

 namespace Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax
 {
+    using System.Diagnostics.CodeAnalysis;


Top of the file? #Resolved

333fred · 2022-02-24T18:16:49Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordStructTests.cs

+        {
+            var source = @"
+interface I { }
+struct record<T> : I { }


Add a non-generic version? #Resolved

333fred · 2022-02-24T18:17:43Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordStructTests.cs

@@ -11159,5 +11159,86 @@ public void ExplicitConstructors_11()
                //     static S() : this() { }
                Diagnostic(ErrorCode.ERR_StaticConstructorWithExplicitConstructorCall, "this").WithArguments("S").WithLocation(4, 18));
        }
+
+        [Fact]
+        public void StructNamedRecord()


Probably want class record versions of these tests as well. #Resolved

333fred · 2022-02-24T18:18:16Z

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

-        [Fact, CompilerTrait(CompilerFeature.RecordStructs)]
-        public void RecordStructParsing_WrongOrder()
+        [Fact, CompilerTrait(CompilerFeature.RecordStructs), WorkItem(59534, "https://github.com/dotnet/roslyn/issues/59534")]
+        public void RecordStructParsing_WrongOrder_CSharp10()


Consider making these theories over struct vs class. #Resolved

I think we have sufficient coverage for class case now (added a few tests)

cston · 2022-02-28T21:51:16Z

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

@@ -3237,11 +3282,116 @@ public void RecordStructParsing_WrongOrder()
            EOF();
        }

+        [Fact, CompilerTrait(CompilerFeature.RecordStructs), WorkItem(59534, "https://github.com/dotnet/roslyn/issues/59534")]
+        public void StructNamedRecord_CSharp8()


StructNamedRecord_CSharp8

Consider merging these three into a single test, either with [Theory] or with a verify(ParseOptions) helper method, and add a similar test for class record { }.

We have an IDE PR waiting for this to be merged, so I'd rather avoid one more iteration to combine two tests. Thanks

jcouv self-assigned this Feb 17, 2022

dotnet-issue-labeler bot added the Area-Compilers label Feb 17, 2022

jcouv added the Feature - Records Records label Feb 17, 2022

jcouv force-pushed the struct-record branch from 7fa3212 to 7aae81d Compare February 17, 2022 18:40

Produce specific error for misplaced 'record' keyword

05f6d6e

jcouv force-pushed the struct-record branch from 7aae81d to 05f6d6e Compare February 17, 2022 18:56

jcouv marked this pull request as ready for review February 17, 2022 19:00

jcouv requested a review from a team as a code owner February 17, 2022 19:00

CyrusNajmabadi reviewed Feb 17, 2022

View reviewed changes

Fix diagnostic message

9a8d633

Youssef1313 reviewed Feb 17, 2022

View reviewed changes

Address feedback from Youssef

742673c

CyrusNajmabadi approved these changes Feb 18, 2022

View reviewed changes

333fred approved these changes Feb 18, 2022

View reviewed changes

Youssef1313 approved these changes Feb 18, 2022

View reviewed changes

Improve error recovery for transposed record keyword

e4066e8

jcouv closed this Feb 18, 2022

jcouv reopened this Feb 18, 2022

Merge pull request #3 from CyrusNajmabadi/struct-record-2

d8471cf

jcouv requested a review from 333fred February 18, 2022 19:00

CyrusNajmabadi mentioned this pull request Feb 18, 2022

Add feature to fixup code when user transposes 'record' in a declaration #59652

Merged

333fred reviewed Feb 24, 2022

View reviewed changes

jcouv added 2 commits February 26, 2022 18:01

Add tests

6fab98f

Move using

6cdcf32

jcouv requested a review from cston February 27, 2022 19:22

333fred approved these changes Feb 28, 2022

View reviewed changes

cston reviewed Feb 28, 2022

View reviewed changes

cston approved these changes Feb 28, 2022

View reviewed changes

jcouv enabled auto-merge (squash) March 1, 2022 01:57

RikkiGibson disabled auto-merge March 1, 2022 18:07

RikkiGibson merged commit 44956a2 into dotnet:main Mar 1, 2022

ghost added this to the Next milestone Mar 1, 2022

jcouv deleted the struct-record branch March 1, 2022 18:16

runfoapp bot mentioned this pull request Mar 1, 2022

roslyn.visualstudio.integrationtests.visualbasic.basiclinecommit.commitonsave #59855

Closed

allisonchou modified the milestones: Next, 17.2.P3 Mar 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Produce specific error for misplaced 'record' keyword #59622

Produce specific error for misplaced 'record' keyword #59622

jcouv commented Feb 17, 2022 •

edited

Loading

CyrusNajmabadi Feb 17, 2022 •

edited by jcouv

Loading

CyrusNajmabadi Feb 17, 2022

jcouv Feb 18, 2022

CyrusNajmabadi Feb 18, 2022

CyrusNajmabadi Feb 17, 2022 •

edited by jcouv

Loading

jcouv Feb 17, 2022

Youssef1313 Feb 17, 2022 •

edited by jcouv

Loading

jcouv Feb 18, 2022

jcouv commented Feb 23, 2022

jcouv commented Feb 24, 2022

333fred left a comment

333fred Feb 24, 2022 •

edited by jcouv

Loading

333fred Feb 24, 2022 •

edited by jcouv

Loading

333fred Feb 24, 2022 •

edited by jcouv

Loading

333fred Feb 24, 2022 •

edited by jcouv

Loading

jcouv Feb 27, 2022

cston Feb 28, 2022 •

edited

Loading

jcouv Mar 1, 2022

Produce specific error for misplaced 'record' keyword #59622

Produce specific error for misplaced 'record' keyword #59622

Conversation

jcouv commented Feb 17, 2022 • edited Loading

CyrusNajmabadi Feb 17, 2022 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Feb 17, 2022

Choose a reason for hiding this comment

jcouv Feb 18, 2022

Choose a reason for hiding this comment

CyrusNajmabadi Feb 18, 2022

Choose a reason for hiding this comment

CyrusNajmabadi Feb 17, 2022 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Feb 17, 2022

Choose a reason for hiding this comment

Youssef1313 Feb 17, 2022 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Feb 18, 2022

Choose a reason for hiding this comment

jcouv commented Feb 23, 2022

jcouv commented Feb 24, 2022

333fred left a comment

Choose a reason for hiding this comment

333fred Feb 24, 2022 • edited by jcouv Loading

Choose a reason for hiding this comment

333fred Feb 24, 2022 • edited by jcouv Loading

Choose a reason for hiding this comment

333fred Feb 24, 2022 • edited by jcouv Loading

Choose a reason for hiding this comment

333fred Feb 24, 2022 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Feb 27, 2022

Choose a reason for hiding this comment

cston Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

jcouv Mar 1, 2022

Choose a reason for hiding this comment

jcouv commented Feb 17, 2022 •

edited

Loading

CyrusNajmabadi Feb 17, 2022 •

edited by jcouv

Loading

CyrusNajmabadi Feb 17, 2022 •

edited by jcouv

Loading

Youssef1313 Feb 17, 2022 •

edited by jcouv

Loading

333fred Feb 24, 2022 •

edited by jcouv

Loading

333fred Feb 24, 2022 •

edited by jcouv

Loading

333fred Feb 24, 2022 •

edited by jcouv

Loading

333fred Feb 24, 2022 •

edited by jcouv

Loading

cston Feb 28, 2022 •

edited

Loading