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

Proposal: Local function parameter shadowing #15793

Closed
alrz opened this issue Dec 9, 2016 · 5 comments
Closed

Proposal: Local function parameter shadowing #15793

alrz opened this issue Dec 9, 2016 · 5 comments
Labels
Area-Language Design Language-C# Resolution-Won't Fix A real bug, but Triage feels that the issue is not impactful enough to spend time on.

Comments

@alrz
Copy link
Member

alrz commented Dec 9, 2016

In case of an async or iterator local function, to defer state machine creation before parameter validation (and avoid allocations thereof) we can use a local function but if we have captured variables it would not be deferred as intended, because if the local function is async or iterator the variables will be captured in a class rather than a struct. To resolve this issue one must not capture any variable or parameter but since local functions do not allow to re-declare any locals, one should use different names from original parameters. For example:

public static IEnumerable<T> Where<T>(this IEnumerable<T> source, Func<T, bool> predicate) {
    if (source == null) ...
    if (predicate == null) ...
    IEnumerable<T> WhereImpl(IEnumerable<T> _source, Func<T, bool> _predicate) {
        foreach (var item in _source) {
            if (_predicate(item))
                yield return item;
        }
    }
    return WhereImpl(source, predicate);
}

I want to propose to allow local functions to shadow parameters of the enclosing method to make this easier and prevent unintended behavior due to the misspelling of parameter names.

@gafter
Copy link
Member

gafter commented Dec 9, 2016

@dotnet/ldm There is a suggestion here for relaxing the "single definition rule" for local function parameters shadowing parameters. I think it has merit, but it is late for C# 7. Fortunately relaxing later would not be a breaking change.

@alrz
Copy link
Member Author

alrz commented Jan 11, 2017

I think it's not just parameters shadowing parameters, I think local function parameters should be able to shadow any kind of variable/field in the scope just like regular methods, it's just inconvenient to be forced to choose new names specially when they are leaking from declaration expressions,

{
  bool F(StatemetSyntax statement) { ... }  // NOPE
  if (node is StatementSyntax statement) { ... }
}

Since we explicitly declared parameter list, there is nothing to be mistaken really.

@aluanhaddad
Copy link

If it works for local functions, it should work for lambdas.

var task = Task.Run(() => 1).ContinueWith(task => task.Result);

Hang on, that works in the C# interactive window but is definitely an error.

@alrz
Copy link
Member Author

alrz commented Jan 15, 2017

@aluanhaddad I don't think that would be extremely useful considering that lambdas are most likely inlined deep inside other expressions. In my opinion, to prevent accidental shadowing it is actually a desirable restriction. In fact, capture list is proposed to make it even more visible. None of these are true for local functions, they are top-level statements with a formal parameter list. Personally, I've never found myself desperate to use shadowing names for lambda parameters and I've already demonstrated a recurring use case in which you really need to have identical names. Furthermore, in any case if you need a lambda with shadowing parameters you could use a local function instead to make that more explicit; that's probably the case for other lambda's shortcomings as well, e.g. iterators. Note that an important use case for local functions is to "reuse" but you usually pass lambdas to other methods and a single letter parameter name suffices most of the time e.g if the meaning of the parameter could be inferred by the context in which you are writing the lambda.

@gafter
Copy link
Member

gafter commented Mar 17, 2017

The LDM discussed this and are concerned that permitting this may make it too easy to introduce bugs, so we do not intend to move forward with this as a championed proposal. The original motivaring example can be written this way

public static IEnumerable<T> Where<T>(this IEnumerable<T> source, Func<T, bool> predicate) {
    if (source == null) ...
    if (predicate == null) ...
    IEnumerable<T> WhereImpl() {
        foreach (var item in source) {
            if (predicate(item))
                yield return item;
        }
    }
    return WhereImpl();
}

This does not cause any additional allocation compared to the requested feature.

@gafter gafter closed this as completed Mar 17, 2017
@gafter gafter added Resolution-Won't Fix A real bug, but Triage feels that the issue is not impactful enough to spend time on. and removed 1 - Planning labels Mar 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Language Design Language-C# Resolution-Won't Fix A real bug, but Triage feels that the issue is not impactful enough to spend time on.
Projects
None yet
Development

No branches or pull requests

3 participants