Unions: Support inheritance for Value/HasValue property#82779
Unions: Support inheritance for Value/HasValue property#82779AlekseyTs merged 3 commits intodotnet:features/Unionsfrom
Conversation
|
@RikkiGibson, @jjonescz, @dotnet/roslyn-compiler Please review |
| static bool hasUnionValueSignature(PropertySymbol property) | ||
| { | ||
| // https://github.com/dotnet/roslyn/issues/82636: Cover individual conditions with tests | ||
| // https://github.com/dotnet/roslyn/issues/82636: Cover scenaros with a setter present |
There was a problem hiding this comment.
(typo in word "scenarios")
| // https://github.com/dotnet/roslyn/issues/82636: Cover scenarios with a setter present |
|
|
||
| PropertySymbol? valueProperty = null; | ||
| var useSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded; | ||
| PropertySymbol? valueProperty = TryGetOwnOrInheritedUnionMember<PropertySymbol>(inputUnionType, WellKnownMemberNames.HasValuePropertyName, isSuitableProperty, ref useSiteInfo); |
| Debug.Assert(inputUnionType.IsUnionType); | ||
|
|
||
| PropertySymbol? valueProperty = null; | ||
| var useSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded; |
There was a problem hiding this comment.
Why are we discarding use site info here? #Resolved
There was a problem hiding this comment.
Why are we discarding use site info here?
It is not an error to not find matching HasValue property, therefore we don't want to report any errors or warnings.
| public S1(string x) : base(x) {} | ||
| public S1(bool x) : base(x) {} | ||
|
|
||
| public override object Value => base.Value; |
There was a problem hiding this comment.
Do we have a tests for when the overridden Value returns something else than base.Value? To verify what gets actually called at runtime. #Resolved
There was a problem hiding this comment.
Added ValueProperty_13_Override. If that is not what you had in mind, please provide more details
|
@RikkiGibson, @jjonescz, @dotnet/roslyn-compiler Please review |
| class S1 : S0 | ||
| { | ||
| public S1(string x) : base(x) {} | ||
| public override string Value => (string)_value; |
There was a problem hiding this comment.
I imagined we would write something to console in the override and see that it gets executed rather than the overridden property (e.g., if we called it non-virtually) but I guess that might not be really related to the changes in this PR so this looks good too.
There was a problem hiding this comment.
(Or perhaps return the string modified from the override and output the value.)
There was a problem hiding this comment.
Added 'throw` to base implementation
|
@RikkiGibson, @dotnet/roslyn-compiler For a second review |
RikkiGibson
left a comment
RikkiGibson
left a comment
There was a problem hiding this comment.
Still looking through tests
| } | ||
| else | ||
| { | ||
| for (NamedTypeSymbol baseType1 = declaringType.BaseTypeWithDefinitionUseSiteDiagnostics(ref useSiteInfo), baseType2 = inputUnionType.BaseTypeNoUseSiteDiagnostics; |
There was a problem hiding this comment.
Is there any need to defend against cycles like class MyUnion1 : MyUnion2; class MyUnion2 : MyUnion1 here?
There was a problem hiding this comment.
Is there any need to defend against cycles like
class MyUnion1 : MyUnion2; class MyUnion2 : MyUnion1here?
I do not think so. I do not expect us to get here while bases are being resolved.
| { | ||
| if (getMemberDeclaredInType(baseType1, memberName, isSuitableUnionMember, out member)) | ||
| { | ||
| if (!inputUnionType.IsDefinition) |
There was a problem hiding this comment.
Checking my understanding: is this just an optimization to avoid creating an unnecessary constructed member symbol?
There was a problem hiding this comment.
Checking my understanding: is this just an optimization to avoid creating an unnecessary constructed member symbol?
Yes, If we are dealing with a type definition, no additional substitution is needed for members from it's definition base.
3 participants
Microsoft Reviewers: Open in CodeFlow