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

Champion: readonly object initializers #1684

Closed
1 of 5 tasks
jaredpar opened this issue Jul 2, 2018 · 53 comments
Closed
1 of 5 tasks

Champion: readonly object initializers #1684

jaredpar opened this issue Jul 2, 2018 · 53 comments
Assignees
Milestone

Comments

@jaredpar
Copy link
Member

jaredpar commented Jul 2, 2018

This proposal will extend object initializers to allow assignment of readonly fields and get only auto-implemented properties.

  • Proposal added
  • Discussed in LDM
  • Decision in LDM
  • Finalized (done, rejected, inactive)
  • Spec'ed

Proposal link #1683

LDM history:

@jaredpar jaredpar self-assigned this Jul 2, 2018
@Joe4evr
Copy link
Contributor

Joe4evr commented Jul 2, 2018

Related to #348.
Now that I've read the proposal, I guess it isn't.

@YairHalberstadt
Copy link
Contributor

If you're assigning a read-only field in an object- initializer, isn't that a code smell that either it shouldn't be read-only, or it should be initialised in the constructor.

If it's set- once rather than read-only, then surely best to make a method TrySet or use the constructor to make this explicit.

@DavidArno
Copy link

DavidArno commented Jul 2, 2018

@YairHalberstadt,

The general idea is that, when constructing an object, I'd like just one consistent way of setting everything, whether read-only or not. I don't want to access positional constructor parameters and name-based properties is two different syntaxes:

var x = new SomeType(1, true, "bar")
{
    Prop1 = "foo",
    Prop2 = 42
};

Instead, I want to do it all using initializer syntax:

var x = new SomeType
{
    Prop1 = "foo",
    Prop2 = 42,
    ReadOnlyProp3 = 1, 
    ReadOnlyProp4 = true, 
    ReadOnlyProp5 = "bar"
};

The fact that the compiler than lowers this to the first version of the code to satisfy the CLR is nicely hidden away from me. I get to express my intent in code and the compiler works out the practicalities of the implementation for me.

And this then leads neatly into records being able to work this way too:

readonly struct Point(double x, double y);

var p1 = new Point { x = 0.0, y = 1.0 };
var (x1, y1) = (p1.X, p2.Y);
var p2 = new Point (2.0, 3.0);
var (x2, y2) = p2;

and again, I don't need to worry about how all this gets implemented by the compiler; it's just there and works for me, whether I want to use the named approach or position construction/deconstruction).

@YairHalberstadt
Copy link
Contributor

YairHalberstadt commented Jul 2, 2018

@DavidArno. You are suggesting that the compiler should create a constructor that takes the read-only properties, and replaces an object-initializer with that constructor. What happens if auther of the class then adds a constructor which sets the read-only property. Should that break the calling code? This sounds like a right can of worms.
Read-only properties should only be set by the class.

@AndreasHeisel
Copy link

If I understand the proposal right, then it breaks object encapsulation.

This is valid C#:

public abstract class AccessorBase
{
  protected AccessorBase(bool canRead, bool canWrite)
  {
    CanRead = canRead;
    CanWrite = canWrite;
    BufferSize = 2048;
  }

  public bool CanRead { get; }

  public bool CanRead { get; }

  public int BufferSize { get; }

   // ...
}

public ReadOnlyAccessor : AccessorBase
{
   public ReadOnlyAccessor() : base(true, false) {}

  // ...
}

It guaranties that no one can modify CanRead and CanWrite from outside of the object.

The proposal would enable this:

var accessor = new ReadOnlyAccessor() { CanWrite = true };

In my opinion this is a no go. It breaks existing code. I fully understand the intent to enable a generalized object initialization pattern, but I think it must be explicitly enabled on a per property base.

Maybe an attribute

[Initializable]
public int BufferSize { get; }

Or a (contextual) keyword

public int BufferSize { get; init; }

@Richiban
Copy link

Richiban commented Jul 2, 2018

A feature like this one would be incredibly useful for records and other types that shouldn't really have constructors (they currently do because there's currently no other way in C# to pass data through from the caller to the fields of the class).

@DavidArno
Copy link

DavidArno commented Jul 2, 2018

@AndreasHeisel,

... It guaranties that no one can modify CanRead and CanWrite from outside of the object ... The proposal would enable this ...

