dotnet / csharplang

The official repo for the design of the C# programming language
11.56k stars 1.03k forks source link

Proposal: Use static keyword for lambdas to disallow capturing locals and parameters (VS 16.8, .NET 5) #275

Open mattwar opened 7 years ago

mattwar commented 7 years ago

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.

int y = 10;
someMethod(x => x + y); // captures 'y', causing unintended allocation.

with this proposal you could the static keyword to make this an error.

int y = 10;
someMethod(static x => x + y); // error!
const int y = 10;
someMethod(static x => x + y); // okay :-)

LDM history:

mattwar commented 7 years ago

Related issues/proposals: Lambda Capture lists: https://github.com/dotnet/roslyn/issues/117 Non-Capture Lambdas with ==> syntax: https://github.com/dotnet/roslyn/issues/11620

alrz commented 7 years ago

I'd really prefer capture lists overs this, that'd be something like this []() => {}. what advantages we'd have over capture lists?

mattwar commented 7 years ago

@alzr just considering an alternative to capture lists in case we chose not to do capture lists.

MgSam commented 7 years ago

I like this better than capture lists, which I think are way too heavyweight.

DavidArno commented 7 years ago

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?

iam3yal commented 7 years ago

@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. 😆

jnm2 commented 7 years ago

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?

yaakov-h commented 7 years ago

Does this need to be a language feature or is this something you could reasonably achieve with an Analyzer?

alrz commented 7 years ago

As another alternative: https://github.com/dotnet/roslyn/issues/6671 (see the example).

MgSam commented 7 years ago

@eyalsk

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" 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

DavidArno commented 7 years ago

@MgSam,

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.

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 static keyword, I'd still end up making that mistake.

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...

MgSam commented 7 years ago

@DavidArno You seem to be arguing two contradictory things:

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.

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 static would be tiresome and noisy. Perhaps that's a reason for instead taking an attribute based approach to this that can either be applied locally to a given lambda or to an entire method / class to ensure no captures anywhere.

DavidArno commented 7 years ago

@MgSam,

Fair point: I didn't express myself very well there 😁

I'll retry, in a hopefully less muddled way:

  1. The only use I can see for local functions is for closures. Ergo I see no point to protecting local functions against closures.
  2. If I create a closure with a lambda, it's because I want to. Sure, I might accidentally do this, but to protect against this I'd have to put static everywhere and consciously remove it when I want a closure.
  3. If we were creating lambdas now, I can just about see a benefit to making them not support closures by default and needing to explicitly declare them as such if I needed a closure. But they exist and create closures by default. Therefore this proposal comes too late.

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 [closure] attribute that an analyzer would need to allow closures in lambdas, which then creates an opt-in "pit of success" solution:

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 .

iam3yal commented 7 years ago

@MgSam

"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.

Well, you didn't mention anyone so I thought that you referred to just any C# dev out there. :)

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.

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:

public static void ContainingMethod()
{
    int i = 42;

    {int iCopy = i;
    Action a = () =>
    {
        Console.WriteLine(iCopy);
    };}
}

But I think that the following reads a lot better:

public static void ContainingMethod()
{
    int i = 42; // variable to be captured by value
    ...
    Action a = [int iCopy = i]() => {
        Console.WriteLine(iCopy); 
    };
}

Example taken from the capture lists proposal.

Plus, anytime a feature suggestion is ripped straight from C++, I'm immediately suspicious. :P

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.

iam3yal commented 7 years ago

@DavidArno

there was a proposal (which I now can't find) to allow attributes to appear anywhere.

I'm not sure whether you found it but just to let you know, @alrz linked it above you. 😉

DavidArno commented 7 years ago

@eyalsk,

Thanks. Should have known that @alrz would have a link to it as his GitHub-search-fu is unsurpassed in my experience 😁 Comment updated.

HaloFour commented 7 years ago

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.

iam3yal commented 7 years ago

@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 😄

tannergooding commented 7 years ago

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 this).

mattwar commented 7 years ago

@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.

iam3yal commented 7 years ago

@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. :)

rjamesnw commented 7 years ago

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.

