Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Readonly members syntax and symbol API #32888

Merged
merged 19 commits into from
Mar 1, 2019
Merged

Readonly members syntax and symbol API #32888

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
8bfceb2
Add syntax tests for readonly members
RikkiGibson Jan 24, 2019
3310634
Add simple test for readonly struct method
RikkiGibson Jan 24, 2019
3eabdda
Allow readonly modifier on methods in structs
RikkiGibson Jan 24, 2019
245aaa8
Add simple readonly class method test
RikkiGibson Jan 28, 2019
92f16a1
Add SourceMemberMethodSymbol.IsReadOnly
RikkiGibson Jan 28, 2019
efb9e97
Test readonly static method
RikkiGibson Feb 13, 2019
6329c70
Disallow readonly accessors on static properties
RikkiGibson Feb 19, 2019
e0fd8a2
Check usages of readonly keyword in property (not accessor) declarations
RikkiGibson Feb 20, 2019
4997b24
Add ref-returning readonly method test
RikkiGibson Feb 22, 2019
c35df0e
Add readonly ref readonly parsing test
RikkiGibson Feb 24, 2019
c3daaae
Allow readonly modifier on property decls in general. Add more tests.
RikkiGibson Feb 26, 2019
00fab4c
Add indexer tests
RikkiGibson Feb 26, 2019
4b322e7
Readonly events on structs
RikkiGibson Feb 27, 2019
ee3228a
Remove comment
RikkiGibson Feb 27, 2019
7da2c02
Disallow readonly on events with associated fields
RikkiGibson Feb 27, 2019
3f73721
Test static readonly auto properties and destructors
RikkiGibson Feb 27, 2019
893a8d4
Disallow redundant readonly on accessor. Simplify diagnostics for sta…
RikkiGibson Feb 27, 2019
d1a1cca
Add tests based on feedback
RikkiGibson Feb 28, 2019
69b1942
Note that feature checks and corresponding tests are needed
RikkiGibson Mar 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceEventSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,11 @@ public sealed override bool IsVirtual
get { return (_modifiers & DeclarationModifiers.Virtual) != 0; }
}

internal bool IsReadOnly
{
get { return (_modifiers & DeclarationModifiers.ReadOnly) != 0; }
}

public sealed override Accessibility DeclaredAccessibility
{
get { return ModifierUtils.EffectiveAccessibility(_modifiers); }
Expand Down Expand Up @@ -435,6 +440,11 @@ private DeclarationModifiers MakeModifiers(SyntaxTokenList modifiers, bool expli
}
}

if (this.ContainingType.IsStructType())
{
allowedModifiers |= DeclarationModifiers.ReadOnly;
}

if (!isInterface)
{
allowedModifiers |= DeclarationModifiers.Extern;
Expand Down Expand Up @@ -468,6 +478,11 @@ protected void CheckModifiersAndType(DiagnosticBag diagnostics)
// A static member '{0}' cannot be marked as override, virtual, or abstract
diagnostics.Add(ErrorCode.ERR_StaticNotVirtual, location, this);
}
else if (IsReadOnly && (IsStatic || HasAssociatedField))
{
// The modifier '{0}' is not valid for this item
diagnostics.Add(ErrorCode.ERR_BadMemberFlag, location, SyntaxFacts.GetText(SyntaxKind.ReadOnlyKeyword));
}
else if (IsOverride && (IsNew || IsVirtual))
{
// A member '{0}' marked as override cannot be marked as new or virtual
Expand Down
8 changes: 8 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,14 @@ public sealed override bool IsAsync
}
}

internal bool IsReadOnly
{
get
{
return (this.DeclarationModifiers & DeclarationModifiers.ReadOnly) != 0;
}
}

