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: Class-scoped blocks for fields / explicit field usage #12361

Closed
lachbaer opened this issue Jul 6, 2016 · 26 comments
Closed

Proposal: Class-scoped blocks for fields / explicit field usage #12361

lachbaer opened this issue Jul 6, 2016 · 26 comments

Comments

@lachbaer
Copy link
Contributor

lachbaer commented Jul 6, 2016

Often there are fields in a class that are only used by a few methods or even only one property.

In addition to #850 I suggest adding block syntax for grouping fields to their associated methods only.

public class Person()
{
    private _classField_1_;
    private _classField_2_;

    {   // start scope block
        int _age;
        public int Age
        {
            public get => _age;
            private set => _age = value >= 0 ? value : 0;
        }

        public void CelebrateBirthday()
            => _age += 1;
    }   // end scope block

    /* rest of class and further blocks */
}

In this example _age won't clutter the IntelliSense and also generate a compiler error if used outside the block accidentially (by older code versions).

It is important that only fields are bound to the block, not the contained methods, properties, subclasses, etc. To make that more verbose prepending the block by a keyword, like var, could be better

public class Person()
{
    var {   // start scope block
        int _age;
        /* ... */
    }
        /* ... */
}
@bondsbw
Copy link

bondsbw commented Jul 7, 2016

When blocks are used in other areas of the language, they define the scope of contained members or locals. Whatever is inside is not accessible to the outside (unless prefixed with an appropriate access modifier, where applicable).

This suggestion breaks that pattern.

One possible solution is to add a new access modifier such as local for things that do not leak out of the scope, while the traditional public, internal, protected, and private modifiers specify visibility at the class level or beyond.

@lachbaer
Copy link
Contributor Author

lachbaer commented Jul 7, 2016

This suggestion breaks that pattern.

Yes and no. At some points you always encounter some things that break existing rules, because otherwise these things would be much more difficult to realize, i.e. write- and readable. In my opinion this (optically minor) extension would justify this (again - minor -) deviation.

And as stated, that block could be prepended by a (preferably existing) keyword to emphasize that in this block only the (implicitly) private variables are block-visible only.

One possible solution is to add a new accessibility modifier such as local

Local to what? To a surrounding block? There we are again...

@bondsbw
Copy link

bondsbw commented Jul 7, 2016

Yes, local to the surrounding block. Its scope would not leak out of the block, whereas any member which is at least private is visible to anything else within the class.

@lachbaer
Copy link
Contributor Author

lachbaer commented Jul 7, 2016

@bondsbw Indeed a nice idea and approach. Everything within a class-nested block would be block-scoped only, including fields, methods, properties and sub-classes, unless a modifier is explicitly prepended. Then there is even no need for a local keyword.

public class Person
{
    {
        int _age;    // no modifier -> scoped to this block only
        private _name;    // private modifier -> class-scope
        int _HelperMethod() { /**/ }    // no modifier -> scoped to this block only
        private int Gender { get; set; }    // get and set imply private modifier, so class scoped
     }
}

@bondsbw
Copy link

bondsbw commented Jul 8, 2016

Agreed that local should be implicit, but I prefer to include an explicit mechanism wherever an implicit one exists.

@HaloFour
Copy link

HaloFour commented Jul 8, 2016

@lachbaer

Everything within a class-nested block would be block-scoped only, including fields, methods, properties and sub-classes, unless a modifier is explicitly prepended.

I think that would be very confusing to visually parse. int foo; and internal int foo; would immediately mean two different things depending on whether or not you can find another open curly brace somewhere before that definition.

Is it really that necessary to limit visibility of members within a class to these additional scopes? If a class is so big that proper maintenance requires such rigid protection from yourself (or team mates) it seems to me that this class is too big. Local variables and functions would also likely suffice in many of those scenarios.

@lachbaer
Copy link
Contributor Author

lachbaer commented Jul 8, 2016

@HaloFour
Blended by @bondsbw local idea I forgot about internal ;-) You're absolutely right. I guess in this case and scenario an additional modifier like "scoped" would make sense.

In a perfect coding world you would also be right regarding the size of a class. But IRL I have already seen so many long classes, just look at Roslyn. Even there are classes that make use of direct access to private fields instead of encapsulating every private field in a property, what is okay. As a made-up example, imagine something needs to be refactored to a property. It could accidentially lead to bugs if the private field is visible to the whole class.

#850 only addresses property-bound fields. This issue extends it to 'regions' in classes.

BTW, nesting classes is ugly for the inteded use - before someone comes up with it ;-)

Maybe @bondsbw is right that adding a local keyword, that is allowed only at member level, is a good idea to group logical members of a class with block-scoped visibility to some members only.

@dsaf
Copy link

dsaf commented Jul 8, 2016

In this specific case, I would focus on picking apart the suggested use examples. E.g. consider this refactoring:

public class Person() //original syntax preserved
{
    private _classField_1_; //original syntax preserved
    private _classField_2_; //original syntax preserved

    public Age Age { get; private set; }

    public void CelebrateBirthday() => Age.IncreaseByYear();

    /* rest of class and further blocks */
}

public sealed class Age
{
    public Age(int years)
    {
        Years = years >= 0 ? years : 0; //use uint instead
    }

    public int Years { get; private set; }

    public void IncreaseByYear() => Years += 1;
}

I bet any example would most benefit from refactoring to extract new types / functions rather than the suggested language feature.

...block syntax for grouping fields to their associated methods only...

Like a class?

@lachbaer
Copy link
Contributor Author

lachbaer commented Jul 8, 2016

@dsaf
Really? A new heap stored class for one actual integer value?
I think even a struct would be overhead, besides it doesn't actually read or code better. And never forget, not all programmers who write in C# are total C# pros with all design patterns & co fluently to their hands!

Besides, please, either silently correct the sample code or keep your comments for yourself! It is SAMPLE code, quickly written. And no, I do not want to use uint, because this is a SAMPLE and has no reference to real world usage whatsoever...

@DerpMcDerp
Copy link

I like this proposal but the syntax of it leads to non-tight scopes e.g. lets say your existing class has:

class Foo {
    var {
        int backing1_;
        public int Backing1 { get { blah(this.backing1_); ... } }
    }

    var {
        int backing2_;
        public int Backing2 { get { blah(this.backing2_); ... } }
    }
}

And later on you want to add a method that needs access to backing1_ and backing2_. You now have to merge both scopes even though the Backing1 and Backing2 properties are unrelated.

I think a better proposal would be something akin to how C++'s lambdas explicitly require you to list which member variables you can access within scope. e.g.

class Foo {
    int backing1_;
    int backing2_;

    public int Backing1 {
        get {
            using backing1_; // this lets us gain access to this.backing1_ within this scope
            blah(this.backing1_);
        }
    }

    public int Backing2 {
        get {
            using backing2_; // this lets us gain access to this.backing2_ within this scope
            blah(this.backing2_);
        }
    }
}

@bondsbw
Copy link

bondsbw commented Jul 9, 2016

I feel like this proposal is driven by different goals than #850. Properties best fit a limited set of scenarios for simple field enhancements--like encapsulation and constraint enforcement--which represent a very common usage pattern. The goal of #850 is to further enhance those scenarios.

This proposal is for deeper needs. But as pointed out by @dsaf, a separate encapsulating class is probably as good a tool. The syntax for a private nested class is little different from what you proposed.

The reason @dsaf's class suggestion seems bloated is because the example in the original proposal is better achieved via the property. _age += 1 should be rewritten as Age += 1, which even enforces the constraint. Then throw in #850 to handle encapsulation.

@lachbaer
Copy link
Contributor Author

lachbaer commented Jul 9, 2016

I get the feeling that I haven't made clear enough the purpose of this proposal. I'll try again

Target group

I know that in many scenarios completely different approaches with different data structures or a completely different OOP design will lead to the same goal. But, C# is also used by thousands of hobby or part-time programmers, who realise their project without big teams and conceptual meetings. This proposal aims to those programmers. C# is multi-paradigm, not only big-data OOP!

Q'n'D code

