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

[semi-auto-props]: Support assigning properties from constructor #60601

Merged
merged 42 commits into from
May 12, 2022
Merged
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
884827e
[semi-auto-props]: Support assigning properties from constructor
Youssef1313 Apr 6, 2022
92abdde
Update tests
Youssef1313 Apr 6, 2022
ea5a0fc
Update tests
Youssef1313 Apr 6, 2022
ef9ed78
Update tests
Youssef1313 Apr 6, 2022
24acdbb
Fix cycle
Youssef1313 Apr 6, 2022
0e1283c
Unskip a test
Youssef1313 Apr 6, 2022
90e306b
Merge branch 'features/semi-auto-props' into semi-assign-ctor
Youssef1313 Apr 6, 2022
60e3cc6
Fix formatting
Youssef1313 Apr 6, 2022
e8f985a
Merge remote-tracking branch 'upstream/features/semi-auto-props' into…
Youssef1313 Apr 8, 2022
310585f
Update failing tests
Youssef1313 Apr 8, 2022
2b3703f
Support auto-default struct in conjunction of semi-auto-prop
Youssef1313 Apr 8, 2022
2b81ca9
Fix test
Youssef1313 Apr 8, 2022
67964c7
Remove assert
Youssef1313 Apr 8, 2022
ba5e951
Address some review comments
Youssef1313 Apr 16, 2022
6925606
Reorder conditions
Youssef1313 Apr 17, 2022
6ff3c8d
Chain VerifyDiagnostics call
Youssef1313 Apr 17, 2022
d1208dd
Test struct
Youssef1313 Apr 17, 2022
a80af8a
Update assert
Youssef1313 Apr 17, 2022
6119c22
Address feedback
Youssef1313 Apr 19, 2022
1e62ba7
Address part of review
Youssef1313 Apr 22, 2022
28e32eb
More review comments
Youssef1313 Apr 22, 2022
29322b9
Address small part of feedback
Youssef1313 Apr 23, 2022
44fd4aa
Update some tests and fix a NRE
Youssef1313 Apr 24, 2022
e441ad3
Move struct circularity to CompilationStage.Compile
Youssef1313 Apr 24, 2022
f530c53
Assert non-null diagnostic bag
Youssef1313 Apr 24, 2022
a0e2749
Small fix
Youssef1313 Apr 25, 2022
ea57d56
Fix test
Youssef1313 Apr 27, 2022
45937be
Fix tests
Youssef1313 Apr 27, 2022
e89e6b8
Reduce binding, add test for static property
Youssef1313 Apr 28, 2022
bcf6974
Simplify
Youssef1313 Apr 28, 2022
b95f088
Address part of review comments
Youssef1313 Apr 29, 2022
e5abf64
Update test
Youssef1313 Apr 29, 2022
c5e8afe
WaitForWorkers
Youssef1313 Apr 30, 2022
08eb543
Merge remote-tracking branch 'upstream/features/semi-auto-props' into…
Youssef1313 Apr 30, 2022
a93f24a
Fix definite assignment for field keyword
Youssef1313 Apr 30, 2022
69a89bf
Update AbstractFlowPass.cs
Youssef1313 Apr 30, 2022
6553698
Update BaseTypeAnalysis.cs
Youssef1313 May 4, 2022
0b3dcee
Address review comments
Youssef1313 May 5, 2022
3de5181
Address review comment
Youssef1313 May 5, 2022
fcb5e3f
Fix assertion failure
Youssef1313 May 5, 2022
7ea8b7f
Merge branch 'features/semi-auto-props' into semi-assign-ctor
Youssef1313 May 10, 2022
c42ed4a
Add test
Youssef1313 May 11, 2022
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
11 changes: 4 additions & 7 deletions src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1703,18 +1703,15 @@ private static bool IsPropertyAssignedThroughBackingField(BoundExpression receiv
propertySymbol = propertySymbol.OriginalDefinition;
}

