Skip to content

Commit

Permalink
PR Feedback.
Browse files Browse the repository at this point in the history
  • Loading branch information
@333fred
333fred committed Apr 15, 2022
1 parent 728b6da commit 106378e
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 23 deletions.
35 changes: 20 additions & 15 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -886,23 +886,28 @@ ImmutableArray<Symbol> getMembersNeedingDefaultInitialState()
return ImmutableArray<Symbol>.Empty;
}

bool includeRequiredMembers = true;
bool includeCurrentTypeRequiredMembers = true;
bool includeBaseRequiredMembers = true;
bool hasThisConstructorInitializer = false;

if (method is SourceMemberMethodSymbol { SyntaxNode: ConstructorDeclarationSyntax { Initializer: { RawKind: var initializerKind } } })
{
// We have multiple ways of entering the nullable walker: we could be just analyzing the initializers, with a BoundStatementList body and _baseOrThisInitializer
// having been provided, or we could be analyzing the body of a constructor, with a BoundConstructorBody body and _baseOrThisInitializer being null.
var baseOrThisInitializer = (_baseOrThisInitializer ?? GetConstructorThisOrBaseSymbol(this.methodMainNode));
// If there's an error in the base or this initializer, presume that we should set all required members to default.
includeBaseRequiredMembers = baseOrThisInitializer?.ShouldCheckRequiredMembers() ?? true;
if (initializerKind == (int)SyntaxKind.ThisConstructorInitializer)
{
hasThisConstructorInitializer = true;
includeRequiredMembers = includeBaseRequiredMembers;
// If we chained to a `this` constructor, a SetsRequiredMembers attribute applies to both the current type's required members and the base type's required members.
includeCurrentTypeRequiredMembers = includeBaseRequiredMembers;
}
else if (initializerKind == (int)SyntaxKind.BaseConstructorInitializer)
{
includeRequiredMembers = true;
// If we chained to a `base` constructor, a SetsRequiredMembers attribute applies to the base type's required members only, and the current type's required members
// are not assumed to be initialized.
includeCurrentTypeRequiredMembers = true;
}
}

Expand All @@ -913,37 +918,37 @@ ImmutableArray<Symbol> getMembersNeedingDefaultInitialState()
|| method.IsStatic
|| compilation.IsFeatureEnabled(MessageID.IDS_FeatureAutoDefaultStructs)))
{
return membersToBeInitialized(method.ContainingType, includeAllMembers: true, includeRequiredMembers, includeBaseRequiredMembers);
return membersToBeInitialized(method.ContainingType, includeAllMembers: true, includeCurrentTypeRequiredMembers, includeBaseRequiredMembers);
}

// We want to presume all required members of the type are uninitialized, and in addition we want to set all fields to
// default if we can get to this constructor by doing so (ie, : this() in a value type).
return membersToBeInitialized(method.ContainingType, includeAllMembers: method.IncludeFieldInitializersInBody(), includeRequiredMembers, includeBaseRequiredMembers);
return membersToBeInitialized(method.ContainingType, includeAllMembers: method.IncludeFieldInitializersInBody(), includeCurrentTypeRequiredMembers, includeBaseRequiredMembers);

