-
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
Proposal: Use static keyword for lambdas to disallow capturing locals and parameters (VS 16.8, .NET 5) #275
Comments
Related issues/proposals: |
I'd really prefer capture lists overs this, that'd be something like this |
@alzr just considering an alternative to capture lists in case we chose not to do capture lists. |
I like this better than capture lists, which I think are way too heavyweight. |
I don't see the point to this, but it's got a bunch of up-votes. So rather than just down-vote, I'll assume I'm ignorant of the issue. Why is this (and/or capture lists) needed? Is accidental closure capture a problem for people? |
@MgSam Too heavyweight for whom? people want to have control over what gets captured and sometimes how it gets captured and this locks the language into a specific behaviour which seems too limited. People can always have this as a syntactic sugar to capture lists but doing this first would be a shame. I have a bad feeling about capture lists, they won't make it. 😆 |
I'm in the same place as @DavidArno. If there is a special case where you need to disrupt the syntax to be absolutely sure that you aren't capturing anything, is it so bad to refactor it to a static method? |
Does this need to be a language feature or is this something you could reasonably achieve with an Analyzer? |
As another alternative: https://github.com/dotnet/roslyn/issues/6671 (see the example). |
"People" is kind of vague and arbitrary, no? I've never had a need for such a feature and even here among C# enthusiasts demand seems tepid at best. While on the other hand there have occasionally been times when I want to ensure that a lambda (or now a local function!) is not closing over anything. Lambda capture lists add a bunch of new syntax for a narrow use case. And you'll undoubtedly have some programmers overuse them every single time they write a lambda, needlessly complicating code. Whereas this proposal reuses an existing keyword in a lightweight, thoughtful, and natural way. Plus, anytime a feature suggestion is ripped straight from C++, I'm immediately suspicious. :P |
This is what I'm not getting here. If I "want to ensure that a lambda (or now a local function!) is not closing over anything" then I make it a private method. The only reason I see for creating local functions is because I want to close over something. And regarding lambdas, again I'm struggling to see how I'd "accidentally" create a closure and - even if I did - unless I make a habit of cluttering my lambdas with eg the If we didn't yet have lambdas, then there could be a case for them not allowing closures by default, but this proposal seems a classic case of swinging the stable doors shut as the horse disappears over the horizon... |
@DavidArno
I very highly doubt that you do this. The vast majority of the time when you are using LINQ, you are probably not calling private methods with it, but rather creating lambdas. It is true that for a chain of LINQ methods prefixing them all with |
Fair point: I didn't express myself very well there 😁 I'll retry, in a hopefully less muddled way:
Now if this were combined with capture lists, then it becomes a more useful feature. Then I could have an analyzer report a "bare" lambda as an error and I get to explicitly say whether it's a closure or not: someMethod(x => x + y); // analyzer error. Bare closure lambdas are now bad.
someMethod(static x => x + y); // compiler error, y can't be used in a non-closing lambda
someMethod([y] x => x + y); // all is well, compiler and analyzer happy. However, if the proposal to allow attributes to appear anywhere were implemented, this could be the best solution in my view as I could introduce a someMethod(x => x + y); // analyzer error. Bare closure lambdas are now bad.
someMethod([closure]x => x + y); // analyzer happy. Update added the link to the attributes anywhere proposal, thanks to @eyalsk and @alrz . |
Well, you didn't mention anyone so I thought that you referred to just any C# dev out there. :)
You're right but then you wouldn't have to do it for every lambda and in my opinion this proposal/feature isn't mutually exclusive with capture lists simply because this feature can be a syntactic sugar to capture lists. Just to give an example where this feature wouldn't really help, today we can do something like this:
But I think that the following reads a lot better:
Example taken from the capture lists proposal.
I don't know, I mean I know that many C++ idioms don't fit in C# but I can't see why this wouldn't make sense here, I mean I'm not suggesting the same rules but syntactically in my opinion, it fits nicely. |
An alternative which could be accomplished today with normal attributes and analyzers would be to mark on the method which variables may be enclosed: [AllowClosure("x", "func")]
public void MyMethod() {
int x = 123;
int y = 456;
Func<int, int> addX = arg => arg + x; // fine, closure encloses "x" which is explicitly allowed
Func<int, int> addY = arg => arg + y; // error, closure encloses "y" which is not allowed
Func<int> func = () => x + y; // fine, closure is named "func" which is explicitly allowed to enclose
} It's certainly more clumsy than a language feature or support for call-site argument attributes, but it would get the job done. |
@HaloFour Yeah I guess this can work as a trade-off for nothing in the language or lack of solution but dunno why shouldn't we get proper solution? rhetorical 😄 |
This may conflict or overlap with the championed static delegate proposal https://github.com/dotnet/csharplang/blob/master/proposals/static-delegates.md. It might conflict if we wanted to allow lambdas for static delegates. It might overlap since those will already not capture some information (won't capture |
@eyalsk you should always assume that all proposals start tied to a brick and dropped to the bottom of the ocean. The hardest part of getting any feature added to the language is convincing others its a good idea, it has low cost and high benefit for most users, and its high enough on the pile of good ideas to ever get implemented. |
@mattwar Sure, I don't think otherwise, I know my arguments are not convincing so it's all good, I'm with @DavidArno and others on this though, I'd prefer to enhance existing features in the language such as attributes and then introduce it through "Attributes everywhere" than having this feature. :) |
I'm pushing for non-capturing/static lambdas and this is why: http://stackoverflow.com/q/43217853/1236397 Since C# 6, people shouldn't be forced to push all static lambdas for expression building now into the class making clutter. I vote for either some non-capturing syntax, or the "static" modifier, which is basically much the same thing. |
I like this better than capture lists. The vast majority of the time i am am either wanting closures, or not. And if i am not wanting to allocate this is a simple way of enforcing that. Capture lists are technically superior but at the cost of much more complex syntax that will only be used in very niche scenarios. I think this proposal is a judicious compromise. |
Because a method group allocates so you have to create a lambda for your static function anyway. static int Add(int x, int y) => x + y;
static int Use(int x, int y)
{
SomeMethod(Add); // Allocates delegate per call
SomeMethod((x, y) = > Add(x, y)); // Caches delegate, doesn't allocate
} So you end up in a weird pattern of having to use either a static function plus a lambda or a static lambda at class level readonly static Func<int, int, int> Add = (int x, int y) => x + y;
static int Use(int x, int y)
{
SomeMethod(Add); // Delegate statically allocated once
} |
@yugabe With |
@svick if there was an analyzer, there was a rule (or rules) that would report on diagnostics. This analyzer would report on all lambdas in scope which were closing over anything. In my mind, this report would be disabled (or "silent") by default. You would have to opt-in to use this kind of diagnostic, either by flipping a switch in the IDE (I wouldn't prefer this, it would use discrepancy between team members) or using an .editorconfig file. By using an .editorconfig, you can scope where you would like the diagnostic reported as errors (or warnings, suggestions, etc.) in a folder-hierarchy. If you put it in your project root, it will report capturing lambdas as errors. Optimally you can suppress this message locally. Also, you can enhance the rule in any way, maybe by putting an If going down this route, you can get compile-time checking in a controlled environment for closures without having to modify the language. The Making lambdas |
But i don't want this. Because i'm not setting this rule for a scope. I'm choosing it for particular lambdas.
That's what
You need a way to do attributes on lambdas with your proposal. That's a change to the language.
Yes. And we have precedence for that. For example we literally created We also have the exactly analogous feature with
How would you write this as an analyzer today? |
That would cause a lot of noise for all the lambdas that do and should capture variables.
If you're talking about something like
AFAICT, no such thing is on the roadmap. "Permit attributes on local functions" (#1888) is coming, but that's a very different feature. The closest I can think of to this is "Attributes everywhere" (#2478), but that's not being pursued.
The same arguments would apply to |
@yugabe Right and I disagree with what you've said, analyzers doesn't solve all of the things, mainly two things that this feature guarantees and I'm not going to repeat what other people said but if it bothers you I can remove them. p.s. Extension methods and/or analyzers are not silver bullets, some people think they are. |
@eyalsk no, it's all right, I just wanted to know if there was any specific reason. P.S.: Same goes for language features, I suppose :)
You can control .editorconfig in a folder hierarchy. I was on the assumption that some assemblies need this more than others, and in those you would like to enforce not capturing. If that is too broad, you can put it in a folder, and it will only apply to items in that folder. By suppressing, I was thinking about the two approaches, yes, the Generally I am not against the feature as a whole, just that I don't find it clear when someone should use it and when someone shouldn't (application and/or library developers, mainly). I mean, of course, you want to avoid allocations, but that's something the framework and language should try to default to, maybe by not trying to allocate when passing method groups as delegate parameters, but I'm not sure that is possible; or even if it is, I don't know whether it would be a big breaking change or what other aspects of the language/framework it affects. The other one is the intent of the developer. If the developer intends to capture variables, they do. If they "unintentionally" capture variables, why would they intentionally put the static keyword on the lambda? I don't think it makes much sense.
Even using the |
@CyrusNajmabadi sorry, I just saw your reply. I guess you are right, this is quite analogous to static local methods (and as I stated above, I was under the impression that attributes were coming to lambdas too). The only difference is when I declare a local method, there is a code refactoring that suggests I should make it The other question still unanswered: is there going to be a symmetric |
I think that you want this on a per-lambda basis. Anything above that is too broad.
The issue about that is dotnet/roslyn#5835, though it hasn't seen any activity in quite a while.
You can have a refactoring that doesn't produce any messages, so I don't think that's a reason against this feature.
The proposal says:
I think this means that that answer is yes. |
@svick thanks, that does it for me then, I'm on board. |
I'd rather see the function being called decide that it doesn't want to be given a lambda that has captured anything. For example, on ConcurrentDictionary we could define AddOrUpdate like this:
Now there's no way the caller can accidentally capture the context and cause a memory allocation. They would need to pass any state they require in the the
This is particularly useful in high performance code where you may be queuing lambdas up to be processed on work queues. You don't want to have the caller accidentally do an allocation in order to create the lambda and the captured context, but you've got no way of enforcing it. Yes, someone can add You could also allow the caller to specify |
That's not true. This is exactly what analyzers are for. Writing one that checks that a particular api is only passed non allocating lambdas would be trivial. |
Are you seriously telling me that I've got a requirement to not allocate memory then I need to write an analyzer to enforce this? What's to stop someone disabling the analyzer? Likewise, if the answer is to write an analyzer then why do we need the ability to declare the lambda as NOTE: Writing an analyzer is not trivial! |
Regardless of the merits of a different feature shape, this feature has already been implemented. If you have a new request, please open a new discussion. |
Yes. I'm seriously telling you that.
Because there is no way to annotate a lambda to say that it shouldn't allocate. It's trivial to annotate a method to do that.
Yes. That's what i said. -- So, to sum up:
|
No they're not, as you can just remove the |
That's a fair point. I would open a request, but it seems these days the people in charge of C# are more interested in adding pointless features to the language, such as:
|
I mean, you're talking to 2 members of the ldm right now, so we're not going to agree with that assessment. It also does not change the fact that this feature, static lambdas, shipped at the beginning of the month. If you are interested in proposing a new feature, please open a new discussion. |
Yes. If you remove the thing enfocing something, it is no longer enforced. I can't help you there. The purpose of those was to be able to be explicit that you want callsite enforcement, and you'd then get that.
All indications are not requirements if you allow those indications to be removed. |
I wonder if https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-9.0/function-pointers would fit your use case. Beyond that, the language doesn't provide a facility for a method to restrict the set of things that callers can do in the lambda that is passed in the way that you're asking. This would be a separate proposal. |
@foxesknow wrote:
If you don't participate, you guarantee that your voice won't be heard. Open the issue, contribute to the discussion. Don't expect everyone to agree with you. |
As long as we are not getting that horrible C++ syntax for capturing locals with Also, as someone who extensively uses F# too, I like the idea of
const int y = 10;
someMethod( static x => x + y ); // okay :-)
int z = 9;
someMethod( x => (static (w, a) => w + a + y)(x, z) ); // also okay, and captures the z only, we don't really need capture lists.
//it could even be
someMethod( (const x) => (static w => w + x + y)(z) ); // with this pattern you can precisely say what you want to capture
int k = 8;
someMethod( (const x) => (static (k, z) => k + z + x + y)(k, z) ); //even better, hide outer scope to avoid misuse If you are already paying the cost of a heap allocation, you may as well pay an extra call. And that's not even needed, as the compiler could (in theory) easily inline |
Allow
Func<int, string> = static i => i.ToString();
.To avoid accidentally capturing any local state when supplying lambda functions for method arguments, prefix the lambda declaration with the static keyword. This makes the lambda function just like a static method, no capturing locals and no access to this or base.
with this proposal you could the static keyword to make this an error.
LDM history:
The text was updated successfully, but these errors were encountered: