Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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]: [call local static functions in base constructor] #3980

Closed
1 of 4 tasks
mstaros opened this issue Oct 7, 2020 · 27 comments
Closed
1 of 4 tasks

[Proposal]: [call local static functions in base constructor] #3980

mstaros opened this issue Oct 7, 2020 · 27 comments
Assignees
Labels
Needs Approved Specification This issue needs an LDM-approved specification Proposal champion Proposal
Milestone

Comments

@mstaros
Copy link

mstaros commented Oct 7, 2020

call local static functions in base constructor

  • Proposed
  • Prototype: Not Started
  • Implementation: Not Started
  • Specification: Not Started

Summary

Today to calculate parameters to be sent from derived class constructor to base constructor one has to define additional static method in derived class. It would be nice to use local static function instead.

Motivation

Not to litter class members scope with functions that should be encapsulated as local ones.

Example



class A
 {
     public A(int x)
    {
        ...
    }
}
class B:A {
   public B(int z):base(Local(z))
   {
            static int Local(int h)
            {
                 ...
            }
   }
}


Drawbacks

Alternatives

Unresolved questions

Design meetings

https://github.com/dotnet/csharplang/blob/master/meetings/2020/LDM-2020-10-26.md#static-local-functions-in-base-calls
https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-09-28.md#ungrouped

@YairHalberstadt
Copy link
Contributor

Are there any other things ordinary static methods can do, but local static methods can't? It's probably worthwhile to collect a list of them all and try to do as many of them as possible in one go.

@333fred
Copy link
Member

333fred commented Oct 7, 2020

It surprises me that this isn't something we already support. I'm interested in championing this, specifically for static local functions only.

@YairHalberstadt
Copy link
Contributor

@333fred the reason this isn't supported is because of name lookup. I think it will be a breaking change to make local functions in scope in the base constructor.

@YairHalberstadt
Copy link
Contributor

e.g.

using System;

Console.WriteLine(new Derived().A);

class Base
{
    public int A;
    protected Base(int a) => A = a;
}

class Derived : Base
{
    public Derived() : base(GetInt())
    {
        static int GetInt() => 42;
    }
    
    static int GetInt() => 41;
}

😢

@Youssef1313
Copy link
Member

Youssef1313 commented Oct 14, 2020

@YairHalberstadt I don't think it's a big deal. We can give the static methods at class level (i.e, the one returns 41 in your example) a priority when it's being called in base. (With a warning in a warning wave when a local function has the same name as a method in the class level)

@YairHalberstadt
Copy link
Contributor

@Youssef1313

I think that would be concerning, because imaging I had this code:

using System;
using static SomeClass;

class Base
{
    public int A;
    protected Base(int a) => A = a;
}

class Derived : Base
{
    public Derived() : base(GetInt())
    {
        static int GetInt() => 42;
    }
}

This would work fine. Now somebody adds a static int GetInt() method to SomeClass and my code changes to call that method silently, instead of the local function.

@HaloFour
Copy link
Contributor

IMO I'd rather evaluate whether it's acceptable to take the source-breaking change and have only the local function be in scope, to keep it consistent with local functions in normal methods. What is the likelihood that someone is calling a static function in their base constructor call that happens to have the same name as a local function defined within that constructor?

@YairHalberstadt
Copy link
Contributor

If the breaking change were considered acceptable, would it be ideal to try and stuff this into C# 9 to limit the chance of anyone being broken by this?

@YairHalberstadt
Copy link
Contributor

Also would local functions and static local functions have different scope to limit the breakages? local functions were introduced 3 years ago, and static local functions only last year. Only static local functions are useful in base classes.

Furthermore, would we want to do this for this constructor calls as well?

@Youssef1313
Copy link
Member

@Youssef1313

I think that would be concerning, because imaging I had this code:

using System;
using static SomeClass;

class Base
{
    public int A;
    protected Base(int a) => A = a;
}

class Derived : Base
{
    public Derived() : base(GetInt())
    {
        static int GetInt() => 42;
    }
}

This would work fine. Now somebody adds a static int GetInt() method to SomeClass and my code changes to call that method silently, instead of the local function.

In this case, I think a normal warning ("The call to '{0}' in base() is ambiguous between a static local function and a static method") could be added (keep the warning limited to calls in base)

@Youssef1313
Copy link
Member

Furthermore, would we want to do this for this constructor calls as well?

IMO Yes. There is no reason for this to be allowed in base(...) but not this(...).

@YairHalberstadt
Copy link
Contributor

In this case, I think a normal warning ("The call to '{0}' in base() is ambiguous between a static local function and a static method") could be added (keep the warning limited to calls in base)

That would break existing code.

@HaloFour
Copy link
Contributor

For context, currently when the C# compiler finds a local function in scope with a given name it immediately stops attempting to resolve that name in any other scope. That includes static members, imported static functions, etc. This is true even if the other members are a better match, or the local function isn't a match at all:

SharpLab

@333fred
Copy link
Member

333fred commented Oct 14, 2020

If the breaking change were considered acceptable, would it be ideal to try and stuff this into C# 9 to limit the chance of anyone being broken by this?

Even if it was, C# 9 is locked. This definitely couldn't make it in.

Now, as to the breaking change, I fall in the camp of "the number of people who will be bitten by this is likely vanishingly small" camp, but I'm willing to discuss alternative overload resolution rules to make it work.

@IS4Code
Copy link

IS4Code commented Jan 30, 2021

What about this? Seems like it should have the same lookup rules.

class Derived : Base
{
    public Derived() : base(x)
    {
        const int x = 0;
    }
}

@333fred 333fred modified the milestones: Working Set, Any Time Sep 28, 2022
@333fred 333fred added the Needs Approved Specification This issue needs an LDM-approved specification label Sep 28, 2022
@333fred
Copy link
Member

333fred commented Sep 28, 2022

This issue is open for community contribution. To start with, needs an approved specification for how it would work, which can be posted here as a comment or have a PR made to the proposals folder. Please follow the proposal template when doing so.

@TahirAhmadov
Copy link

TahirAhmadov commented Sep 30, 2022

I think there should be an error if a static local function is defined and a identically named type-level static method exists, as well, and a function with this name is called as part of base ctor call. This error should be raised at the base ctor call site.

That would break existing code.

Yes but 1) I doubt it has a significant real world impact because it's mostly limited to new code going forward, and 2) the concerns raised by others by suddenly binding to a different method because a base class was modified, etc., are serious enough that an error is warranted.

@CyrusNajmabadi
Copy link
Member

Yes but 1) I doubt it has a significant real world impact because it's mostly limited to new code going forward,

Why would it be limited to new code going forward? Wouldn't it affect existing code that already has written this?

@TahirAhmadov
Copy link

My thinking was, the circumstance where 1) a static type-level method is in scope, 2) an identically named static local function is defined in child ctor, and 3) base ctor call uses the static method, is highly unlikely. I know, this is an empirical question, but I think even if we tried to write a tool which would look for this in publicly available codebases, we wouldn't find this much, if at all.
On the other hand, if we make it a warning or introduce a silent resolution rule, it's very likely introducing an evil bug. So the question isn't whether we break code, but how we break code. I think a loud and clear break is better than silent and evil one.

@CyrusNajmabadi
Copy link
Member

My point was that it's not limited to new code going forward. It may absolutely break existing code. I'm personally ok with that, i just want to categorize things properly :)

@TahirAhmadov
Copy link

Oh yes, I never denied that it's impossible to break existing code, just that it's 1) unlikely and 2) kind of preferable.

@Code-Chops
Copy link

I really wish this could be fixed. It feels like the feature local functions was a feature that has been implemented incompletely. It is very logical to convert (or do other things with) some arguments first before passing them to the base constructor. This way the logic is local and not exposed to the rest of the class.

And I don't understand the discussion about breaking changes. I would expect this feature is no different than the initial support of local functions?

@HaloFour
Copy link
Contributor

I really wish this could be fixed.

The team is willing to accept community contributions on this proposal, including spec changes. But it does not seem to have bubbled up to the point where they are going to devote resources to address it themselves, aside from marshaling through the community contribution, which isn't insignificant.

And I don't understand the discussion about breaking changes. I would expect this feature is no different than the initial support of local functions?

The difference is that local functions didn't exist, so it was never possible to break the meaning of existing code since that code wasn't legal previously. In this case the local function syntax already exists, and it's possible for someone to have declared a static local function within the constructor yet have a call to a static member in the base constructor call. That would result in the meaning of the existing code silently changing. However, it seems that members of the language design team are fine with that.

@IS4Code
Copy link

IS4Code commented Jan 20, 2023

That would result in the meaning of the existing code silently changing

Not necessarily. The local function could be used only if the name couldn't be matched to a method outside, for example. Then it wouldn't change existing code, but of course adding a method to the base class would take precedence over the local function, but considering a single class var{} could break everything too, I think it's not that big of an issue.

Of course this too depends on the hypothetical concrete proposal.

@HaloFour
Copy link
Contributor

@IS4Code

The local function could be used only if the name couldn't be matched to a method outside, for example.

That would make this one situation behave differently than local functions do in every other situation, which, IMO, is much less desirable.

The team has already suggested that they're willing to accept the source breaking change in this scenario given the low likelihood that it will actually affect anybody, so I don't think it's necessary to consider ways to work around it.

@IS4Code
Copy link

IS4Code commented Jan 20, 2023

Well this is also (to my knowledge) the only situation where local functions are used outside of the scope they are defined, so a different behaviour is not all that surprising.

@HaloFour
Copy link
Contributor

Well this is also (to my knowledge) the only situation where local functions are used outside of the scope they are defined, so a different behaviour is not all that surprising.

I expect it's also extremely unlikely to have defined a local function with the same name as a member without having the intention to call it. Either way, the team seems totally fine with the potential for that source breaking change, that is not holding up the implementation of this feature.

@dotnet dotnet locked and limited conversation to collaborators Nov 19, 2024
@333fred 333fred converted this issue into discussion #8619 Nov 19, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
Needs Approved Specification This issue needs an LDM-approved specification Proposal champion Proposal
Projects
None yet
Development

No branches or pull requests

9 participants