dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.97k stars 4.03k forks source link

[Proposal] Assignment readonly members from methods called from constructor #3200

Closed KalitaAlexey closed 7 years ago

KalitaAlexey commented 9 years ago

Sometimes when a class has many members and calculation of first value for each member is hard. Code in constructor may be hard to maintenance and to understand.

internal class GameSettingsLayer : CCLayer
{
    private readonly CCMenuItem decreaseFieldCountMenuItem;
    private readonly CCMenuItem increaseFieldCountMenuItem;
    private int fieldCount = 1;

    internal GameSettingsLayer()
    {
        var decreaseLabel = new CCLabel("Decrease", "Helvetica", 36, CCLabelFormat.SystemFont);
        decreaseFieldCountMenuItem = new CCMenuItemLabel(decreaseLabel, sender =>
        {
            --fieldCount;
            ShowFields();
        });

        var increaseLabel = new CCLabel("Increase", "Helvetica", 36, CCLabelFormat.SystemFont);
        increaseFieldCountMenuItem = new CCMenuItemLabel(increaseLabel, sender =>
        {
            ++fieldCount;
            ShowFields();
        });

        var menu = new CCMenu(decreaseFieldCountMenuItem, increaseFieldCountMenuItem);
        AddChild(menu);

        ShowFields();
    }
}

Instead it may be written like

internal class GameSettingsLayer : CCLayer
{
    private readonly CCMenuItem decreaseFieldCountMenuItem;
    private readonly CCMenuItem increaseFieldCountMenuItem;
    private int fieldCount = 1;

    internal GameSettingsLayer()
    {
        CreateMenu();
        ShowFields();
    }

    private void CreateMenu()
    {
        var decreaseLabel = new CCLabel("Decrease", "Helvetica", 36, CCLabelFormat.SystemFont);
        decreaseFieldCountMenuItem = new CCMenuItemLabel(decreaseLabel, sender =>
        {
            --fieldCount;
            ShowFields();
        });

        var increaseLabel = new CCLabel("Increase", "Helvetica", 36, CCLabelFormat.SystemFont);
        increaseFieldCountMenuItem = new CCMenuItemLabel(increaseLabel, sender =>
        {
            ++fieldCount;
            ShowFields();
        });

        var menu = new CCMenu(decreaseFieldCountMenuItem, increaseFieldCountMenuItem);
        AddChild(menu);
    }
}
svick commented 9 years ago
  1. I would find it really confusing if I tried calling a normal-looking method from some other method and my code suddenly stopped compiling. I think that such methods should be marked with some kind of annotation (a keyword or at least an attribute).
  2. How would such code compile to IL? If you used the straightforward approach, where a readonly field in C# is compiled to an intionly field in IL and a C# method is compiled to IL method, then that would produce unverifiable code. One way to solve that would be to always inline such methods (is the current compiler even capable of doing that for all method signatures?). Another way would be not to emit the relevant fields as initonly.
KalitaAlexey commented 9 years ago

@svick Keyword - is good. Let the compiler inline this methods

whoisj commented 9 years ago

I'm still on bored with using const to prefix methods that keep no references, modify no local or global variables, and only operate on constants, local variables, and the arguments supplied. Calling these from constructors should be allowed.

Joe4evr commented 9 years ago

