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

Champion "static local functions" (16.3, Core 3) #1565

Open
5 tasks
Tracked by #829 ...
jcouv opened this issue May 24, 2018 · 50 comments
Open
5 tasks
Tracked by #829 ...

Champion "static local functions" (16.3, Core 3) #1565

jcouv opened this issue May 24, 2018 · 50 comments
Assignees
Labels
Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification Proposal champion Smallish Feature
Milestone

Comments

@jcouv
Copy link
Member

jcouv commented May 24, 2018

The proposal is to declare a local function as non-capturing. The keyword to indicate that would be static.

Note: if we do this, then we should also consider allowing parameters and locals of the local function to shadow parameters and locals from the containing method.

  • Proposal added
  • Discussed in LDM
  • Decision in LDM
  • Finalized (done, rejected, inactive)
  • Spec'ed

Related discussion thread, including a discussion of "capture lists" (a more elaborate feature): #1277

Note: EE scenarios are a concern.

Do we want to allow some other modifiers (like unsafe)?

LDM history:

@bondsbw
Copy link

bondsbw commented May 24, 2018

To be clear, would this fit @benaadams's definition of static?

A static method in a class hides its enclosing scope; you can't access and of its instance data or call any of its instance methods - only call static functions and access static data.

Likewise a static local function shouldn't have access the the parent functions local state or the classes instance state, much like a regular static function.

The static local function is providing scoping so as not to pollute the class with a static function which is only called in a single method.

@jcouv
Copy link
Member Author

jcouv commented May 24, 2018

@bondsbw Yes, that's what I'd have in mind. The local function would not be able to access variables from parent method or instance fields. It could only access variables that it declares (parameters or locals) and static fields.

@HaloFour
Copy link
Contributor

Is there a big reason to do this for local functions, aside avoiding accidental mutations? Aren't local functions significantly more efficient with captures?

@jcouv
Copy link
Member Author

jcouv commented May 25, 2018

@HaloFour The primary motivation for this proposal is readability/maintainability. Knowing that some code is isolated makes it easier to understand, review and re-use. That's the same general argument as with any kind of isolation like object encapsulation or functional programming.

There is also a practical motivation. There are good reasons why parameters or locals of local functions should not shadow variables from the enclosing scope. But in practice, there are many pieces of pure logic that make sense to extract as local function, but where this restriction becomes inconvenient (prevents you from using obvious variable names).
Having static local functions and additionally allowing static local functions to declare their own variables regardless of context solves that problem (while avoiding the pitfalls of shadowing).

@popcatalin81
Copy link

What’s the Benefit over a private static function? Local functions benefit is exactly capturing

@orthoxerox
Copy link

@popcatalin81 more fine-grained access control, primarily

@mgravell
Copy link
Member

mgravell commented May 25, 2018

@HaloFour it isn't just mutations; it is also to incredibly useful to avoid accidentally creating the capture machinery, especially in hot paths. This can have a surprising amount of overhead, especially in scenarios like async. Again, on hot paths, a common pattern in some async pipes is to try to avoid the async machinery completely if it will often be synchronous, falling back into an async completion if it turns out to be incomplete - which means all the local state (including the capture machinery) is going to end up on the heap (because of how await works when tasks are incomplete); doing this manually can have significant savings for performance critical code, and it turns out that local functions as long as you don't capture are a very tidy way of doing it

@popcatalin81 scoping and expressiveness. I actually strongly disagree with you that the benefit of local functions is "exactly capturing" - I've written plenty of local functions, and the ones that make use of capture are by far the minority. Well, let me rephrase: the ones where I intentionally used capture are the minority. But yes, functionally they're just like regular private static methods; in fact, my usual "did I capture without realising it?" check is exactly to move the local function out of the method (so: a private static), see if the code still compiles, then move it back (and remove the modifiers).

@HaloFour
Copy link
Contributor

HaloFour commented May 25, 2018

@mgravell

If the goal is to avoid creating async machinery for capturing then I don't see why it would be limited to avoiding capture on local functions where said machinery requires significantly less overhead.

And, like most proposals that involve limiting functionality/syntax for the purpose of improving code generation and performance, why is this not suitably managed by an analyzer? Most people will not care about this degree of optimization.

I've never reached for a local function unless I wanted to capture. I see zero point to them otherwise.

@HaloFour
Copy link
Contributor

HaloFour commented May 25, 2018

@jcouv

