Parse `field` as a contextual keyword #73947

cston · 2024-06-11T22:08:37Z

Spec change: dotnet/csharplang#8239, dotnet/csharplang#8283

CyrusNajmabadi · 2024-06-12T00:30:59Z

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

+        /// <summary>
+        /// If an accessor kind changes, "field" and "value" within the accessor may need to be reinterpreted.
+        /// </summary>
+        internal bool IsInValueKeywordContext;


you might want to mention how this differs from the above. #Resolved

CyrusNajmabadi · 2024-06-12T00:32:44Z

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

-            // Check if we have an initializer
-            else if (this.CurrentToken.Kind == SyntaxKind.EqualsToken)
+            // PROTOTYPE: SyntaxKind.GetAccessorDeclaration does seem correct for an initializer.
+            // PROTOTYPE: Confirm that field should be a keyword, and bind to the backing field, in a property initializer: object P { get; } = field;


this is interesting case for sure. i actually have no idea what should happen here. Property intitializers can't actually reference this or fields. But if we do not have this bind to the field keyword, this could then bind to somethgin higher... and i have no idea if we want that :) #Resolved

Removed this case, assuming we will decide field is not a keyword (and should not bind to the backing field) in the property initializer.

CyrusNajmabadi · 2024-06-12T00:33:49Z

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

+        private readonly struct FieldAndValueKeywordContext : IDisposable
+        {
+            private readonly LanguageParser _parser;
+            private readonly (bool, bool) _isInFieldAndValueKeywordContext;


maybe _originalXXX to indicate it was the starting value. #Resolved

nit: separate fields just seems simpler :)

CyrusNajmabadi · 2024-06-12T00:34:37Z

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

@@ -4191,6 +4221,9 @@ private AccessorDeclarationSyntax ParseAccessorDeclaration(bool isEvent)
                accessorName = ConvertToKeyword(accessorName);
            }

+            // PROTOTYPE: How should ‘field’ and ‘value’ be interpreted in the attributes, modifiers? Those parts have already been parsed.


we already decided this in ldm, but the answer, afair, is: they are not in scope there. #Resolved

CyrusNajmabadi · 2024-06-12T00:34:50Z

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

@@ -4266,6 +4299,20 @@ private static SyntaxKind GetAccessorKind(SyntaxToken accessorName)
            };
        }

+        private static (bool, bool) GetFieldAndValueKeywordContext(AccessorDeclaringKind declaringKind, SyntaxKind accessorKind)


name these bools. #Resolved

CyrusNajmabadi · 2024-06-12T00:34:58Z

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

