-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Unions: Support inheritance for Value/HasValue property #82779
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,31 +34,9 @@ partial class Binder | |
| } | ||
|
|
||
| internal static PropertySymbol? GetUnionTypeValueProperty(NamedTypeSymbol inputUnionType, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo) | ||
| { | ||
| PropertySymbol? valueProperty = GetUnionTypeValuePropertyOriginalDefinition(inputUnionType, ref useSiteInfo); | ||
| if (valueProperty is null) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| return valueProperty.AsMember(inputUnionType); | ||
| } | ||
|
|
||
| private static PropertySymbol? GetUnionTypeValuePropertyOriginalDefinition(NamedTypeSymbol inputUnionType, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo) | ||
| { | ||
| Debug.Assert(inputUnionType.IsUnionType); | ||
|
|
||
| // https://github.com/dotnet/roslyn/issues/82636: Not dealing with inheritance for now. | ||
| // The inherited property might not be a definition, therefore the name of the function should probably change | ||
| PropertySymbol? valueProperty = null; | ||
| foreach (var m in inputUnionType.OriginalDefinition.GetMembers(WellKnownMemberNames.ValuePropertyName)) | ||
| { | ||
| if (m is PropertySymbol prop && hasUnionValueSignature(prop)) | ||
| { | ||
| valueProperty = prop; | ||
| break; | ||
| } | ||
| } | ||
| PropertySymbol? valueProperty = TryGetOwnOrInheritedUnionMember<PropertySymbol>(inputUnionType, WellKnownMemberNames.ValuePropertyName, isSuitableProperty, ref useSiteInfo); | ||
|
|
||
| if (valueProperty is not null) | ||
| { | ||
|
|
@@ -67,11 +45,23 @@ partial class Binder | |
| } | ||
| else | ||
| { | ||
| useSiteInfo.AddDiagnosticInfo(new CSDiagnosticInfo(ErrorCode.ERR_MissingPredefinedMember, inputUnionType, WellKnownMemberNames.ValuePropertyName)); // https://github.com/dotnet/roslyn/issues/82636: Cover this code path | ||
| useSiteInfo.AddDiagnosticInfo(new CSDiagnosticInfo(ErrorCode.ERR_MissingPredefinedMember, inputUnionType.OriginalDefinition, WellKnownMemberNames.ValuePropertyName)); | ||
| } | ||
|
|
||
| return valueProperty; | ||
|
|
||
| static bool isSuitableProperty(Symbol m, [NotNullWhen(true)] out PropertySymbol? valueProperty) | ||
| { | ||
| if (m is PropertySymbol prop && hasUnionValueSignature(prop)) | ||
| { | ||
| valueProperty = prop; | ||
| return true; | ||
| } | ||
|
|
||
| valueProperty = null; | ||
| return false; | ||
| } | ||
|
|
||
| static bool hasUnionValueSignature(PropertySymbol property) | ||
| { | ||
| // https://github.com/dotnet/roslyn/issues/82636: Cover individual conditions with tests | ||
|
|
@@ -88,6 +78,52 @@ static bool hasUnionValueSignature(PropertySymbol property) | |
| } | ||
| } | ||
|
|
||
| private delegate bool IsSuitableUnionMember<T>(Symbol m, [NotNullWhen(true)] out T? member) where T : Symbol; | ||
|
|
||
| private static T? TryGetOwnOrInheritedUnionMember<T>(NamedTypeSymbol inputUnionType, string memberName, IsSuitableUnionMember<T> isSuitableUnionMember, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo) | ||
| where T : Symbol | ||
| { | ||
| T? member; | ||
| NamedTypeSymbol declaringType = inputUnionType.OriginalDefinition; | ||
| if (getMemberDeclaredInType(declaringType, memberName, isSuitableUnionMember, out member)) | ||
| { | ||
| member = (T)member.SymbolAsMember(inputUnionType); | ||
| } | ||
| else | ||
| { | ||
| for (NamedTypeSymbol baseType1 = declaringType.BaseTypeWithDefinitionUseSiteDiagnostics(ref useSiteInfo), baseType2 = inputUnionType.BaseTypeNoUseSiteDiagnostics; | ||
| baseType1 is not null; | ||
| baseType1 = baseType1.BaseTypeWithDefinitionUseSiteDiagnostics(ref useSiteInfo), baseType2 = baseType2.BaseTypeNoUseSiteDiagnostics) | ||
| { | ||
| if (getMemberDeclaredInType(baseType1, memberName, isSuitableUnionMember, out member)) | ||
| { | ||
| if (!inputUnionType.IsDefinition) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Checking my understanding: is this just an optimization to avoid creating an unnecessary constructed member symbol?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, If we are dealing with a type definition, no additional substitution is needed for members from it's definition base. |
||
| { | ||
| member = (T)member.OriginalDefinition.SymbolAsMember(baseType2); | ||
| } | ||
|
|
||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return member; | ||
|
|
||
| static bool getMemberDeclaredInType(NamedTypeSymbol declaringType, string memberName, IsSuitableUnionMember<T> isSuitableUnionMember, [NotNullWhen(true)] out T? member) | ||
| { | ||
| foreach (var m in declaringType.GetMembers(memberName)) | ||
| { | ||
| if (isSuitableUnionMember(m, out member)) | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| member = null; | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| internal static PropertySymbol? GetUnionTypeValuePropertyNoUseSiteDiagnostics(NamedTypeSymbol inputUnionType) | ||
| { | ||
| var useSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded; | ||
|
|
@@ -96,34 +132,35 @@ static bool hasUnionValueSignature(PropertySymbol property) | |
|
|
||
| internal static bool IsUnionTypeValueProperty(NamedTypeSymbol unionType, Symbol symbol) | ||
| { | ||
| // https://github.com/dotnet/roslyn/issues/82636: Deal with inheritance? | ||
| var useSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded; | ||
| return Binder.GetUnionTypeValuePropertyOriginalDefinition(unionType, ref useSiteInfo) == (object)symbol.OriginalDefinition; | ||
| return Symbol.Equals(Binder.GetUnionTypeValuePropertyNoUseSiteDiagnostics(unionType), symbol, TypeCompareKind.AllIgnoreOptions); | ||
| } | ||
|
|
||
| private static PropertySymbol? GetUnionTypeHasValuePropertyOriginalDefinition(NamedTypeSymbol inputUnionType) | ||
| internal static PropertySymbol? GetUnionTypeHasValueProperty(NamedTypeSymbol inputUnionType) | ||
| { | ||
| Debug.Assert(inputUnionType.IsUnionType); | ||
|
|
||
| PropertySymbol? valueProperty = null; | ||
| var useSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we discarding use site info here? #Resolved
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It is not an error to not find matching |
||
| PropertySymbol? valueProperty = TryGetOwnOrInheritedUnionMember<PropertySymbol>(inputUnionType, WellKnownMemberNames.HasValuePropertyName, isSuitableProperty, ref useSiteInfo); | ||
|
||
|
|
||
| // https://github.com/dotnet/roslyn/issues/82636: Not dealing with inheritance for now. | ||
| foreach (var m in inputUnionType.OriginalDefinition.GetMembers(WellKnownMemberNames.HasValuePropertyName)) | ||
| if (valueProperty?.GetUseSiteInfo().DiagnosticInfo?.DefaultSeverity == DiagnosticSeverity.Error) | ||
| { | ||
| return null; // https://github.com/dotnet/roslyn/issues/82636: Cover this code path | ||
| } | ||
|
|
||
| return valueProperty; | ||
|
|
||
| static bool isSuitableProperty(Symbol m, [NotNullWhen(true)] out PropertySymbol? valueProperty) | ||
| { | ||
| if (m is PropertySymbol prop && hasHasValueSignature(prop)) | ||
| { | ||
| valueProperty = prop; | ||
| break; | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| if (valueProperty is null || valueProperty.GetUseSiteInfo().DiagnosticInfo?.DefaultSeverity == DiagnosticSeverity.Error) | ||
| { | ||
| return null; // https://github.com/dotnet/roslyn/issues/82636: Cover this code path | ||
| valueProperty = null; | ||
| return false; | ||
| } | ||
|
|
||
| return valueProperty; | ||
|
|
||
| static bool hasHasValueSignature(PropertySymbol property) | ||
| { | ||
| // https://github.com/dotnet/roslyn/issues/82636: Cover individual conditions with tests | ||
|
|
@@ -140,17 +177,6 @@ static bool hasHasValueSignature(PropertySymbol property) | |
| } | ||
| } | ||
|
|
||
| internal static PropertySymbol? GetUnionTypeHasValueProperty(NamedTypeSymbol inputUnionType) | ||
| { | ||
| PropertySymbol? valueProperty = GetUnionTypeHasValuePropertyOriginalDefinition(inputUnionType); | ||
| if (valueProperty is null) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| return valueProperty.AsMember(inputUnionType); | ||
| } | ||
|
|
||
| internal static MethodSymbol? GetUnionTypeTryGetValueMethod(NamedTypeSymbol inputUnionType, TypeSymbol type) | ||
| { | ||
| Debug.Assert(inputUnionType.IsUnionType); | ||
|
|
@@ -196,8 +222,7 @@ static bool isTryGetValueSignature(MethodSymbol method) | |
|
|
||
| internal static bool IsUnionTypeHasValueProperty(NamedTypeSymbol unionType, PropertySymbol property) | ||
| { | ||
| // https://github.com/dotnet/roslyn/issues/82636: Deal with inheritance? | ||
| return (object)property.OriginalDefinition == Binder.GetUnionTypeHasValuePropertyOriginalDefinition(unionType); | ||
| return Symbol.Equals(Binder.GetUnionTypeHasValueProperty(unionType), property, TypeCompareKind.AllIgnoreOptions); | ||
| } | ||
|
|
||
| private BoundExpression BindIsPatternExpression(IsPatternExpressionSyntax node, BindingDiagnosticBag diagnostics) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any need to defend against cycles like
class MyUnion1 : MyUnion2; class MyUnion2 : MyUnion1here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think so. I do not expect us to get here while bases are being resolved.