-
Notifications
You must be signed in to change notification settings - Fork 4k
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
"Arbitrary async returns" should use attribute #13405
Conversation
Bring ljw1004 fork up to date
FI into ljw1004/roslyn
FI from main
FI from main
FI from main
…ibute, no longer off CreateAsyncMethodBuilder method.
I also filed one outstanding bug: #13406 (Also, I'm not that good at github... I don't know why all the "Merge..." commits are showing up, don't know if they're a problem, don't know how to get rid of them. Will investigate.) |
/// no more, no less. Validation of builder type B is left for elsewhere. This method returns B | ||
/// without validation of any kind. | ||
/// </remarks> | ||
internal static bool IsCustomTaskType(this NamedTypeSymbol type, out object builderArgument) |
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.
Should builderArgument
be TypeSymbol
rather than object
?
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 went back and forth on this. If you write [AsyncBuilder("hello")]
then it is indeed a tasklike.
Now I said that I'm looking specifically for the AsyncBuilder(System.Type)
overload, so if I find it then the above code will be an error. But I figured that in this case the builderArgument would still be a string literal.
So if anyone wants to report errors on what the argument was, then returning it just as object
is more useful. If it were TypeSymbol
then there'd be the odd case where the method returns true, but it returns a null builderArgument
, so there's not much for the caller to go on to know how to report the problem.
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 the [AsyncBuilder]
argument is not a type, why is the type decorated with that attribute considered task-like?
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.
Why not? :)
I figure it could have gone either way. There is a spectrum of possibilities...
[AsyncBuilder(System.Type)]
identifies a tasklike, regardless of what argument is passed, regardless if it is an error (e.g.[AsyncBuilder(NonExistentType)]
or[AsyncBuilder("3")]
), regardless of anything.[AsyncBuilder(System.Type)]
identifies a tasklike only if the argument is a type[AsyncBuilder(System.Type)]
identifies a tasklike only if the argument is a type with the same generic arity and same accessibility[AsyncBuilder(System.Type)]
identifies a tasklike only if the argument is a type with the same generic arity, same accessibility, and a suitableCreate
method and a suitableTask
property[AsyncBuilder(System.Type)]
identifies a tasklike only if the argument is a type that's 100% suitable as a builder.
I picked the first option because (A) the extremes seem more defensible than the middle grounds, (B) it felt a touch more efficient because places like overload resolution and type inference which might potentially be used a lot only do the minimal checking, (C) I think it's a better user experience while you're developing your custom tasklike if it remains tasklike even while you're in the process of editing+fixing the code of your builder.
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.
We should allow [AsyncBuilder(expr)]
if expr
is a type expression, even an error type. Are there useful scenarios that are supported if we allow expr
to be a string though?
And is returning object
rather than TypeSymbol
from this method more efficient? At line 1318, we've already determined the argument is a System.Type
.
Returning an object
and requiring callers to check the type of the result seems unnecessary.
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 like using extension methods when the target type is defined in the same assembly and namespace. It's just harder to find all the places where applicable extension methods are defined. Especially since this method is not even defined in NamedTypeSymbolExtensions
but in TypeSymbolExtensions
. By adding extension methods like these the code base is becoming more and more complex to navigate for someone outside the compiler team, who doesn't spend 100% of their time in the compiler code base and doesn't know whether or not it "feels" like non-core functionality.
Not sure what you mean by public API -- NamedTypeSymbol
doesn't have any public API since it's internal.
Even if it had there is no enforcement of the fact that it only uses public API of NamedTypeSymbol
. It may now but perhaps we refactor something and it won't later.
If the method was implemented using the true public API, e.g. ITypeSymbol
then it would be indeed another thing altogether.
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'm not sure where to go with this...
- Maybe an extension method
public bool ITypeSymbolExtensions.IsTasklike(this ITypeSymbol t)
, similar to IsAbstractClass, IsAwaitableNonDynamic, ... This concept "IsTasklike" would be used in overload resolution and method declarations and the like. It would be implemented by scanning for the attribute on the type. Meanwhile, the functionality to extract the attribute's argument would be solely used insideAssembleAsyncMethodCollection.cs
. - Maybe leave as is, as an extension method in
TypeSymbolExtensions
- Maybe move to an extension method in
NamedTypeSymbolExtensions
- Maybe move to an instance method in
NamedTypeSymbol
.
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 vote for an instance method on NamedTypeSymbol
, unless we can/want to reuse the code for both C# and VB in which case it would make sense implementing it using ITypeSymbol
.
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.
We have very few methods that operate on ISymbol
interfaces. It isn't necessary here.
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.
In that case, "leave as is" :)
…tch accessibility of builder" rule.
builderType = builderArgument as NamedTypeSymbol; | ||
if (builderType?.IsErrorType() == true) | ||
{ | ||
} |
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.
Rather than an empty if
block, consider changing to if (builderType?.IsErrorType() != true) { ... }
.
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.
The way I'd done cascading conditions was "(1) if an error has been reported, (2) otherwise if it's an error but one hasn't been reported, (3) otherwise success". How about I add a comment to the first branch to indicate this?
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.
Has the error necessarily been reported? What if the task-like type and builder type are from metadata and the builder type is missing? Do we need to report a use-site error here?
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.
Definitely agree, the empty if
block here seems off. It's definitely something I'd poke at if I came across this code in the future.
In reply to: 76644154 [](ancestors = 76644154)
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.
NamedTypeSymbol builderType) | ||
{ | ||
// The Create method's return type is expected to be buildderType. | ||
// The WellKnownMembers routines aren't able to enforce that, which is this method exists. |
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.
which is why ...
?
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.
@ljw1004 The merge commits are created each time you update your branch to bring it up-to-date with the master branch. They can be eliminated during the merge by "squashing" the iterations. I can show you how to do that or do it for you. |
…meters, and for single parameter of wrong type
… remove noise about "missing Create method" and "missing Task" method.
…t arity/accessibility). Added an error report when builder can't be imported (and added a test for this).
if (customBuilder) | ||
{ | ||
taskProperty = GetCustomTaskProperty(F, builderType); | ||
builderType = ValidateBuilderType(F, builderArgument, returnType.DeclaredAccessibility, desiredArity:0); | ||
if (builderType != null) |
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.
By convention, symbol comparisons with null
should cast to (object)
to use reference equals: if ((object)builderType != null)
. Same comment below.
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.
Debug.Assert((object)builderType != null); | ||
builderType = builderType.Construct(resultType); | ||
} | ||
NamedTypeSymbol builderType = null; |
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.
Minor point: initializer is not needed.
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.
Minor comments. Otherwise LGTM. |
Diagnostic(ErrorCode.ERR_MissingPredefinedMember, "=> await Task.FromResult(1)").WithArguments("N.BN", "Create").WithLocation(22, 22) | ||
); | ||
|
||
} |
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.
Are we testing task-like interfaces?
[AsyncBuilder(typeof(Builder))] interface ITask { }
class Builder { public static ITask Create() { ... } }
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.
Good thought! That's the key advantage of the attribute! I've added tests here: f2cdba8#diff-c55f1eb2556bfc07b602d0647778d699R3641
…Added more unit tests for bad builder, and for interface tasklike.
This PR changes "arbitrary async return" to detect tasklikeness from the presence of an
[AsyncBuilder]
attribute, not from aCreateAsyncMethodBuilder
method.Spec for the change is in two posts:
#10902 (comment)
#10902 (comment)