Extensions: account for new extensions in function type scenarios #77755
Conversation
jcouv
added
Area-Compilers
Feature - Extension Everything
dotnet-issue-labeler
bot
added
the
untriaged
| // For lookup APIs in the semantic model, we can return symbols that aren't fully inferred. | ||
| // But for function type inference, if the symbol isn't fully inferred with the information we have (the receiver and any explicit type arguments) | ||
| // then we won't return it. | ||
| internal static Symbol? GetReducedAndFilteredSymbol(this Symbol member, ImmutableArray<TypeWithAnnotations> typeArguments, TypeSymbol receiverType, CSharpCompilation compilation, bool checkFullyInferred) |
There was a problem hiding this comment.
📝 This logic is extracted from AddReducedAndFilteredSymbol, but the check for "fully inferred" was added.
| if (node.ResultKind == LookupResultKind.Viable) | ||
| { | ||
| var methods = ArrayBuilder<MethodSymbol>.GetInstance(capacity: node.Methods.Length); | ||
| foreach (var memberMethod in node.Methods) | ||
| { | ||
| switch (node.ReceiverOpt) | ||
| { | ||
| case BoundTypeOrValueExpression: |
There was a problem hiding this comment.
📝 Fixed a bug here. The relevant test is FunctionType_ColorColorReceiver_04.
|
@AlekseyTs @jjonescz for review. Thanks |
| } | ||
|
|
||
| [Fact] | ||
| public void Params_ExtensionScopes_01() |
| } | ||
| } | ||
|
|
||
| if ((object)constructed == null) |
There was a problem hiding this comment.
This looks unnecessary since we return constructed below anyway. #Resolved
| Debug.Assert(property.GetIsNewExtensionMember()); | ||
| var constructedProperty = (PropertySymbol)SourceNamedTypeSymbol.GetCompatibleSubstitutedMember(compilation, property, receiverType)!; | ||
|
|
||
| if (constructedProperty is null) |
| if (member is MethodSymbol method) | ||
| { | ||
| // 1. construct with explicit type arguments if provided | ||
| MethodSymbol constructed; |
There was a problem hiding this comment.
Shouldn't this be nullable? We seem to be comparing constructed to null in several places below.
| MethodSymbol constructed; | |
| MethodSymbol? constructed; | |
| ``` #Resolved |
| // infer type arguments based off the receiver type if needed, check applicability | ||
| Debug.Assert(receiverType is not null); | ||
| Debug.Assert(property.GetIsNewExtensionMember()); | ||
| var constructedProperty = (PropertySymbol)SourceNamedTypeSymbol.GetCompatibleSubstitutedMember(compilation, property, receiverType)!; |
There was a problem hiding this comment.
Why do we suppress nullability here but compare with null below? #Resolved
| else | ||
| { | ||
| Debug.Assert(method.GetIsNewExtensionMember()); | ||
| constructed = (MethodSymbol)SourceNamedTypeSymbol.GetCompatibleSubstitutedMember(compilation, constructed, receiverType)!; |
There was a problem hiding this comment.
Should we be also checking this result for null instead of suppressing nullability? Since we do that below for properties. #Resolved
There was a problem hiding this comment.
Good catch. Thanks. Added a test that hits that
Relates to test plan #76130