Knowing that some code is isolated makes it easier to understand, review and re-use.
Having static local functions and additionally allowing static local functions to declare their own variables regardless of context solves that problem (while avoiding the pitfalls of shadowing).

IMO, those two statements are in direct contradiction. Allowing a local function to shadow variables would negatively impact the readability of said function. Now anytime you see a local function you have to double-back to the signature to see what that identifier refers to.

@jcouv
Copy link
Member Author

jcouv commented May 25, 2018

@HaloFour Any design involves some trade-offs and tensions. I recognize it is a fair concern to discuss and experiment with.
From experience with static methods, I've found them to reduce the cognitive load rather than increase it, despite their introducing a "mode". When reading a static local function with variables defined independently of the parent scope, I suspect the same modality will kick in so it won't be confusing.

@HaloFour
Copy link
Contributor

@jcouv

Any design involves some trade-offs and tensions. I recognize it is a fair concern to discuss and experiment with.

I agree. Consider this just part of that discussion. 😁 Despite my arguments I'm fairly ambivalent about the proposal.

Where this feature tries to address performance of certain aspects of generated code, particularly around async method machinery, it feels like this could be easily (and appropriately) addressed via analyzers. By itself this feature seems like it would barely scratch that surface. Local function capture, by itself, is not the performance problem. It's only in conjunction with those other features like async methods or delegates where the compiler resorts to the less efficient machinery. So why target a language change that barely scratches the actual problem?

I'm much more opposed to the notion of the shadowing. I agree that having to come up with a lot of different names for identifiers can be restrictive. The team was very quick to dismiss that argument in the context of leaky out declaration variables and pattern variables.

From experience with static methods, I've found them to reduce the cognitive load rather than increase it, despite their introducing a "mode".

Current static methods don't have to deal with other locals being potentially in scope, so I don't feel that this comparison is appropriate. The rules regarding shadowing fields don't change between instance or static methods.

@alrz
Copy link
Member

alrz commented May 25, 2018

@HaloFour

Current static methods don't have to deal with other locals being potentially in scope

You can conceptually map captures to instance fields,

class C
{
   int name = 0;
   void Method1() => Use(name); // OK
   static void Method2() => Use(name); // Error
}

void M()
{
   int name = 0;
   void Local1() => Use(name); // OK
   static void Local2() => Use(name); // Error
}

Also, methods can shadow fields with parameters and locals regardless of being static.

@jnm2
Copy link
Contributor

jnm2 commented May 25, 2018

@HaloFour To me, shadowing is a feature. I occasionally shadow deliberately in order to prevent uncontrolled access to a field; var controlledAccess = Interlocked.Exchange(ref this.controlledAccess, null); for example.

I've long been wishing I could declare local function parameters that shadow locals that would otherwise have been captured, in order to prevent their capture and accidental use out-of-band, while still capturing others or simply while declaring a fully-static helper method closer to where it is relevant.

@jaredpar
Copy link
Member

@HaloFour

And, like most proposals that involve limiting functionality/syntax for the purpose of improving code generation and performance, why is this not suitably managed by an analyzer?

For me this has nothing to do with improving code generation, it's all about improving code readability.

Local functions are making it much more common to have top level functions which are several hundred lines long. The logic gets split between the top level method body and a number of local functions whose behavior is specific to the top level method. Before local functions such code would be split into a number of top level method bodies.and the helper functions would be needlessly polluting the containing types scope. Now the entirety of the logic can be contained with in the top level method body.

Such local functions are harder to review than they would be as a top level function. The silent capture of state can affect how they execute and must be accounted for. There is nothing inherently wrong with this and many times it's hugely beneficial to the developer. In the cases where no capture is done, or when the developer specifically wants to avoid it, the static modifier helps significantly with readability. It tells me as a reviewer that I can review this local function in isolation. No need to scan for subtle capture problems.

For small functions this really isn't that important. But in larger methods it can make a huge difference.

It does have the additional benefit of ensuring local functions are capture free and hence will be a tiny bit more efficient as we don't even have to pass a struct closure around and have local indirection. If that were the primary motivation of this feature I'd be against it. Not worth the cost IMHO.

I'm much more opposed to the notion of the shadowing.

I'm still a bit on the fence about this. In the absence of a static modifier I'd be 100% against shadowing. It creates too much opportunity subtle behavior changes. Imagine you delete a parameter that shadows a local in the top level method, oops now you just introduced a capture you probably didn't intend.

Restricting shadowing to local functions marked as static though removes this problem and most of the issues around shadowing. It also makes local functions significantly more usable. I didn't really appreciate this problem until I started using local functions a lot in C#.