That seems extremely unlikely. If you can't create an instance of a class due to it only having a protected or private constructor, then it would make no sense whatsoever for this purely syntactic sugar proposal to provide a hacky way of creating such an object instance.

... but I think it must be explicitly enabled on a per property base ...

Why overcomplicate this? just have the solution respect the visibility of constructors and property setters when determining if an initialisation expression is valid or not. Keep it simple.

@Richiban
Copy link

Richiban commented Jul 2, 2018

To further my point, when it comes to records I much prefer this syntax:

var p = new Point { X = 4, Y = 5 };

to this:

var p = new Point(x: 4, y: 5);

It feels to me that it's much more intuitive as to what's going on: "I am creating a Point object and giving the value 4 to the X property" etc. as opposed to the second example where it's read as "I am calling a constructor on the Point class and passing in these values. Boy, I hope the x parameter on the constructor sets the value of the X property on the object I get back!".

It would also avoid the naming convention conflict that we have between constructor arguments and property names, where we currently don't know whether to write:

class Point(int x, int y);

// or

class Point(int X, int Y);

// or 

class Point (int x : X, int y : Y);

I do agree with @AndreasHeisel though, in that types supporting this should be marked in some way, since it does feel that they drop some of the protections that Object Orientation give you.

@AndreasHeisel
Copy link

@DavidArno

In the sample, I create an instance of a public class with a public constructor.

@DavidArno
Copy link

@AndreasHeisel,

And that constructor takes no parameters, therefore could not be used to set CanWrite outside of the declaring assembly. For your expression,

var accessor = new ReadOnlyAccessor() { CanWrite = true };

to work, you'd need ReadOnlyAccessor to be declared like:

public ReadOnlyAccessor : AccessorBase
{
   public ReadOnlyAccessor(bool canWrite) : base(true, canWrite) {}

  // ...
}

which rather defeats the object of a readonly accessor...

@AndreasHeisel
Copy link

The more I think about it, the more I like the init accessor. It has several goodies:

It is explicit, not breaking anything.

It naturally extends to non auto generated properties:

private int bufferSize;

public int BufferSize 
{
  get => bufferSize;

  init
  {
    // some logic
    bufferSize = value;
  }
}

with the guaranty that ist can be set only once.

@AndreasHeisel
Copy link

@DavidArno

I missesd that the proposal is limited to structs, wich renders my sample invalid.

Besides that, I don't understand

outside of the declaring assembly

The proposal states

+- The accessibility of the backing field will match the accessibility of the property.

And everything was public in my sample.

I still think the explicit route is the better one. No one knows wich rules a constructor applies to its parameters before it feeds the properties.

My concern ist not that I dislike the idea of initializers, my concern is that it lowers guaranties that the current language version provides.

The init accesor could solve this as it doesn't change existing code and provides a way to have custom initialization logic outside of the constructor.

@DavidArno
Copy link

@YairHalberstadt,

I'm sorry but I genuinely do not follow what you are saying. Could you provide a code sample that explains it, please?

@DavidArno
Copy link

DavidArno commented Jul 2, 2018

@AndreasHeisel,

The proposal states

+- The accessibility of the backing field will match the accessibility of the property.

Hmm, I missed that. In that case my apologies: this proposal is a pile of crud if they really intend that. 😄

The implied accessibility due to the accessibility of constructors must be taken into account for this to be a useful feature, in my view.

@Joe4evr
Copy link
Contributor

Joe4evr commented Jul 2, 2018

@AndreasHeisel I do understand your concern, but the proposal is limited to only structs and ref structs. There's even an entire paragraph about why it can't be enabled for reference types even without inheritance as a factor.

@AndreasHeisel
Copy link

@Joe4evr structs can also be part of a public API.

@HaloFour
Copy link
Contributor

HaloFour commented Jul 2, 2018

Oh, this time we have an issue and a PR?

Anyway, I don't think I need to reiterate my problems with the principal of this idea, so I'll just link the start of my comments there for any reader who cares to know my opinion.

#1667 (comment)

@Joe4evr
Copy link
Contributor

Joe4evr commented Jul 2, 2018

@AndreasHeisel Yes, but your initial argument was about an abstract base class and inheritance, which simply don't apply to structs.

Also, noted in the discussion is:

No. The fields are still marked as readonly hence they are blocked [from assignment after construction] by default.

