-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add proposal for lambda improvements #4451
Conversation
|
||
lambda_parameter | ||
: identifier | ||
| (attribute_list* modifier* type)? identifier equals_value_clause? |
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.
couold we do: attribute_list? (modifier* type)? identifier equals_value_clause?
instead?
it seems like ([x] a) => ...
is reasonable, and doesn't require the user to have to provide full types for lambdas that want attributes. #Resolved
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 of 2 minds on this:
- We don't allow you specify just the
ref
today. If you want more signature than just a name, you have to have the whole signature. This decision is in line with that. - Declaration of ref/out parameters in lambdas without typename #338 is championed and would remove that restriction, so why add a similar restriction here? #Resolved
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.
Exactly :) but even if we don't do #338, i wouldn't perpetuate this. :) #Resolved
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.
f = delegate ([MyAttribute] int x) { return x; }; // syntax error | ||
``` | ||
|
||
Attributes on the lambda or lambda parameters will be emitted to metadata on the method that maps to the lambda. (If the lambda does not require a closure class, the lambda is emitted as a method on the containing type. Otherwise the lambda is as a method on the generated closure class.) |
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.
Do we have existing guarantees this strong about how lambdas are emitted? #Resolved
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.
No we do not. This would be a change from previous versions of C#. The goal is to mirror the decisions and language we made for local functions. #Resolved
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.
iirc this is a huge perf hit for some lambdas due to the this
shuffle with delegates. remember that we only ever emit local functions like this when they're not converted to delegates
An explicit return type may be specified after the parameter list. | ||
```csharp | ||
f = () : T => default; // () : T | ||
f = x : int => 1; // <unknown> : int |
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.
Might be a better example if it's a type we couldn't infer, like short
. #Resolved
## Natural type | ||
A lambda expression has a natural type if the parameters types are explicit and either the return type is explicit or there is a common type from the natural types of all `return` expressions in the body. Otherwise there is no natural type. | ||
|
||
A method group has a natural type if the method group contains a single method and the method or reduced extension method has no unbound type parameters. |
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.
Do we need to explicitly mention reduced extension methods? I think just "the method" would be fine? #Resolved
``` | ||
|
||
### Anonymous delegate type | ||
If synthesized delegate types are required, the compiler generates generic anonymous delegate types that are shared across all anonymous delegates in the module that have the same number of parameters and same parameter ref kinds. |
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.
And parameter types, return type, return type ref kind. #Resolved
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.
Unless you're saying that these anonymous types are generic? Not sure from reading this. Would still need to have return ref matching, and matching on whether the return is void
or not. #Resolved
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.
Also should mention async
-ness.
Unless you're saying that these anonymous types are generic?
In practice the implementation would be generic, just as anonymous types are today in C# and anonymous types + anonymous delegates are in VB. As for what the spec says, I would re-use the language in the C# spec for anonymous types. #Resolved
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.
Unless you're saying that these anonymous types are generic?
Yes, the anonymous delegate types are generic with type parameters for parameter and return types. The type of a specific lambda expression or method group would be a constructed type of the shared anonymous delegate type.
In reply to: 579440707 [](ancestors = 579440707)
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.
Updated the comment for re-using synthesized delegate types.
In reply to: 579984320 [](ancestors = 579984320)
|
||
The names of the synthesized delegate types and the names of the parameters are unspeakable. | ||
|
||
The anonymous delegate types are not co- or contra-variant unlike the delegates constructed from `System.Action<>` and `System.Func<>`. |
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.
This seems possibly unnecessary. If I'm reading correctly that these anonymous types are generic, it feels like we should make value params and returns contra/covariant, as appropriate, to ease other friction points. #Resolved
|
||
```antlr | ||
lambda_expression | ||
: attribute_list* modifier* lambda_parameters (':' type)? '=>' (block | body) |
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 :
does feel like the natural thing to do, but I'm concerned that there's going to be ambiguities with ?:
expressions we have today. Are we certain this is a syntax form that we can support? #Resolved
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.
Might easier to parse if we put it inside the ()
? Like var a = (: string) => "";
? Looks a bit ugly, but I don't believe it's ambiguous. #Resolved
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 hate that form @333fred :)
THe potential ambiguity would be something like:
x ? (y) : ...
is that one of these new lambdas on the RHS of the ?
or is this a parenthesized expression? Given that this is a legal start, it might be painful to disambiguate.
--
Note: supplying the return type explicitly is one of hte least interesting parts of this proposal for me. I wouldn't gate it on that, and i'd be happy to drop it and come up with an independent solution there. #Resolved
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 hate that form @333fred :)
I didn't say I liked my form either :P. I just don't think that :
postfixes are resolvable. #Resolved
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.
How about (int x, int y => int)
?
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've recorded the possible ambiguity as an issue for now.
In reply to: 579504322 [](ancestors = 579504322)
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.
What about (int a, int b) -> int
?
We have ->
only for pointers and it makes it more clear - at least to me - that this specifies the return type.
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.
Doesn't that end up looking like this?
var f = (int a, int b) -> int => a + b;
VS
var f = (int a, int b) : int => a + b;
Not sure about the -> followed by the =>. It does give me haskell vibes though...
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 this case I dislike the second "arrow", too.
Perhaps the arrow could be omitted in case of an explicit return type followed by a block syntax - which means that we could write for example:
var fkt = (int a, int b) -> int
{
return a + b;
};
For simple expression lambdas the explicit return type is not so important - but for complex lambdas this proposal is a very huge improvement.
## Natural type | ||
A lambda expression has a natural type if the parameters types are explicit and either the return type is explicit or there is a common type from the natural types of all `return` expressions in the body. Otherwise there is no natural type. | ||
|
||
A method group has a natural type if the method group contains a single method and the method or reduced extension method has no unbound type parameters. |
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.
A method group has a natural type if the method group contains a single method [](start = 0, length = 78)
See #129 (comment) and #129 (comment). How this address those points?
Also #129 itself seems to be covered by this change? #Resolved
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.
``` | ||
|
||
## Natural type | ||
A lambda expression has a natural type if the parameters types are explicit and either the return type is explicit or there is a common type from the natural types of all `return` expressions in the body. Otherwise there is no natural type. |
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 there be a natural type for async lambdas? If so, Task
or ValueTask
? #Resolved
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.
Ah good catch yes there should. We should spell out the rules here in the spec to make it clear that's supported #Resolved
var f6 = F1; // error: multiple methods | ||
var f7 = "".F1; // System.Action | ||
var f8 = F2; // System.Action<string> | ||
``` |
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.
One part I think we should elaborate on is that this specifically would allow us to call methods which take a Delegate
as a parameter with arguments that are just lambda expressions. For example:
void M(Delegate d) { ... }
M(() => 42); // okay
M(x: int => 13); // okay because natural type takes over
#Resolved
### Anonymous delegate type | ||
If synthesized delegate types are required, the compiler generates generic anonymous delegate types that are shared across all anonymous delegates in the module that have the same number of parameters and same parameter ref kinds. | ||
|
||
The names of the synthesized delegate types and the names of the parameters are unspeakable. |
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 I understand this. Does this mean that Delegate.Method.GetParameters()
won't return ParameterInfo
s with the actual Name
of the parameters for lambdas requiring synthesized delegate types? The parameter names are important to MapAction(). Or is this referring to some other parameter name? #Resolved
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 parameter names are important to MapAction().
Then don't we have a more fundamental problem? If the parameter names matter then any re-use of the Action
and Func
types will not work because we can't change the names of them. #Resolved
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.
Considering we already get preserved parameter names when we explicitly cast a method group to the appropriate Action
or Func
, I assumed this is something that was stored in the Action
or Func
instance rather than baked into the types themselves.
Ex: https://github.com/halter73/HoudiniPlayground/blob/574242d9b971620b4e7e7e71c69240be1a053667/Program.cs#L16-L21 #Resolved
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 think this isn't a problem as the synthesized delegate type's invoke method may have generic names, but Delegate.Method.GetParameters()
will still have the correct names.
Func<int,int> foo = (int horse)=>horse;
Console.WriteLine(foo.Method.GetParameters()[0].Name)
prints "horse". #Resolved
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 assumed this is something that was stored in the
Action
orFunc
instance rather than baked into the types themselves.
All delegates track the runtime method handle of the target method, which is what you're getting when you reference the Method
property. That's how you get the parameter names of the target method instead of the placeholder parameter names of the Func<...>
delegates themselves, e.g. arg1
, arg2`, etc. #Resolved
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.
Right. This works fine, we care about the target method, not the delegate parameters. #Resolved
@halter73 and I were just discussion something we want to make sure is called out in the spec wrt type inference and overload resolution. Consider the following overloads: public static class EndpointRouteBuilderExtensions
{
public static IEndpointConventionBuilder MapGet(this IEndpointRouteBuilder routes, string pattern, Func<HttpContext, Task> requestDelegate)
{
return null;
}
public static IEndpointConventionBuilder MapGet(this IEndpointRouteBuilder routes, string pattern, Delegate @delegate)
{
return null;
}
}
public class Startup
{
public void Configure(IApplicationBuilder app)
{
app.UseRouting();
app.UseEndpoints(endpoints =>
{
// This should pick the `Func<HttpContext, Task>` overload
endpoints.MapGet("/hello", context => context.Response.WriteAsync("Hello World"));
// This should pick the Delegate overload
endpoints.MapGet("/hello2", (int c) => Task.CompletedTask);
});
}
} Normally before lambdas have a natural type the second overload wouldn't be possible but now it is and it should choose the right overload appropriately (though I know overloading is a source of complication in the language) #Resolved |
@davidfowl indeed, was chatting with @cston this morning about adding similar style samples to the spec to make the updates clear here #Resolved |
Regarding natural lambda types, does that come with any relaxation when passing a lambda to a method that accepts a different delegate type that has a compatible signature? If not it seems like you couldn't really pass those synthetically created lambdas anywhere without casting to the explicit delegate type which would make the feature a lot less useful, IMO. |
So to be clear, you're asking about this scenario: void Main()
{
var f = (Span<int> s) => s[0] + 1; // Compiler generates: delegate int CustomDelegate$0(Span<int> s);
Call(f); // Compiler error because CustomDelegate$0 != CompatibleButDifferent
}
public delegate int CompatibleButDifferent(Span<int> s);
void Call(CompatibleButDifferent del)
{
d(new byte[100]);
} Feels like a much rarer scenario to me. It would matter in places where a Func/Action type couldn't be used so that means, byref types (because of generics), out and ref parameters (not sure what else besides those). |
Or with any API that doesn't use |
I get it, I don't think its as pervasive anymore since the dawn of Func and Action. I hear you on the parameter types and documentation as well (though those don't show up super well in the IDE today). ASP.NET does this today for the RequestDelegate and it means this wouldn't work: public delegate Task RequestDelegate(HttpContext context);
void Configure(IApplicationBuilder app)
{
var f = (HttpContext context) => context.Response.WriteAsync("Hello World"); // infers Func<HttpContext, Task>
app.Run(f); // Fails to convert Func<HttpContext, Task> to RequestDelegate
} This does seem related to the overall set of features here but could be generalized as structurally compatible delegate conversions should work. I think that goes against the mantra that delegates are type safe . dotnet/runtime#4331 (ah look it's your issue 😉 ) |
Which goes against the design of Discounting the prevalence of Anyway the argument against lambda inference in the past has always been that you need to know the type of the delegate so that you can actually do something with the instance of the delegate, like pass it to a method, and that the compiler doesn't (and shouldn't) have special knowledge of |
Trying to solve both during the same timeframe seem fine but I don't think they are coupled and one can exist without the other. |
I'll also reference a previous proposal/discussion I posted on the subject of inferred lambdas: #1694 What I like about the approach is that the compiler doesn't take a position on the actual type generated until it is used, at which point the compiler knows which delegate type to use, if one at all. If the lambda is used as a delegate anywhere in the method then the compiler will emit the appropriate display class and create/cache the delegate instance by type at the point of first usage. The cost would be a single allocation for the display class and one allocation for each delegate type referencing the lambda. If the lambda is not used as a delegate at all then the compiler can emit the more optimized machinery for a local function instead. Local functions I think offer precedence for this behavior: public void M(List<Item> list) {
bool f(Item item) => item.IsExpired;
list.RemoveAll(f);
list = list.Where(f).ToList();
} And to me I think it makes sense for "natural lambdas" to be an alternate syntax to local functions. You can't convert a local function to a |
The compiler already has special knowledge of http://sourceroslyn.io/#Microsoft.CodeAnalysis/WellKnownTypes.cs,3bb7bf76580e8f66
This is essentially an entire new kind of type inference though. Essentially delay the decision of var local = 42;
M(out local);
void M(out long l) { ... } That would benefit from the same type of delayed type inference. As such it seems, while useful and very challenging to implement, an orthogonal concept.
This is definitely the type of feature I'd like to see.
Short term this is likely a better avenue to approach. For example:
|
Possibly, although I'm not arguing for taking it that far as
Why? It doesn't seem to solve the biggest problem, which is that you can't use |
I'd suggest that the ASP.NET API use cases can all be solved without adding inference of lambda variables to the language. If the plan is to allow conversion from method group to app.UseEndpoints(endpoints =>
{
Task hello1(int x) => Task.CompletedTask;
endpoints.MapGet("/hello1", hello1);
endpoints.MapGet("/hello2", (int c) => Task.CompletedTask);
}); Because of this my opinion is that it isn't necessary to explore inference of lambda variables at this time. I think having to choose between I'd rather see this space re-explored with the appropriate runtime support for signature equivalence and/or a more flexible approach rather than implementing something now that could not be changed without observable breaking changes in the future. |
I don't understand how suggesting a runtime feature changes any of this... |
Can we also add support for optional parameters in lambdas? It would be useful for endpoints like the following and would be natural to developers used to defining similar actions inside of MVC controllers. endpoints.MapGet("/products", ([FromQuery] int pageIndex = 0, [FromService] ProductsDb db) =>
db.GetProductsAsync(pageIndex)); Tangentially related: It looks like local functions lose default parameters in the compiler generated method. This could cause problems for developers trying to use local functions instead of lambdas with MapAction. I filed a separate issue for that at dotnet/roslyn#51518 Thanks @Kahbazi for finding this! |
This is something we should be tracking in this proposal. Part of doing attributes on the emitted methods rationalizing that it's important, and okay, for these members to be seen from metadata. That would include logic like emitting our optional member metadata. |
f = delegate ([MyAttribute] int x) { return x; }; // syntax error | ||
``` | ||
|
||
Attributes on the lambda or lambda parameters will be emitted to metadata on the method that maps to the lambda. |
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.
Think we should elaborate on this point a bit. In the past we have taken a very hard stance that we don't want customers taking a dependency on how lambdas and local funcs map from source to metadata. The structure of how the are emitted can, and has, changed between compiler versions. This has, and will continue, to break customers that attempt to take a dependency on how source maps to metadata.
The changes we are making here are more targeted at the Delegate
driven scenario. It's completely valid to look at the MethodInfo
attached to a Delegate
and dig into its properties. Emitting the explicit attributes to metadata here, as well as any other supporting attribute in code or emitted by compiler (items like default params), is intended to support this type of usage by customers. It allows for teams like ASP.NET to make the same types of behaviors on lambdas that they do for normal functions as well as allowing for features like trimming to handle lambdas appropriately.
It does not change the overall stance that customer should not depend on how items like lambdas map from source to metadata . #Resolved
Invoke(GetString); // Invoke(Func<string>) | ||
Invoke(GetInt); // Invoke(Delegate) [new] | ||
``` | ||
|
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.
Think we need a couple of counter examples here showing where method group and lambda conversions will not work after this change. Specifically:
- Cases where there are multiple candidates in the method group which means we can't infer a natural type
- Cases where the return type of the lambda cannot be inferred hence the lambda does not have a natural type #Resolved
|
||
[ASP.NET MapAction](https://github.com/dotnet/aspnetcore/pull/29878) without proposed changes (`MapAction()` takes a `System.Delegate` argument): | ||
```csharp | ||
[HttpGet("/")] Todo GetTodo() => new(Id: 0, Name: "Name"); |
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.
@halter73 we should get these samples updated.
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.
Yeah. One issue with that is the current updated samples rely heavily on conventions for parameter binding and we no longer use an attribute for specifying the route pattern or HTTP method. That said, all the parameter attributes can still be optionally applied.
app.MapPut("/todos/{id}", async ([FromRoute(Name = "id")] int identifier, [FromBody] Todo inputTodo, [FromServices] TodoDb db) =>
{
var todo = await db.Todos.FindAsync(identifier);
if (todo is null) return NotFound();
todo.Title = inputTodo.Title;
todo.IsComplete = inputTodo.IsComplete;
await db.SaveChangesAsync();
return NoContent();
});
## Attributes | ||
Attributes may be added to lambda expressions. | ||
```csharp | ||
f = [MyAttribute] x => x; // [MyAttribute]lambda |
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.
This is the form that I think should not be allowed because it could plausibly mean either [MyAttribute] (int x) => x
or ([MyAttribute] int x) => x
. I think we should require parens around the argument list if you're using attributes. #Resolved
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.
@cston is there an open question in this doc for that? #Resolved
app.MapAction([HttpPost("/")] ([FromBody] Todo todo) => todo); | ||
``` | ||
|
||
## Attributes |
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.
So excited to change all of our roslyn tests to lambdas :) Right now they can only be local functions :(
Proposal needs to be moved from the proposals/csharp-10.0 folder to proposals. The 10.0 folder will be filled when we know what actually shipped. #Resolved |
Sounds good. Perhaps we can move the proposal after merging, to avoid breaking the links to existing comments in the this PR. In reply to: 791597365 [](ancestors = 791597365) |
```csharp | ||
f = [MyAttribute] x => x; // [MyAttribute]lambda | ||
f = [MyAttribute] (int x) => x; // [MyAttribute]lambda | ||
f = [MyAttribute] static x => x; // [MyAttribute]lambda |
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.
This form is also probably included in the question about parameter parenthesization #Resolved
; | ||
``` | ||
|
||
_Does the `: type` return type syntax introduce ambiguities with `?:` that cannot be resolved easily?_ |
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.
Consider including the alternative forms we discussed in LDM.
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'll update the syntax based on the LDM and subsequent meetings in a separate PR.
In reply to: 588782316 [](ancestors = 588782316)
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.
LGTM (commit 9). My remaining comments can be resolved in a follow up, if you prefer. In that followup, please update the link in https://github.com/dotnet/csharplang/blob/master/meetings/2021/LDM-2021-03-03.md#natural-type-for-lambdas to point to the checked in document, rather than this PR.
Will do, thanks. In reply to: 605683764 [](ancestors = 605683764) |
Support lambdas with attributes, explicit return type, and natural type.