+        {
+            return accessorKind switch
+            {
+                SyntaxKind.GetAccessorDeclaration => (declaringKind == AccessorDeclaringKind.Property, false),


named args for the literals. #Resolved

CyrusNajmabadi · 2024-06-12T00:36:39Z

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

@@ -5108,7 +5155,7 @@ private static bool CanReuseVariableDeclarator(CSharp.Syntax.VariableDeclaratorS
            // declaration.  However, in the specific case of variable declarators, Dev10
            // specifically treats it as a variable name, even if it could be interpreted as a
            // keyword.
-            var name = this.ParseIdentifierToken();
+            var name = this.ParseIdentifierToken(reportErrorRatherThanConvertingFieldOrValue: true);


for somethign as core as ParseIdentifierToken, it feels very strange to need such a specialized arg. esp. since we haven't needed that sort of arg for other contextual tokens? #Resolved

CyrusNajmabadi · 2024-06-12T00:36:45Z

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

@@ -5439,7 +5486,7 @@ private EnumMemberDeclarationSyntax ParseEnumMemberDeclaration()
                equalsValue = _syntaxFactory.EqualsValueClause(
                    this.EatToken(SyntaxKind.EqualsToken),
                    this.CurrentToken.Kind is SyntaxKind.CommaToken or SyntaxKind.CloseBraceToken
-                        ? this.ParseIdentifierName(ErrorCode.ERR_ConstantExpected)
+                        ? this.ParseIdentifierName(null, ErrorCode.ERR_ConstantExpected)


named arg for literal. #Resolved

CyrusNajmabadi · 2024-06-12T00:38:09Z

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

                SyntaxToken identifierToken = this.EatToken();

                if (this.IsInAsync && identifierToken.ContextualKind == SyntaxKind.AwaitKeyword)
                {
                    identifierToken = this.AddError(identifierToken, ErrorCode.ERR_BadAwaitAsIdentifier);
                }
+                else if (isFieldOrValueInKeywordContext) // PROTOTYPE: Are we testing this with "value"?
+                {
+                    if (reportErrorRatherThanConvertingFieldOrValue.GetValueOrDefault())


not gettnig why we need this bool?. seems out of sorts with how we genrally do contexts and parsing of identifiers/contextual-keywords. #Resolved

CyrusNajmabadi · 2024-06-12T00:39:01Z

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

@@ -8562,7 +8641,7 @@ private bool IsPossibleStatement(bool acceptAccessibilityMods)
                    return true;

                case SyntaxKind.IdentifierToken:
-                    return IsTrueIdentifier();
+                    return IsTrueIdentifier() || IsCurrentTokenFieldOrValueInKeywordContext();


this logic is what IsTrueIdentifier is supposed to already check. #Resolved

CyrusNajmabadi · 2024-06-12T00:39:43Z

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

@@ -11624,11 +11718,11 @@ private ArgumentSyntax ParseArgumentExpression(bool isIndexer)

            if (isIndexer && this.CurrentToken.Kind is SyntaxKind.CommaToken or SyntaxKind.CloseBracketToken)
            {
-                expression = this.ParseIdentifierName(ErrorCode.ERR_ValueExpected);
+                expression = this.ParseIdentifierName(null, ErrorCode.ERR_ValueExpected);


named parametres. #Resolved

CyrusNajmabadi · 2024-06-12T00:41:39Z

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

@@ -13722,17 +13834,26 @@ private new struct ResetPoint
            internal readonly TerminatorState TerminatorState;
            internal readonly bool IsInAsync;
            internal readonly bool IsInQuery;
+            // PROTOTYPE: Why do we need this? We don't need something similar for 'partial'


we need this because partial can look locally to figure out what it means. it is not affected by outer contexts. that's not the case for async/queries/field/value. field/value shuld be treated very closely to async/await. An outer context pushes us into a context where those tokens now become keywords.

fortunately field/value is also simpler because we never change the value as we go deeper into the accessor.

CyrusNajmabadi · 2024-06-12T00:43:24Z

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

+    </TypeComment>
+    <FactoryComment>
+      <summary>Creates a ValueExpressionSyntax node.</summary>
+    </FactoryComment>


Alternatively, we could use LiteralExpressionSyntax, just like we use that for boolean expressions with true and false keyword. but i'm not sure how compiler would feel about fallout of these being thought of as literals, so i think it's totally ok to not do that. #Resolved

Will leave as is.

CyrusNajmabadi · 2024-06-12T00:44:47Z

src/Compilers/Core/Portable/Syntax/GreenNode.cs

@@ -286,6 +286,9 @@ internal enum NodeFlags : ushort
            ContainsStructuredTrivia = 1 << 9,

            InheritMask = IsNotMissing | ContainsAnnotations | ContainsAttributes | ContainsDiagnostics | ContainsDirectives | ContainsSkippedText | ContainsStructuredTrivia,
+
+            FactoryContextIsInFieldKeywordContext = 1 << 10,
+            FactoryContextIsInValueKeywordContext = 1 << 11,


i would move these up next to the other FactoryContext values. #Resolved

note: this also makes us out of bits again.

CyrusNajmabadi · 2024-06-12T00:46:49Z

src/Compilers/Core/Portable/Syntax/GreenNode.cs

+            }
+        }
+
+        internal bool ParsedInValueKeywordContext


where is the code for this checked. there should be some point that is checking this, and thus ensuring that if we do an incremental parse, where a statement used to be in a property, and it moves to be in a method, that the meaning of field/value go back to being an identifier (and vice versa).

note: this is not a strange scenario to be in. people convert properties to methods and back. So incremental parsing def needs to be good here.

Is this property necessary? To switch the context between a property accessor and a method would require changing the containing member definition kind which I suspect invalidates the body of property, accessor, or method.

CyrusNajmabadi · 2024-06-12T00:47:20Z

done with initial pass.

333fred

I've looked through most of the tests, but not FieldKeywordTests.cs yet. Will get to that later.

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

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs

src/Compilers/CSharp/Test/Emit3/FieldKeywordTests.cs

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

RikkiGibson

Looks good but haven't gotten through tests yet.

RikkiGibson · 2024-07-31T22:50:54Z

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs

+        {
+            string name = identifier.Text;
+            var requiredVersion = MessageID.IDS_FeatureFieldKeyword.RequiredVersion();
+            diagnostics.Add(ErrorCode.INF_IdentifierConflictWithContextualKeyword, syntax, name, requiredVersion.ToDisplayString());


Is this an open issue that we are tracking resolving in a future PR?

RikkiGibson · 2024-07-31T23:14:37Z

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

                    {
                        accessorsHaveImplementation = true;
                    }
+
+                    usesFieldKeyword = usesFieldKeyword || containsFieldKeyword(accessor);


The other usage of this local function passes the body as an argument, should we do the same here? #Resolved

The reason for passing accessor rather than body is to include attributes from the accessor.

RikkiGibson · 2024-07-31T23:25:29Z

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

-                                                                      isReadOnly: (hasGetAccessor && !hasSetAccessor) || isInitOnly,
+                                                                      // Synthesized backing field for 'field' should not be marked 'initonly'
+                                                                      // since the field might be modified in the get accessor.
+                                                                      isReadOnly: !usesFieldKeyword && ((hasGetAccessor && !hasSetAccessor) || isInitOnly),


I'm not sure I follow this logic.. what if we are in a readonly property or a property on a readonly struct? But it seems like this would give a non-readonly field in that case.

I would expect to be able to use the field keyword in such cases, but it would need to refer to a readonly field. You would be able to write to the field in an init accessor, or a property initializer, or by assigning to the property in the constructor.

Should the synthesized field of a readonly property be readonly, or should the field be mutable, regardless? Should this be an open question for language design?

class C { object P { get { field ??= ""; // ok? return field; } } }

The field should be writable except when the accessor is readonly in the "readonly modifier" sense. i.e. the get-only case you have shown here should be writable.

Added a PROTOTYPE comment for now.

RikkiGibson · 2024-07-31T23:26:55Z

src/Compilers/CSharp/Portable/Syntax/InternalSyntax/CSharpSyntaxNode.cs

@@ -230,21 +230,11 @@ protected void SetFactoryContext(SyntaxFactoryContext context)
            {
                SetFlags(NodeFlags.FactoryContextIsInQuery);
            }
-        }
-
-        internal static NodeFlags SetFactoryContext(NodeFlags flags, SyntaxFactoryContext context)


Was this just dead code? #Resolved

RikkiGibson · 2024-07-31T23:28:25Z

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

+
+            static bool containsFieldKeyword(SyntaxNode syntax)
+            {
+                return syntax.DescendantTokens().Any(static t => t.Kind() == SyntaxKind.FieldKeyword);


Does this traversal avoid creating the red tree? #Resolved

No, but haven't we already created the red tree for the property simply by referencing the root SyntaxNode?

no. walking down the red tree causes the red allocations. ideally this walk is done on the green tree to avoid actual allocs.

RikkiGibson · 2024-07-31T23:43:38Z

src/Compilers/CSharp/Test/Emit3/FieldKeywordTests.cs

+                    }
+                }
+                """;
+            var verifier = CompileAndVerify(source, expectedOutput: """


It will be good to also verify contents of compilation.GetMember("C").GetMembers(). I would expect that because other backing fields are present there that this would be too.

No need to do this across all the tests, just one or two places would probably be fine. #Resolved

RikkiGibson · 2024-07-31T23:49:30Z

src/Compilers/CSharp/Test/Emit3/FieldKeywordTests.cs

+            comp.VerifyEmitDiagnostics(
+                // (5,22): info CS9258: 'field' is a contextual keyword in property accessors starting in language version preview. Use '@field' instead.
+                //         get { return field; }
+                Diagnostic(ErrorCode.INF_IdentifierConflictWithContextualKeyword, "field").WithArguments("field", "preview").WithLocation(5, 22),


I am confused by these INF diagnostics. If we are already in LangVersion preview I wouldn't expect them to be reported. These will show up in the IDE error list if someone is just using the feature normally and intentionally. Is this something we're planning on fixing later?

I believe we want to report this info-level diagnostic always, even with language version preview, to allow users to discover where there may be a breaking change. But perhaps we should only report the diagnostic when field would otherwise bind to a member other than the synthesized field. cc @jaredpar

I think reporting it in all LangVersions made sense when we were just shipping the diagnostic in order to grasp the severity of the potential break. I don't think it makes sense to report in the case that the feature is being used normally and there is no symbol field in scope.

I have a lot more thoughts here, but basically, I think there is a need to design this experience more deeply/holistically, thinking about what should happen in various combinations of LangVersion, and the kind of declaration which is named field, and the way the field identifier/keyword is being used.

Added a PROTOTYPE comment to Binder.ReportFieldContextualKeywordConflict.

RikkiGibson · 2024-07-31T23:52:55Z

src/Compilers/CSharp/Test/Emit3/FieldKeywordTests.cs

+        }
+
+        [Fact]
+        public void FieldInInitializer_03()


tests for case when accessor uses field and property has an initializer? #Resolved

RikkiGibson · 2024-07-31T23:54:20Z

src/Compilers/CSharp/Test/Emit3/FieldKeywordTests.cs

+        }
+
+        [Fact]
+        public void Attribute_02()


also tests for when property or accessor attribute uses [field: ...]? I would expect these to behave the same as they do on auto-properties today.

I've added an item to the test plan to verify attributes can be attached to the synthesized field using [field: ...], and I'll handle that in a subsequent PR.

RikkiGibson · 2024-08-01T00:00:40Z

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

+                        N(SyntaxKind.EqualsValueClause);
+                        {
+                            N(SyntaxKind.EqualsToken);
+                            IdentifierNameOrFieldExpression(languageVersion, escapeIdentifier: false, expectedParsedAsToken);


It feels like it would be more straightforward to just use the generated baseline when we expect it to be the same in all the cases of the theory. #Resolved

RikkiGibson · 2024-08-01T00:18:59Z

src/Compilers/CSharp/Test/Syntax/Syntax/FieldAndValueKeywordTests.cs

@@ -561,6 +582,7 @@ class C
            [CombinatorialValues(LanguageVersion.CSharp12, LanguageVersion.Preview)] LanguageVersion languageVersion)
        {
            string source = """
+                #pragma warning disable 168


Please include something like the message of the diagnostic which is being disabled or its name in ErrorCode to hint at what is going on here. #Resolved

CyrusNajmabadi · 2024-08-01T15:26:26Z

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

@@ -3876,7 +3879,32 @@ private SyntaxToken TryEatCheckedOrHandleUnchecked(ref SyntaxToken operatorKeywo
                semicolon);
        }

-        private AccessorListSyntax ParseAccessorList(bool isEvent)
+        private readonly struct FieldKeywordContext : IDisposable


could make a ref-struct, so this doesn't get put on the heap accidentally. #Resolved

Field and value as contextual keywords

9afc7e1

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 11, 2024

Fix bootstrap build

62bdd75

CyrusNajmabadi reviewed Jun 12, 2024

View reviewed changes

Fix tests

538db7e

cston force-pushed the field-value-keywords branch from 393e476 to 538db7e Compare June 12, 2024 23:57

cston added 8 commits June 13, 2024 16:23

Additional test

55da141

Merge remote-tracking branch 'upstream/main' into field-value-keywords

96b23b5

Fix tests

dee9526

Do not treat field as keyword in property initializer

4d227d3

Address feedback

0ede824

Merge remote-tracking branch 'upstream/main' into field-value-keywords

5f40ed7

Address feedback

afcbacb

Remove event support

4919f39

cston requested review from a team as code owners July 29, 2024 19:48

333fred reviewed Jul 29, 2024

View reviewed changes

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

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs Show resolved Hide resolved

cston changed the base branch from main to features/field-keyword July 30, 2024 17:09

Simplify FieldExpression case

2b068b2

333fred reviewed Jul 30, 2024

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs Show resolved Hide resolved

333fred reviewed Jul 30, 2024

View reviewed changes

Address feedback

7ed1c3d

333fred approved these changes Jul 30, 2024

View reviewed changes

cston requested review from a team and RikkiGibson July 30, 2024 20:03

RikkiGibson self-assigned this Jul 31, 2024

RikkiGibson reviewed Jul 31, 2024

View reviewed changes

RikkiGibson approved these changes Aug 1, 2024

View reviewed changes

cston added 3 commits July 31, 2024 22:42

Traverse green nodes

219d868

Simplify names

b4717dc

Update tests

74f6f88

CyrusNajmabadi reviewed Aug 1, 2024

View reviewed changes

333fred approved these changes Aug 1, 2024

View reviewed changes

Address feedback

247c82c

RikkiGibson approved these changes Aug 1, 2024

View reviewed changes

Add PROTOTYPE comments

1858006

cston merged commit a55ed9c into dotnet:features/field-keyword Aug 1, 2024
28 checks passed

cston deleted the field-value-keywords branch August 1, 2024 20:17

jcouv added the Feature - Field Keyword label Aug 27, 2024

cston mentioned this pull request Aug 29, 2024

[API proposal] Syntax changes for field keyword #74937

Closed

Parse field as a contextual keyword #73947

Parse field as a contextual keyword #73947

Conversation

cston commented Jun 11, 2024 • edited Loading

CyrusNajmabadi Jun 12, 2024 • edited by cston Loading

Choose a reason for hiding this comment

CyrusNajmabadi Jun 12, 2024 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Jun 12, 2024 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Jun 12, 2024 • edited by cston Loading

Choose a reason for hiding this comment

CyrusNajmabadi Jun 12, 2024 • edited by cston Loading

Choose a reason for hiding this comment

CyrusNajmabadi Jun 12, 2024 • edited by cston Loading

Choose a reason for hiding this comment

CyrusNajmabadi Jun 12, 2024 • edited by cston Loading

Choose a reason for hiding this comment

CyrusNajmabadi Jun 12, 2024 • edited by cston Loading

Choose a reason for hiding this comment

CyrusNajmabadi Jun 12, 2024 • edited by cston Loading

Choose a reason for hiding this comment

CyrusNajmabadi Jun 12, 2024 • edited by cston Loading

Choose a reason for hiding this comment

CyrusNajmabadi Jun 12, 2024 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Jun 12, 2024 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Jun 12, 2024 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jun 12, 2024

333fred left a comment

Choose a reason for hiding this comment

RikkiGibson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Jul 31, 2024 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Jul 31, 2024 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Jul 31, 2024 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Jul 31, 2024 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Jul 31, 2024 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Aug 1, 2024 • edited by cston Loading

Choose a reason for hiding this comment

RikkiGibson Aug 1, 2024 • edited by cston Loading

Choose a reason for hiding this comment

CyrusNajmabadi Aug 1, 2024 • edited by cston Loading

Choose a reason for hiding this comment

Parse `field` as a contextual keyword #73947

Parse `field` as a contextual keyword #73947

cston commented Jun 11, 2024 •

edited

Loading

CyrusNajmabadi Jun 12, 2024 •

edited by cston

Loading

CyrusNajmabadi Jun 12, 2024 •

edited by cston

Loading

CyrusNajmabadi Jun 12, 2024 •

edited by cston

Loading

CyrusNajmabadi Jun 12, 2024 •

edited by cston

Loading

CyrusNajmabadi Jun 12, 2024 •

edited by cston

Loading

CyrusNajmabadi Jun 12, 2024 •

edited by cston

Loading

CyrusNajmabadi Jun 12, 2024 •

edited by cston

Loading

CyrusNajmabadi Jun 12, 2024 •

edited by cston

Loading

CyrusNajmabadi Jun 12, 2024 •

edited by cston

Loading

CyrusNajmabadi Jun 12, 2024 •

edited by cston

Loading

CyrusNajmabadi Jun 12, 2024 •

edited by cston

Loading

CyrusNajmabadi Jun 12, 2024 •

edited by cston

Loading

CyrusNajmabadi Jun 12, 2024 •

edited by cston

Loading

RikkiGibson Jul 31, 2024 •

edited by cston

Loading

cston Aug 1, 2024 •

edited

Loading

RikkiGibson Jul 31, 2024 •

edited by cston

Loading

RikkiGibson Jul 31, 2024 •

edited by cston

Loading

RikkiGibson Jul 31, 2024 •

edited by cston

Loading

RikkiGibson Jul 31, 2024 •

edited by cston

Loading

RikkiGibson Aug 1, 2024 •

edited by cston

Loading

RikkiGibson Aug 1, 2024 •

edited by cston

Loading

CyrusNajmabadi Aug 1, 2024 •

edited by cston

Loading