Use attribute to designate Union types#82408
Conversation
|
This PR modifies public API files. Please follow the instructions at https://github.com/dotnet/roslyn/blob/main/docs/contributing/API%20Review%20Process.md for ensuring all public APIs are reviewed before merging. |
|
@333fred, @RikkiGibson, @dotnet/roslyn-compiler Please review |
|
\azp run |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
@333fred, @RikkiGibson, @dotnet/roslyn-compiler Please review |
| if (CSharpAttributeData.IsTargetEarlyAttribute(arguments.AttributeType, arguments.AttributeSyntax, AttributeDescription.UnionAttribute)) | ||
| { | ||
| (attributeData, boundAttribute) = arguments.Binder.GetAttribute(arguments.AttributeSyntax, arguments.AttributeType, beforeAttributePartBound: null, afterAttributePartBound: null, out hasAnyDiagnostics); | ||
| if (!attributeData.HasErrors && attributeData.CommonConstructorArguments.IsEmpty) |
There was a problem hiding this comment.
Does anything test the significance of attributeData.CommonConstructorArguments.IsEmpty check? #Resolved
There was a problem hiding this comment.
Does anything test the significance of
attributeData.CommonConstructorArguments.IsEmptycheck?
I do not think so.
There was a problem hiding this comment.
I don't think anything can; if it weren't empty, the attribute wouldn't have matched the description, which is for void only. You could consider just asserting, but I'm not overly bothered by the extra check.
There was a problem hiding this comment.
Note: if you don't have it, it might be good to have a test with an attribute definition like this:
public class UnionAttribute
{
public UnionAttribute() { }
[OverloadResolutionPriority(1)]
public UnionAttribute(int i = 1) { }
}There was a problem hiding this comment.
Note: if you don't have it, it might be good to have a test with an attribute definition like this:
I'll make a note in the Test Plan.
|
@333fred, @dotnet/roslyn-compiler For a second review |
| if (CSharpAttributeData.IsTargetEarlyAttribute(arguments.AttributeType, arguments.AttributeSyntax, AttributeDescription.UnionAttribute)) | ||
| { | ||
| (attributeData, boundAttribute) = arguments.Binder.GetAttribute(arguments.AttributeSyntax, arguments.AttributeType, beforeAttributePartBound: null, afterAttributePartBound: null, out hasAnyDiagnostics); | ||
| if (!attributeData.HasErrors && attributeData.CommonConstructorArguments.IsEmpty) |
There was a problem hiding this comment.
I don't think anything can; if it weren't empty, the attribute wouldn't have matched the description, which is for void only. You could consider just asserting, but I'm not overly bothered by the extra check.
| if (CSharpAttributeData.IsTargetEarlyAttribute(arguments.AttributeType, arguments.AttributeSyntax, AttributeDescription.UnionAttribute)) | ||
| { | ||
| (attributeData, boundAttribute) = arguments.Binder.GetAttribute(arguments.AttributeSyntax, arguments.AttributeType, beforeAttributePartBound: null, afterAttributePartBound: null, out hasAnyDiagnostics); | ||
| if (!attributeData.HasErrors && attributeData.CommonConstructorArguments.IsEmpty) |
There was a problem hiding this comment.
Note: if you don't have it, it might be good to have a test with an attribute definition like this:
public class UnionAttribute
{
public UnionAttribute() { }
[OverloadResolutionPriority(1)]
public UnionAttribute(int i = 1) { }
}
333fred
left a comment
There was a problem hiding this comment.
Thanks for the clarification on existing testing.
No description provided.