Spec determination of builder type from overrides #4512
Conversation
proposals/async-method-builders.md
Outdated
| A developer that wants to using a specific custom builder for all of their methods can do so by putting the relevant attribute on each method. | ||
| Example of usage on module: | ||
| ```C# | ||
| [module: AsyncMethodBuilderOverride(typeof(PoolingAsyncValueTaskMethodBuilder), typeof(ValueTask)] |
There was a problem hiding this comment.
, typeof(ValueTask) [](start = 78, length = 19)
Is this argument needed? #Closed
proposals/async-method-builders.md
Outdated
| A developer that wants to using a specific custom builder for all of their methods can do so by putting the relevant attribute on each method. | ||
| Example of usage on module: | ||
| ```C# | ||
| [module: AsyncMethodBuilderOverride(typeof(PoolingAsyncValueTaskMethodBuilder), typeof(ValueTask)] |
There was a problem hiding this comment.
] [](start = 97, length = 1)
Missing ). #Closed
proposals/async-method-builders.md
Outdated
| 1. looking in the containing scopes for an override attribute that specifies a builder type compatible with the method's return type. | ||
| 2. otherwise, falling back to the builder type determined by previous approach. (see [spec for task-like types](https://github.com/dotnet/csharplang/blob/master/proposals/csharp-7.0/task-types.md)) | ||
|
|
||
| The containing scope are considered starting from the method, then its containing types and finally the module. Each scope is considered |
There was a problem hiding this comment.
scope [](start = 15, length = 5)
"scopes"? #Closed
| in turn. | ||
| If a scope has one or more override attributes, the following process is used to determine if it is compatible with the current async method: | ||
| 1. take the override type specified by the attribute and construct it if necessary. | ||
| If the override type is an open generic type, take the single type argument of the async method's return type and substitute it into the override type. If |
There was a problem hiding this comment.
If the override type is an open generic type, take the single type argument of the async method's return type and substitute it into the override type. [](start = 2, length = 151)
Does this mean an open type can be used for the attribute even when applied to a method directly?
[AsyncMethodBuilderOverride(typeof(MyBuilder<int>))] async Task<int> F1();
[AsyncMethodBuilderOverride(typeof(MyBuilder<>))] async Task<int> F2();
``` #ClosedThere was a problem hiding this comment.
Yes, it would. Treating attributes on methods just like attributes on other scopes is a double-edged sword: it's simpler (more regular) but it does open the door to some strange usages...
There was a problem hiding this comment.
Added an open question.
Do you think we should disallow open generics when the attribute is applied to a method directly? Seems unnecessarily verbose.
| 4. consider the type of that `Task` property (a task-like type): | ||
| If the task-like type matches the return type of the async method, then the override is compatible. Otherwise, it is not compatible. | ||
|
|
||
| If no compatible override is found on a given scope, then look at the next scope. |
There was a problem hiding this comment.
If no compatible override is found on a given scope, then look at the next scope. [](start = 0, length = 81)
If the attribute is applied to a method directly but is incompatible, should we report an error instead of silently ignoring that attribute? #Closed
There was a problem hiding this comment.
Added special rule when the attribute is applied to method. Thanks
proposals/async-method-builders.md
Outdated
| static async ValueTask<int> ExampleAsync() { ... } | ||
| ``` | ||
|
|
||
| A developer that wants to using a specific custom builder for all of their methods can do so by putting the relevant attribute on each method. |
There was a problem hiding this comment.
using [](start = 26, length = 5)
"use"? #Closed
proposals/async-method-builders.md
Outdated
| } | ||
| ``` | ||
|
|
||
| ### Determining the builder type for an async method |
There was a problem hiding this comment.
an async method [](start = 37, length = 15)
Do the attributes on containing scope apply to any async lambdas within that scope? #Closed
There was a problem hiding this comment.
I would not expect that to be the case however I do think we need to spec out whether or not this should apply to async lambda as well once we have the ability to add attributes.
There was a problem hiding this comment.
I'm proposing that async local functions and lambdas be subject to the same rules. Also tracking this in open questions section.
proposals/async-method-builders.md
Outdated
| @@ -46,23 +46,70 @@ namespace System.Runtime.CompilerServices | |||
| /// Indicates the type of the async method builder that should be used by a language compiler to | |||
| /// build the attributed method. | |||
| /// </summary> | |||
| [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Interface | AttributeTargets.Method | AttributeTargets.Module, Inherited = false, AllowMultiple = false)] | |||
| [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Interface | AttributeTargets.Method | AttributeTargets.Module, Inherited = false, AllowMultiple = true)] | |||
There was a problem hiding this comment.
Think we should include AttributeTargets.Assembly here. Yes the compiler team is typically focused on modules because that is the unit of emit that we have to deal with. 99% of users though don't understand the difference between assembly and module and they are all used to assembly level attributes. Think we should support that and ask users to write it.
There was a problem hiding this comment.
For skiplocalsinit, the attribute is allowed on modules, but not assemblies.
I think our other attribute-based features did the same. We should probably stick to that approach.
On the other hand, should we allow Property (accessors could use async lambdas or local functions), Event (ditto) and Constructor?
There was a problem hiding this comment.
I'm proposing that we should allow Property, Event and Constructor, but not Assembly. Also added a tracking issue in open questions.
| { | ||
| public async ValueTask Method1Async() { ... } // would use PoolingAsyncValueTaskMethodBuilder | ||
| public async ValueTask<int> Method2Async() { ... } // would use PoolingAsyncValueTaskMethodBuilder<int> | ||
| public async ValueTask<string> Method3Async() { ... } // would use PoolingAsyncValueTaskMethodBuilder<string> |
There was a problem hiding this comment.
Consider adding a counter example here using a task-like type that does not hit the custom builder. Essentially demonstrating the fallback / default case.
There was a problem hiding this comment.
Added example of picking inner scope and also falling back to non-override behavior.
proposals/async-method-builders.md
Outdated
| } | ||
| ``` | ||
|
|
||
| ### Determining the builder type for an async method |
There was a problem hiding this comment.
I would not expect that to be the case however I do think we need to spec out whether or not this should apply to async lambda as well once we have the ability to add attributes.
proposals/async-method-builders.md
Outdated
| 1. looking in the containing scopes for an override attribute that specifies a builder type compatible with the method's return type. | ||
| 2. otherwise, falling back to the builder type determined by previous approach. (see [spec for task-like types](https://github.com/dotnet/csharplang/blob/master/proposals/csharp-7.0/task-types.md)) | ||
|
|
||
| The containing scopes are considered starting from the method, then its containing types and finally the module. Each scope is considered |
There was a problem hiding this comment.
Couple of items that I think we should add clarity on:
- How does this work for local functions? Does a local function consider the attributes on the function that contains it or is it containing scope the type?
- By containing types here we mean we consider the inner most nested type and walk outwards to the containing type correct?
There was a problem hiding this comment.
-
Good catch for local functions and lambdas. I'm proposing that the attribute also apply to those and those scopes contribute to the lookup chain.
-
Yes, we'll look at the containing types in turn, walking outwards.
proposals/async-method-builders.md
Outdated
| 4. consider the type of that `Task` property (a task-like type): | ||
| If the task-like type matches the return type of the async method, then the override is compatible. Otherwise, it is not compatible. | ||
|
|
||
| If the attribute was specified on a method, but it turned out to be incompatible, an error is produced. |
There was a problem hiding this comment.
What do we do in the case multiple attributes are defined on the method?
My recommendation is we should emit an error. The AllowMultiple = true is necessary for the assembly / type case but doesn't make much sense at the method level.
proposals/async-method-builders.md
Outdated
| 6. **Passthrough state to enable more efficient pooling**. Consider a type like SslStream or WebSocket. These expose read/write async operations, and allow for reading and writing to happen concurrently but at most 1 read operation at a time and at most 1 write operation at a time. That makes these ideal for pooling, as each SslStream or WebSocket instance would need at most one pooled object for reads and one pooled object for writes. Further, a centralized pool is overkill: rather than paying the costs of having a central pool that needs to be managed and access synchronized, every SslStream/WebSocket could just maintain a field to store the singleton for the reader and a singleton for the writer, eliminating all contention for pooling and eliminating all management associated with pool limits. The problem is, how to connect an arbitrary field on the instance with the pooling mechanism employed by the builder. We could at least make it possible if we passed through all arguments to the async method into a corresponding signature on the builder's Create method (or maybe a separate Bind method, or some such thing), but then the builder would need to be specialized for that specific type, knowing about its fields. The Create method could potentially take a rent delegate and a return delegate, and the async method could be specially crafted to accept such arguments (along with an object state to be passed in). It would be great to come up with a good solution here, as it would make the mechanism significantly more powerful and valuable. | ||
|
|
||
| ## Design meetings | ||
| 1. Should the compiler produce a diagnostic if the method-level attribute was found not compatible? |
There was a problem hiding this comment.
Strongly believe yes. Putting an attribute on a method is about as strong of a declaration the customer can make that they intend for the builder to be used there. If we instead silently fell back to the default or one on the containing type it seems a virtual certainty that this would not be the behavior the user expected and hence we should produce a diagnostic.
proposals/async-method-builders.md
Outdated
|
|
||
| ## Design meetings | ||
| 1. Should the compiler produce a diagnostic if the method-level attribute was found not compatible? | ||
| 2. Should the compiler produce a diagnostic if multiple override attributes are specified on a method? |
There was a problem hiding this comment.
Yes. Similar logic as above. It's a bit of a non-sensical declaration. Hard to even justify it from say a source generator perspective.
proposals/async-method-builders.md
Outdated
| /// <summary>Gets the <see cref="Type"/> of the associated builder.</summary> | ||
| public Type BuilderType { get; } | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| In the C# compiler, prefer the attribute on the method when determining what builder to use over the one defined on the type. For example, today if a method is defined as: | ||
| The attribute can be applied on methods (or local function), constructors, events, properties, types and modules. |
There was a problem hiding this comment.
function [](start = 50, length = 8)
"functions"?
LDM recommend that we try and simplify the module-level usage of overrides. This proposal explains how we determine the applicable override/builder type when the attribute doesn't specify a "target return type".
@stephentoub @dotnet/roslyn-compiler for review.
Relates to async override proposal: #1407