AlgorithmsAreCool commented 7 years ago

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.

benaadams commented 7 years ago

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?

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
}
gulshan commented 6 years ago

In my opinion, there should be all-capturing lambdas and a non-capturing lambdas. And things in-between like capture-lists is a complexity not necessary in C#. By non-capturing, I mean not even statics and constants. Then the non-capturing lambdas almost become pure functions. And using the same non-capturing lambda syntax, there can be even expression bodied pure functions.

Regarding syntax, I would love to see thin arrow -> for non-capturing lambdas while fat arrow => remains for all-capturing lambdas. I know -> is already a valid operator in C# for pointer member access. But the usage is very limited, only in unsafe code. So, thin arrow operator -> can be member access or non-capturing lambda depending on context. But if this seems to be a breaking change, --> or ==> can be other options. dotnet/roslyn#11620 proposed ==>.

RikkiGibson commented 5 years ago

I think we should consider this feature again now that static local functions are in.

CyrusNajmabadi commented 5 years ago

I'm going to add some personal experience that makes me want this even more. While investigating and changing our sqlite usage in roslyn I found numerous cases where we had attempted to move to non-capturing lambas. only to fail to do so. i.e. a simple case like:

                    return connection.RunInTransaction((tuple) =>
                    {
                        // If we were passed a checksum, make sure it matches what we have
                        // stored in the table already.  If they don't match, don't read
                        // out the data value at all.
                        if (tuple.checksumOpt != null &&
                            !ChecksumsMatch_MustRunInTransaction(tuple.connection, tuple.rowId, tuple.checksumOpt))
                        {
                            return null;
                        }

                        return connection.ReadBlob_MustRunInTransaction(tuple.self.DataTableName, tuple.columnName, tuple.rowId);
                    }, (self: this, connection, columnName, checksumOpt, rowId));

This had several issues. First, while 'this' was passed in as an arg, it wasn't used when calling ChecksumsMatch_MustRunInTransaction. Second, it later got updated to pass in a cancellation token like so: ChecksumsMatch_MustRunInTransaction(tuple.connection, tuple.rowId, tuple.checksumOpt, cancellationToken).

In my PR i basically changed these all to be static delegates as that was the only way i could be sure that they wouldn't actually capture anything.

I strongly believe there's a need for an easy way to create a non-capturing lambda. Just trying to be vigilant with a lambdas is too hard to do. And the alternatives are extremely heavyweight and onerous. With the above, here's the signature you need for the static field:

private static readonly Func<(Accessor<TKey, TWriteQueueKey, TDatabaseId> self, SqlConnection connection, bool writeCacheDB,
    string columnName, Checksum checksumOpt, long rowId, CancellationToken cancellationToken), Stream> s_validateChecksumAndReadBlock =

That's pretty awful, and something that lambdas make so much nicer. We're just lacking a way to get the best of both worlds.

alrz commented 5 years ago

@CyrusNajmabadi What is the argument against using an analyzer? assuming we'll have lambda attributes at some point (though that wouldn't be a blocker - we could even use a marker comment)

CyrusNajmabadi commented 5 years ago

@alrz i personally view the no-capture-single-alloc case to be important enough to just support directly with a keyword :)

Also, I view this as parallel with what we did with local functions (both in syntax and semantics). We had local functions, then we made static versions to prevent allocations. I'm just bringing that to lambdas for the same benefits with the same semantics.

madelson commented 4 years ago

To me, the downside of this is that it in many cases it will lead to more verbose code without benefit.

Here's the scenario I'm imagining:

  1. Dev who is new to C# writes some clean LINQ: foo.Select(x => 2 * x)
  2. IDE message (or code reviewer) says "hey, you should add static" => changes to foo.Select(static x => 2 * x);
  3. Dev: "why is C# so verbose compared to Javascript (for example)?"

Unfortunately, this is already playing out with IDE0062 (Local function can be made static) in VS2019.