Now, I'm not a fan of this "feature" either way, purely for the change to the visibility of generated backing fields. Either this has to only apply to fields that the developer has made non-private, or it needs to be opt-in on the developer's side to allow the visibility change of unspeakable fields (which in turn is what allows the compiler to perform the assignment).

@AndreasHeisel
Copy link

@Joe4evr

I used inheritance only to get ReadOnlyAccessor and CanWrite=true on one line to form a "funny" example.

My concern is about the breaking change, not on inheritance. I missed the limitation to structs, so it was a bad sample.

@bbarry
Copy link
Contributor

bbarry commented Jul 2, 2018

Have you considered an implementation like this:

source:

readonly struct Name {
    public string FirstName { get; }
    public string LastName { get; }
}

Name Foo() {
    return new Name { FirstName = "Frank", LastName = "Smith" };
}

emits

readonly struct Name {
    public string FirstName { get; }
    public string LastName { get; }

    public struct Builder {
        public Builder(in Name name) {
            this = Unsafe.As<Name, Builder>(ref Unsafe.AsRef(name));
        }

        public string FirstName { get; set; }
        public string LastName { get; set; }

        public Name Build() => Unsafe.As<Builder, Name>(ref this);
    }
}

Name Foo() {
    var temp1 = new Name();
    var temp2 = new Name.Builder(temp1);
    temp2.FirstName = "Frank";
    temp2.LastName = "Smith";
    return temp2.Build();
}

As opposed to exposing the backing fields.

edit: refined implementation...

I didn't want to modify the constructors on Name. As written this pattern enables any constructor on Name.

I think if the struct is partial or is not readonly, this builder pattern shouldn't be produced. Only members that are automatic properties or fields should be available in the builder.

@HaloFour
Copy link
Contributor

HaloFour commented Jul 2, 2018

@bbarry

I agree, although I think that the Builder shouldn't be "unspeakable". That way other languages can interop with the feature directly rather than needing special compiler support. See #1667 (comment).

@bbarry
Copy link
Contributor

bbarry commented Jul 2, 2018

I would like that it wasn't mangled either but I think not doing so is potentially conflicting with existing code.

edit: perhaps exposing a nested Builder if it is not ambiguous, otherwise the type would not be permitted to be used this way?

@HaloFour
Copy link
Contributor

HaloFour commented Jul 2, 2018

@bbarry

I would like that it wasn't mangled either but I think not doing so is potentially conflicting with existing code.

Potentially, but at worst that would mean that the compiler would consider allowing the language feature in incorrect circumstances. The compiler could limit the feature to only allowing initialization syntax when the nested struct and parent type fit a particular shape and mitigate most of those situations. That's not unlike a lot of features like tuples, deconstructor, collection initializers, etc., which aim to enable a language feature on top of existing syntax.

@HaloFour
Copy link
Contributor

HaloFour commented Jul 2, 2018

@bbarry

Also, if this feature relied on something that could be expressed with existing code not only does it make it safer and accessible to other languages that might not support it, but it also makes it possible to extend existing types without having to migrate to a new form of type that may come with additional opinions/baggage.

I think that the following rules should suffice:

  1. The type has a nested struct with a specific name, e.g. Data, Builder, Initializer, Frob or whatever is decided upon.
  2. The type has a constructor which accepts the nested struct, possibly by ref.
  3. The nested struct has accessible and writable fields or properties with names matching (readonly?) fields/properties on the type

e.g.

public class Point {
    public struct Builder {
        public int X;
        public int Y;
    }

    public Point(ref Builder builder) {
        this.X = builder.X;
        this.Y = builder.Y;
    }

    public int X { get; }
    public int Y { get; }
}

//

var point = new Point { X = 2, Y = 3 };
// translates to
var $temp1 = new Point.Builder { X = 2, Y = 3 };
var point = new Point(ref $temp1);

@DavidArno
Copy link

OK, having read @HaloFour's and other comments on the PR and read the PR proposal properly I've switched firmly to downvoting this. What's being proposed around "pretend" read only fields exposed via "unspeakable" public properties is ugly and hacky.

  1. I see no reason for restricting this feature to structs,
  2. It should be implemented either via matching "properties" to constructor parameters in the initializer (as I assumed the feature was going to do) or via an auto-generated builder as @bbarry suggests.

But as it stands, this gets a 👎 from me.