@HaloFour
Copy link
Contributor

@alrz

You can conceptually map captures to instance fields

I don't think that's the intuition that most people have regarding captured locals.

Also, methods can shadow fields with parameters and locals regardless of being static.

Yes, but never without a way to explicitly refer to that shadowed field, and you're always required to explicitly qualify the field anytime such a local is in scope.

@jaredpar

Local functions are making it much more common to have top level functions which are several hundred lines long.

Sounds like a good argument for removing local functions from the language. 😉

But seriously, cramming hundreds of lines into a single method definition doesn't sound like a good idea, with or without local functions. In my opinion that's exactly when those chunks of logic should be top-level functions, specifically so that they can maintained and tested in isolation from one another.

I can't fathom a local function that's more than maybe 4-5 LOC, and I would count that against the LOC of the function in which its contained.

Such local functions are harder to review than they would be as a top level function.

Which is probably why they shouldn't be local functions, regardless of whether they capture. Locality isn't an excuse to hide a massive ball of logic behind a blackbox.

Restricting shadowing to local functions marked as static though removes this problem and most of the issues around shadowing.

The technical problems, maybe. But I still see the problem of having to mentally parse through the contents of the local function and upon seeing any given identifier having to double-back to the signature to see whether the lexical scoping has been reset. IPU forbid having to deal with that when nesting local functions (which I hope ain't becoming a thing.)

It's not that a might refer to multiple things, it's that you can't look at a single LOC within the body of a top-level function and have any idea what a is.

@jaredpar
Copy link
Member

@HaloFour

But seriously, cramming hundreds of lines into a single method definition doesn't sound like a good idea, with or without local functions

I used to believe the same. Then I spent a bunch of time coding in F# which has local functions and gradually I opened up to the advantages it provides. There is value in keeping all the logic necessary for an operation located together.

Locality isn't an excuse to hide a massive ball of logic behind a blackbox.

The reverse is polluting a type with a number of methods that are either a) useful to a single method or worse b) only legal to be called from a specific method. Over times types grow and local functions help avoid polluting types with a number of members that simply aren't generally useful. When I type . into the editor I want to see the members that are useful to me, not some one time helper that I should never be calling.

It's not that a might refer to multiple things, it's that you can't look at a single LOC within the body of a top-level function and have any idea what a is.

That is true today without local functions. In addition to a parameter or local the value a may refer to instance field, instance member, static field, or static member all of which can come from some combination of the type hierarchy, containing types or members brought in by a using static directive. Lookup is anything but simple.

@dangi12012
Copy link

This has MANY speed advantages. We could declare a static lambda like this:

Object slow = "Any Object Here"

Action f = () => Console.WriteLine(slow); //obj is shadowed and slow in critical paths

Action f = static () => Console.WriteLine(slow); //Error slow not declared
Action f = static (x) => Console.WriteLine(x); //We dont capture, we pass as argument, which does not need to to have the exact same reference, and we dont capture more than we need.

@HaloFour
Copy link
Contributor

@dangi12012

The proposal above doesn't touch on lambdas, although they are probably worth bringing up. You really wouldn't be able to avoid the allocation of the delegate itself, particularly if the delegate instance escaped the method in which it was declared.

@jaredpar
Copy link
Member

@HaloFour

This has MANY speed advantages.

Only for lambdas which this proposal doesn't currently cover. It's a natural extension to consider but the focus for now is on local functions. For local functions there really isn't a significant speed up static vs. non static in the general case.

@popcatalin81
Copy link

popcatalin81 commented May 26, 2018

I think this thread presents a better case for improved capturing mechanisms of local functions rather than static modifier.

Example: instead of capturing in a compiler generated class the following could be viable

  • Generate a static method with parameters if variables are not mutated, the method is not assigned to a delegate and the number of parameters is small.

  • Generate ref parameters and ref returns if the variables are mutated and the method is not captured in a delegate and the number of parameters is small.

This does not give perfect control to those who whish it, but makes refactoring a lot easier compared to static non static local function.

As a second benefit, even local functions that are not marked static will benefit performance wise, by eliminating captures when possible.

@tannergooding
Copy link
Member

Static delegates are semi-related (both to the static lambda discussion above and to the static local functions here): https://github.com/dotnet/csharplang/blob/master/proposals/static-delegates.md

@JamesNK
Copy link
Member

JamesNK commented May 26, 2018

What about when you want to disable capturing but still have access to instance fields?

