- 
                Notifications
    You must be signed in to change notification settings 
- Fork 4.2k
Extensions: Add ReduceExtensionMember API #80659
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
Conversation
bd1e5af    to
    cadd91d      
    Compare
  
    cadd91d    to
    a151018      
    Compare
  
    | IMethodSymbol? ReduceExtensionMethod(ITypeSymbol receiverType); | ||
|  | ||
| /// <summary> | ||
| /// If this is a (new) extension method that can be applied to a receiver of the given type, | 
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.
| if (_underlying.GetIsNewExtensionMember() && SourceMemberContainerTypeSymbol.IsAllowedExtensionMember(_underlying)) | ||
| { | ||
| var csharpReceiver = receiverType.EnsureCSharpSymbolOrNull(nameof(receiverType)); | ||
| return (IMethodSymbol?)SourceNamedTypeSymbol.GetCompatibleSubstitutedMember(compilation: null, _underlying, csharpReceiver, wasExtensionFullyInferred: out _).GetPublicSymbol(); | 
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.
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 don't think so. See description in OP: "For lookup, we do want to return members that can't be fully inferred."
| if (_underlying.GetIsNewExtensionMember() && SourceMemberContainerTypeSymbol.IsAllowedExtensionMember(_underlying)) | ||
| { | ||
| var csharpReceiver = receiverType.EnsureCSharpSymbolOrNull(nameof(receiverType)); | ||
| return (IPropertySymbol?)SourceNamedTypeSymbol.GetCompatibleSubstitutedMember(compilation: null, _underlying, csharpReceiver, wasExtensionFullyInferred: out _).GetPublicSymbol(); | 
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.
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.
Answered elsewhere
|  | ||
| INamedTypeSymbol systemObject = comp.GetSpecialType(SpecialType.System_Object).GetPublicSymbol(); | ||
|  | ||
| AssertEx.Equal("void E.<G>$66F77D1E46F965A5B22D4932892FA78B<T>.M()", method.ToTestDisplayString()); | 
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.
| } | ||
|  | ||
| [Fact] | ||
| public void ReduceExtensionMember_12() | 
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.
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.
That's correct, this is testing the internal API which has a different name, but is morally equivalent. I don't think this warrants a different test name. An alternative would be to rename the internal API to use the same name as the public one.
| } | ||
|  | ||
| [Fact] | ||
| public void ReduceExtensionMember_13() | 
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.
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.
Asnwered above
| Done with review pass (Commit 1) #Closed | 
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.
LGTM (commit 4)
| @RikkiGibson @CyrusNajmabadi for second review. Thanks | 
    
      
        1 similar comment
      
    
  
    | @RikkiGibson @CyrusNajmabadi for second review. Thanks | 
| /// <summary> | ||
| /// If this is a method of an extension block that can be applied to a receiver of the given type, | ||
| /// returns the method symbol in the substituted extension for that receiver type. Otherwise, returns null. | ||
| /// </summary> | 
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 think it would be really good to give an explicit code example of what this is versus ReduceExtensionMethod
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'll do a follow-up PR for this
Closes #80273 (pending API review to decide whether to have an API on ISymbol or split as implemented here)
There is a small change in behavior when dealing with extensions that aren't fully inferred. Previously,
GetCompatibleSubstitutedMemberdropped them. Now, it's up to the caller to decide.The
LookupSymbols_WasNotFullyInferredtest illustrates such a change. For lookup, we do want to return members that can't be fully inferred.Relates to test plan #76130