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.

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

Add support for init-only members #43275

Merged
merged 22 commits into from
Apr 30, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
2 changes: 1 addition & 1 deletion docs/contributing/Compiler Test Plan.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ This document provides guidance for thinking about language interactions and tes
- Performance and stress testing

# Type and members
- Access modifiers (public, protected, internal, protected internal, private protected, private), static, ref
- Access modifiers (public, protected, internal, protected internal, private protected, private), static, ref, init
Copy link
Member

@jaredpar jaredpar Apr 21, 2020

Choose a reason for hiding this comment

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

As of yet this isn't an access modifier. #Resolved

- types
- methods
- fields
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,9 @@ private static void ComputeDeclarations(

case SyntaxKind.AddAccessorDeclaration:
case SyntaxKind.RemoveAccessorDeclaration:
case SyntaxKind.SetAccessorDeclaration:
case SyntaxKind.GetAccessorDeclaration:
case SyntaxKind.SetAccessorDeclaration:
case SyntaxKind.InitAccessorDeclaration:
{
var t = (AccessorDeclarationSyntax)node;
var blocks = ArrayBuilder<SyntaxNode>.GetInstance();
Expand Down
64 changes: 59 additions & 5 deletions src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
Original file line number Diff line number Diff line change
Expand Up @@ -709,7 +709,8 @@ private bool CheckFieldValueKind(SyntaxNode node, BoundFieldAccess fieldAccess,
{
MethodSymbol containingMethod = (MethodSymbol)containing;
MethodKind desiredMethodKind = fieldIsStatic ? MethodKind.StaticConstructor : MethodKind.Constructor;
canModifyReadonly = containingMethod.MethodKind == desiredMethodKind;
canModifyReadonly = (containingMethod.MethodKind == desiredMethodKind) ||
isAssignedFromInitOnlySetterOnThis(fieldAccess.ReceiverOpt);
}
else if (containing.Kind == SymbolKind.Field)
{
Expand Down Expand Up @@ -745,6 +746,24 @@ private bool CheckFieldValueKind(SyntaxNode node, BoundFieldAccess fieldAccess,

// for other fields defer to the receiver.
return CheckIsValidReceiverForVariable(node, fieldAccess.ReceiverOpt, valueKind, diagnostics);

bool isAssignedFromInitOnlySetterOnThis(BoundExpression receiver)
{
// bad: other.readonlyField = ...
// bad: base.readonlyField = ...
if (!(receiver is BoundThisReference))
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 26, 2020

Choose a reason for hiding this comment

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

if (!(receiver is BoundThisReference)) [](start = 16, length = 38)

It looks like the speclet doesn't have this restriction. Is only says: "Assign readonly fields declared on the same type". However, I think the intention was to have this restriction. Please make sure to reconcile the difference. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated speclet


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

{
return false;
}

var containingMember = ContainingMemberOrLambda;
Copy link
Member

@agocke agocke Apr 22, 2020

Choose a reason for hiding this comment

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

Consider inlining this local #Resolved

if (!(containingMember is MethodSymbol method))
{
return false;
}

return method.MethodKind == MethodKind.PropertySet && method.IsInitOnly;
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 25, 2020

Choose a reason for hiding this comment

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

method.MethodKind == MethodKind.PropertySet [](start = 23, length = 43)

Is this check redundant? Doesn't method.IsInitOnly imply MethodKind.PropertySet? #Closed

}
}

private bool CheckSimpleAssignmentValueKind(SyntaxNode node, BoundAssignmentOperator assignment, BindValueKind valueKind, DiagnosticBag diagnostics)
Expand Down Expand Up @@ -931,9 +950,7 @@ private bool CheckPropertyValueKind(SyntaxNode node, BoundExpression expr, BindV

// Addendum: Assignment is also allowed for get-only autoprops in their constructor

BoundExpression receiver;
SyntaxNode propertySyntax;
var propertySymbol = GetPropertySymbol(expr, out receiver, out propertySyntax);
var propertySymbol = GetPropertySymbol(expr, out BoundExpression receiver, out SyntaxNode propertySyntax);
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 25, 2020

Choose a reason for hiding this comment

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

out BoundExpression receiver, out SyntaxNode propertySyntax) [](start = 57, length = 60)

I think refactorings like this negatively impact readability of the code. Was there something wrong with the code? Does it violate style, analyzer complained? Was this change necessary to accomplish the task? #Closed


Debug.Assert((object)propertySymbol != null);
Debug.Assert(propertySyntax != null);
Expand Down Expand Up @@ -970,7 +987,7 @@ private bool CheckPropertyValueKind(SyntaxNode node, BoundExpression expr, BindV
{
var setMethod = propertySymbol.GetOwnOrInheritedSetMethod();

if ((object)setMethod == null)
if (setMethod is null)
{
var containing = this.ContainingMemberOrLambda;
if (!AccessingAutoPropertyFromConstructor(receiver, propertySymbol, containing))
Expand All @@ -981,6 +998,13 @@ private bool CheckPropertyValueKind(SyntaxNode node, BoundExpression expr, BindV
}
else
{
if (setMethod.IsInitOnly &&
!isAllowedInitOnlySet(receiver))
{
Error(diagnostics, ErrorCode.ERR_AssignmentInitOnly, node, propertySymbol);
return false;
}

var accessThroughType = this.GetAccessThroughType(receiver);
bool failedThroughTypeCheck;
HashSet<DiagnosticInfo> useSiteDiagnostics = null;
Expand Down Expand Up @@ -1069,6 +1093,36 @@ private bool CheckPropertyValueKind(SyntaxNode node, BoundExpression expr, BindV
}

return true;

bool isAllowedInitOnlySet(BoundExpression receiver)
{
// ok: new C() { InitOnlyProperty = ... }
if (receiver is BoundObjectOrCollectionValuePlaceholder)
{
return true;
}

// bad: other.InitOnlyProperty = ...
if (!(receiver is BoundThisReference || receiver is BoundBaseReference))
{
return false;
}

var containingMember = ContainingMemberOrLambda;
if (!(containingMember is MethodSymbol method))
{
return false;
}

bool isInitOnlySetter = method.MethodKind == MethodKind.PropertySet && method.IsInitOnly;
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 25, 2020

Choose a reason for hiding this comment

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

method.MethodKind == MethodKind.PropertySet [](start = 40, length = 43)

Is this condition redundant? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but it follows the letter of the spec, "Inside the init accessor of any property, on this or base"


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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but it follows the letter of the spec, "Inside the init accessor of any property, on this or base"

method.IsInitOnly already follows the letter of the spec


In reply to: 415204497 [](ancestors = 415204497,415102012)

Copy link
Contributor

Choose a reason for hiding this comment

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

Rename it to IsInitOnlySet if it is not obvious


In reply to: 415213918 [](ancestors = 415213918,415204497,415102012)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that's necessary.


In reply to: 415214044 [](ancestors = 415214044,415213918,415204497,415102012)

if (method.IsConstructor() || isInitOnlySetter)
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 25, 2020

Choose a reason for hiding this comment

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

IsConstructor [](start = 27, length = 13)

It looks like this check returns true for a static constructor. The spec doesn't allow assignment in a static constructor. Even though some other error is likely to be reported in this scenario, I think it would be better to use more precise condition that follows the spec "to the letter". Also, please add a test that gets here for a static constructor, even if that would be an error due to reasons unrelated to this feature. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this check returns true for a static constructor. The spec doesn't allow assignment in a static constructor. Even though some other error is likely to be reported in this scenario, I think it would be better to use more precise condition that follows the spec "to the letter". Also, please add a test that gets here for a static constructor, even if that would be an error due to reasons unrelated to this feature.

This thread was marked as resolved. However, I don't see any changes for the corresponding code and no other response. If the context for the thread is not clear, it is about the method.IsConstructor() check.


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

Copy link
Member Author

Choose a reason for hiding this comment

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

The resolution was the added because assertion above (!method.IsStatic).
Based on your comment above, sounds like you prefer an explicit check even if unreachable. I'm okay with that. Will change.


In reply to: 416728343 [](ancestors = 416728343,415103737)

Copy link
Contributor

@AlekseyTs AlekseyTs Apr 28, 2020

Choose a reason for hiding this comment

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

The resolution was the added because assertion above (!method.IsStatic).
Based on your comment above, sounds like you prefer an explicit check even if unreachable. I'm okay with that. Will change.

Given that the explicit check is simple and even less expensive than method.IsConstructor() check, it feels reasonable to adjust the check rather than adding an assert, which would "save" us only if we ran into a bad situation during testing.


In reply to: 416822362 [](ancestors = 416822362,416728343,415103737)

{
// ok: setting on `this` or `base` from a constructor or init-only setter
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 25, 2020

Choose a reason for hiding this comment

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

constructor [](start = 62, length = 11)

instance constructor? #Closed

return true;
}

return false;
}
}

private bool IsBadBaseAccess(SyntaxNode node, BoundExpression receiverOpt, Symbol member, DiagnosticBag diagnostics,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ public override Binder VisitAccessorDeclaration(AccessorDeclarationSyntax parent
var propertySymbol = GetPropertySymbol((BasePropertyDeclarationSyntax)propertyOrEventDecl, resultBinder);
if ((object)propertySymbol != null)
{
accessor = (parent.Kind() == SyntaxKind.SetAccessorDeclaration) ? propertySymbol.SetMethod : propertySymbol.GetMethod;
accessor = (parent.Kind() == SyntaxKind.GetAccessorDeclaration) ? propertySymbol.GetMethod : propertySymbol.SetMethod;
}
break;
}
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Binder/Binder_Crefs.cs
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,7 @@ private static ImmutableArray<Symbol> PerformCrefOverloadResolution(ArrayBuilder
containingType: null,
name: null,
refKind: RefKind.None,
isInitOnly: false,
returnType: default,
refCustomModifiers: ImmutableArray<CustomModifier>.Empty,
explicitInterfaceImplementations: ImmutableArray<MethodSymbol>.Empty);
Expand Down
13 changes: 11 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/Binder_Symbols.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1442,9 +1442,18 @@ internal static bool ReportUseSiteDiagnostics(Symbol symbol, DiagnosticBag diagn
/// </summary>
internal NamedTypeSymbol GetWellKnownType(WellKnownType type, DiagnosticBag diagnostics, SyntaxNode node)
{
NamedTypeSymbol typeSymbol = this.Compilation.GetWellKnownType(type);
return GetWellKnownType(this.Compilation, type, diagnostics, node.Location);
}

/// <summary>
/// This is a layer on top of the Compilation version that generates a diagnostic if the well-known
/// type isn't found.
/// </summary>
internal static NamedTypeSymbol GetWellKnownType(CSharpCompilation compilation, WellKnownType type, DiagnosticBag diagnostics, Location location)
{
NamedTypeSymbol typeSymbol = compilation.GetWellKnownType(type);
Debug.Assert((object)typeSymbol != null, "Expect an error type if well-known type isn't found");
ReportUseSiteDiagnostics(typeSymbol, diagnostics, node);
ReportUseSiteDiagnostics(typeSymbol, diagnostics, location);
return typeSymbol;
}

Expand Down
20 changes: 19 additions & 1 deletion src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,9 @@
<data name="ERR_BadMemberFlag" xml:space="preserve">
<value>The modifier '{0}' is not valid for this item</value>
</data>
<data name="ERR_BadInitAccessor" xml:space="preserve">
<value>The 'init' accessor is not valid on static members</value>
</data>
<data name="ERR_BadMemberProtection" xml:space="preserve">
<value>More than one protection modifier</value>
</data>
Expand Down Expand Up @@ -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}' should both be init-only or neither</value>
</data>
<data name="ERR_ExplicitPropertyMissingAccessor" xml:space="preserve">
<value>Explicit interface implementation '{0}' is missing accessor '{1}'</value>
</data>
Expand Down Expand Up @@ -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 setter of the class in which the field is defined or a variable initializer))</value>
</data>
<data name="ERR_AssgReadonly2" xml:space="preserve">
<value>Members of readonly field '{0}' cannot be modified (except in a constructor or a variable initializer)</value>
Expand Down Expand Up @@ -4918,6 +4924,9 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_CantChangeRefReturnOnOverride" xml:space="preserve">
<value>'{0}' must match by reference return of overridden member '{1}'</value>
</data>
<data name="ERR_CantChangeInitOnlyOnOverride" xml:space="preserve">
<value>'{0}' must match by init-only of overridden member '{1}'</value>
</data>
<data name="ERR_MustNotHaveRefReturn" xml:space="preserve">
<value>By-reference returns may only be used in methods that return by reference</value>
</data>
Expand All @@ -4930,6 +4939,9 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_CloseUnimplementedInterfaceMemberWrongRefReturn" xml:space="preserve">
<value>'{0}' does not implement interface member '{1}'. '{2}' cannot implement '{1}' because it does not have matching return by reference.</value>
</data>
<data name="ERR_CloseUnimplementedInterfaceMemberWrongInitOnly" xml:space="preserve">
<value>'{0}' does not implement interface member '{1}'. '{2}' cannot implement '{1}' because it does not match by init-only.</value>
</data>
<data name="ERR_BadIteratorReturnRef" xml:space="preserve">
<value>The body of '{0}' cannot be an iterator block because '{0}' returns by reference</value>
</data>
Expand Down Expand Up @@ -6049,6 +6061,9 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="IDS_FeatureRecords" xml:space="preserve">
<value>records</value>
</data>
<data name="IDS_FeatureInitOnlySetters" xml:space="preserve">
<value>init-only setters</value>
</data>
<data name="ERR_BadRecordDeclaration" xml:space="preserve">
<value>Records must have both a 'data' modifier and non-empty parameter list</value>
</data>
Expand All @@ -6067,4 +6082,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>
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 19, 2020

Choose a reason for hiding this comment

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

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

"in" vs. "from"? #Closed

Copy link
Contributor

@AlekseyTs AlekseyTs Apr 25, 2020

Choose a reason for hiding this comment

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

Init-only member [](start = 11, length = 16)

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

Copy link
Member Author

Choose a reason for hiding this comment

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

The spec was written in those terms. We now updated it, I'll update here too.


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

Copy link
Contributor

@AlekseyTs AlekseyTs Apr 25, 2020

Choose a reason for hiding this comment

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

from a constructor, object initialization or 'with' expression of that type [](start = 55, length = 75)

The spec also says: "Inside the init accessor of any property, on this or base" #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

The full quote:
An instance property containing an init accessor is considered settable in the following circumstances:

  • During an object initializer
  • Inside an instance constructor of the containing or derived type, on this or base
  • Inside the init accessor of any property, on this or base

The spec also doesn't mention 'with' expression. I suggest we drop that part for now and make appropriate changes later when the interaction between the features is specified and being implemented.


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

</data>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -2098,6 +2098,7 @@ internal protected virtual CSharpSyntaxNode GetBindableSyntaxNode(CSharpSyntaxNo
{
case SyntaxKind.GetAccessorDeclaration:
case SyntaxKind.SetAccessorDeclaration:
case SyntaxKind.InitAccessorDeclaration:
case SyntaxKind.AddAccessorDeclaration:
case SyntaxKind.RemoveAccessorDeclaration:
case SyntaxKind.MethodDeclaration:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ internal override BoundNode Bind(Binder binder, CSharpSyntaxNode node, Diagnosti
case SyntaxKind.DestructorDeclaration:
case SyntaxKind.GetAccessorDeclaration:
case SyntaxKind.SetAccessorDeclaration:
case SyntaxKind.InitAccessorDeclaration:
case SyntaxKind.AddAccessorDeclaration:
case SyntaxKind.RemoveAccessorDeclaration:
return binder.BindMethodBody(node, diagnostics);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -763,6 +763,7 @@ private MemberSemanticModel GetMemberModel(int position)
case SyntaxKind.RemoveAccessorDeclaration:
case SyntaxKind.GetAccessorDeclaration:
case SyntaxKind.SetAccessorDeclaration:
case SyntaxKind.InitAccessorDeclaration:
// NOTE: not UnknownAccessorDeclaration since there's no corresponding method symbol from which to build a member model.
outsideMemberDecl = !LookupPosition.IsInBody(position, (AccessorDeclarationSyntax)memberDecl);
break;
Expand Down Expand Up @@ -836,6 +837,7 @@ internal override MemberSemanticModel GetMemberModel(SyntaxNode node)

case SyntaxKind.GetAccessorDeclaration:
case SyntaxKind.SetAccessorDeclaration:
case SyntaxKind.InitAccessorDeclaration:
case SyntaxKind.AddAccessorDeclaration:
case SyntaxKind.RemoveAccessorDeclaration:
// NOTE: not UnknownAccessorDeclaration since there's no corresponding method symbol from which to build a member model.
Expand Down Expand Up @@ -1034,6 +1036,7 @@ private MemberSemanticModel CreateMemberModel(CSharpSyntaxNode node)
}
case SyntaxKind.GetAccessorDeclaration:
case SyntaxKind.SetAccessorDeclaration:
case SyntaxKind.InitAccessorDeclaration:
case SyntaxKind.AddAccessorDeclaration:
case SyntaxKind.RemoveAccessorDeclaration:
{
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1775,6 +1775,11 @@ internal enum ErrorCode

ERR_BadRecordDeclaration = 8800,
ERR_DuplicateRecordConstructor = 8801,
ERR_AssignmentInitOnly = 8802,
ERR_CantChangeInitOnlyOnOverride = 8803,
ERR_CloseUnimplementedInterfaceMemberWrongInitOnly = 8804,
ERR_ExplicitPropertyMismatchInitOnly = 8805,
ERR_BadInitAccessor = 8806,

// Note: you will need to re-generate compiler code after adding warnings (eng\generate-compiler-code.cmd)
}
Expand Down
4 changes: 3 additions & 1 deletion src/Compilers/CSharp/Portable/Errors/MessageID.cs
Original file line number Diff line number Diff line change
Expand Up @@ -191,8 +191,9 @@ internal enum MessageID
IDS_FeatureExternLocalFunctions = MessageBase + 12767,
IDS_FeatureMemberNotNull = MessageBase + 12768,

IDS_FeatureRecords = MessageBase + 12770,
IDS_FeatureNativeInt = MessageBase + 12769,
IDS_FeatureRecords = MessageBase + 12770,
IDS_FeatureInitOnlySetters = MessageBase + 12771,
}

// Message IDs may refer to strings that need to be localized.
Expand Down Expand Up @@ -306,6 +307,7 @@ internal static LanguageVersion RequiredVersion(this MessageID feature)
case MessageID.IDS_FeatureMemberNotNull:
case MessageID.IDS_FeatureRecords:
case MessageID.IDS_FeatureNativeInt:
case MessageID.IDS_FeatureInitOnlySetters: // semantic check
return LanguageVersion.Preview;

// C# 8.0 features.
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading