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: Allow record constructors and properties to have different access modifiers to that of the record itself #8798

Closed
DavidArno opened this issue Feb 17, 2016 · 19 comments

Comments

@DavidArno
Copy link

The original proposal was for some means of specifying a public record, with an internal constructor. My example below is arguably a poor one, as it could be viewed a misuse of records. Further, as per F#, it could be useful to specify the access modifier for properties too. These points are covered in subsequent comments, so I've left my original proposal as is.

Consider the following real-world piece of code:

public sealed class WhereForFuncHandler<TMatcher, T1, TResult>
{
    private readonly Func<T1, bool> _expression;
    private readonly Action<Func<T1, bool>, Func<T1, TResult>> _recorder;
    private readonly TMatcher _matcher;

    internal WhereForFuncHandler(Func<T1, bool> expression,
                                 Action<Func<T1, bool>, Func<T1, TResult>> recorder,
                                 TMatcher matcher)
    {
        _expression = expression;
        _recorder = recorder;
        _matcher = matcher;
    }

    public TMatcher Do(Func<T1, TResult> action)
    {
        _recorder(_expression, action);
        return _matcher;
    }

    public TMatcher Do(TResult value)
    {
        _recorder(_expression, x => value);
        return _matcher;
    }
}

With records, I'd be able to almost replace the above with:

public class WhereForFuncHandler<TMatcher, T1, TResult>(
    Func<T1, bool> expression,
    Action<Func<T1, bool>, Func<T1, TResult>> recorder,
    TMatcher matcher)
{
    public TMatcher Do(Func<T1, TResult> action)
    {
        recorder(expression, action);
        return matcher;
    }

    public TMatcher Do(TResult value)
    {
        recorder(expression, x => value);
        return matcher;
    }
}

There would be one, very important, difference though: the constructor would now be public. It would therefore be useful if the accessibility of the constructor could be specified for a record, if it needs to be different to the record itself. Possibly something like:

public class WhereForFuncHandler<TMatcher, T1, TResult>(
    Func<T1, bool> expression,
    Action<Func<T1, bool>, Func<T1, TResult>> recorder,
    TMatcher matcher)
{
    internal WhereForFuncHandler();  // tells the compiler to generate an internal constructor

    public TMatcher Do(Func<T1, TResult> action)
    {
        recorder(expression, action);
        return matcher;
    }

    public TMatcher Do(TResult value)
    {
        recorder(expression, x => value);
        return matcher;
    }
}

This would allow the boilerplate-avoidance features of records to be utilised, without compromising the ability to specify the accessibility of the constructor.

@bbarry
Copy link

bbarry commented Feb 17, 2016

That seems very easy to miss.

What if instead of allowing this constructor signature to be placed somewhere inside the class, a modifier was permitted between the class name and parameter list type parameter list and record parameters:

public class WhereForFuncHandler<TMatcher, T1, TResult> internal (
    Func<T1, bool> expression,
    Action<Func<T1, bool>, Func<T1, TResult>> recorder,
    TMatcher matcher)
...

that is:

class-declaration
    : attributes?
      class-modifiers?
      partial?
      'class'
      identifier
      type-parameter-list?
      record-parameters?
      class-base?
      type-parameter-constraints-clauses?
      class-body

becomes

class-declaration
    : attributes?
      class-modifiers?
      partial?
      'class'
      identifier
      type-parameter-list?
      attributes?
      constructor-modifiers?
      record-parameters? 
      class-base?
      type-parameter-constraints-clauses?
      class-body

@HaloFour
Copy link

What is the reason for making said type a "record"? Just to have more short-hand for the properties?

I'd argue that a tenet of being a record is more than just the shape, it's also the usage. A record is expected to be publicly creatable. Can you identify a programming language that contains record types where the constructor can have a narrower accessibility than the type itself? F#, for example, doesn't allow this.

@FunctionalFirst
Copy link

@HaloFour Actually, F# does allow this, using syntax similar to what @bbarry suggested: type T = private { P: int }

@HaloFour
Copy link

@FunctionalFirst

That's the accessibility of the record type itself. F# doesn't allow defining a public record that has a constructor with a different accessibility modifier.

@FunctionalFirst
Copy link

@HaloFour Yes, F# does allow it. The accessibility modifier for the type precedes the name:
type public T = private { P: int }

@HaloFour
Copy link

@FunctionalFirst

Hm, you're correct, you can have two accessibility modifiers. However, that second one affects both the constructor and the record properties, so you'd end up with the following:

public sealed class T {
    internal int _P;

    internal T() { }

    internal int P { get; set; }
}

@FunctionalFirst
Copy link

@HaloFour Correct. That allows you to treat the type as a record internally, but expose it differently externally (i.e., through type extensions or module-scoped functions).

@HaloFour
Copy link

@FunctionalFirst

Gotcha.

In this example it doesn't appear that the properties are intended to be exposed, even internally, nor does it appear that the shape of the type would be utilized in pattern matching, so I would still argue that the type is probably not a good candidate for being a record type. This proposal seems to more seek to push primary constructors as an alternate short-hand for defining general purpose types.

@alrz
Copy link
Member

alrz commented Feb 17, 2016

public class WhereForFuncHandler<TMatcher, T1, TResult> internal (
    Func<T1, bool> expression,
    Action<Func<T1, bool>, Func<T1, TResult>> recorder,
    TMatcher matcher)

@HaloFour This syntax suggested by @bbarry is totally reasonable why not? internal constructors would be useful to disallow creating the record beyond that particular access modifier (as you would usually use it for regular types), while you would be able to deconstruct it with pattern matching or pass them around. I presume #8415 would also consider internal constructors for positional deconstruction for regular types.

@HaloFour
Copy link

@alrz

why not?

Primarily because I think that "why not?" is not a sufficient reason to implement any feature. 😄

I don't particularly have any issues with this proposal, I just think that every idea needs to be sufficient beat up in order to demonstrate why it's useful enough to be worth the effort in implementing it and support it.

As it stands the record proposal explicitly states that the constructor and the properties are to be public, so the visibility of the record class is limited only by the accessibility modifier of the type itself. The fields in the example are never intended to be surfaced, but making the type a record forces this issue and will create properties. The compiler is then forced to make a decision: either the properties are made public, and those fields which were never intended to be exposed are now public properties on a public class, or the properties take the same accessibility as the constructor, and you have a record which is both not publicly creatable or publicly usable as a record. Or you open the can of worms that is adding accessibility modifiers to every single constructor parameter.

The one thing I'd really like to not see is the big discussion to make primary constructors become a full-fledged alternate class definition syntax, which is what happened on CodePlex.

I'd like to see an example case that better fits the proposal, one where the type is actually intended to be used as a record and those fields have a reason to be exposed as properties, even internally.

@alrz
Copy link
Member

alrz commented Feb 17, 2016

@DavidArno The title should be "allow access modifiers for record constructors" it doesn't make sense to restrict it to only public and internal constructors (and I think it's orthogonal to the access modifier of the record type itself).

class Foo private (Bar bar : Bar, Baz baz : Baz) {
  public static Foo Create() { ... }
}

So public will be the default.

@alrz
Copy link
Member

alrz commented Feb 17, 2016

@HaloFour See the example in my previous comment.

The one thing I'd really like to not see is the big discussion to make primary constructors become a full-fledged alternate class definition syntax.

I wasn't talking about properties' access modifiers, or immutability, they will be public and immutable by default. This proposal is about the record's primary constructor access modifier, period.

@DavidArno
Copy link
Author

@HaloFour,

You have a good point in that my example code could very well be an abuse of record types. That's not something I'd considered. As you point out, a record would have side effects, such as exposing the fields via properties.

@bbarry hits upon a really nice syntax that I hadn't considered that could have other benefits. If I have a record type that can be exposed via an API, it needs to be public. However, I might want to treat it as a token: external code can accept a copy, pass it around and ultimately pass it back. I therefore might want the properties to be internal. So I may have accidentally proposed a solution to a need quite different to my own.

@HaloFour
Copy link

@alrz

I meant a real world example, but I do see the utility of a factory method for creating records. However, I think that the following behavior may not be particularly intuitive:

var foo = new Foo(bar, baz); // compiler error
if (foo is Foo(bar, baz)) { ... } // ok

@alrz
Copy link
Member

alrz commented Feb 17, 2016

@HaloFour That's currently the case,

var foo = new Foo(bar, baz); // error
if (foo is Foo) { ... } // ok

Extended is operator with pattern matching doesn't have much to do with ctor accessibility, yes, it does use it for positional deconstruction, but in a pattern you don't create the type, you are just matching its type and members (actually it's just a pretty useful syntactic sugar).

@DavidArno DavidArno changed the title Proposal: Allow public records to have internal constructors Proposal: Allow record constructors and properties to different have different access modifiers to that of the record itself Feb 17, 2016
@DavidArno
Copy link
Author

@alrz,

I've updated the title as suggested. I'm still a little unsure whether I should close this proposal though as my initial use case doesn't really fit the new title so well.

@alrz
Copy link
Member

alrz commented Feb 17, 2016

@DavidArno I'm not native but it doesn't sound grammatically correct 😄 anyway, I wasn't talking about properties' access modifiers, that complicates a lot of other things like with etc. Just for constructors as you originally proposed.

@DavidArno
Copy link
Author

It isn't grammatically correct! I tried to edit it on my phone and messed it up! I'll re-edit it when I'm at a computer :)

@DavidArno DavidArno changed the title Proposal: Allow record constructors and properties to different have different access modifiers to that of the record itself Proposal: Allow record constructors and properties to have different access modifiers to that of the record itself Feb 17, 2016
@gafter
Copy link
Member

gafter commented Apr 21, 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 am not moving this particular issue because the current draft proposal for records already supports this:

Primary constructor body
primary_constructor_body
    : attributes? constructor_modifiers? identifier block
    ;

A primary_constructor_body may only be used within a record type declaration. The identifier of a primary_constructor_body shall name the record type in which it is declared.

The primary_constructor_body does not declare a member on its own, but is a way for the programmer to provide attributes for, and specify the access of, a record type's primary constructor. It also enables the programmer to provide additional code that will be executed when an instance of the record type is constructed.

@gafter gafter closed this as completed Apr 21, 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

7 participants