static ImmutableArray<Symbol> membersToBeInitialized(NamedTypeSymbol containingType, bool includeAllMembers, bool includeRequiredMembers, bool includeBaseRequiredMembers)
static ImmutableArray<Symbol> membersToBeInitialized(NamedTypeSymbol containingType, bool includeAllMembers, bool includeCurrentTypeRequiredMembers, bool includeBaseRequiredMembers)
{
return (includeAllMembers, includeRequiredMembers, includeBaseRequiredMembers) switch
return (includeAllMembers, includeCurrentTypeRequiredMembers, includeBaseRequiredMembers) switch
{
(includeAllMembers: false, includeRequiredMembers: false, includeBaseRequiredMembers: false)
(includeAllMembers: false, includeCurrentTypeRequiredMembers: false, includeBaseRequiredMembers: false)
=> ImmutableArray<Symbol>.Empty,

(includeAllMembers: false, includeRequiredMembers: true, includeBaseRequiredMembers: false)
(includeAllMembers: false, includeCurrentTypeRequiredMembers: true, includeBaseRequiredMembers: false)
=> containingType.GetMembersUnordered().SelectAsArray(predicate: SymbolExtensions.IsRequired, selector: getFieldSymbolToBeInitialized),

(includeAllMembers: false, includeRequiredMembers: true, includeBaseRequiredMembers: true)
(includeAllMembers: false, includeCurrentTypeRequiredMembers: true, includeBaseRequiredMembers: true)
=> containingType.AllRequiredMembers.SelectAsArray(static kvp => getFieldSymbolToBeInitialized(kvp.Value)),

(includeAllMembers: true, includeRequiredMembers: _, includeBaseRequiredMembers: false)
(includeAllMembers: true, includeCurrentTypeRequiredMembers: _, includeBaseRequiredMembers: false)
=> containingType.GetMembersUnordered().SelectAsArray(getFieldSymbolToBeInitialized),

(includeAllMembers: true, includeRequiredMembers: true, includeBaseRequiredMembers: true)
=> getAllTypeAndRequiredMembers(),
(includeAllMembers: true, includeCurrentTypeRequiredMembers: true, includeBaseRequiredMembers: true)
=> getAllTypeAndRequiredMembers(containingType),

(includeAllMembers: _, includeRequiredMembers: false, includeBaseRequiredMembers: true)
(includeAllMembers: _, includeCurrentTypeRequiredMembers: false, includeBaseRequiredMembers: true)
=> throw ExceptionUtilities.Unreachable,
};

ImmutableArray<Symbol> getAllTypeAndRequiredMembers()
static ImmutableArray<Symbol> getAllTypeAndRequiredMembers(TypeSymbol containingType)
{
var members = containingType.GetMembersUnordered();
var requiredMembers = containingType.BaseTypeNoUseSiteDiagnostics?.AllRequiredMembers ?? ImmutableSegmentedDictionary<string, Symbol>.Empty;
Expand Down
6 changes: 0 additions & 6 deletions src/Compilers/CSharp/Test/Symbol/Symbols/RequiredMembersTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1277,7 +1277,6 @@ public class C
[CombinatorialData]
public void EnforcedRequiredMembers_NoInheritance_NoneSet_HasSetsRequiredMembers(bool useMetadataReference, [CombinatorialValues("", " C")] string constructor, [CombinatorialValues("", "method: ")] string target)
{
#pragma warning disable IDE0055 // Fix formatting
var c = $$"""
using System.Diagnostics.CodeAnalysis;
public class C
Expand All @@ -1289,7 +1288,6 @@ public class C
public C() {}
}
""";
#pragma warning restore IDE0055 // Fix formatting

var creation = $@"C c = new{constructor}();";
var comp = CreateCompilationWithRequiredMembers(new[] { c, creation });
Expand Down Expand Up @@ -2309,7 +2307,6 @@ private static string GetShadowingRecordIl(bool propertyIsRequired)
01 00 00 00
)
""" : "";
#pragma warning disable IDE0055 // Fix formatting
return $$"""
.assembly extern original {}
Expand Down Expand Up @@ -2473,7 +2470,6 @@ .set instance void modreq([mscorlib]System.Runtime.CompilerServices.IsExternalIn
} // end of class Derived
""";
#pragma warning restore IDE0055 // Fix formatting
}

[Fact]
Expand Down Expand Up @@ -3373,7 +3369,6 @@ public C(bool unused) : this()
[InlineData("")]
public void RequiredMemberSuppressesNullabilityWarnings_ChainedConstructor_04(string baseSyntax)
{
#pragma warning disable IDE0055 // Fix formatting
var code = $$"""
#nullable enable
public class Base
Expand All @@ -3395,7 +3390,6 @@ public Derived() {{baseSyntax}}
}
}
""";
#pragma warning restore IDE0055 // Fix formatting

var comp = CreateCompilationWithRequiredMembers(code);
comp.VerifyDiagnostics(
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/Test/Utilities/CSharp/CSharpTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -629,7 +629,7 @@ public UnmanagedCallersOnlyAttribute() { }
namespace System.Runtime.CompilerServices
{
[AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Field | AttributeTargets.Property, Inherited = false, AllowMultiple = false)]
public class RequiredMemberAttribute : Attribute
public sealed class RequiredMemberAttribute : Attribute
{
public RequiredMemberAttribute()
{
Expand All @@ -642,7 +642,7 @@ public RequiredMemberAttribute()
namespace System.Diagnostics.CodeAnalysis
{
[AttributeUsage(AttributeTargets.Constructor, Inherited = false, AllowMultiple = false)]
public class SetsRequiredMembersAttribute : Attribute
public sealed class SetsRequiredMembersAttribute : Attribute
{
public SetsRequiredMembersAttribute()
{
Expand Down

0 comments on commit 106378e

Please sign in to comment.