With the target group in mind, many programs are written quick and dirty - even for production scenarios in SMBs. And as "quick and dirty" implies, this code may contain bugs due to some loose structures. Having easy coding technics, without completely refactoring the code on whiteboards, could lead to much more stable code and hinder bugs to occur. Especially when the codebase grows.

Cost effectiveness

You don't need to use a sledgehammer to crack a nut. Very often a lightweight coding leads to the same resolution and is much, much quicker written - therefore cheaper (if that is of interest). Having a construct that improves stability in theese cases is preferable, again without a complete OOP refactoring.

For this proposal, please keep theese three targets in mind. It targets tens of thousands of programmers who use C# on a more or less frequent basis, thus I think a relevant group.

@lachbaer
Copy link
Contributor Author

lachbaer commented Jul 9, 2016

@DerpMcDerp
Intriguing, indeed! But your sample code would imply that every not modifier decorated field must be using'd in the methods. This would break code and isn't very elegant. (Besidesusing is already a valid statement in mehtods, but with a different meaning).

How about this. Adding a new modifier local, that implies private.
Members that are decorated by local must be mentioned (declared) in the methods where they are used.

class Foo {
  local uint _age;    // member declaration
  local uint _previousAge;

  public void CelebrateBirthday()
  {
    local _age, _previousAge;  // "re-declaration", meaning these memebers are used in this block
    _previousAge = _age++;
  }

  public void GetOlder(uint years)  // obsolete method
  {
    _age += years;  // compiler-error, member _age not locally declared!
  }
}

Another advantage is that this wouldn't break the existing API, because it is just another modifier.

The IDE can easily help finding occurences to make sure that the members are only used in methods where intended. Obviously IntelliSense does not propose redeclared local members.

The local modifier can be used for all class members, thus hiding methods, properties, subclasses, etc. in blocks where not re-localed.

In case of a refactoring, a previously private field can be decorated local instead and the compiler will complain about every use in methods/blocks, where not explicitly allowed - this avoids accidential use and bugs.

@HaloFour
Copy link

HaloFour commented Jul 9, 2016

@lachbaer

The problem with that entire concept is that the very target you intend is also the least likely to actually employ this feature. Someone who slops together a giant monolithic class full of interdependencies isn't going to take the time to whittle down those dependencies into concrete scoping blocks anymore than they're likely to refactor that giant class into smaller classes. C# shouldn't add features which encourage abuse of the language and incorrect design patterns.

@dsaf
Copy link

dsaf commented Jul 9, 2016

@dsaf

Really? A new heap stored class for one actual integer value?
I think even a struct would be overhead...

The _age field was already on the heap. Also it has method(s) tied to it. Besides, if this kind of performance impact is of real concern, then C# might not be the best choice. Many useful C# features result in new types generated implicitly e.g. async / await, lambdas with captured variables.

...besides it doesn't actually read or code better.

I would argue that it does.

And never forget, not all programmers who write in C# are total C# pros with all design patterns & co fluently to their hands!

Novice programmers don't care about scoping.

Besides, please, either silently correct the sample code or keep your comments for yourself!

I did not know whether that syntax was a mistake or not.

It is SAMPLE code, quickly written.

Why not take time? Code is read more than it is written.

...has no reference to real world usage whatsoever...

That's my point. This suggestion would benefit from a sample that demonstrates a practical benefit in addition to syntax.

@lachbaer
Copy link
Contributor Author

lachbaer commented Jul 9, 2016

@dsaf
Maybe it is a generation problem?

The _age field was already on the heap

But I nevertheless guess that a class is much more overhead.

...besides it doesn't actually read or code better.

I would argue that it does.

Contra, again ;-)

And never forget, not all programmers who write in C# are total C# pros with all design patterns & co fluently to their hands!

Novice programmers don't care about scoping.

Sure, we are digital, there is nothing in between novice and pro programmers.... c'mon...

Why not take time? Code is read more than it is written.

Sorry, I don't have time to take time ;-)

This suggestion would benefit from a sample that demonstrates a practical benefit in addition to syntax.

