Skip to content

Commit

Permalink
Error when required members are not set (#60101)
Browse files Browse the repository at this point in the history
Implements reading the required members list of a type, and enforces that required members are all set by an initializer on the constructor. Required members must be initialized with values, not with nested object initializers.

Test plan #57046.
Specification https://github.com/dotnet/csharplang/blob/main/proposals/required-members.md
  • Loading branch information
333fred authored Mar 17, 2022
1 parent 44f9669 commit 47c81fe
Show file tree
Hide file tree
Showing 23 changed files with 2,228 additions and 37 deletions.
8 changes: 7 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,8 @@ private BoundAttribute BindAttributeCore(AttributeSyntax node, NamedTypeSymbol a
diagnostics,
out var memberResolutionResult,
out var candidateConstructors,
allowProtectedConstructorsOfBaseType: true);
allowProtectedConstructorsOfBaseType: true,
suppressUnsupportedRequiredMembersError: false);
attributeConstructor = memberResolutionResult.Member;
expanded = memberResolutionResult.Resolution == MemberResolutionKind.ApplicableInExpandedForm;
argsToParamsOpt = memberResolutionResult.Result.ArgsToParamsOpt;
Expand Down Expand Up @@ -238,6 +239,11 @@ private BoundAttribute BindAttributeCore(AttributeSyntax node, NamedTypeSymbol a
ImmutableArray<BoundAssignmentOperator> boundNamedArguments = analyzedArguments.NamedArguments?.ToImmutableAndFree() ?? ImmutableArray<BoundAssignmentOperator>.Empty;
Debug.Assert(boundNamedArguments.All(arg => !arg.Right.NeedsToBeConverted()));

if (attributeConstructor is not null)
{
CheckRequiredMembersInObjectInitializer(attributeConstructor, ImmutableArray<BoundExpression>.CastUp(boundNamedArguments), node, diagnostics);
}

analyzedArguments.ConstructorArguments.Free();

return new BoundAttribute(
Expand Down
131 changes: 125 additions & 6 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4064,7 +4064,8 @@ private BoundExpression BindConstructorInitializerCore(
diagnostics,
out memberResolutionResult,
out candidateConstructors,
allowProtectedConstructorsOfBaseType: true);
allowProtectedConstructorsOfBaseType: true,
suppressUnsupportedRequiredMembersError: true);
MethodSymbol resultMember = memberResolutionResult.Member;

validateRecordCopyConstructor(constructor, baseType, resultMember, errorLocation, diagnostics);
Expand Down Expand Up @@ -4967,6 +4968,102 @@ private static void ReportDuplicateObjectMemberInitializers(BoundExpression boun
}
}

