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

Error when required members are not set #60101

Merged
merged 8 commits into from
Mar 17, 2022
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
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)
333fred marked this conversation as resolved.
Show resolved Hide resolved
{
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
333fred marked this conversation as resolved.
Show resolved Hide resolved
};

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 })
333fred marked this conversation as resolved.
Show resolved Hide resolved
{
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
122 changes: 122 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;

/// <remarks>
/// This field cannot be used to determine whether <see cref="_lazyRequiredMembers"/> has been initialized.
/// Ensure that <see cref="_lazyRequiredMembers"/> is checked for defaultness _before_ reading this field, which potentially
/// means using an `Interlocked.MemoryBarrier()` to ensure that reading this location is not reordered before the read to <see cref="_lazyRequiredMembers"/>.
/// </remarks>
private bool _lazyHasRequiredMembersErrorDoNotAccessDirectly = false;
private ImmutableSegmentedDictionary<string, Symbol> _lazyRequiredMembers;

// Only the compiler can create NamedTypeSymbols.
internal NamedTypeSymbol(TupleExtraData tupleData = null)
{
Expand Down Expand Up @@ -499,6 +509,118 @@ 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 _lazyHasRequiredMembersErrorDoNotAccessDirectly;
}
}

/// <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);
return _lazyRequiredMembers;
}
}

private void CalculateRequiredMembersIfRequired()
{
if (_lazyRequiredMembers.IsDefault)
{
ImmutableSegmentedDictionary<string, Symbol>.Builder? builder = null;
bool success = TryCalculateRequiredMembers(ref builder);
var requiredMembers = success
? builder?.ToImmutable() ?? ImmutableSegmentedDictionary<string, Symbol>.Empty
: ImmutableSegmentedDictionary<string, Symbol>.Empty;

_lazyHasRequiredMembersErrorDoNotAccessDirectly = !success;
// InterlockedInitialize uses `Interlocked.CompareExchange` under the hood, which will introduce a full
// memory barrier, ensuring that the _lazyHasRequiredMembersErrorDoNotAccessDirectly write is not reordered
// after the initialization of _lazyRequiredMembers.
RoslynImmutableInterlocked.InterlockedInitialize(ref _lazyRequiredMembers, requiredMembers);
}
else
{
// Ensure that, after this method, _lazyRequiredMembersErrorDoNotAccessDirectly is able to be accessed safely.
Interlocked.MemoryBarrier();
333fred marked this conversation as resolved.
Show resolved Hide resolved
}
}

/// <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.IsDefault)
{
requiredMembersBuilder = lazyRequiredMembers.ToBuilder();
// Ensure that this read is not reordered before the read to `IsDefault`.
Interlocked.MemoryBarrier();
return !_lazyHasRequiredMembersErrorDoNotAccessDirectly;
}

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