@bbarry
Copy link
Contributor

bbarry commented Jul 2, 2018

@DavidArno I dislike the idea of it being possible on class based types because inheritance adds an ugly mess of wrinkles around expectations of readonly properties and observable strangeness in references. Most of those problems would be solved by making the feature opt-in somehow (which is basically how I read the data type proposal), causing it to not become available to existing types merely via a rebuild.

I think it is worth considering this proposal as covering 2 distinct features which may overlap with other proposals in the future:

  1. consumption of a builder pattern
  2. producing a builder pattern for struct types

With the abstraction injected here to separate consumption from production we can consider both other potential producers of the pattern (data classes? / explicit implementations?) and consumers of the pattern (With-ers). I wonder if making the builder type an open type could solve the decapitation issue (and if that is good)?

As for the rules @HaloFour I would suggest:

for enabling the pattern on a type (in this proposal; other proposals may do other things to light up):

  • The type must be a struct or ref struct.
  • The type does not declare a Builder type (aka "nested type with a specific name")

for consumption:

  • The type must have an accessible nested type named Builder
  • The nested type must have constructor with a ref or in parameter accepting the type
  • The nested type must have a method member Build() with no parameters returning the type

The implementation above does not have the requirement of the type not having a constructor with a ref or out parameter because it never mutates the original type (it mutates a copy). The build method on the nested type gets around the possibility of constructor side effects by not running the constructor a second time.

I would rather require a named method on the nested type over a constructor because I wouldn't want to invoke a constructor on the type twice and do want to invoke the constructor before assigning read only properties.

The major issue with this particular formalism is that it is a breaking change to existing types that happen to have a nested Builder type with the necessary constructor and method but differently named properties and fields. That could be fixed by an unspeakable name or a modreq or a new attribute. I'd prefer a modreq new attribute to solve that because I like the idea of being able to control the implementation of the builder pattern.


More formally, a struct or ref struct without a nested type named Builder would have a builder pattern generated when compiled. The process for producing the builder pattern is as follows:

  1. a new nested type public struct Builder is added to the type
  2. a constructor to this nested type is added that accepts the type as an in parameter if the type is readonly and a ref parameter otherwise.
  3. the method body of this constructor is this = Unsafe.As<{type}, Builder>(ref Unsafe.AsRef(arg)); if the parameter was in and this = Unsafe.As<{type}, Builder>(ref arg); otherwise
  4. members of the type are iterated in compilation order
    4.1. if member is a field, a field matching the name and accessibility is added to the nested type
    4.2. if member is an automatic property, an automatic property with both get and set matching the name and accessibility of the get on the type is added
    4.3. otherwise the member is skipped
  5. A method named public {type} Build() is added with the body return Unsafe.As<Builder, {type}>(ref this);

The builder builder pattern is identified on types with a nested Builder type that has a constructor with a ref or in parameter matching the type and a Build() method that has no parameters and returns the type.

If an object_creation_expression is parsed and the type is implementing the builder pattern, the type T is created by calling the constructor necessary for passing the argument_list and object_or_collection_initializer is processed as the T.Builder nested type. Following processing of the object_or_collection_initializer ASN, the type is converted back to T by calling the instance Build() method.

@HaloFour
Copy link
Contributor

HaloFour commented Jul 2, 2018

@bbarry

I would rather require a named method on the nested type over a constructor because I wouldn't want to invoke a constructor on the type twice and do want to invoke the constructor before assigning read only properties.

Works for me. I'm less concerned about the actual details for the builder type than I am about using one at all over some magic writable readonly field approach.

The major issue with this particular formalism is that it is a breaking change to existing types that happen to have a nested Builder type with the necessary constructor and method but differently named properties and fields. That could be fixed by an unspeakable name or a modreq. I'd prefer a modreq to solve that because I like the idea of being able to control the implementation of the builder pattern.

Both approaches unnecessarily gate this feature to the C# language and I disagree with them on principal. Half of the point of going with a "builder" is that down-level languages can consume them just as normal types with normal APIs without special magic.

If there is concern about overlap with a convention and existing APIs then I think that can easily be solved by adding an attribute to the mix which could adorn the builder type or the type being built. Then there'd be no concern at all. But, as with Deconstruct, I think that the likelihood that an existing "builder" type would accidentally fall into the pattern would be exceptionally low, and even if it did there is virtually no harm if it did.