Well, I think that everybody who has done some coding actually knows where his real world samples are. This proposal would make sense in some mid-size classes (w/o refactoring them to 100 other related classes!).

If you like a real world story: I have several projects in different languages. In some projects I started to build a nice OOP design on top of a running solution, just to refactor the code for the better. Very often the code grew and grew - still no real functional improvement. That took a lot of time. And often at some point I regreted to do so, because it got more and more complicated and I somehow lost control. I took it as a nice practice then. But the productive solutions today still runs on the old 'easy' code, with some improvements here and there.

As it mostly points out when talking to others, in the real word there are many, many, many, many people who work the same as I do and who experience the same things.

So instead of discussing about how something should be coded it would be better to stand on both feets, have a look in the real world and discuss whether a feature could be of real practical use to many of its potential users. It would still be a discussion, nevertheless.

The latest local suggestion could be at least a very nice kind of 'tool' during refactoring:

  • It is easy and quickly done on the existing code
  • It can point out conflicts during refactoring
  • It prepares and already 'marks' where a better OOP design could be of benefit
  • It doesn't break running projects when refactoring to it here and there
  • quickly write a class with future OOP refactoring already in mind, local marks possible future redesign

You see, it might be worth having a deeper look into this when considering that not all projects/programs are thoroughly planned on a whiteboard as learned on the college ;-)

@DerpMcDerp
Copy link

Another issue is with partial classes, e.g.

partial class Foo {
    var {
        int foo;
    }
}

partial class Foo {
    // no way to access this.foo here
}

v.s.

partial class Foo {
    local int foo;
}

partial class Foo {
    // should we be able to access this.foo by using "local foo;"?
}

@lachbaer
Copy link
Contributor Author

With var { } it is obvious that everything is bound to the containing block unless modified differently. But after a night of sleep I'm going to have problems with this concept (see below).

As locals implicitly are private, like privates they will be available for re-declaration in other partial'ed sections. I doubt that it will be of practical use, because in a case of refactoring to partial class stubs it will be more likely that a previously private and now local variable is re-declared in the other stub. After marking it local it is likely to issue a compiler error and thus giving a hint to where further inspections are needed, exactly what is it meant for.

But here we see, that the use of local is misleading. Local to what? To the surrounding block, to the next 10 lines of code? To this partial stub only? And redeclaring with local somehow violates its meaning, because the redeclared member is not local to the method but even used elsewhere in the class.

A better keyword might be explicit instead of local. It would imply that this variable must be explicitly redeclared, or within the method, that it is explicitly redeclared. Btw, redeclaring might just be literally, meaning the complete signature must be redeclared, introducing one level more of security in case the signature of the initial declaration changes.

  explicit int foo = 1;
  explicit void Bar(int Value);

  public void DoIt()
  {
    explicit int foo;    // this time without initialisation
    explicit void Bar(int);    // parameter name not important for signature
    /* ... */
  }

[explicit explicit operators will most likely never be used irw, so reusing explicit here would be appropriate, and not introducing a further keyword]

Maybe someone, e.g. native english speaking, can come up with a better keyword that starts with p, so that it better fits in the row of member access modifiers like private, protected, public (polished, perpetrade ;-) and so on.

Again some words about var { } or just { }:
Though the conecpt of grouping members this way is still nice in my eyes, the caused indention would violate the styling conventions.

Like with Python the IDE (and you also, hopefully) formats the code nicely with indention that respects the logical hierachy of things. Sibling members start on the same vertical column, more indented variables and (local) methods are not visible to the less indented ones (roughly, without respect to access modifiers).

A further block on class level would introduce another level of indention, but its access decorated members would be visible for the less indented ones, though they are actually siblings in the class. That somehow uargh's me.

Therefore I now think that the approach with additional class level blocks actually isn't appropriate. We should focus on the explicit declaration.

@lachbaer lachbaer changed the title Proposal: Class-scoped blocks for fields Proposal: Class-scoped blocks for fields / explicit field usage Jul 10, 2016
@HaloFour
Copy link

