Skip to content
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

Clarify task types #875

Merged
merged 2 commits into from
Aug 15, 2023
Merged

Clarify task types #875

merged 2 commits into from
Aug 15, 2023

Conversation

jskeet
Copy link
Contributor

@jskeet jskeet commented Aug 7, 2023

  • Task and Task<T> are classified as task types despite not specifying builder types
  • Interfaces are allowed to be task types
  • Enums and methods are prohibited from being decorated with AsyncMethodBuilderAttribute
  • Task types must not be generic in more than one type parameter (including in terms of containing types)

Discussion required for all of this, but if merged, would fix #854, #856, #858 and #859.

- `Task` and `Task<T>` are classified as task types despite not
  specifying builder types
- Interfaces are allowed to be task types
- Enums and methods are prohibited from being decorated with
  AsyncMethodBuilderAttribute
- Task types must not be generic in more than one type parameter
  (including in terms of containing types)

Discussion required for all of this, but if merged, would
fix dotnet#854, dotnet#856, dotnet#858 and dotnet#859.
@jskeet jskeet requested a review from BillWagner August 7, 2023 14:17
@jskeet
Copy link
Contributor Author

jskeet commented Aug 7, 2023

cc @KalleOlaviNiemitalo for thoughts

@BillWagner I figure we can probably get this hammered into better shape between ourselves (I'm not entirely happy with the way the generic restriction is expressed) and then get broader review.

@KalleOlaviNiemitalo
Copy link
Contributor

Task types must not be generic in more than one type parameter (including in terms of containing types)

That is not right. Type arguments of types that contain the task type should be ignored; Outer<char, int>.Inner<long> should be allowed as a task type if it has the correct attribute. But not as a task builder type.

@jskeet
Copy link
Contributor Author

jskeet commented Aug 7, 2023

That is not right. Type arguments of types that contain the task type should be ignored; Outer<char, int>.Inner<long> should be allowed as a task type if it has the correct attribute. But not as a task builder type.

That would certainly be simpler to specify. Can I ask what your reasoning is here? I suspect it's more advanced than mine... and we should probably have a note to explain either way.

@KalleOlaviNiemitalo
Copy link
Contributor

Or is the idea that, if the task type is nested in a generic type, then the task builder type would not be able to have a Task property with the correct type, unless it hardcodes the type arguments of the containing type? So that allowing such task types would not be useful in practice. Which would then make the Roslyn behaviour an extension.

@jskeet
Copy link
Contributor Author

jskeet commented Aug 7, 2023

Or is the idea that, if the task type is nested in a generic type, then the task builder type would not be able to have a Task property with the correct type, unless it hardcodes the type arguments of the containing type? So that allowing such task types would not be useful in practice. Which would then make the Roslyn behaviour an extension.

That was my thinking, I think - but I feel like I'm still wading through cotton wool trying to think about this. I'm definitely not feeling like we should be bound by exactly what Roslyn allows, but I'm open to useful justifications.

@jskeet
Copy link
Contributor Author

jskeet commented Aug 8, 2023

I've added another commit to basically copy the existing text from 15.15.2 (for task builders) and make it apply to task types.
This may be more restrictive than is strictly required (and Roslyn can allow a little more leeway), but I think it's a good starting point.

This wording follows the wording in 15.15.2 for task builders.
@jskeet
Copy link
Contributor Author

jskeet commented Aug 14, 2023

I need to go back and reapply changes from #846 to this...

@jskeet
Copy link
Contributor Author

jskeet commented Aug 15, 2023

Oops - no, I don't need to reapply changes from #846. It's all fine; @BillWagner does this look ready to merge?

Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM @jskeet

Let's :shipit:

@BillWagner BillWagner merged commit c48d041 into dotnet:draft-v7 Aug 15, 2023
6 checks passed
@jskeet jskeet deleted the task-types branch August 15, 2023 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

C# 7.x §15.15.1 must special-case Task and Task<TResult> because they lack AsyncMethodBuilderAttribute
3 participants