Readonly members syntax and symbol API #32888

RikkiGibson · 2019-01-28T22:19:21Z

Modified syntax checks and added symbol API to support the readonly members feature. See dotnet/csharplang#1710.

I outlined how I think the entire development of this feature breaks down in #32911. Would also appreciate feedback on that.

tannergooding · 2019-01-28T23:11:06Z

Any other kinds of declarations I need to cover?

While somewhat nonsensical from an API design point of view, I would think that any method (including property setters) should be applicable.

That is, it is entirely possible for a setter to not mutate any instance state (perhaps, for example, it is an instance property that only modifies some static state), which would make it valid as a readonly member. You can already do this in existing readonly structs, for example.

So, I believe the end-result should be that:

A readonly struct implicitly marks all members as readonly and requires that all fields be explicitly readonly
A regular struct can therefore have any of its members explicitly marked as readonly (but does not require fields to be readonly)

Additionally, given that a readonly struct requires that its fields be marked as readonly (thereby explicitly disallowing assignment), it would be beneficial it a readonly method would warn if you try to assign a non-readonly field. I realize we can't do this for method calls, due to back-compat reasons with the existing readonly struct feature; but I think field assignment for readonly methods would be fair game (since it is a new warning that only shows in new code).

RikkiGibson · 2019-01-29T01:05:13Z

I think you're right @tannergooding. It's probably not uncommon for a struct setter to modify an object contained in a struct field, without violating the readonly constraint on the struct itself.

jaredpar · 2019-01-29T01:20:55Z

@tannergooding

Additionally, given that a readonly struct requires that its fields be marked as readonly (thereby explicitly disallowing assignment), it would be beneficial it a readonly method would warn if you try to assign a non-readonly field.

This is explicitly an error. The type of this in a readonly member is in this hence field assignment is not possible.

That being said any invocation on this or a field of this of a member that isn't readonly should be flagged with a warning. Such an invocation will involve a defensive copy which is probably un-intended.

That being said (part 2) we didn't do this for readonly struct which in reprotrospect we probably should have. Should take this up with LDM to see where they want to land on this.

jaredpar · 2019-01-29T01:24:43Z

@RikkiGibson

My understanding is that the possible "readonly members" include get accessors and methods on structs. Any other kinds of declarations I need to cover?

All members types: operators, constructors, etc ... 😄

Even though readonly is only legal on a subset of members we need validate that it's illegal on the others. Hence testing should include non-sense cases like a readonly constructor on a struct. Note: for the property case you also need to validate the expression bodied and traditional syntaxes.

tannergooding · 2019-01-29T01:43:09Z

This is explicitly an error. The type of this in a readonly member is in this hence field assignment is not possible.

Great. This is even better.

That being said any invocation on this or a field of this of a member that isn't readonly should be flagged with a warning. Such an invocation will involve a defensive copy which is probably un-intended.