So for a proposal targeted at the audience of novices and hobbyists who aren't as familiar with proper programming guidelines, there is a lot of additional necessary syntax required in order to utilize the feature. Syntax that a novice or a hobbyist would not be very likely to know or understand how to use, let alone spend the time and energy planning out. This doesn't create a pit of success; it creates a mountain.

@bondsbw
Copy link

bondsbw commented Jul 10, 2016

I don't see much value in declaring the field in the class scope and then again in the method. It's confusing. It's bloated. I understand wanting to generalize #850 for use in methods, but being able to opt-in to accessing the field from anywhere else somewhat defeats the point.

Perhaps #49 covers another way that neither causes more indentation nor adds extra declarations:

public int IncrementRunningTotal()
{
    static var runningTotal = 0;  // Initialization only occurs the first time, per instance.

    return runningTotal++;  // The incremented value will be retained the next time
                            // this method is called on this instance.
}

EDIT: I just realized that VB.NET's Static behavior already works like I originally suggested, and issue #49 already covers bringing it to C#.

@bradphelan
Copy link

bradphelan commented Jul 11, 2016

This should solve the scoping rules. Sorry if it has been described above in another way.

public class Person : ReactiveObject
{
        // public declarations inside the scope are also public outside the class
        public scope {

            // private is only available in the scope
            private double  _data;

            // public is available outside the scope
            public double Height {
                get{ return _data;}
                set{ this.RaiseAndSetIfChanged(ref _data, value);}
            }   
        }

        // public declaration inside the scope are visible through the class
        // but invisible outside the class.
        private scope {
            private double _data;
            public double Wealth {
                get{ return _data;}
                set{ this.RaiseAndSetIfChanged(ref _data, value);}
            }   
        }
}

It would also be nice to be able to factor out and reuse scopes. For example declare
a scope at the same level as a class with a place holder for an injected name for
public methods.

public scope Property<T> : ReactiveObject {
    private T _data;
    public T {{Name}} {
        get{ return _data;}
        set{ this.RaiseAndSetIfChanged(ref _data, value);}
    }   
}

This declaration of Person is the same as the above declaration of person

public class Person : ReactiveObject
with Property<double> as public Height
with Property<double> as private Wealth
{
}

@togakangaroo
Copy link

I'm not sure that I like this feature, but I want to point out that the current conversation has focused on the target being relatively novice developers, whereas the only real world usage given by the OP was more about refactoring messy code to better patterns.

Maybe novice developers isn't actually the right use case here?

@lachbaer
Copy link
Contributor Author

@togakangaroo Thank you for pointing this out again! "about refactoring messy code to better patterns", exactly. And doing so without the effort of costly hours by whiteboarding complex design patterns. Just a quick, clean and lean refactor to reduce side effects and to prevent from accidentially building up new bugs in other places.

@bradphelan
Doesn't the mix of public and private become a bit too mixed up and misunderstandable? How about protected, internal and default accessibility?

@bradphelan
Copy link

public and private mean what they mean. How does replacing them with other words make it any more clear. A declaration is either public to a scope or private to a scope. If you chose to use "protected" and to make it consistent then it would mean that perhaps you could do scope inheritance and then protected declarations would become available to inheritors. However I can't really think a use case for scope inheritance. That's a step too far for me.

@lachbaer
Copy link
Contributor Author

lachbaer commented Aug 9, 2016

Maybe it is better to let the analyzer check if a field belongs to a region of code, by using a preprocessor directive:

#varregion
int _counter = 0;
public void Increase() { _counter++; }
public void Decrease() { _counter--; }
#endvarregion

A preceding #pragma could trigger a using of fields declared within that #varregion as a warning or error when used outside. IntelliSense will respect it as well and will not propose the fields outside their region.

@gafter
Copy link
Member

gafter commented Sep 11, 2017

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 #18002. Comments and discussion about this issue can take place here or on an issue in the relevant repo.


I have not moved this feature request to the csharplang repo because I don't believe it would ever rise in priority over other requests to be something we would ever do in any particular release. However, you are welcome to move discussion to the new repo if this still interests you.

@gafter gafter closed this as completed Sep 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants