Allow async methods to return Task-like types #12434
Conversation
|
CC @roslyn-compiler, @ljw1004 |
| @@ -660,6 +660,7 @@ Microsoft.CodeAnalysis.Semantics.UnaryOperationKind.UnsignedPrefixIncrement = 77 | |||
| abstract Microsoft.CodeAnalysis.Diagnostics.OperationBlockStartAnalysisContext.RegisterOperationAction(System.Action<Microsoft.CodeAnalysis.Diagnostics.OperationAnalysisContext> action, System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.OperationKind> operationKinds) -> void | |||
| abstract Microsoft.CodeAnalysis.Diagnostics.OperationBlockStartAnalysisContext.RegisterOperationBlockEndAction(System.Action<Microsoft.CodeAnalysis.Diagnostics.OperationBlockAnalysisContext> action) -> void | |||
| abstract Microsoft.CodeAnalysis.SemanticModel.GetOperationCore(Microsoft.CodeAnalysis.SyntaxNode node, System.Threading.CancellationToken cancellationToken) -> Microsoft.CodeAnalysis.IOperation | |||
| const Microsoft.CodeAnalysis.WellKnownMemberNames.CreateAsyncMethodBuilder = "CreateAsyncMethodBuilder" -> string | |||
There was a problem hiding this comment.
Do we mean for this to be public?
cston
force-pushed
the
tasklike
branch
2 times, most recently
from
dd9e4fd
to
451eb45
Compare
|
@ljw1004, @dotnet/roslyn-compiler The PR has been updated with the changes to overload resolution and tests ported from Lucian's |
cston
force-pushed
the
tasklike
branch
2 times, most recently
from
a6fe2ac
to
9f8f875
Compare
| @@ -1252,9 +1252,12 @@ private static TypeSymbol GetParameterType(int argIndex, MemberAnalysisResult re | |||
| out okToDowngradeToNeither); | |||
| } | |||
|
|
|||
| var type1Normalized = type1.NormalizeTaskTypes(Compilation); | |||
| var type2Normalized = type2.NormalizeTaskTypes(Compilation); | |||
There was a problem hiding this comment.
Note NormalizeTaskTypes() is called in OverloadResolution.BetterFunctionMember<> whenever comparing parameter types to ignore differences in Task types. And NormalizeTaskTypes() walks the type calling GetAsyncMethodBuilderType() on each type part, and substituting System.Threading.Tasks.Task or System.Threading.Tasks.Tasks<T> for any Task-like types. However, the overhead should by low since GetAsyncMethodBuilderType() is essentially a dictionary lookup, and substitution only occurs if Task-like types are found.
There was a problem hiding this comment.
What happens in the case where Task and Task<T> are not available? Does it break our ability to do overload resolution on other task-like types?
There was a problem hiding this comment.
Good catch. If Task and Task<T> are not available, we are currently substituting an error type. I've changed NormalizeTaskTypes to skip substitution in that case but that means we will not consider two different task-like types as equivalent if the well known types are missing.
There was a problem hiding this comment.
That's funny! Even if we fix this particular problem (to consider two different task-like types as equivalent even in the absence of System.Threading.Tasks.Task/Task), the compiler is still hard-coded to prefer Task in certain situations e.g.
void f<T>(Func<T> lambda)
f(async () => 3);I think it's a mistake that the compiler does this inference, but we're stuck with it. Imagine if we came up with a set of libraries which removed Task completely, and only used other tasks, as a way to force the compiler never to do these dangerous inferences!!!
Still it's a far-removed way-out scenario. Task has found its way into the deepest part of the BCL and will never be removed.
|
@ljw1004, @dotnet/roslyn-compiler, ping, please review. |
| } | ||
| else if (type2IsGenericTask) | ||
| { | ||
| // A shortcut, Task<T> type cannot satisfy other rules. |
There was a problem hiding this comment.
You can only keep this shortcut if a stronger invariant holds, that NO TASKLIKE can satisfy the other rules. I'm not sure what are the "other rules" mentioned by this comment. Could you check?
There was a problem hiding this comment.
The remaining rules are for delegate types and integral types.
| @@ -132,10 +146,25 @@ private static TypeSymbol InferReturnType(BoundBlock block, Binder binder, bool | |||
|
|
|||
| // Async: | |||
|
|
|||
| NamedTypeSymbol taskType = null; | |||
There was a problem hiding this comment.
Could you comment what this chunk of code is doing? It's not obvious. I think it says:
// This lambda has an "async" modifier so we need to figure out what tasklike the lambda returns.
// The default assumption is it will return Task or Task (depending on whether it has returns or not).
// But if the lambda is being matched against a delegate type (i.e. not a generic type parameter, and
// not a non-delegate type), and if the delegate type has a return type, and if that return type is
// tasklike (as identified by the presence of a createBuilderMethod), then the delegate's return type
// indicates which tasklike the lambda should return. It might be Task, or Task, or Task,
// or Tasklike, or Tasklike, or Tasklike.
|
Updated PR checked in to |
Allow async methods to return types other than
void,Task, orTask<T>. Specifically allowTask-like types (andAsyncMethodBuildertypes) if the types contain an expected set of members.Next: Compare full signature including type constraints when looking up members. (Currently, lookup is by name only.) And add more tests.
[Thanks to Lucian for prototyping this feature.]