@bbarry
Copy link
Contributor

bbarry commented Jul 2, 2018

A new well known attribute would be sufficient. I'd be happy with the breaking change but I understand the reluctance and would rather be careful to point it out for consideration.

I think the builder pattern is preferable over exposing fields and "property/field to constructor parameter binding" because the abstraction layer minimizes the necessary changes to https://github.com/dotnet/csharplang/blob/7f39331672cf8edbda8867de004138e0f711c877/spec/expressions.md#object-creation-expressions and any actual implementation complexities are secondary to complexities in the language specification.

@jaredpar
Copy link
Member Author

jaredpar commented Jul 3, 2018

@HaloFour

F# emits a metric ton of magic metadata that is expected to be understood only by the F# compiler.

C# emits an equal amount of magic metadata that is understood only by the C# compiler. Or at least initially only understood by the C# compiler and picked up by other languages on a case by case basis. Pretty much every feature we created in 7.2 and beyond involved magic metadata, modreq, etc ... in order to accomplish our goals.

C# 7.X is not the only place where this was done. It's happened in virtually every release of the language to facilitate the type system, debugger, ENC, etc ...

I don't think that the comparison with F# is appropriate.

I think it's extremely appropriate because it's not just F#, or C#. Virtually every managed language emits some amount of metadata to layer their type system on top of the primitives exposed by IL. Yes the majority of the language has a 1:1 mapping with IL but there are always cases that simply can't be expressed in pure IL and hence metadata is layered on top of it. F# isn't the exception here, it's the norm.

Another example of this pattern: C++/CLI. Their metadata layering is as impressive as F# is.

@HaloFour
Copy link
Contributor

HaloFour commented Jul 3, 2018

@jaredpar

Pretty much every feature we created in 7.2 and beyond involved magic metadata, modreq, etc ... in order to accomplish our goals.