@mgravell
Copy link
Member

mgravell commented May 26, 2018

@JamesNK I can see the merit of a "no capture, but instance" scenario... You're right in that this aspect of the proposal is being influenced - perhaps incorrectly - by the tempting convenience of that static keyword. Maybe that is the wrong keyword. But I'm at a loss for a better one.

Thinking pragmatically, perhaps static
with the proposed semantic is fine - if you need it, just pass in this explicitly as an argument? Since a local function can't be overridden (polymorphism), virt-call seems an unnecessary step anyway.

@svick
Copy link
Contributor

svick commented May 26, 2018

@popcatalin81

If a local function is not assigned to a delegate, a non-allocating capturing mechanism is already used. Though it's not done the way you suggest, instead, a struct with the state is created and passed to the local function by ref.

@dangi12012
Copy link

@svick
If it is implemented the way you say then there really is no need for a static Delegate. The only advantage would be that you cant create a heavyweight captured class with static by accident.

@MgSam
Copy link

MgSam commented Sep 23, 2018

@mgravell
Copy link
Member

mgravell commented Feb 1, 2019

Seems to be available in VS 16 preview 2, using <LangVersion>8.0</LangVersion>. Great work folks, thanks!

@Happypig375
Copy link
Member

Secret feature?? 🤤
Not mentioned in the announcements at all!

@TKharaishvili
Copy link

TKharaishvili commented Mar 25, 2019

If you guys end up adding this feature to the language, maybe local extension methods are worth considering too? It's not a must have thing but it does sound like a nice to have thing.

The syntax would be similar to the classic extension methods:

bool CanPairUp(Token t1, Token t2)
{
    if (t1.IsKeywordOrIdentifier() && t2.IsKeywordOrIdentifier())
    {
        return false;
    }
    return true;

    bool IsKeywordOrIdentifier(this Token t)
    {
        return t.Kind == TokenKind.Keyword || t.kind == TokenKind.Identifier;
    }
}

@jaredpar
Copy link
Member

@TKharaishvili local extension method sis something we've considered but at this point don't have firm plans for. I think we'd need to see compelling scenarios to add the complexity to member lookup here.

Extension methods solve a couple of problems:

  1. Adding behavior to a type that's not a part of it's declaration
  2. Making the behavior discoverable by allowing the behavior to have member level syntax

In the case of local functions there is no discovery issue. The behavior is right there in the same method that you are editing. Can't even use partial tricks to put it into a different file. Hence it would seem to not have as much value as normal extension methods.

@TKharaishvili
Copy link

TKharaishvili commented Mar 25, 2019

@jaredpar

Adding behavior to a type that's not a part of it's declaration

The benefits of this being method chaining and the fact that it reads better.
But as I said, this is not a must have. So if you guys simply don't have time for this that's totally cool with me.

@NetMage
Copy link

NetMage commented Apr 2, 2019

It seems like implementing this undoes most of the arguments against static local variables.

@HaloFour
Copy link
Contributor

HaloFour commented Apr 2, 2019

@NetMage

It seems like implementing this undoes most of the arguments against static local variables.

Could you clarify? Other than sharing part of the name static local functions aren't at all related to static locals.

@juliusfriedman
Copy link

juliusfriedman commented Apr 2, 2019

No... but it's rather confusing at first glance until you think about how a regular static method is invoked and then you realize it doesn't matter, it just saves someone else from outside the function being able to easily call it from outside the scope and thus wouldn't pollute intellisense, as well as give you access to statics...

@333fred
Copy link
Member

333fred commented Jun 16, 2020

@zspitz if local extensions are something you want, I'd suggest commenting on that issue: #850. This is for a different feature.

@zspitz
Copy link

zspitz commented Jun 16, 2020

@333fred

This is for a different feature.

It seems to me to be an extension of local static methods, much as extension methods are in themselves an extension of methods.

I'd suggest commenting on that issue.

I've done so. Thanks.

@333fred 333fred added the Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification label Oct 16, 2020
@Thaina
Copy link

Thaina commented Jul 28, 2024

Are there any discussions about local property? Is it also included in local function?

So we can define getter and setter with one shared signature in the function

@333fred
Copy link
Member

333fred commented Jul 28, 2024

Iirc there have been other discussions about more local things, like types and properties. They haven't gone anywhere.

@dotnet dotnet locked and limited conversation to collaborators Nov 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification Proposal champion Smallish Feature
Projects
None yet
Development

No branches or pull requests