:+1: I recently ran into this, where I couldn't compute the value of an anonymous type's member through a method, forcing me to manually inline it into a really long expression (the result works, but isn't pretty). The ability to mark a method to conform to what @whoisj just summed up would help a great deal, although I'm not quite 100% sold on using const (but also not completely opposed).

HaloFour commented 9 years ago

I'm not sure that I like the idea of having C# syntax to define a method that isn't really a method at all.

In these cases I usually write a static method that accepts whichever fields I want to initialize as out parameters. That could be unwieldy for a lot of readonly fields but it otherwise works pretty well, and it's frankly something I rarely feel the need to do.

sumtec commented 9 years ago

I think it is a problem not rare but common. We just ignore it because it usually doesn't "harm", and, the most import is, we can't do anything about it under the current syntax. To see how common it is, you can open any WinForm / WPF solution and take a look at any InitializeComponent method. All the members initialized there should not be changed actually but not marked "readonly" because we can't assign a read only field outside the constructor.

Another closely related issue is,it's not possible, for now, to design a field which is defined in the base class but initialized in the sub class, and should not be modified once it's initialized.

As what we already know, there are lots of walk around like giving up marking them as read only fields. Or maybe having some template method get called inside the base class and override in the sub class. It's kind of obscuring the real intent of the design and exposing things which should not be.

Maybe I should write a detailed proposal to demonstrate the problem.

springcomp commented 8 years ago

I would love this as I need it very frequently. Why not introduce the concept of readonly methods? Something like this:

public sealed class SomeType
{
    private readonly SupportingType value_;

    /// ctor
    private SomeType()
    {
        Initialize();
    }

    private readonly void Initialize()
    {
         // can initialize readonly (and assign to other) private members
         value_ = MethodThatReturnsSupportingType();
    }
}

A class could have any number of readonly methods. Those methods should only be private and should not be virtual. Those methods could only be invoked from a constructor, or another readonly method.

Those methods should also probably have a void return type, but I think there should not be many restrictions to those methods.

HaloFour commented 8 years ago

@springcomp

It's a CLR limitation that a readonly field cannot be assigned to from any method except from the constructor. Attempting to do so will result in an unverifiable assembly. At best you could have "pseudo-methods" which don't actually exist at all and are copied directly into the constructor body, but nothing else would be able to call that method, not even constructors from derived types (outside of the current assembly).

bbarry commented 8 years ago

Particularly with generators, I would like methods that are potentially inlined as part of compilation. I am not sure precisely what such a feature should be capable of, but I think it is beyond the scope of merely inlined into constructors.

In the meantime I continue to play around with private static void methods that have several ref/out parameters as substitutes to see how far it reaches. Actual inlining could have spec conforming behavior with perhaps unexpected results. For example inlining into a syntax tree that represents an expression tree...

gafter commented 7 years ago

We are now taking language feature discussion in other repositories:

Features that are under active design or development, or which are "championed" by someone on the language design team, have already been moved either as issues or as checked-in design documents. For example, the proposal in this repo "Proposal: Partial interface implementation a.k.a. Traits" (issue 16139 and a few other issues that request the same thing) are now tracked by the language team at issue 52 in https://github.com/dotnet/csharplang/issues, and there is a draft spec at https://github.com/dotnet/csharplang/blob/master/proposals/default-interface-methods.md and further discussion at issue 288 in https://github.com/dotnet/csharplang/issues. Prototyping of the compiler portion of language features is still tracked here; see, for example, https://github.com/dotnet/roslyn/tree/features/DefaultInterfaceImplementation and issue 17952.

In order to facilitate that transition, we have started closing language design discussions from the roslyn repo with a note briefly explaining why. When we are aware of an existing discussion for the feature already in the new repo, we are adding a link to that. But we're not adding new issues to the new repos for existing discussions in this repo that the language design team does not currently envision taking on. Our intent is to eventually close the language design issues in the Roslyn repo and encourage discussion in one of the new repos instead.

Our intent is not to shut down discussion on language design - you can still continue discussion on the closed issues if you want - but rather we would like to encourage people to move discussion to where we are more likely to be paying attention (the new repo), or to abandon discussions that are no longer of interest to you.

If you happen to notice that one of the closed issues has a relevant issue in the new repo, and we have not added a link to the new issue, we would appreciate you providing a link from the old to the new discussion. That way people who are still interested in the discussion can start paying attention to the new issue.

Also, we'd welcome any ideas you might have on how we could better manage the transition. Comments and discussion about closing and/or moving issues should be directed to https://github.com/dotnet/roslyn/issues/18002. Comments and discussion about this issue can take place here or on an issue in the relevant repo.


I am not moving this particular issue because I don't have confidence that the LDM would likely consider doing this.