With a single exception (that I'm aware of) every one of those features is encoded with normal IL constructs that can be used from any other language. This is a virtue.

C# 7.X is not the only place where this was done. It's happened in virtually every release of the language to facilitate the type system, debugger, ENC, etc ...

Hidden implementation details that don't effect how the types interact between different managed languages.

F# isn't the exception here, it's the norm.

This isn't a binary proposition. F# does this to an extreme degree, to the point of encoding a boatload of metadata in a magic binary resources. C++/CLI probably also does this to some extreme degree. C# does this to a very limited degree and I believe in is the only place where the language has gated a feature behind modreq. The language has yet to encode anything unspeakable publicly. This is a virtue.

We're going to have to agree to disagree because there's nothing you can say that would convince me that C# should start emitting illegal programs (and to change the definition of what constitutes a legal program) for the sake of coming up with a short-hand syntax for some of the most common programming tasks that any developer is already doing today. There are idiomatic approaches to solving this problem which requires zero hackery and zero modifications to the runtime.

@DavidArno
Copy link

@jaredpar,

Thanks for the constructive feedback. Always appreciated!

Ouch. Sorry! I'll try to find a more constructive way of expressing why I think this a bad idea.

@jaredpar
Copy link
Member Author

jaredpar commented Jul 3, 2018

@HaloFour

With a single exception (that I'm aware of) every one of those features is encoded with normal IL constructs that can be used from any other language. This is a virtue

I'm not sure what you classify as "normal" IL. But at the same time C#, F# and C++/CLI are all pretty much equally guilty in how they encode their non-IL standard type annotations. F# appears different at a glance because they're quite regular about how they encode it while C# has taken a variety of approaches for their features (typically need based).

C# does this to a very limited degree and I believe in is the only place where the language has gated a feature behind modreq.

The unmanaged constraint is also gated behind a modreq and did plenty of emit tricks for ref struct.

We're going to have to agree to disagree because there's nothing you can say that would convince me that C# should start emitting illegal programs

Going to have to define illegal here. Assuming for a sec that you mean verifiable.

C# already emits plenty of programs that do not verify out of the gate. Pretty much all of that is related to the inability of the verification system to distinguish between mutable and readonly memory properly (and the fact that peverify just hasn't kept up with the framework in general). Our goal is to adjust the verification rules and implementation to reflect the new standard all languages agree on. In terms of verification this is very much in line with other changes we made around readonly struct.

@HaloFour
Copy link
Contributor

HaloFour commented Jul 3, 2018

@jaredpar

I'm not sure what you classify as "normal" IL.

Something that any other language can be expected to consume by simply following the conventions of the CLS and the CLR without requiring any additional knowledge as to the language that happened to emit that IL.

But at the same time C#, F# and C++/CLI are all pretty much equally guilty in how they encode their non-IL standard type annotations.

There is a big difference between "all the time" and "in a small handful of cases".

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Jul 3, 2018

@jaredpar Before me, i see two proposals. One seems to have a bunch of negatives associated with it. For example, needing to change verifier rules, emit unspeakable names for public surface area, and generally not be usable by any language until they bake in knowledge of this approach. The other seems to sidestep all those issues quite cleanly. It works with today's verifier. It can be consumed or emitted by any language trivially. it doesn't depend on anything unspeakable.

It's hard for me to tell why i would want to support the former proposal. Basically, because of all the negatives and baggage it comes with, i woudl want it to be substantially better in some very important way vs the alternative. But I'm not seeing how it substantively better. If i had to decide on where to spend the resources, hte latter proposal seems to be a far better place. It gets you the same language feature at the end of the day (i.e. readonly object initializers) but with an implementation that seems far better and far healthier for the ecosystem as a whole.

Can you help give context as to why we would want to go with the former when a seemingly, quite good, alternative exists? Thanks!

@jaredpar
Copy link
Member Author

jaredpar commented Jul 3, 2018

@CyrusNajmabadi

One seems to have a bunch of negatives associated with it. For example, needing to change verifier rules, emit unspeakable names, and generally not be usable by any language until they bake in knowledge of this approach.

  1. The verifier has to change independent of this proposal. This is just another facete of readonly memory differences that needs to be addressed.
  2. Unspeakable names are already emitted today. That's not a change and the language uses them. It simply changes where they are used.
  3. Other languages typically lag behind in adoption of new features in another language. This was true for Span<T>, unmanaged constraint, etc ... I'm not worried about other languages lagging behind.

Basically, because of all the negatives and baggage it comes with, i woudl want it to be substantially better in some very important way vs the alternative

I simply don't see these as negatives. There is nothing substantially new in this proposal. It's taking existing concepts that are in use today and applying them to a new feature.

The builder approach to me is hitting a fly with a giant hammer. The core issue here is at what point do readonly fields on a struct become readonly. This is already a bit of a murky concept and it's pretty easy to observe them mutating today with simple ref tricks. Hence I see no reason to prevent us from mutating readonly fields when it's less observable than the ref tricks.

I understand the opposition to the auto-properties part of the proposal in terms of it being a breaking change to the language. That's valid point and something to consider. I don't agree with the opposition to how such a thing would be achieved. The implementation design is not a new trick. It's using established methods for implementing a feature. The only part which is substantively new there is the accessibility of the field (has some precedence in F# but not a perfect comparison).

@CyrusNajmabadi
Copy link
Member

Unspeakable names are already emitted today. That's not a change and the language uses them. It simply changes where they are used.

I have updated my critique. I am referring to unspeakable names being used for public surface area. C# rarely does that. And I'm going to strongly prefer a proposal that does not take us down that line.

@bbarry
Copy link
Contributor

bbarry commented Jul 3, 2018

Why would the verifier have to change? The builder pattern emits verifiable code.

@CyrusNajmabadi
Copy link
Member

I simply don't see these as negatives.

It's a negative to me that i would not be able to use one of these types from a prior version of C#, or from a langauge that isn't updated to understand this pattern.

There is nothing substantially new in this proposal. It's taking existing concepts that are in use today and applying them to a new feature.

We have almost no prior cases of public surface area generating unspeakable names. It's extremely rare, and very localized.

And, again, this is not a critique in isolation. If there was no other way to do this, i would be ok wiht htis. But I would far rather go with a solution that feels much more natural and usable by the entire .net ecosystem.

@vcsjones
Copy link
Member

vcsjones commented Jul 3, 2018

The builder approach to me is hitting a fly with a giant hammer.

Agreed.

Though, I agree with the reasons for not wanting this feature as-designed as well. It's... surprising behavior. Many years of working with C# and existing literature on the subject more or less boil down to "readonly fields may not be assigned after the object has been constructed", approximately.

I am in favor of simply doing nothing.

@CyrusNajmabadi
Copy link
Member

The builder approach to me is hitting a fly with a giant hammer.

Really? It seems exceptionally tiny. And it's already a pattern well-hewn in the ecosystem. When dealing with immutable data, i commonly see people use builders as it enables easy generation of the data, and then a clear step that transfers/freezes/builds the final immutable form.

From a language perspective, it also seems super tiny. Just as we describe mutable-property initialization as a translation of:

new Type
{
    X = //...
    //...
}

// into

var __t = new Type();
__t.X = // ...
__t //...

Now we would describe immutable-property initialization as a translation of:

new Type
{
    X = //...
    //...
}

// into

new Type.Builder 
{
    X = //...
    //...
}.Build();

(or something close to that).

It seems exceptionally tiny, and trivially composes over the rest of the existing language. It also just makes a 'pattern' that the rest of the ecosystem can consume and generate trivially without actually needing any changes to their tooling.

@tannergooding
Copy link
Member

readonly struct already exists today and enforces that all members in the struct are declared readonly.

@kkm000
Copy link

kkm000 commented Nov 2, 2018

@MadsTorgersen added this to the Likely Never milestone on 19 Sep

R. I. P. useful readonly struct. :(

(@kkm000 reads new features in C#7.2. Looks excited.)
I: “Oh, look, readonly struct. Wow!” The in for readonly structs.WOW!!! Yesss!!! So I can convert this 7-member class FooRequest, representing operation's input arguments, to a readonly struct FooRequest and pass it by const reference. Readable, more correct (as it's really readonly), and efficient! I can finally have all three!”

C#7.2: “Ah, sure thing, glad to be of help! But first define a 7-argument constructor and assign every field in this constructor in 7 explicit separate assignments. And please double-check you do not make a stupid typo, like assigning a to b and b to a. And then, tell the callers not to mess the order of arguments at the call site. It's not hard to do. I do that all the time, so you can, too, and they can.”

I: “FUUUUUUUUUUUU. That's disgustingly ugly. And, by the way, this is why I pass 7 arguments in a class rather than directly to the method in the first place. I'll keep them readwrite classes, initialize with the object initializer and pass by reference, as I always did. I am writing programs to be read by my fellows, not by computers, so if my code cannot be efficient, correct and readable, I'll make it readable. Thanks, but no, thanks. And by the way, I am not a compiler to emit this jumble of code that only hides the intent. May I remind you that it's you who were supposed to be the compiler and generate code for me?”

(Enter C#8, cheerful.)
C#8: “Nah.”

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Nov 2, 2018

C#7.2: “Ah, sure thing, glad to be of help! But first define a 7-argument constructor and assign every field in this constructor in 7 explicit separate assignments.

Seems pretty easy:

image

And then, tell the callers not to mess the order of arguments at the call site. It's not hard to do.

True, it's definitely not hard to do. It's how probably 95%+ of all objects are created :)

I am writing programs to be read by my fellows, not by computers, so if my code cannot be efficient, correct and readable, I'll make it readable.

Nothing is really unreadable about object initializers :)

And by the way, I am not a compiler to emit this jumble of code that only hides the intent.

How is it any less clear to do new State(localSymbol: Whatever) vs. new State { LocalSymbol = Whatever }? What intent is hidden here?

@kkm000
Copy link

kkm000 commented Nov 4, 2018

@CyrusNajmabadi, I believe you are looking at the problem from a different angle, and it's really hard to play checkers when your pieces are on black squares and mine are on white. I think the essence of what I am talking about is reflected in your earlier comment:

Now we would describe immutable-property initialization as a translation of:

new Type
{
    X = //...
}
// into
new Type.Builder 
{
    X = //...
}.Build();

So we heap-allocate an object that returns a heap-allocated boxed readonly struct. This is exactly not why the readonly structs were added to the language, if I understand the driving force behind it. I am not thinking about patterns, gang-of-how-many-chaps-there-are, builders, shmuilders or all this buzz. Heck, I survived the Goto Considered Harmful debate, or rather sidestepped it--my quantum chemistry simulations were all FORTRAN at the time. I'm just concerned with getting job done, cleanly and efficiently. Nobody requires me to use the readonly structs after all. The computation stuff that is tight on memory and hogs all CPUs and GPUs it can get its avaricious hands on we do in C++ anyway; for logically complex type-driven code there is F#, which does not stand in the way of using types. And C# is good for gluing stuff together, when you need neither complexity nor efficiency, and enjoys a great support from VS designers. Kind of like VB did before .NET. Every language has its niche use. It's really an amazing development that we have them all, easy to interop. I am glad that at the least some of Gabriel's predictions that he was talking about in 1986, IIRC, did not come to fruition--that fruit would be quite unsavory.

True, it's definitely not hard to do. It's how probably 95%+ of all objects are created :)

I am not arguing with statistics (in fact, I am not even arguing, I'm grumbling). Since this is the only way the language allows readonly field initialization, you can safely make that 100%. It's not that everybody is jumping with joy writing this redundant error-prone code. We just have no choice. Another good example of skewed statistics is the fact that nearly 100% of people who ever ate apples are dead.

This is why I do more F# than C# these days. C# has been steadily adopting cool F# features, and this is an to me, as a staunch foe of mutability in computation, looks an excellent move forward. The problem is, it's getting about half of mostly each, are the half-baked features are about as useful as no features. This particular one is the F# record types, half adopted. Glad we have local function closures though, I do not want to sound as if it were not all as bad as this one. But the worse could indeed be better...

I'll be frank, I see no point continuing the discussion. The proposal is as dead as the dodo.

@CyrusNajmabadi
Copy link
Member

So we heap-allocate an object that returns a heap-allocated boxed readonly struct.

AFAICT, every example showing builders showed them as structs. My post was meant to be understood withing that context. So i feel you have very greatly misinterpreted the proposals being mentioned and hte posts around them.

@CyrusNajmabadi
Copy link
Member

I'm just concerned with getting job done, cleanly and efficiently

this seems to be quite possible. See the post above that you were responding to. The code is still as clean and efficient if you use the constructor form versus the Object-Initializer form. I even gave an example of that and asked you specifically what the cleanliness/efficiency difference is between:

How is it any less clear to do new State(localSymbol: Whatever) vs. new State { LocalSymbol = Whatever }? What intent is hidden here?

You keep stating that things are less clean, but you are not willing to engage on an actual explanation of why you believe that is.

@kkm000
Copy link

kkm000 commented Nov 5, 2018

@CyrusNajmabadi

AFAICT, every example showing builders showed them as structs. My post was meant to be understood withing that context.

I am sorry, I must have misunderstood you entirely. So .Build() would essentially ref-return a stack-allocated readonly struct it would build, while being a zero-length struct itself. That could work, if the builder would avoid the requirement to initialize all members in a readonly struct in its constructor.

How is it any less clear to do new State(localSymbol: Whatever) vs. new State { LocalSymbol = Whatever }

A mandatory human-generated trivial multiple-argument constructor that may possibly have one and only one form for the case of readonly struct that can be passed by in reference, versus no such restriction on the existing struct.

you are not willing to engage on an actual explanation of why you believe that is.

My unwillingness to discuss the proposal has to do solely with the fact that it has been dropped. I cannot agree more that this was a very good proposal. Even syntactic ambiguity (which exists, for example, in the similar C++ initializer) could have been avoided, as in field assignment expression new T() { Field=expr } opposed to a construction expression new T { Field=expr }.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Nov 5, 2018

A mandatory human-generated trivial multiple-argument constructor that may possibly have one and only one form for the case of readonly struct that can be passed by in reference, versus no such restriction on the existing struct.

I really have no idea what this is trying to say. You've made claims of clarity and how intent is hidden. But i cannot see how it's any less clear to have new State(localSymbol: Whatever). To which users is that unclear? It's entirely idiomatic. It's self documenting. It's even shorter than the property-initializer form (though i don't really think that's even relevant).

Where is the lack of clarity? What intent is hidden?

I am sorry, I must have misunderstood you entirely.

No worries. Perhaps there has been a misunderstanding of much of the rest of hte proposal? As well as how structs/constructors already fit into the language. It seems like you may be commenting based on other misunderstandings and misapprehensions about things work.

Even syntactic ambiguity (which exists, for example, in the similar C++ initializer) could have been avoided

Sorry, it's unclear to what you're referring to. What syntactic ambiguity problem are you trying to address?

@333fred
Copy link
Member

333fred commented Feb 26, 2021

This is covered by init accessors. Closing out.

@333fred 333fred closed this as completed Feb 26, 2021
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