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

Permit lambda and local function parameters to hide names from the enclosing scope (16.3, Core 3) #2777

Open
gafter opened this issue Sep 4, 2019 · 15 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
Milestone

Comments

@gafter
Copy link
Member

gafter commented Sep 4, 2019

I believe that in C# 8 we permit lambda and local function parameters to hide names from the enclosing scope. We should specify the change and add it to the feature list for C# 8.

@gafter gafter self-assigned this Sep 4, 2019
@gafter gafter added this to TRIAGE NEEDED in Language Version Planning Sep 4, 2019
@YairHalberstadt
Copy link
Contributor

Better to ask for forgiveness than permission aye ;-)

Just joking - I like this change, and think it will decrease developer frustration, with only limited chance of causing bugs.

@HaloFour
Copy link
Contributor

HaloFour commented Sep 4, 2019

I recall this subject coming up but it seems to have largely slipped under the radar. I'd love to know more about the conversations that caused the LDM to reverse their position here and whether or not it might open the doors to more hiding scenarios in the future.

@ronnygunawan
Copy link

Should linq range variables hide names from the enclosing scope too?

@CyrusNajmabadi
Copy link
Member

The basic concern was that it is much more common now to want to create:

  1. a local function to do work, so that you don't clutter the class.
  2. have that local function not want to capture in order to help with perf.
  3. have the outer function need to then pass in locals/parameters into the local-function so it can use them.

In that case having to come up with new names for the exact same data passed in is actually a negative in multiple ways:

  1. having to come up with different names for the variables even though they're the same.
  2. potentially accidentally capturing the wrong thing because you'll have similarly named variables (though 'static local functions' helps here).

In this case, hiding is felt to be a good thing because it's not like you're hiding in a way that could be confusing (i.e. the nested name might refer to something totally different), but rather you're likely 'hiding' based on the lang spec, but in reality you'll be referencing the same thing, so having the same name be usable for that purpose is desirable.

Does that make sense?

@CyrusNajmabadi
Copy link
Member

Should linq range variables hide names from the enclosing scope too?

In general, no. Using my above post, we can see why. It would be much rarer to have a linq query that wanted to use the same name as some local/parameter in scope. That linq variable would be unlikely to be a reference to the same symbol, and thus could be quite confusing in code as to what it meant. With local-functions, it's very reasonable to want to pass in the 'outer symbol' into the local function as a parameter and then reference it with the same name. The same doesn't hold true really for linq queries in general.

In my own experience i can't remember ever wanting to do this. Whereas with local functions it happens a lot.

@HaloFour
Copy link
Contributor

HaloFour commented Sep 4, 2019

@CyrusNajmabadi

have that local function not want to capture in order to help with perf.

Does copying the same parameters really help with perf? I thought that the display struct + ref approach used by capturing local functions already solved that problem.

have the outer function need to then pass in locals/parameters into the local-function so it can use them.

This does make sense, but it also involves duplicate declarations and assignments which would be quite verbose plus a potential bug vector since the developer has to then plumb those values through to the local function manually.

I'd make the argument that if what you were looking to accomplish was to reuse locals/parameters in a local function without having to declare new names and without the perf costs of capturing that the language would benefit much more from capture-by-value.

@HaloFour
Copy link
Contributor

HaloFour commented Sep 4, 2019

@CyrusNajmabadi

It would be much rarer to have a linq query that wanted to use the same name as some local/parameter in scope.

I've often found myself wanting to do this:

var query = from employee in db.Employees
    where employee.IsActive
    select employee;

var employee = query.FirstOrDefault();

Maybe if you wrote more business applications and fewer compilers you'd agree? 😉

@CyrusNajmabadi
Copy link
Member

Does copying the same parameters really help with perf? I thought that the display struct + ref approach used by capturing local functions already solved that proble

The issue is:

  1. I want to make it static to not accidentally capture anything.
  2. so i need to pass in the values.
  3. but i don't want to have to make new names for things that are actually supposed to be the same thing.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Sep 4, 2019

Maybe if you wrote more business applications and fewer compilers you'd agree?

:)

This may also be due to the common pattern for lambdas/queries of accepting and encouraging short names. So i would have written the above as:

var activeEmployees =
    from e in db.Employees
    where e.IsActive
    select e;

var employee = activeEmployees.FirstOrDefault();

The issue is exacerbated by wanting local-functions to continue using normal parameter names.