#nullable enable
private static void CheckRequiredMembersInObjectInitializer(
MethodSymbol constructor,
ImmutableArray<BoundExpression> initializers,
SyntaxNode creationSyntax,
BindingDiagnosticBag diagnostics)
{
if (!constructor.ShouldCheckRequiredMembers())
{
return;
}

if (constructor.ContainingType.HasRequiredMembersError)
{
// An error will be reported on the constructor if from source, or a use-site diagnostic will be reported on the use if from metadata.
return;
}

var requiredMembers = constructor.ContainingType.AllRequiredMembers;

if (requiredMembers.Count == 0)
{
return;
}

var requiredMembersBuilder = requiredMembers.ToBuilder();

if (initializers.IsDefaultOrEmpty)
{
reportMembers();
return;
}

foreach (var initializer in initializers)
{
if (initializer is not BoundAssignmentOperator assignmentOperator)
{
continue;
}

var memberSymbol = assignmentOperator.Left switch
{
// Regular initializers
BoundObjectInitializerMember member => member.MemberSymbol,
// Attribute initializers
BoundPropertyAccess propertyAccess => propertyAccess.PropertySymbol,
BoundFieldAccess fieldAccess => fieldAccess.FieldSymbol,
// Error cases
_ => null
};

if (memberSymbol is null)
{
continue;
}

if (!requiredMembersBuilder.TryGetValue(memberSymbol.Name, out var requiredMember))
{
continue;
}

if (!memberSymbol.Equals(requiredMember, TypeCompareKind.ConsiderEverything))
{
continue;
}

requiredMembersBuilder.Remove(memberSymbol.Name);

if (assignmentOperator.Right is BoundObjectInitializerExpressionBase initializerExpression)
{
// Required member '{0}' must be assigned a value, it cannot use a nested member or collection initializer.
diagnostics.Add(ErrorCode.ERR_RequiredMembersMustBeAssignedValue, initializerExpression.Syntax.Location, requiredMember);
}
}

reportMembers();

void reportMembers()
{
Location location = creationSyntax switch
{
ObjectCreationExpressionSyntax { Type: { } type } => type.Location,
BaseObjectCreationExpressionSyntax { NewKeyword: { } newKeyword } => newKeyword.GetLocation(),
AttributeSyntax { Name: { } name } => name.Location,
_ => creationSyntax.Location
};

foreach (var (_, member) in requiredMembersBuilder)
{
// Required member '{0}' must be set in the object initializer or attribute constructor.
diagnostics.Add(ErrorCode.ERR_RequiredMemberMustBeSet, location, member);
}
}
}
#nullable disable

private BoundCollectionInitializerExpression BindCollectionInitializerExpression(
InitializerExpressionSyntax initializerSyntax,
TypeSymbol initializerType,
Expand Down Expand Up @@ -5365,7 +5462,8 @@ protected BoundExpression BindClassCreationExpression(
diagnostics,
out MemberResolutionResult<MethodSymbol> memberResolutionResult,
out ImmutableArray<MethodSymbol> candidateConstructors,
allowProtectedConstructorsOfBaseType: false) &&
allowProtectedConstructorsOfBaseType: false,
suppressUnsupportedRequiredMembersError: false) &&
!type.IsAbstract)
{
var method = memberResolutionResult.Member;
Expand Down Expand Up @@ -5411,7 +5509,7 @@ protected BoundExpression BindClassCreationExpression(
}

boundInitializerOpt = makeBoundInitializerOpt();
result = new BoundObjectCreationExpression(
var creation = new BoundObjectCreationExpression(
node,
method,
candidateConstructors,
Expand All @@ -5427,7 +5525,9 @@ protected BoundExpression BindClassCreationExpression(
type,
hasError);

return result;
CheckRequiredMembersInObjectInitializer(creation.Constructor, creation.InitializerExpressionOpt?.Initializers ?? default, creation.Syntax, diagnostics);

return creation;
}

LookupResultKind resultKind;
Expand Down Expand Up @@ -5699,7 +5799,8 @@ internal bool TryPerformConstructorOverloadResolution(
BindingDiagnosticBag diagnostics,
out MemberResolutionResult<MethodSymbol> memberResolutionResult,
out ImmutableArray<MethodSymbol> candidateConstructors,
bool allowProtectedConstructorsOfBaseType) // Last to make named arguments more convenient.
bool allowProtectedConstructorsOfBaseType,
bool suppressUnsupportedRequiredMembersError) // Last to make named arguments more convenient.
{
// Get accessible constructors for performing overload resolution.
ImmutableArray<MethodSymbol> allInstanceConstructors;
Expand Down Expand Up @@ -5747,7 +5848,25 @@ internal bool TryPerformConstructorOverloadResolution(
}
}

diagnostics.Add(errorLocation, useSiteInfo);
if (suppressUnsupportedRequiredMembersError && useSiteInfo.AccumulatesDiagnostics && useSiteInfo.Diagnostics is { Count: not 0 })
{
diagnostics.AddDependencies(useSiteInfo);
foreach (var diagnostic in useSiteInfo.Diagnostics)
{
// We don't want to report this error here because we'll report ERR_RequiredMembersBaseTypeInvalid. That error is suppressable by the
// user using the `SetsRequiredMembers` attribute on the constructor, so reporting this error would prevent that from working.
if ((ErrorCode)diagnostic.Code == ErrorCode.ERR_RequiredMembersInvalid)
{
continue;
}

diagnostics.ReportUseSiteDiagnostic(diagnostic, errorLocation);
}
}
else
{
diagnostics.Add(errorLocation, useSiteInfo);
}

if (succeededIgnoringAccessibility)
{
Expand Down
12 changes: 12 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -7007,6 +7007,18 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_RequiredMemberMustBeSettable" xml:space="preserve">
<value>Required member '{0}' must be settable.</value>
</data>
<data name="ERR_RequiredMemberMustBeSet" xml:space="preserve">
<value>Required member '{0}' must be set in the object initializer or attribute constructor.</value>
</data>
<data name="ERR_RequiredMembersMustBeAssignedValue" xml:space="preserve">
<value>Required member '{0}' must be assigned a value, it cannot use a nested member or collection initializer.</value>
</data>
<data name="ERR_RequiredMembersInvalid" xml:space="preserve">
<value>The required members list for '{0}' is malformed and cannot be interpreted.</value>
</data>
<data name="ERR_RequiredMembersBaseTypeInvalid" xml:space="preserve">
<value>The required members list for the base type '{0}' is malformed and cannot be interpreted. To use this constructor, apply the 'SetsRequiredMembers' attribute.</value>
</data>
<data name="ERR_LineContainsDifferentWhitespace" xml:space="preserve">
<value>Line contains different whitespace than the closing line of the raw string literal: '{0}' versus '{1}'</value>
</data>
Expand Down
4 changes: 4 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2062,5 +2062,9 @@ internal enum ErrorCode
ERR_RequiredMemberCannotBeLessVisibleThanContainingType = 9503,
ERR_ExplicitRequiredMember = 9504,
ERR_RequiredMemberMustBeSettable = 9505,
ERR_RequiredMemberMustBeSet = 9506,
ERR_RequiredMembersMustBeAssignedValue = 9507,
ERR_RequiredMembersInvalid = 9508,
ERR_RequiredMembersBaseTypeInvalid = 9509,
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1346,6 +1346,11 @@ internal override UseSiteInfo<AssemblySymbol> GetUseSiteInfo()
}
}