That would be great, but I understand the limitations around "breaking" changes for the existing readonly struct. Perhaps, as a compromise (while we don't have warning waves), it could be enabled for methods explicitly marked readonly. That would be non-breaking (and easy to use for the new feature) and would allow users of the existing readonly struct feature to have a "backdoor" (although a verbose one) to get those warnings if desired.

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

CyrusNajmabadi · 2019-02-25T19:40:18Z

Not looking deeply into this, but this commit message seems super strange:
Add ReadOnlyKeyword to ArrowExpressionClauseSyntax.

Why would a ReadOnlyKeyword be attached to the ArrowExpression? That seems like a super bizarre place to put it.

src/Compilers/CSharp/Portable/Generated/Syntax.xml.Internal.Generated.cs

RikkiGibson · 2019-02-27T00:18:35Z

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

RikkiGibson · 2019-02-27T20:17:30Z

FYI @sharwell the release mode integration tests failed. Retrying.

cston · 2019-02-27T20:18:38Z

Do we need to check IsStatic && IsReadOnly here?

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbol.cs:938 in 7da2c02. [](commit_id = 7da2c02, deletion_comment = False)

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

chsienki · 2019-02-27T20:22:57Z

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

@@ -410,7 +410,12 @@ private DeclarationModifiers MakeModifiers(AccessorDeclarationSyntax syntax, Loc
            const DeclarationModifiers defaultAccess = DeclarationModifiers.None;

            // Check that the set of modifiers is allowed
-            const DeclarationModifiers allowedModifiers = DeclarationModifiers.AccessibilityMask;
+            DeclarationModifiers allowedModifiers = DeclarationModifiers.AccessibilityMask;


Do we need to check that the parent property this accessor belongs to isn't readonly?

For access modifiers it's an error when you specify an accessor modifier that isn't more restrictive https://sharplab.io/#v2:D4AQDABCCMDcCwAoEBmKAmCBhCBvJEhBhUaMkAysYfoiYQJADmApgC4J30QOoQDO7TvQC+SEUA==

It seems like that would be consistent with the behavior for visibility modifiers.

Do you propose giving a similar error for this case?

struct S { readonly string P { readonly get; } }

Yeah, that's what I was thinking. Should we be giving an error in that case? I don't really have an opinion one way or the other, but it seems like it would be consistent if we did.

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

I went ahead and made it an error to provide readonly on an accessor for a property marked readonly.

RikkiGibson · 2019-02-27T20:25:12Z

I believe no because the accessors will "inherit" the static readonly modifiers from the property, and we will catch it there. It might give better diagnostics in some cases, however-- e.g. static readonly int P { get; set; } may produce 2 very similar diagnostics. I'll ensure there's a test for that.

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

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbol.cs:938 in 7da2c02. [](commit_id = 7da2c02, deletion_comment = False)

…tic readonly property.

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

cston · 2019-02-28T17:42:32Z

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

@@ -451,6 +456,11 @@ private void CheckModifiers(Location location, bool hasBody, bool isAutoProperty
            {
                diagnostics.Add(AccessCheck.GetProtectedMemberInSealedTypeError(ContainingType), location, this);
            }
+            else if (IsStatic && IsReadOnly && !_property.HasReadOnlyModifier)


&& !_property.HasReadOnlyModifier [](start = 44, length = 33)

Is this check needed?

In this scenario: static readonly int P { get; set; } this check allows us to give a diagnostic only on the property and not on the accessors, which inherit their modifiers from the property. (is there a better term to use than "inherit"?)

Is there a better pattern to use to prevent redundant diagnostics?

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

RikkiGibson · 2019-02-28T19:32:38Z

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

@@ -627,6 +627,8 @@ internal bool IsNew
            get { return (_modifiers & DeclarationModifiers.New) != 0; }
        }

+        internal bool HasReadOnlyModifier => (_modifiers & DeclarationModifiers.ReadOnly) != 0;


This might have to become public API at some point, so we'll have to look at naming. IsReadOnly means something else on properties.

RikkiGibson · 2019-02-28T20:31:11Z

@dotnet/roslyn-compiler could I get a second review please?

CyrusNajmabadi · 2019-02-28T21:48:09Z

@RikkiGibson is it also on you to do the IDE side of things? If so, are you intending to do that through another set of PRs?

RikkiGibson · 2019-02-28T21:53:09Z

Yeah, that will happen in separate PRs.

jaredpar · 2019-02-28T22:30:34Z

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

+                // (9,9): error CS1609: Modifiers cannot be placed on event accessor declarations
+                //         readonly remove {}
+                Diagnostic(ErrorCode.ERR_NoModifiersOnAccessor, "readonly").WithLocation(9, 9));
+        }


Should add a test or PROTOTYPE about ensuring this only works when C# 8.0 or greater is enabled.

Done. I will probably have a PR specifically about ensuring feature checks occur appropriately in all scenarios (basically wherever the readonly modifier can be added).

RikkiGibson added the Area-Compilers label Jan 28, 2019

RikkiGibson added this to the 16.1.P1 milestone Jan 28, 2019

RikkiGibson requested a review from a team as a code owner January 28, 2019 22:19

jcouv reviewed Jan 30, 2019

View reviewed changes

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

RikkiGibson added 8 commits February 12, 2019 12:41

Add syntax tests for readonly members

8bfceb2

Add simple test for readonly struct method

3310634

Allow readonly modifier on methods in structs

3eabdda

Add simple readonly class method test

245aaa8

Add SourceMemberMethodSymbol.IsReadOnly

