Add support for init-only members #43275

jcouv · 2020-04-10T21:13:28Z

Part of records feature:

Champion "Records" (VS 16.8, .NET 5) csharplang#39 (championed issue)
Test plan for "records" #40726 (test plan for records, including init-only setters)
Speclet: https://github.com/dotnet/csharplang/blob/master/proposals/init.md

The first commit only has a refactoring (splitting and flattening very long/nested method).

A couple of design points that I expect might be need discussion:

the syntax model adds a new kind of accessor kind. I would have preferred a set accessor just with a different keyword, but I don't think we can do that.

Note: the first commit is a refactoring to one file.

jcouv · 2020-04-13T19:03:43Z

Will update this PR based on today's LDM decisions. #Closed

jcouv · 2020-04-17T17:26:02Z

@dotnet/roslyn-compiler This PR is open for review.

I've updated this PR with latest decisions from LDM and API design meeting:

no init-only fields
init-only properties can update readonly fields under narrow conditions
metadata uses a modreq on return type of accessor, but no attribute #Closed

AlekseyTs · 2020-04-19T21:03:25Z

src/Compilers/Core/Portable/MetadataReader/MetadataDecoder.cs

@@ -717,24 +717,32 @@ private TypeSymbol GetTypeOfTypeDef(TypeDefinitionHandle typeDef, out bool isNoP
                {
                    var isAllowed = false;

-                    switch (allowedRequiredModifierType)
+                    if ((allowedRequiredModifierType & AllowedRequiredModifierType.System_Runtime_InteropServices_InAttribute) != 0 &&


if ((allowedRequiredModifierType & AllowedRequiredModifierType.System_Runtime_InteropServices_InAttribute) != 0 && [](start = 20, length = 114)

It looks like these changes are going to conflict with changes made in #43355. Consider coordinating with @333fred to make changes in both PRs in a manner that would minimize the churn during merge #Closed

Fred, this change allows for looking for more than one modreq at a certain position. Would you mind taking this change into your branch to minimize merge conflicts?

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

AlekseyTs · 2020-04-19T21:14:12Z

src/Compilers/Core/Portable/MetadataReader/MetadataDecoder.cs

@@ -1178,17 +1186,18 @@ private void DecodeParameterOrThrow(ref BlobReader signatureReader, /*out*/ ref
        {
            info.CustomModifiers = DecodeModifiersOrThrow(
                ref signatureReader,
-                AllowedRequiredModifierType.System_Runtime_InteropServices_InAttribute,
+                AllowedRequiredModifierType.System_Runtime_InteropServices_InAttribute |
+                    AllowedRequiredModifierType.System_Runtime_CompilerServices_IsInitOnly,


AllowedRequiredModifierType.System_Runtime_CompilerServices_IsInitOnly [](start = 20, length = 70)

Allowing this on any parameter doesn't feel right. #Closed

AlekseyTs · 2020-04-19T21:18:49Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -1592,6 +1595,9 @@ If such a class is used as a base class and if the deriving class defines a dest
  <data name="ERR_ExplicitPropertyAddingAccessor" xml:space="preserve">
    <value>'{0}' adds an accessor not found in interface member '{1}'</value>
  </data>
+  <data name="ERR_ExplicitPropertyMismatchInitOnly" xml:space="preserve">
+    <value>Accessors '{0}' and '{1}' don't match by init-only</value>


don't match by init-only [](start = 37, length = 24)

Consider rewording for clarity. #Closed

AlekseyTs · 2020-04-19T21:19:49Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -2891,7 +2897,7 @@ A catch() block after a catch (System.Exception e) block can catch non-CLS excep
    <value>Members of readonly field '{0}' cannot be used as a ref or out value (except in a constructor)</value>
  </data>
  <data name="ERR_AssgReadonly" xml:space="preserve">
-    <value>A readonly field cannot be assigned to (except in a constructor of the class in which the field is defined or a variable initializer))</value>
+    <value>A readonly field cannot be assigned to (except in a constructor or init-only member of the class in which the field is defined or a variable initializer))</value>


init-only member [](start = 78, length = 16)

Is there a concept of an "init-only member"? #Closed

AlekseyTs · 2020-04-19T21:22:01Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -6067,4 +6079,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
  <data name="WRN_GeneratorFailedDuringInitialization_Title" xml:space="preserve">
    <value>Generator failed to initialize.</value>
  </data>
+  <data name="ERR_AssignmentInitOnly" xml:space="preserve">
+    <value>Init-only member '{0}' can only be assigned from a constructor, object initialization or 'with' expression of that type.</value>


from [](start = 55, length = 4)

"in" vs. "from"? #Closed

AlekseyTs · 2020-04-19T21:27:45Z

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

@@ -200,6 +200,7 @@ public enum SyntaxKind : ushort
        WhenKeyword = 8437,
        DataKeyword = 8438,
        WithKeyword = 8439,
+        InitKeyword = 8440,
        /// when adding a contextual keyword following functions must be adapted:
        /// <see cref="SyntaxFacts.GetContextualKeywordKinds"/>
        /// <see cref="SyntaxFacts.IsContextualKeyword(SyntaxKind)"/>


/// [](start = 8, length = 61)

Consider also adding GetContextualKeywordKind to the list #Closed

AlekseyTs · 2020-04-28T21:20:16Z

        comp.VerifyDiagnostics();

I'm not sure what you're suggesting to change. I already have a PROTOTYPE for this problem on the line below (decoding is not restrictive enough)

I am suggesting to add a prototype comment, explicitly stating that we expect an error on the C.M(); line.

In reply to: 620859393 [](ancestors = 620859393,620753699)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/InitOnlyMemberTests.cs:2862 in 7cce780. [](commit_id = 7cce780, deletion_comment = False)

jcouv · 2020-04-28T21:23:37Z

        // PROTOTYPE(init-only): can we make this more restrictive (ie. disallow aside from the return value of an instance setter)?

FWIW, the reason why it's a question in my mind is because the decoding of the accessors is done before we decide that they are accessors.

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/InitOnlyMemberTests.cs:3128 in 1d77553. [](commit_id = 1d77553, deletion_comment = False)

AlekseyTs · 2020-04-28T21:28:42Z

        // PROTOTYPE(init-only): can we make this more restrictive (ie. disallow aside from the return value of an instance setter)?

FWIW, the reason why it's a question in my mind is because the decoding of the accessors is done before we decide that they are accessors.

I can understand that you have a question about how to do that, i.e. how to implement. However, there is no question what the behavior should be, i.e. what the expectations are. And the expectations are that we should be more restrictive. No question.

In reply to: 620863035 [](ancestors = 620863035,620760072)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/InitOnlyMemberTests.cs:3128 in 7cce780. [](commit_id = 7cce780, deletion_comment = False)

AlekseyTs · 2020-04-28T21:29:46Z

        // PROTOTYPE(init-only): can we make this more restrictive (ie. disallow aside from the return value of an instance setter)?

And we also certainly can do that. Again, no question.

In reply to: 620865345 [](ancestors = 620865345,620863035,620760072)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/InitOnlyMemberTests.cs:3128 in 7cce780. [](commit_id = 7cce780, deletion_comment = False)

AlekseyTs · 2020-04-29T17:36:00Z

src/Compilers/Core/Portable/MetadataReader/MetadataDecoder.cs


            if (typeCode == SignatureTypeCode.ByReference)
            {
                info.IsByRef = true;
+
+                // PROTOTYPE(init-only): we're incorrectly letting modreq(IsExternalInit) into the RefCustomModifiers


// PROTOTYPE(init-only): we're incorrectly letting modreq(IsExternalInit) into the RefCustomModifiers [](start = 16, length = 101)

It looks like this comment is obsolete, the if at the end of this block takes care of the issue. BTW, consider moving that if here. #Resolved

I'll remove the comment. Thanks
It's a nit, but the reason the if/throw is at the end of this block is that the decoded modreq will get stored rather than lost.

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

the reason the if/throw is at the end of this block is that the decoded modreq will get stored rather than lost.

This isn't really useful. I prefer us to follow consistent handling of unsupported modereqs within this type - throw as soon as we recognize the fact

In reply to: 417569537 [](ancestors = 417569537,417491981)

It might make sense to do that after the assignmentinfo.RefCustomModifiers = info.CustomModifiers, but it is really not worth trying to decode the next set of modifiers because we aren't even going to try decoding the type of the parameter.

In reply to: 417585593 [](ancestors = 417585593,417569537,417491981)

AlekseyTs · 2020-04-29T17:43:49Z

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

+                    return false;
+                }
+
+                if ((!method.IsStatic && method.IsConstructor()) || method.IsInitOnly)


(!method.IsStatic && method.IsConstructor()) [](start = 20, length = 44)

Why so complicated? Check out how IsConstructor is implemented. method.MethodKind == MethodKind.Constructor? #Closed

AlekseyTs · 2020-04-29T17:47:46Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Members.cs

@@ -160,13 +161,21 @@ public override void VisitProperty(IPropertySymbol symbol)
                AddPunctuation(SyntaxKind.OpenBraceToken);

                AddAccessor(symbol, symbol.GetMethod, SyntaxKind.GetKeyword);
-                AddAccessor(symbol, symbol.SetMethod, SyntaxKind.SetKeyword);
+                // PROTOTYPE(init-only): adjust SymbolDisplayVisitor once we have a public IsInitOnly API


// PROTOTYPE(init-only): adjust SymbolDisplayVisitor once we have a public IsInitOnly API [](start = 16, length = 89)

Consider moving the comment into the IsInitOnly helper below, this is where the change should happen. #Closed

AlekseyTs · 2020-04-29T17:48:45Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Members.cs

@@ -190,7 +199,7 @@ private void AddPropertyNameAndParameters(IPropertySymbol symbol)
                this.builder.Add(CreatePart(SymbolDisplayPartKind.PropertyName, symbol, symbol.Name));
            }

-            if (this.format.MemberOptions.IncludesOption(SymbolDisplayMemberOptions.IncludeParameters) && symbol.Parameters.Any())
+            if (this.format.MemberOptions.IncludesOption(SymbolDisplayMemberOptions.IncludeParameters) && symbol.Parameters.Any<IParameterSymbol>())


[](start = 127, length = 18)

This looks like an unintentional change. #Closed

AlekseyTs · 2020-04-29T17:51:52Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Members.cs


                AddSpace();
                AddPunctuation(SyntaxKind.CloseBraceToken);
            }
        }

+        private static bool IsInitOnly(IPropertySymbol symbol)
+        {
+            return (symbol.SetMethod as Symbols.PublicModel.MethodSymbol)?.UnderlyingMethodSymbol.IsInitOnly == true;


symbol.SetMethod [](start = 20, length = 16)

Consider extracting a helper that takes IMethodSymbol, so that we can use it below. #Closed

AlekseyTs · 2020-04-29T17:54:10Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Members.cs

@@ -410,7 +419,8 @@ public override void VisitMethod(IMethodSymbol symbol)
                        }
                        AddPropertyNameAndParameters(associatedProperty);
                        AddPunctuation(SyntaxKind.DotToken);
-                        AddKeyword(symbol.MethodKind == MethodKind.PropertyGet ? SyntaxKind.GetKeyword : SyntaxKind.SetKeyword);
+                        AddKeyword(symbol.MethodKind == MethodKind.PropertyGet ? SyntaxKind.GetKeyword :
+                            IsInitOnly(associatedProperty) ? SyntaxKind.InitKeyword : SyntaxKind.SetKeyword);


IsInitOnly(associatedProperty) [](start = 28, length = 30)

It feels like we should have a helper that takes IMethodSymbol. We have a method at hand, why go back to the property? #Closed

AlekseyTs · 2020-04-29T17:57:18Z

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

            TypeWithAnnotations returnType,
            ImmutableArray<CustomModifier> refCustomModifiers,
            ImmutableArray<MethodSymbol> explicitInterfaceImplementations)
        {
+            Debug.Assert(isInitOnly == CustomModifierUtils.HasIsExternalInitModifier(returnType.CustomModifiers));


Debug.Assert(isInitOnly == CustomModifierUtils.HasIsExternalInitModifier(returnType.CustomModifiers)); [](start = 12, length = 102)

Not quite the assert I suggested to add, but, if it holds, let's keep it this way. #Closed

AlekseyTs · 2020-04-29T18:12:38Z

Done with review pass (iteration 15) #Closed

AlekseyTs

LGTM (iteration 16)

AlekseyTs · 2020-04-30T01:31:22Z

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

+
+        internal static bool HasIsExternalInitModifier(this ImmutableArray<CustomModifier> modifiers)
+        {
+            if (modifiers.IsDefault)


if (modifiers.IsDefault) [](start = 12, length = 24)

Why do we need this check here, but not in the helper above. What is different? #Closed

This feels wrong, It feels unexpected for a symbol to expose modifiers as ImmutableArray<CustomModifier>.Default. I think we should understand the root cause.

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

AlekseyTs · 2020-04-30T01:33:57Z

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

@@ -265,7 +265,7 @@ internal TypeWithAnnotations MergeEquivalentTypes(TypeWithAnnotations other, Var
        /// <summary>
        /// The list of custom modifiers, if any, associated with the <see cref="Type"/>.
        /// </summary>
-        public ImmutableArray<CustomModifier> CustomModifiers => _extensions.CustomModifiers;
+        public ImmutableArray<CustomModifier> CustomModifiers => _extensions?.CustomModifiers ?? default;


_extensions?.CustomModifiers ?? default; [](start = 65, length = 40)

Has something changed? Why do we need to add this logic in this PR? #Closed

CI flagged an issue (null ref crash)

in this assertion in SignatureOnlyMethodSymbol:
Debug.Assert(isInitOnly == CustomModifierUtils.HasIsExternalInitModifier(returnType.CustomModifiers));

@AlekseyTs Two more tests failed in CI, desktop only. I'm pushing fixes now. Thanks

in this assertion in SignatureOnlyMethodSymbol:
Debug.Assert(isInitOnly == CustomModifierUtils.HasIsExternalInitModifier(returnType.CustomModifiers));

Why do we have a a type without _extensions? Is it default value of the structure? Why SignatureOnlyMethodSymbol uses it? I think we should understand the root cause, other places could be affected as well.

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

We hit this problem in PerformCrefOverloadResolution which does pass a default type.
The reason I'm fixing the issue in TypeWithAnnotations is that it mostly already handled when _extensions is null. Do you think it would be better to return an empty array instead?

We hit this problem in PerformCrefOverloadResolution which does pass a default type.

It looks like the comparer that is used there is supposed to ignore return type and also is supposed to ignore custom modifiers. So. I think we need to understand why exactly do we get here for the default instance, what are we trying to do and why.

The reason I'm fixing the issue in TypeWithAnnotations is that it mostly already handled when _extensions is null. Do you think it would be better to return an empty array instead?

I am not sure if we even should touch this property. Default instances of this structure are not safe by design, I see other properties that will throw or return something that consumers would not expect normally. We simply shouldn't be operating on them.

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

If the new assert is what causing the problem, then I think we should simply adjust it to deal with default instance and not access the property.

In reply to: 418044532 [](ancestors = 418044532,418013638)

AlekseyTs · 2020-04-30T13:16:47Z

@jcouv Please do not merge without closing the loop for two active threads related to recent changes. #Closed

A discussion is ongoing around more recent changes

AlekseyTs

LGTM (iteration 22)

jcouv added Area-Compilers New Language Feature - Records Records labels Apr 10, 2020

jcouv added this to the Compiler.Net5 milestone Apr 10, 2020

jcouv self-assigned this Apr 10, 2020

jcouv force-pushed the init-only branch from 0d63269 to 5d4c71a Compare April 10, 2020 21:55

jcouv marked this pull request as ready for review April 10, 2020 23:15

jcouv requested a review from a team as a code owner April 10, 2020 23:15

Refactor implementation checks

68426e7

jcouv force-pushed the init-only branch 3 times, most recently from 0475dde to c69d069 Compare April 13, 2020 16:46

jcouv marked this pull request as draft April 13, 2020 19:03

RikkiGibson removed the request for review from a team April 13, 2020 19:06

jcouv force-pushed the init-only branch 2 times, most recently from e9aea11 to 5cc692a Compare April 16, 2020 05:04

jcouv closed this Apr 16, 2020

jcouv reopened this Apr 16, 2020

jcouv force-pushed the init-only branch 2 times, most recently from 230ad67 to f83c7a5 Compare April 17, 2020 16:46

Add support for init-only members

4eeaa66

jcouv force-pushed the init-only branch from f83c7a5 to 4eeaa66 Compare April 17, 2020 17:24

jcouv marked this pull request as ready for review April 17, 2020 17:24

AlekseyTs reviewed Apr 19, 2020

View reviewed changes

more feedback

f1c9883

jcouv requested a review from AlekseyTs April 28, 2020 22:49

AlekseyTs reviewed Apr 29, 2020

View reviewed changes

PR feedback

0cdecfe

jcouv requested a review from AlekseyTs April 29, 2020 20:37

AlekseyTs previously approved these changes Apr 29, 2020

View reviewed changes

jcouv added 3 commits April 29, 2020 15:04

tweak decoding

9aadc79

Merge remote-tracking branch 'dotnet/features/records' into init-only

526c09f

Fix null handling in assertion

84bae27

AlekseyTs reviewed Apr 30, 2020

View reviewed changes

Fix two more test issues

a67b061

jcouv added 2 commits April 30, 2020 06:27

use empty array instead

7d60226

Modify assertion instead

527f006

AlekseyTs approved these changes Apr 30, 2020

View reviewed changes

jcouv merged commit f423b85 into dotnet:features/records Apr 30, 2020

jcouv deleted the init-only branch April 30, 2020 22:41

Add support for init-only members #43275

Add support for init-only members #43275

Conversation

jcouv commented Apr 10, 2020 • edited Loading

jcouv commented Apr 13, 2020 • edited Loading

jcouv commented Apr 17, 2020 • edited Loading

AlekseyTs Apr 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 19, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 19, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 19, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 19, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 19, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Apr 28, 2020

jcouv commented Apr 28, 2020

AlekseyTs commented Apr 28, 2020

AlekseyTs commented Apr 28, 2020

AlekseyTs Apr 29, 2020 • 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

Choose a reason for hiding this comment

AlekseyTs Apr 29, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 29, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 29, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 29, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 29, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 29, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Apr 29, 2020 • edited Loading

AlekseyTs left a comment

Choose a reason for hiding this comment

AlekseyTs Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 30, 2020 • edited 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Apr 30, 2020 • edited Loading

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv commented Apr 10, 2020 •

edited

Loading

jcouv commented Apr 13, 2020 •

edited

Loading

jcouv commented Apr 17, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 19, 2020 •

edited

Loading

AlekseyTs Apr 29, 2020 •

edited by jcouv

Loading

AlekseyTs Apr 29, 2020 •

edited

Loading

AlekseyTs Apr 29, 2020 •

edited

Loading

AlekseyTs Apr 29, 2020 •

edited

Loading

AlekseyTs Apr 29, 2020 •

edited

Loading

AlekseyTs Apr 29, 2020 •

edited

Loading

AlekseyTs Apr 29, 2020 •

edited

Loading

AlekseyTs commented Apr 29, 2020 •

edited

Loading

AlekseyTs Apr 30, 2020 •

edited

Loading

AlekseyTs Apr 30, 2020 •

edited

Loading

AlekseyTs commented Apr 30, 2020 •

edited

Loading