So, the change in attitude is just around common coding patterns and not wanting to make things too painful for what seems liek a fairly reasonable case.

@HaloFour
Copy link
Contributor

HaloFour commented Sep 4, 2019

@CyrusNajmabadi

The issue is:

  1. I want to make it static to not accidentally capture anything.
  2. so i need to pass in the values.
  3. but i don't want to have to make new names for things that are actually supposed to be the same thing.

Sure, but you trade in what should be a minimal cost for a lot of verbosity and plumbing. I'm suggesting that there are better ways to accomplish this goal. Borrowing a page from C++ lambdas is one of them, rather than making capturing a binary proposition.

So, the change in attitude is just around common coding patterns and not wanting to make things too painful for what seems liek a fairly reasonable case.

I'm arguing that you're trading one pain for another. 😄

@CyrusNajmabadi
Copy link
Member

Borrowing a page from C++ lambdas is one of them, rather than making capturing a binary proposition.

I'm just answering the question around: I'd love to know more about the conversations that caused the LDM to reverse their position here

I have no problem with further work in this space. However, this change was a simple, targeted, intentional, minimal change, to address a pain point.

@CyrusNajmabadi
Copy link
Member

I'm arguing that you're trading one pain for another.

It doesn't seem particularly painful tbh. It actually seems like a preferable coding pattern in some cases.

i.e. i personally push back heavily on many local functions that capture/mutate local state. that's because flow control because exceptionally difficult to understand. i'll literally see a variable declared, seemingly never used, then passed along (in what looks to be hte default/initial state) to something else as if it was complete. I then discover that there were intermediary calls that impacted it, but through capturing.

i.e.

var values = new List<SyntaxNode>();
Foo();
foreach (var whatever in yodawg)
   Bar();
Baz();
UseTheValues(values);

this is just a headscratcher. I often ask that the local functions become static, which may give something like this:

var values = new List<SyntaxNode>();
Foo();
foreach (var whatever in yodawg)
   Bar(values);
Baz(values);
UseTheValues(values);

Now i can see: yup, 'Foo' doesn't do anything with 'values', but 'Bar' and 'Baz' do. That will make things much easier for me to understand and i can then say things like "wait... it doesn't seem like 'Baz' should really be using 'values'... that seems off. etc. etc.

I think capturing should be used sparingly. Not because of perf, but because of clarity. I don't think it's actually the right approach much of the time. And, due to this, i want the language to then not be painful when i go the more explicit route.

@CyrusNajmabadi
Copy link
Member

Note: i'm talking in generalities. Capturing is a-ok in some cases. It's just not appropriate in all cases. And it comes up enough that i do not want capturing, that i also want non-capturing to then not feel like i'm causing myself undue pain with having to invent fresh names all the time to deal with collisions.

@HaloFour
Copy link
Contributor

HaloFour commented Sep 4, 2019

@CyrusNajmabadi

It doesn't seem particularly painful tbh.

Try those examples again with 3+ parameters. It's quite a bit of extra boilerplate, and if you miswire those arguments in the invocation you will introduce bugs without realizing it. That sounds painful to me, enough that I'd likely choose to use local function capturing, which is not slower than passing the arguments directly (unless I start mixing lambdas into the same method).

i personally push back heavily on many local functions that capture/mutate local state.

Sounds like a great argument for capture-by-value (or some kind of in capture) and readonly locals. 😁

@CyrusNajmabadi
Copy link
Member

Sounds like a great argument for capture-by-value (or some kind of in capture) and readonly locals.

That's not actually the case i'm talking about. See the example i provided above. Even with 'capture by value' and 'readonly locals' the code would still be confusing to me with the original form. It's the implicitness that's the problem. Being explicit here is what i want, but it came with costs prior to this language change.

@MadsTorgersen MadsTorgersen added this to the 8.0 candidate milestone Sep 11, 2019
@gafter gafter moved this from TRIAGE NEEDED to C# 8.0 in Language Version Planning Sep 11, 2019
@jcouv jcouv changed the title Permit lambda and local function parameters to hide names from the enclosing scope Permit lambda and local function parameters to hide names from the enclosing scope (16.3, Core 3) Sep 18, 2019
@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
@333fred 333fred removed this from C# 8.0 in Language Version Planning Feb 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification Proposal champion
Projects
None yet
Development

No branches or pull requests

7 participants