92f16a1

Test readonly static method

efb9e97

Disallow readonly accessors on static properties

6329c70

Check usages of readonly keyword in property (not accessor) declarations

e0fd8a2

RikkiGibson force-pushed the features/readonly-members branch from 8721918 to e0fd8a2 Compare February 20, 2019 01:33

RikkiGibson added 2 commits February 22, 2019 15:16

Add ref-returning readonly method test

4997b24

Add readonly ref readonly parsing test

c35df0e

RikkiGibson changed the title ~~Basic syntax tests for readonly members~~ Readonly members syntax and symbol API Feb 25, 2019

CyrusNajmabadi reviewed Feb 25, 2019

View reviewed changes

src/Compilers/CSharp/Portable/Generated/Syntax.xml.Internal.Generated.cs Outdated Show resolved Hide resolved

Allow readonly modifier on property decls in general. Add more tests.

c3daaae

RikkiGibson force-pushed the features/readonly-members branch from 3af5d21 to c3daaae Compare February 26, 2019 21:24

RikkiGibson added 3 commits February 26, 2019 13:30

Add indexer tests

00fab4c

Readonly events on structs

4b322e7

Remove comment

ee3228a

Disallow readonly on events with associated fields

7da2c02

cston reviewed Feb 27, 2019

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs Show resolved Hide resolved

chsienki reviewed Feb 27, 2019

View reviewed changes

RikkiGibson added 2 commits February 27, 2019 12:49

Test static readonly auto properties and destructors

3f73721

Disallow redundant readonly on accessor. Simplify diagnostics for sta…

893a8d4

…tic readonly property.

cston reviewed Feb 28, 2019

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs Outdated Show resolved Hide resolved

cston reviewed Feb 28, 2019

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/ReadOnlyStructsTests.cs Show resolved Hide resolved

Add tests based on feedback

d1a1cca

RikkiGibson commented Feb 28, 2019

View reviewed changes

cston approved these changes Feb 28, 2019

View reviewed changes

jaredpar reviewed Feb 28, 2019

View reviewed changes

jaredpar approved these changes Feb 28, 2019

View reviewed changes

Note that feature checks and corresponding tests are needed

69b1942

RikkiGibson merged commit 6752e31 into dotnet:features/readonly-members Mar 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Readonly members syntax and symbol API #32888

Readonly members syntax and symbol API #32888

RikkiGibson commented Jan 28, 2019 •

edited

Loading

tannergooding commented Jan 28, 2019

RikkiGibson commented Jan 29, 2019

jaredpar commented Jan 29, 2019

jaredpar commented Jan 29, 2019

tannergooding commented Jan 29, 2019

CyrusNajmabadi commented Feb 25, 2019

RikkiGibson commented Feb 27, 2019

RikkiGibson commented Feb 27, 2019

cston commented Feb 27, 2019

chsienki Feb 27, 2019

RikkiGibson Feb 27, 2019

chsienki Feb 27, 2019

RikkiGibson Feb 28, 2019

RikkiGibson commented Feb 27, 2019

cston Feb 28, 2019

RikkiGibson Feb 28, 2019

RikkiGibson Feb 28, 2019

RikkiGibson Feb 28, 2019

RikkiGibson commented Feb 28, 2019

CyrusNajmabadi commented Feb 28, 2019

RikkiGibson commented Feb 28, 2019

jaredpar Feb 28, 2019

RikkiGibson Mar 1, 2019

Readonly members syntax and symbol API #32888

Readonly members syntax and symbol API #32888

Conversation

RikkiGibson commented Jan 28, 2019 • edited Loading

tannergooding commented Jan 28, 2019

RikkiGibson commented Jan 29, 2019

jaredpar commented Jan 29, 2019

jaredpar commented Jan 29, 2019

tannergooding commented Jan 29, 2019

CyrusNajmabadi commented Feb 25, 2019

RikkiGibson commented Feb 27, 2019

RikkiGibson commented Feb 27, 2019

cston commented Feb 27, 2019

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 commented Feb 27, 2019

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 commented Feb 28, 2019

CyrusNajmabadi commented Feb 28, 2019

RikkiGibson commented Feb 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson commented Jan 28, 2019 •

edited

Loading