if (diagnosticInfo == null && this.ShouldCheckRequiredMembers() && ContainingType.HasRequiredMembersError)
{
diagnosticInfo = new CSDiagnosticInfo(ErrorCode.ERR_RequiredMembersInvalid, ContainingType);
}

return InitializeUseSiteDiagnostic(result.AdjustDiagnosticInfo(diagnosticInfo));
}

Expand Down
126 changes: 126 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/NamedTypeSymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
using System.Linq;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Threading;
using Microsoft.CodeAnalysis.Collections;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Symbols;
using Roslyn.Utilities;
Expand All @@ -24,6 +26,14 @@ internal abstract partial class NamedTypeSymbol : TypeSymbol, INamedTypeSymbolIn
{
private bool _hasNoBaseCycles;

private static readonly ImmutableSegmentedDictionary<string, Symbol> RequiredMembersErrorSentinel = ImmutableSegmentedDictionary<string, Symbol>.Empty.Add("<error sentinel>", null!);

/// <summary>
/// <see langword="default"/> if uninitialized. <see cref="RequiredMembersErrorSentinel"/> if there are errors. <see cref="ImmutableSegmentedDictionary{TKey, TValue}.Empty"/> if
/// there are no required members. Otherwise, the required members.
/// </summary>
private ImmutableSegmentedDictionary<string, Symbol> _lazyRequiredMembers = default;

// Only the compiler can create NamedTypeSymbols.
internal NamedTypeSymbol(TupleExtraData tupleData = null)
{
Expand Down Expand Up @@ -499,6 +509,122 @@ internal abstract bool MangleName
/// </summary>
internal abstract bool HasDeclaredRequiredMembers { get; }

#nullable enable
/// <summary>
/// Whether the type encountered an error while trying to build its complete list of required members.
/// </summary>
internal bool HasRequiredMembersError
{
get
{
CalculateRequiredMembersIfRequired();
Debug.Assert(!_lazyRequiredMembers.IsDefault);
return _lazyRequiredMembers == RequiredMembersErrorSentinel;
}
}

/// <summary>
/// The full list of all required members for this type, including from base classes. If <see cref="HasRequiredMembersError"/> is true,
/// this returns empty.
/// </summary>
/// <remarks>
/// Do not call this API if all you need are the required members declared on this type. Use <see cref="GetMembers()"/> instead, filtering for
/// required members, instead of calling this API.
/// </remarks>
internal ImmutableSegmentedDictionary<string, Symbol> AllRequiredMembers
{
get
{
CalculateRequiredMembersIfRequired();
Debug.Assert(!_lazyRequiredMembers.IsDefault);
if (_lazyRequiredMembers == RequiredMembersErrorSentinel)
{
return ImmutableSegmentedDictionary<string, Symbol>.Empty;
}

return _lazyRequiredMembers;
}
}

private void CalculateRequiredMembersIfRequired()
{
if (!_lazyRequiredMembers.IsDefault)
{
return;
}

ImmutableSegmentedDictionary<string, Symbol>.Builder? builder = null;
bool success = TryCalculateRequiredMembers(ref builder);

var requiredMembers = success
? builder?.ToImmutable() ?? ImmutableSegmentedDictionary<string, Symbol>.Empty
: RequiredMembersErrorSentinel;

RoslynImmutableInterlocked.InterlockedInitialize(ref _lazyRequiredMembers, requiredMembers);
}

/// <summary>
/// Attempts to calculate the required members for this type. Returns false if there were errors.
/// </summary>
private bool TryCalculateRequiredMembers(ref ImmutableSegmentedDictionary<string, Symbol>.Builder? requiredMembersBuilder)
{
var lazyRequiredMembers = _lazyRequiredMembers;
if (_lazyRequiredMembers == RequiredMembersErrorSentinel)
{
if (lazyRequiredMembers.IsDefault)
{
return false;
}
else
{
requiredMembersBuilder = lazyRequiredMembers.ToBuilder();
return true;
}
}

if (BaseTypeNoUseSiteDiagnostics?.TryCalculateRequiredMembers(ref requiredMembersBuilder) == false)
{
return false;
}

// We need to make sure that members from a base type weren't hidden by members from the current type.
if (!HasDeclaredRequiredMembers && requiredMembersBuilder == null)
{
return true;
}

return addCurrentTypeMembers(ref requiredMembersBuilder);

bool addCurrentTypeMembers(ref ImmutableSegmentedDictionary<string, Symbol>.Builder? requiredMembersBuilder)
{
requiredMembersBuilder ??= ImmutableSegmentedDictionary.CreateBuilder<string, Symbol>();

foreach (var member in GetMembersUnordered())
{
if (requiredMembersBuilder.ContainsKey(member.Name))
{
// This is only permitted if the member is an override of a required member from a base type, and is required itself.
if (!member.IsRequired()
|| member.GetOverriddenMember() is not { } overriddenMember
|| !overriddenMember.Equals(requiredMembersBuilder[member.Name], TypeCompareKind.ConsiderEverything))
{
return false;
}
}

if (!member.IsRequired())
{
continue;
}

requiredMembersBuilder[member.Name] = member;
}

return true;
}
}
#nullable disable

/// <summary>
/// Get all the members of this symbol.
/// </summary>
Expand Down
Loading

0 comments on commit 47c81fe

Please sign in to comment.