internal sealed override Cci.CallingConvention CallingConvention
{
get
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -772,6 +772,11 @@ private DeclarationModifiers MakeModifiers(SyntaxTokenList modifiers, MethodKind
allowedModifiers |= DeclarationModifiers.Extern | DeclarationModifiers.Async;
}

if (ContainingType.IsStructType())
jcouv marked this conversation as resolved.
Show resolved Hide resolved
{
allowedModifiers |= DeclarationModifiers.ReadOnly;
}

var mods = ModifierUtils.MakeAndCheckNontypeMemberModifiers(modifiers, defaultAccess, allowedModifiers, location, diagnostics, out modifierErrors);

this.CheckUnsafeModifier(mods, diagnostics);
Expand Down Expand Up @@ -933,6 +938,11 @@ private void CheckModifiers(bool hasBody, Location location, DiagnosticBag diagn
// The modifier '{0}' is not valid for this item
diagnostics.Add(ErrorCode.ERR_BadMemberFlag, location, SyntaxFacts.GetText(SyntaxKind.VirtualKeyword));
}
else if (IsStatic && IsReadOnly)
{
// The modifier '{0}' is not valid for this item
diagnostics.Add(ErrorCode.ERR_BadMemberFlag, location, SyntaxFacts.GetText(SyntaxKind.ReadOnlyKeyword));
}
else if (IsAbstract && !ContainingType.IsAbstract && (ContainingType.TypeKind == TypeKind.Class || ContainingType.TypeKind == TypeKind.Submission))
{
// '{0}' is abstract but it is contained in non-abstract class '{1}'
Expand Down
12 changes: 11 additions & 1 deletion src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertyAccessorSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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==

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (this.ContainingType.IsStructType() && !_property.HasReadOnlyModifier)
{
allowedModifiers |= DeclarationModifiers.ReadOnly;
}

var mods = ModifierUtils.MakeAndCheckNontypeMemberModifiers(syntax.Modifiers, defaultAccess, allowedModifiers, location, diagnostics, out modifierErrors);

// For interface, check there are no accessibility modifiers.
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Is this check needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
// The modifier '{0}' is not valid for this item
diagnostics.Add(ErrorCode.ERR_BadMemberFlag, location, SyntaxFacts.GetText(SyntaxKind.ReadOnlyKeyword));
}
}

/// <summary>
Expand Down
12 changes: 12 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,6 +627,8 @@ internal bool IsNew
get { return (_modifiers & DeclarationModifiers.New) != 0; }
}

internal bool HasReadOnlyModifier => (_modifiers & DeclarationModifiers.ReadOnly) != 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


public override MethodSymbol GetMethod
{
get { return _getMethod; }
Expand Down Expand Up @@ -813,6 +815,11 @@ private DeclarationModifiers MakeModifiers(SyntaxTokenList modifiers, bool isExp
}
}

if (ContainingType.IsStructType())
{
allowedModifiers |= DeclarationModifiers.ReadOnly;
}

if (!isInterface)
{
allowedModifiers |=
Expand Down Expand Up @@ -890,6 +897,11 @@ private void CheckModifiers(Location location, bool isIndexer, DiagnosticBag dia
// A static member '{0}' cannot be marked as override, virtual, or abstract
diagnostics.Add(ErrorCode.ERR_StaticNotVirtual, location, this);
}
else if (IsStatic && HasReadOnlyModifier)
{
// The modifier '{0}' is not valid for this item
diagnostics.Add(ErrorCode.ERR_BadMemberFlag, location, SyntaxFacts.GetText(SyntaxKind.ReadOnlyKeyword));
}
else if (IsOverride && (IsNew || IsVirtual))
{
// A member '{0}' marked as override cannot be marked as new or virtual
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/TypeParameterSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,7 +584,7 @@ internal bool GetIsValueType(ConsList<TypeParameterSymbol> inProgress)
}

public sealed override bool IsValueType => GetIsValueType(ConsList<TypeParameterSymbol>.Empty);

internal sealed override ManagedKind ManagedKind
{
get
Expand Down
Loading