// PROTOTYPE(semi-auto-props): TODO: Support assigning semi auto prop from constructors.

return propertySymbol is SourcePropertySymbolBase sourceProperty &&
// PROTOTYPE(semi-auto-props): Consider `public int P { get => field; set; }`
sourceProperty.GetMethod is SourcePropertyAccessorSymbol { IsEquivalentToBackingFieldAccess: true } &&
IsConstructorOrField(fromMember, isStatic: sourceProperty.IsStatic) &&
TypeSymbol.Equals(sourceProperty.ContainingType, fromMember.ContainingType, TypeCompareKind.ConsiderEverything2) &&
(sourceProperty.IsStatic || receiver.Kind == BoundKind.ThisReference) &&
// To be assigned through backing field, either SetMethod is null, or it's equivalent to backing field write
// PROTOTYPE(semi-auto-props): TODO: Do we need to use `GetOwnOrInheritedSetMethod` instead of `SetMethod`?
// Legacy auto-properties are required to override all accessors. If this is not the case with semi auto props, we may need to use GetOwnOrInheritedSetMethod
sourceProperty.SetMethod is null or SourcePropertyAccessorSymbol { IsEquivalentToBackingFieldAccess: true } &&
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
TypeSymbol.Equals(sourceProperty.ContainingType, fromMember.ContainingType, TypeCompareKind.ConsiderEverything2) &&
IsConstructorOrField(fromMember, isStatic: sourceProperty.IsStatic) &&
(sourceProperty.IsStatic || receiver.Kind == BoundKind.ThisReference);
(sourceProperty.GetMethod is SourcePropertyAccessorSymbol { IsEquivalentToBackingFieldAccess: true } || sourceProperty.FieldKeywordBackingField is not null);
}

private static bool IsConstructorOrField(Symbol member, bool isStatic)
Expand Down
12 changes: 12 additions & 0 deletions src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2581,7 +2581,19 @@ internal DiagnosticBag AdditionalCodegenWarnings
}
}

/// <summary>
/// A bag in which circular struct diagnostics should be reported.
/// </summary>
internal DiagnosticBag CircularStructDiagnostics
{
get
{
return _circularStructDiagnostics;
}
}

private readonly DiagnosticBag _additionalCodegenWarnings = new DiagnosticBag();
private readonly DiagnosticBag _circularStructDiagnostics = new DiagnosticBag();

internal DeclarationTable Declarations
{
Expand Down
112 changes: 95 additions & 17 deletions src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,9 @@ public static void CompileMethodBodies(
new LocalizableResourceString(messageResourceName, CodeAnalysisResources.ResourceManager, typeof(CodeAnalysisResources)));
}

addCircularStructDiagnostics(compilation.GlobalNamespace);
Copy link
Contributor

@AlekseyTs AlekseyTs May 5, 2022

Choose a reason for hiding this comment

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

compilation.GlobalNamespace

It looks like we are traversing things from added modules and referenced assemblies. See methodCompiler.CompileNamespace call above. #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.

Yeah compilation.GlobalNamespace is alot of unnecessary work. I'm switching to compilation.SourceModule.GlobalNamespace. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah compilation.GlobalNamespace is alot of unnecessary work.

It not only a lot of unnecessary work, but also the work that is likely to have negative impact on referenced compilations (work done out of "order").

methodCompiler.WaitForWorkers();
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
diagnostics.AddRange(compilation.CircularStructDiagnostics);
diagnostics.AddRange(compilation.AdditionalCodegenWarnings);

// we can get unused field warnings only if compiling whole compilation.
Expand All @@ -214,6 +217,45 @@ public static void CompileMethodBodies(
moduleBeingBuiltOpt.SetPEEntryPoint(entryPoint, diagnostics.DiagnosticBag);
}
}