I understand the desire to be able to add more assertions to the code to protect against subtle regressions like capturing, but I wonder if in cases like this we would be better served by an optional analyzer which allows you to, for example, annotate that all lambdas in a method do not capture via an attribute on the method. Another option is just using static local functions in place of lambdas, right? In the few bottleneck cases where performance really matters, is sacrificing inline lambdas so bad?

CyrusNajmabadi commented 4 years ago

I don't understand the scenario you're imagining. If the code reviewer says: this would be beneficial to be static then why is that a problem?

Dev: "why is C# so verbose compared to Javascript (for example)?"

"To provide stricter assurances about the behavior to ensure performance characteristics about that code."

Another option is just using static local functions in place of lambdas, right?

No. A static local function can still allocate when passed as a delegate. In a loop, for example, this can be bad. You also get at least a fresh allocation per outee method call.

The purpose here is to ensure at most one allocation for the lifetime of your application run.

madelson commented 4 years ago

I don't understand the scenario you're imagining. If the code reviewer says: this would be beneficial to be static then why is that a problem?

Sorry I realize that I stated this in a confusing way. I was imagining a someone who knows just enough to be dangerous. The IDE messages are a more compelling example.

"To provide stricter assurances about the behavior to ensure performance characteristics about that code."

This is true, but I like how in C# writing code in the concise, straightforward way is good enough for the vast majority of apps, but there are tools for writing more performant code (often at the expense of some verbosity) when it matters. My worry is that if the IDE or docs push people too hard towards trading conciseness for performance, then overall it will discourage people from using the language since they will end up with code that is clunkier than it needs to be.

No. A static local function can still allocate when passed as a delegate.

Good to know. For some reason I had it in my head that https://github.com/dotnet/roslyn/issues/5835 was already implemented. Given that it isn't, I find myself more in favor of this feature (although it would be nice to have that one as well). I just wouldn't want it to come along with a default IDE message telling you to add this everywhere.

CyrusNajmabadi commented 4 years ago

My worry is that if the IDE or docs push people too hard towards trading conciseness

I don't see why that would happen. With local functions they are about equally ceremonious as before.

With lambdas there would be no default feature that proposed to users to make them static.

SirUppyPancakes commented 4 years ago

Would this feature allow for some way to require that a passed delegate is static? It may be a pretty specific use-case (though I would consider this one of the major pain-points in desktop UI development), but it would be really helpful in implementing a compile-time-safe WeakSubscribe method on IObservable similar to the one outlined here: http://blog.functionalfun.net/2012/03/weak-events-in-net-easy-way.html

This was back in 2012 when you could still check Target to get a hint about if this was captured, but now C# almost always sets Target, so it doesn't work anymore. It would be really cool to see a way for WeakSubscribe to allow only static delegates, checked at compile-time, which then gives you a really comfortable solution to a very common source of memory leaks.

YairHalberstadt commented 4 years ago

As a highly niche use case, I think that's a good target for an analyser @SirUppyPancakes

Joe4evr commented 4 years ago

Depending on how it'll end up being implemented, the proposed value-type delegates would maybe enforce that no local state from the callsite can be captured.

333fred commented 4 years ago

Given that we are implementing function pointers, static delegates are unlikely to be pursued as proposed today.

yugabe commented 4 years ago

I feel it would be more than sufficient to add some kind of non-obtrusive (but clear) syntax highlighting to supported IDEs for captured variables instead of cluttering the language with such scenarios.

CyrusNajmabadi commented 4 years ago

instead of cluttering the language with such scenarios.

there is no enforcement in that case. so it would be easy to accidentally regress this. for example, you currently call a static method from your lambda. Someone changes that method to be an instance method. Now your lambda captures 'this'. THe changes were far apart, and there was nothing to make you think you needed to rescan all your lambdas to ensure they were still not capturing.

--

Th above is not a speculative scenario. This very issue caused a regression for us. A static lambda would have prevented it.

Joe4evr commented 4 years ago

I feel it would be more than sufficient to add some kind of non-obtrusive (but clear) syntax highlighting to supported IDEs for captured variables instead of cluttering the language with such scenarios.

You can already see this in the Quick Info window: Source

yugabe commented 4 years ago

