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.

Sign up for GitHub

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

Jump to bottom

[semi-auto-props]: Avoid using IsAutoProperty when possible #59109

Merged
merged 7 commits into from
Feb 8, 2022
Merged

[semi-auto-props]: Avoid using IsAutoProperty when possible #59109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
694bdf4
[semi-auto-props]: Minor refactoring
Youssef1313 Jan 27, 2022
aa5826d
Cleanup
Youssef1313 Jan 27, 2022
978ffaf
Rename and revert bad change
Youssef1313 Jan 27, 2022
e98aac1
Remove unused parameter
Youssef1313 Jan 27, 2022
cf8c473
Address feedback
Youssef1313 Feb 3, 2022
2848e88
Update UninitializedNonNullableFieldTests.cs
Youssef1313 Feb 4, 2022
ed988e9
Update implementation
Youssef1313 Feb 5, 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
42 changes: 35 additions & 7 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,8 @@
using System;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.Linq;
using System.Runtime.CompilerServices;
using System.Threading;
using Microsoft.CodeAnalysis.CSharp.Emit;
using Microsoft.CodeAnalysis.CSharp.Syntax;
Expand Down Expand Up @@ -319,7 +317,7 @@ internal bool IsExpressionBodied
=> (_propertyFlags & Flags.IsExpressionBodied) != 0;

private void CheckInitializer(
bool isAutoProperty,
bool allowsInitializer,
bool isInterface,
bool isStatic,
Location location,
Expand All @@ -329,7 +327,7 @@ private void CheckInitializer(
{
diagnostics.Add(ErrorCode.ERR_InstancePropertyInitializerInInterface, location, this);
}
else if (!isAutoProperty)
else if (!allowsInitializer)
{
diagnostics.Add(ErrorCode.ERR_InitializerOnNonAutoProperty, location, this);
}
Expand Down Expand Up @@ -783,6 +781,36 @@ internal SynthesizedBackingFieldSymbol? FieldKeywordBackingField
return null;
}
}

private bool AllowInitializer
{
get
{
// PROTOTYPE(semi-auto-props): Fix implementation for semi auto properties.
return (_setMethod is null && _getMethod?.BodyShouldBeSynthesized == true) ||
_setMethod?.BodyShouldBeSynthesized == true;
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
}
}

private bool AllowFieldAttributeTarget
{
get
{
// PROTOTYPE(semi-auto-props): Fix implementation for semi auto properties.
return _getMethod?.BodyShouldBeSynthesized == true ||
_setMethod?.BodyShouldBeSynthesized == true;
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure if using the same implementation as for AllowInitializer is the right thing to do. Earlier I was suggesting the following: "Could this be as simple as checking if getter exists and is auto-implemented instead?" It feels like that logic will exactly match the name of the property that we are replacing, IsAutoPropertyWithGetAccessor. I could imagine even more relaxed implementation. Something like

_getMethod?.BodyShouldBeSynthesized == true ||
                    _setMethod?.BodyShouldBeSynthesized == true

For an attribute, we just need to have a field. Requirements for an initializer are more strict. Especially taking https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-01-12.md#conclusion into consideration. #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. 

_getMethod?.BodyShouldBeSynthesized == true ||
                    _setMethod?.BodyShouldBeSynthesized == true

@AlekseyTs Both of the logics don't seem to take semi auto properties into account, unless I'm missing something. For example, public string S { get => field; } should be allowed to have a field attribute target. But I agree your logic is a bit more accurate.

}
}

private bool DisallowRefLikeTypes
{
get
{
// PROTOTYPE(semi-auto-props): Fix implementation for semi auto properties.
return _getMethod?.BodyShouldBeSynthesized == true ||
_setMethod?.BodyShouldBeSynthesized == true;
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
}
}
#nullable disable

internal override bool MustCallMethodsDirectly
Expand Down Expand Up @@ -824,7 +852,7 @@ internal override void AfterAddingTypeMembersChecks(ConversionsBase conversions,
bool hasInitializer = (_propertyFlags & Flags.HasInitializer) != 0;
if (hasInitializer)
{
CheckInitializer(IsAutoProperty, ContainingType.IsInterface, IsStatic, Location, diagnostics);
CheckInitializer(AllowInitializer, ContainingType.IsInterface, IsStatic, Location, diagnostics);
}

if (IsAutoPropertyWithGetAccessor)
Expand Down Expand Up @@ -1196,7 +1224,7 @@ private SynthesizedSealedPropertyAccessor MakeSynthesizedSealedAccessor()
AttributeLocation IAttributeTargetSymbol.DefaultAttributeLocation => AttributeLocation.Property;

AttributeLocation IAttributeTargetSymbol.AllowedAttributeLocations
=> IsAutoPropertyWithGetAccessor // PROTOTYPE(semi-auto-props): Adjust this and add tests.
=> AllowFieldAttributeTarget
? AttributeLocation.Property | AttributeLocation.Field
: AttributeLocation.Property;

Expand Down Expand Up @@ -1647,7 +1675,7 @@ protected virtual void ValidatePropertyType(BindingDiagnosticBag diagnostics)
{
diagnostics.Add(ErrorCode.ERR_FieldCantBeRefAny, TypeLocation, type);
}
else if (this.IsAutoPropertyWithGetAccessor && type.IsRefLikeType && (this.IsStatic || !this.ContainingType.IsRefLikeType))
else if (DisallowRefLikeTypes && type.IsRefLikeType && (this.IsStatic || !this.ContainingType.IsRefLikeType))
{
diagnostics.Add(ErrorCode.ERR_FieldAutoPropCantBeByRefLike, TypeLocation, type);
}
Expand Down
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/UninitializedNonNullableFieldTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2361,6 +2361,9 @@ public C()
// (5,19): error CS0548: 'C.P': property or indexer must have at least one accessor
// public string P { }
Diagnostic(ErrorCode.ERR_PropertyWithNoAccessors, "P").WithArguments("C.P").WithLocation(5, 19),
// (7,19): error CS8050: Only auto-implemented properties can have initializers.
// public string P3 { } = string.Empty;
Diagnostic(ErrorCode.ERR_InitializerOnNonAutoProperty, "P3").WithArguments("C.P3").WithLocation(7, 19),
// (7,19): error CS0548: 'C.P3': property or indexer must have at least one accessor
// public string P3 { } = string.Empty;
Diagnostic(ErrorCode.ERR_PropertyWithNoAccessors, "P3").WithArguments("C.P3").WithLocation(7, 19),
Expand Down