void addCircularStructDiagnostics(NamespaceOrTypeSymbol symbol)
{
if (symbol is SourceMemberContainerTypeSymbol sourceMemberContainerTypeSymbol && PassesFilter(filterOpt, symbol))
{
_ = sourceMemberContainerTypeSymbol.KnownCircularStruct;
}

foreach (var member in symbol.GetMembersUnordered())
{
if (member is NamespaceOrTypeSymbol namespaceOrTypeSymbol)
{
if (compilation.Options.ConcurrentBuild)
{
Task worker = addCircularStructDiagnosticsAsAsync(namespaceOrTypeSymbol);
methodCompiler._compilerTasks.Push(worker);
}
else
{
addCircularStructDiagnostics(namespaceOrTypeSymbol);
}
}
}
}

Task addCircularStructDiagnosticsAsAsync(NamespaceOrTypeSymbol symbol)
{
return Task.Run(UICultureUtilities.WithCurrentUICulture(() =>
{
try
{
addCircularStructDiagnostics(symbol);
}
catch (Exception e) when (FatalError.ReportAndPropagateUnlessCanceled(e))
{
throw ExceptionUtilities.Unreachable;
}
}), methodCompiler._cancellationToken);
}
}

// Returns the MethodSymbol for the assembly entrypoint. If the user has a Task returning main,
Expand Down Expand Up @@ -471,6 +513,43 @@ private void CompileNamedType(NamedTypeSymbol containingType)
}

var members = containingType.GetMembers();

// We first compile the accessors that contain 'field' keyword to avoid extra bindings for 'field' keyword when compiling other members.
for (int memberOrdinal = 0; memberOrdinal < members.Length; memberOrdinal++)
{
var member = members[memberOrdinal];

//When a filter is supplied, limit the compilation of members passing the filter.
if (member is not SourcePropertyAccessorSymbol { ContainsFieldKeyword: true } accessor ||
!PassesFilter(_filterOpt, member))
{
continue;
}

Debug.Assert(member.Kind == SymbolKind.Method);
Binder.ProcessedFieldInitializers processedInitializers = default;
CompileMethod(accessor, memberOrdinal, ref processedInitializers, synthesizedSubmissionFields, compilationState);
}

// After compiling accessors containing 'field' keyword, we mark the backing field of the corresponding property as calculated.
foreach (var member in members)
{
if (member is SourcePropertySymbolBase property)
{
// PROTOTYPE(semi-auto-props): This can be optimized by checking for field keyword syntactically first.
// If we don't have field keyword, we can safely ignore the filter check.
// This will require more tests.
var getMethod = property.GetMethod;
var setMethod = property.SetMethod;
if ((getMethod is null || PassesFilter(_filterOpt, getMethod)) &&
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
(setMethod is null || PassesFilter(_filterOpt, setMethod)))
{
property.MarkBackingFieldAsCalculated();
}
}
}