@CyrusNajmabadi I understand, but unless you explicitly planned to not capture variables, you wouldn't use a less concise syntax just for the sake of it, right? It has no added benefit at the time you write the code. And if you did not, you couldn't prevent the problem anyways. I'm not saying it's not possible to capture variables unintentionally, but if you plan for it, you can make it explicit with the current toolset too.

@Joe4evr yes, this gave me the initial idea that you should be able to tell at a glance what variables are closed on (not by explicitly hovering). It could also be reused on local methods and even instance methods, I guess. In your example the M method call and i variable could be slightly highlighted in some way, maybe by dimming the background color of those symbols/expressions.

CyrusNajmabadi commented 4 years ago

but if you plan for it, you can make it explicit with the current toolset too.

You can, but it's onerous and unpleasant. We do it in some places. I would like the terseness and code-locality of lambdas, with the safety of non-capturing.

but unless you explicitly planned to not capture variables, you wouldn't use a less concise syntax just for the sake of it, right?

Yes, we did (several times). Because it's really clear and easy to reason about the code when the lamba is right there associated with the thing that is being operated on.

yugabe commented 4 years ago

I feel this problem should be solved some other way. It adds very high complexity to new learners of the language. And as you pointed out, there were several issues with the code you showed. Only introducing static/noncapturing lambdas won't solve the issue, because the developer whose code you reviewed and refactored didn't know any better. Wouldn't have, even if there were static lambdas in the language.

The only question I can't seem to answer to myself: what added benefit does this language construct have when I could either just not capture any variables or remove the keyword? And when it turns out that I do, I have to go back and modify the code elsewhere. It's like "fighting the framework", instead we're fighting the language. In EF queries, you'll put static before all/most of your lambdas?

CyrusNajmabadi commented 4 years ago

It adds very high complexity to new learners of the language.

How so?

Only introducing static/noncapturing lambdas won't solve the issue

Yes it woudl have. The language would have immediately blocked the user from accidentally making a change that would have caused a capture.

what added benefit does this language construct have when I could either just not capture any variables

"not capturing any variables" is hard to enforce. We've learned that ourselves several times. Mistakes are trivial to make and the implicitness of the language makes that very possible. Being explicit here allows one to ensure the compiler enforces this aspect.

or remove the keyword?

Removing the keyword would be seen and can be flagged during review. Seeing an implicit capture a far distance away cannot be seen when, say, changing a method from static to instance.

In EF queries, you'll put static before all/most of your lambdas?

I'll put static before all the lambdas for which i want to enforce that no captures happen. I will not put it on lambdas where i'm capturing, or do not care about capturing.

tannergooding commented 4 years ago

It also helps avoid issues in performance critical code, like Hardware Intrinsics, where you may want to provide methods encapsulating the various paths (such as ARM vs x86 vs Software) but where they should never be exposed/callable directly and where capturing and other bits can hurt performance or codegen in what is meant to be highly optimized code.

alrz commented 4 years ago

Removing the keyword would be seen and can be flagged during review. Seeing an implicit capture a far distance away cannot be seen when, say, changing a method from static to instance.

I think https://github.com/dotnet/roslyn/issues/39850 is relevant here.

yugabe commented 4 years ago

Wouldn't an analyzer be a better fit for this though, really? You can configure and scope an analyzer to files, folders, extend their use with supporting attributes, code fixes, configure severity and so on. I don't think this as a mainline scenario for the vast majority of people. Even if implemented, should be opt-in, but then, there might not need be a reason to add this to the language. Install/enable the analyzer which checks whether lambdas capture or not (of course, the analyzer can come with the SDK).

Joe4evr commented 4 years ago

Even if implemented, should be opt-in

But it IS opt-in: You have to stick the static keyword in your code to get this compiler-enforced no-capture behavior in the first place.

yugabe commented 4 years ago

@Joe4evr you're right, but with an analyzer I can scope the warnings/errors to say, a specific namespace or file/type in my project. Without that, I have to put the static keyword everywhere. I'd prefer the former.

yugabe commented 4 years ago

@eyalsk I would like to know why thumbs down this? I didn't state anything that was not my opinion.