// Then we compile everything, excluding the accessors that contain field keyword we already compiled in the loop above.
for (int memberOrdinal = 0; memberOrdinal < members.Length; memberOrdinal++)
{
var member = members[memberOrdinal];
Expand All @@ -490,6 +569,22 @@ private void CompileNamedType(NamedTypeSymbol containingType)
case SymbolKind.Method:
{
MethodSymbol method = (MethodSymbol)member;
if (method.MethodKind is MethodKind.PropertyGet or MethodKind.PropertySet)
{
// The loop for compiling accessors was written with these valid assumptions.
// If this requirement has changed, the loop above may need to be modified (e.g, if partial accessors was allowed in future)
Debug.Assert(!method.IsScriptConstructor);
Debug.Assert((object)method != scriptEntryPoint);
Debug.Assert(!IsFieldLikeEventAccessor(method));
Debug.Assert(!method.IsPartialDefinition());
}

if (member is SourcePropertyAccessorSymbol { ContainsFieldKeyword: true })
{
// We already compiled these accessors in the loop above.
continue;
}

if (method.IsScriptConstructor)
{
Debug.Assert(scriptCtorOrdinal == -1);
Expand Down Expand Up @@ -574,23 +669,6 @@ private void CompileNamedType(NamedTypeSymbol containingType)
}
}

foreach (var member in members)
{
if (member is SourcePropertySymbolBase property)
{
// PROTOTYPE(semi-auto-props): This can be optimized by checking for field keyword syntactically first.
// If we don't have field keyword, we can safely ignore the filter check.
// This will require more tests.
var getMethod = property.GetMethod;
var setMethod = property.SetMethod;
if ((getMethod is null || PassesFilter(_filterOpt, getMethod)) &&
(setMethod is null || PassesFilter(_filterOpt, setMethod)))
{
property.MarkBackingFieldAsCalculated();
}
}
}

Debug.Assert(containingType.IsScriptClass == (scriptCtorOrdinal >= 0));

// process additional anonymous type members
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,8 @@ protected void VisitLvalue(BoundExpression node)

if (Binder.IsPropertyAssignedThroughBackingField(access, _symbol))
{
var backingField = (access.PropertySymbol as SourcePropertySymbolBase)?.BackingField;
var property = access.PropertySymbol as SourcePropertySymbolBase;
var backingField = property?.BackingField ?? property?.FieldKeywordBackingField;
if (backingField != null)
{
VisitFieldAccessInternal(access.ReceiverOpt, backingField);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1038,14 +1038,13 @@ protected override bool TryGetReceiverAndMember(BoundExpression expr, out BoundE
case BoundKind.PropertyAccess:
{
var propAccess = (BoundPropertyAccess)expr;

// PROTOTYPE(semi-auto-props): The call to IsPropertyAssignedThroughBackingField isn't sensible.
// We get here for both property read and write.
// This applies to ALL IsPropertyAssignedThroughBackingField calls in this file.
if (Binder.IsPropertyAssignedThroughBackingField(propAccess, this.CurrentSymbol)) // PROTOTYPE(semi-auto-props): Revise this method call is the behavior we want and add unit tests..
{
var propSymbol = propAccess.PropertySymbol;
member = (propSymbol as SourcePropertySymbolBase)?.BackingField;
var propSymbol = propAccess.PropertySymbol as SourcePropertySymbolBase;
member = propSymbol?.BackingField ?? propSymbol?.FieldKeywordBackingField;
if (member is null)
{
return false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,12 +215,15 @@ private FieldSymbol GetActualField(Symbol member, NamedTypeSymbol type)
return (!eventSymbol.HasAssociatedField || ShouldIgnoreStructField(eventSymbol, eventSymbol.Type)) ? null : eventSymbol.AssociatedField.AsMember(type);
case SymbolKind.Property:
// PROTOTYPE(semi-auto-props): Review other event associated field callers and see if we have to do anything special for properties.

// Backing field for semi auto props are not included in GetMembers.
// PROTOTYPE(semi-auto-props): This condition is always false after modifiying BackingField implementation. Figure out a
// test that needs this.
if (member is SourcePropertySymbol { BackingField: SynthesizedBackingFieldSymbol { IsCreatedForFieldKeyword: true } backingField })
if (member is SourcePropertySymbol property)
{
return backingField;
// PROTOTYPE(semi-auto-props): Add definite assignment tests involving initializer, including error scenarios where a FieldKeywordBackingField is still created.
if (property.FieldKeywordBackingField is { } backingField)
{
return backingField;
}
}
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -301,11 +301,11 @@ private BoundExpression MakePropertyAssignment(
if (setMethod is null)
{
var autoProp = (SourcePropertySymbolBase)property.OriginalDefinition;
Debug.Assert(autoProp.IsAutoPropertyWithGetAccessor,
Debug.Assert(autoProp.GetMethod is SourcePropertyAccessorSymbol { IsEquivalentToBackingFieldAccess: true } || autoProp.FieldKeywordBackingField is not null,
"only autoproperties can be assignable without having setters");
Debug.Assert(property.Equals(autoProp, TypeCompareKind.IgnoreNullableModifiersForReferenceTypes));

var backingField = autoProp.BackingField;
var backingField = autoProp.BackingField ?? autoProp.FieldKeywordBackingField;
Debug.Assert(backingField is not null);

return _factory.AssignmentExpression(
Expand Down
20 changes: 17 additions & 3 deletions src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,23 @@ private static void StructDependsClosure(NamedTypeSymbol type, HashSet<Symbol> p
{
foreach (var member in type.GetMembersUnordered())
{
var field = member as FieldSymbol;
var fieldType = field?.NonPointerType();
if (fieldType is null || fieldType.TypeKind != TypeKind.Struct || field.IsStatic)
FieldSymbol field;
if (member is SourcePropertySymbolBase { IsStatic: false, FieldKeywordBackingField: { } fieldKeywordField })
333fred marked this conversation as resolved.
Show resolved Hide resolved
{
field = fieldKeywordField;
}
else if (member.Kind == SymbolKind.Field && !member.IsStatic)
{
field = (FieldSymbol)member;
}
else
{
continue;
}

Debug.Assert(!field.IsStatic);
var fieldType = field.NonPointerType();
if (fieldType is null || fieldType.TypeKind != TypeKind.Struct)
{
continue;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1617,8 +1617,6 @@ protected void AfterMembersChecks(BindingDiagnosticBag diagnostics)
CheckTypeParameterNameConflicts(diagnostics);
CheckAccessorNameConflicts(diagnostics);

bool unused = KnownCircularStruct;

CheckSequentialOnPartialType(diagnostics);
CheckForProtectedInStaticClass(diagnostics);
CheckForUnmatchedOperators(diagnostics);
Expand Down Expand Up @@ -2133,11 +2131,13 @@ internal override bool KnownCircularStruct
else
{
var diagnostics = BindingDiagnosticBag.GetInstance();
Debug.Assert(diagnostics.DiagnosticBag is not null);
var value = (int)CheckStructCircularity(diagnostics).ToThreeState();

if (Interlocked.CompareExchange(ref _lazyKnownCircularStruct, value, (int)ThreeState.Unknown) == (int)ThreeState.Unknown)
{
AddDeclarationDiagnostics(diagnostics);
DeclaringCompilation.AddUsedAssemblies(diagnostics.DependenciesBag);
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
DeclaringCompilation.CircularStructDiagnostics.AddRange(diagnostics.DiagnosticBag);
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
}

Debug.Assert(value == _lazyKnownCircularStruct);
Expand All @@ -2163,16 +2163,22 @@ private bool HasStructCircularity(BindingDiagnosticBag diagnostics)
{
foreach (var member in valuesByName)
{
if (member.Kind != SymbolKind.Field)
FieldSymbol field;
if (member is SourcePropertySymbolBase { IsStatic: false, FieldKeywordBackingField: { } fieldKeywordField })
{
// NOTE: don't have to check field-like events, because they can't have struct types.
continue;
field = fieldKeywordField;
}
var field = (FieldSymbol)member;
if (field.IsStatic)
else if (member.Kind == SymbolKind.Field && !member.IsStatic)
{
field = (FieldSymbol)member;
}
else
{
// NOTE: don't have to check field-like events, because they can't have struct types.
continue;
}

Debug.Assert(!field.IsStatic);
var type = field.NonPointerType();
if (((object)type != null) &&
(type.TypeKind == TypeKind.Struct) &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,7 @@ internal Accessibility LocalAccessibility
/// This is only calculated from syntax, so we don't know if it
/// will bind to something or will create a backing field.
/// </remarks>
// PROTOTYPE(semi-auto-props): Rename to ContainsFieldKeywordSyntactically or similar.
internal bool ContainsFieldKeyword => _containsFieldKeyword;

/// <summary>
Expand Down
Loading