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 "Records" (VS 16.8, .NET 5) #39

Open
1 of 5 tasks
MadsTorgersen opened this issue Feb 9, 2017 · 273 comments
Open
1 of 5 tasks

Champion "Records" (VS 16.8, .NET 5) #39

MadsTorgersen opened this issue Feb 9, 2017 · 273 comments

Comments

@orthoxerox
Copy link

@orthoxerox orthoxerox commented Feb 15, 2017

See #77 regarding with expressions that are more useful than bare With methods.

@gulshan
Copy link

@gulshan gulshan commented Feb 16, 2017

As records are considering positional pattern matching, which is actually a tuple feature, and tuples can have named members, which is actually a record feature, I think there is some overlapping between the two. How about making making seamless translations between struct records and tuples based on position, if types match? Names of the members will be ignored in these translations. Struct records will then just become named tuples I guess. Implementations are already similar.

@gafter
Copy link
Contributor

@gafter gafter commented Feb 16, 2017

@gulshan Tuples are good for places where you might have used a record, but the use is not part of an API and is nothing more than aggregation of a few values. But beyond that there are significant differences between records and tuples.

Record member names are preserved at runtime; tuple member names are not. Records are nominally typed, and tuples are structurally typed. Tuples cannot have additional members added (methods, properties, operators, etc), and its elements are mutable fields, while record elements are properties (readonly by default). Records can be value types or reference types.

@gafter gafter added this to the 7.2 candidate milestone Feb 22, 2017
@gafter gafter added this to the 8.0 candidate milestone Feb 22, 2017
@gafter gafter removed this from the 7.2 candidate milestone Feb 22, 2017
@gulshan
Copy link

@gulshan gulshan commented Feb 22, 2017

Copying my comment on Record from roslyn-

Since almost a year has passed, I want to voice my support for the point mentioned by @MgSam -

I still see auto-generation of Equals, HashCode, is, with as being a completely separable feature from records.
I think this auto-generation functionality should be enabled its own keyword or attribute.

I think the primary constructor should just generate a POCO. class Point(int X, int Y) should just be syntactical sugar for-

class Point
{
    public int X{ get; set; }
    public int Y{ get; set; }
    public Point(int X, int Y)
    {
        this.X = X;
        this.Y = Y;
    }
}

And a separate keyword like data or attribute like [Record] should implement the current immutable, sealed class with auto-generated hashcode and equality functions. The generators may come into play here. Kotlin uses this approach and I found it very helpful. Don't know whether this post even counts, as language design has been moved to another repo.

@gulshan
Copy link

@gulshan gulshan commented Mar 9, 2017

From this recent video by Bertrand Le Roy, it seems records are being defined with a separate keyword and the primary constructor is back with shorter syntax. So far I have understood, the new primary constructor means parameters of primary constructor are also fields of the class-

class Point(int x, int y)
// is same as
class Point
{
    int x { get; }
    int y { get; }

    public Point(int x, int y)
    {
        this.x = x;
        this.y = y;
    }
}

It seems the field access modifier is defult/private and to expose them separate public properties are needed like this-

class Point(int x, int y)
{
    public int X => x;
    public int Y => y;
}

I like the idea and I think there should be more discussions about these ideas here.

@gafter
Copy link
Contributor

@gafter gafter commented Mar 9, 2017

We are hoping to have records defined without a separate keyword. Parameters of the primary constructor become public readonly properties of the class by default. See https://github.com/dotnet/csharplang/blob/master/proposals/records.md#record-struct-example for an example.

@gulshan
Copy link

@gulshan gulshan commented Mar 13, 2017

Object(and collection, index) initializers getting constructor level privilege for initializing fields/properties can enable some interesting scenarios of complex hierarchical object initialization.

@gafter
Copy link
Contributor

@gafter gafter commented Mar 13, 2017

@gulshan Can you please back up that assertion with an example? I don't see how using an object initializer instead of a constructor enables anything.

@miniBill
Copy link
Contributor

@miniBill miniBill commented Mar 21, 2017

I see a little problem with the proposed GetHashCode implementation in the proposal: if one of the properties is null, the object hash code will always be zero. Wouldn't it be better to simply ?? 0 individual hash codes before multiplying and summing?

@jnm2
Copy link
Contributor

@jnm2 jnm2 commented Mar 21, 2017

@miniBill Yes.

@Richiban
Copy link

@Richiban Richiban commented Mar 22, 2017

While I'm very much looking forward to the introduction of Records into the language, I really don't like the chosen syntax: public class Point(int x, int y), primarily because it precludes the possibility of ever re-adding primary constructors into the language:

public class ServiceA(ServiceB serviceB)
{
    public void DoSomething()
    {
        // use field serviceB here...
    }
}

God I miss those... I am so sick of writing dumb constructors! :-P

Isn't

public record Point
{
    int X;
    int Y;
}

A better syntax? It leaves primary constructors open but is still about as syntactically short as is possible.

@gulshan
Copy link

@gulshan gulshan commented Mar 22, 2017

Wouldn't it be better if primary constructors were not exclusive to records? Now, according to this proposal, primary constructors cannot be considered without adding the baggage of extra "record" tidbits. Refactoring a current class to use primary constructors(and thus record) is not a good choice then, as the behavior may change.

@Richiban
Copy link

@Richiban Richiban commented Mar 22, 2017

@orthoxerox I guess so, although the spec doesn't mention record parameters having accessibility modifiers, so I can't write:

public class ServiceA(private ServiceB serviceB)
{
    public void DoSomething()
    {
        // use serviceB here...
    }
}

And anyway, it would be a bit of an abuse of the records feature to accomplish this. My type isn't a record at all, and I don't want structural equality.

I remember when the C# team originally dropped primary constructors they said "We don't need this anymore because we're going to have records!" but I don't understand what this has to do with records... Sure, a record is also a type with a primary constructor, but any type should be able to have a primary constructor, not just records.

For reference, here are other languages which all support both primary constructors and records:

F#:

type Greeter(name : string) =
    member this.SayHello () = printfn "Hello, %s" name

Scala:

class Greeter(name: String) {
    def SayHi() = println("Hello, " + name)
}

Kotlin:

class Greeter(val name: String) {
    fun greet() {
        println("Hello, ${name}");
    }
}

Meanwhile, in C#, we're still writing this:

public class Greeter
{
    private readonly string _name;

    public Greeter(string name)
    {
        _name = name;
    }

    public void Greet()
    {
        Console.WriteLine($"Hello, {_name}");
    }
}
@gafter
Copy link
Contributor

@gafter gafter commented Mar 22, 2017

@Richiban
In the primary constructor feature, you could have only used the primary constructor parameter in field and property initializers; parameters are not captured to a field automatically. You could get what you want using records in essentially the same way you would have done using primary constructors:

public class Greeter(string Name)
{
    private string Name { get; } = Name;
    public void Greet()
    {
        Console.WriteLine($"Hello, {Name}");
    }
}
@lachbaer
Copy link
Contributor

@lachbaer lachbaer commented Mar 22, 2017

Why do you need the with keyword in

p = p with { X = 5 };

Wouldn't it be equally understandable when there were a .{-Operator? It would allow for more brief chaining

var r = p.{ X = 5, Y = 6 }.ToRadialCoordinates();
@Richiban
Copy link

@Richiban Richiban commented Mar 22, 2017

@gafter If we go with the record definition public class Greeter(string Name) doesn't Name get lifted into a public property? That's the main reason I wouldn't want to use it for something that's not strictly a record--I don't necessarily want a type to expose its dependencies. Can I give accessibility modifiers to record fields?

@gafter
Copy link
Contributor

@gafter gafter commented Mar 22, 2017

@Richiban No, if a property is explicitly written into the body by the programmer, as in my example, then the compiler does not produce one. That is described in the specification.

@Richiban
Copy link

@Richiban Richiban commented Apr 4, 2017

By the way, do I understand the spec right that class records must be either sealed or abstract?

There are serious problems with allowing classes to derive types that have defined custom equality: https://richiban.uk/2016/10/26/why-records-must-be-sealed/

@MgSam
Copy link

@MgSam MgSam commented Apr 4, 2017

If records remain the only way to get auto-generation of Equals and HashCode then I think they absolutely should not be sealed. As you yourself state in your post, doing a simple type check in the equals method solves the issue you bring up. Seems pretty Byzantine to wall off an entire use case because of the fact that developers "might" misuse a feature.

Getting structural equality right in C# is already a minefield that most sensible developers let IDE tools generate code for. Compiler autogeneration of the equality methods should be enabled for the widest net of situations possible.

@GeirGrusom
Copy link

@GeirGrusom GeirGrusom commented Oct 22, 2020

You can implement a value type that encloses a single string field and implement IEquatable<T> for that type using the correct string comparer and it should just work.

@Hermholtz
Copy link

@Hermholtz Hermholtz commented Oct 22, 2020

@GeirGrusom that seems like a proper type-first approach to defining the business domain. I'll smuggle the best talk ever about it. I wish C# is able to incorporate as much from this as possible. https://www.slideshare.net/ScottWlaschin/domain-driven-design-with-the-f-type-system-functional-londoners-2014

@lrhoulemarde
Copy link

@lrhoulemarde lrhoulemarde commented Oct 25, 2020

I just noticed this new language feature being released, so I am too late to add my opinion.

I am not sure I like introducing a keyword just for a specialized version of something existing. So I am disappointed to see data class not survive. My initial reaction is confusion on why it is not called readonly class as a mirror of readonly struct, but I understand that record does more and readonly does not convey the extra capabilities.

Yup, likewise, but don't expect much sympathy. record doesn't pass the bar of removing tons of boilerplate and making a pit of success for developers. The main beef I have with it is the default implementation of IEquatible<T> in practical use, minimal "data objects" often come with other meta data, and that breaks that--so quickly this specialized use case is trouble and needs overrides.

It seems like if a new language feature is only minimal amount of code and uses existing language features, why have it? I'd like someone to point out why in a few salient points why we need a new keyword, which is a big deal. async I get it, but so far, not this one.

@CyrusNajmabadi
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Oct 25, 2020

It seems like if a new language feature is only minimal amount of code

To you it may seem like a minimal amount of code. To others it may seem like removing a lot of boiler plate. Similar times we've done this are when we simplified how you can do properties. Yes, it only cleanly up a minimal amount of code (a few lines here and there), but that can still be very nice ot have to not make it feel like you are spending so much of your time on boilerplate.

The same applies here. In practically al codebases we see, we see common repetition of the pattern of:

  1. declare type, want it to be a value type
  2. declare members separately.
  3. initialize members in the most obvious manner possible
  4. define equality in the most obvious manner possible.
  5. define GetHashCode (often poorly, since doing it well is non-obvious)
  6. define the IEquatable contract.
  7. add your operators

Not only is this just drudgery, it imposes a tax on you for every change you make to the type. Want ot add a member, you have to go keep 4 places in sync. A standard simply value type literally can easily hit 40+ lines just to encode all of that. Using records, you can have all of the above, for free, in a single line that is easy to understand, modify and maintain.

@CyrusNajmabadi
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Oct 25, 2020

why have it?

This is not how we judge features. THe vast majority of features in C# are ones that took something you could already do, but made it more pleasant. We actually think this is super important for the health of hte language as people spend a ton of hteir time on this basic stuff. Very few language features are actually in the realm of "you have no way (or it's super difficult) to accomplish this today". Instead, the language balances adding some of those features, along with a ton of "quality of life" features to improve the entire development experience for the community.

async I get it, but so far, not this one.

Examples like generics and async are the extreme outliers. Most features are much more about programming ergonomics. Precisely about doing what you were always able to do, but in a much less fussy manner.

@lrhoulemarde
Copy link

@lrhoulemarde lrhoulemarde commented Oct 25, 2020

It seems like if a new language feature is only minimal amount of code

To you it may seem like a minimal amount of code. To others it may seem like removing a lot of boiler plate. Similar times we've done this are when we simplified how you can do properties. Yes, it only cleanly up a minimal amount of code (a few lines here and there), but that can still be very nice to have to not make it feel like you are spending so much of your time on boilerplate.

The same applies here. In practically al codebases we see, we see common repetition of the pattern of:

  1. declare type, want it to be a value type
  2. declare members separately.
  3. initialize members in the most obvious manner possible

If the problem domain is just a "data record", can we not just create a bunch of readonly structs with readonly fields?

  1. define equality in the most obvious manner possible.
  2. define GetHashCode (often poorly, since doing it well is non-obvious)
  3. define the IEquatable contract.

Basically this comparer is only useful for specific use cases and perhaps even specific applications with specific types of data. The minute we need case-insensitivity or meta data rides along, this major feature is no longer useful and could be a source of bugs itself. The last thing I want to say to my team, is sure use records, but always override IEquality with "not supported" until you know exactly what its doing...

  1. add your operators

This has to be done anyway, record or not.

Not only is this just drudgery, it imposes a tax on you for every change you make to the type. Want to add a member, you have to go keep 4 places in sync. A standard simply value type literally can easily hit 40+ lines just to encode all of that. Using records, you can have all of the above, for free, in a single line that is easy to understand, modify and maintain.

If saving that code is an issue, could we not create an official IRecord<T> where T : struct {...} with a default implementation that reflects its fields? Don't we get with anyway? In other words, maybe when this was proposed, the C# could not help the main use case... but now it can?

All that aside, I thought there was a very, very high bar with adding a language keyword to C#. Properties may have been simple, but they were so general to the language that any syntax help was welcome. records on the other hand are more domain- specific. The train may have already left the station here and many of these points already considered, but I think we are running the real risk of having a feature that is not recommended or even disallowed in places, and not in the way unsafe is. This is the first keyword feature I feel could fall in that bucket.

@CyrusNajmabadi
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Oct 25, 2020

If the problem domain is just a "data record", can we not just create a bunch of readonly structs with readonly fields?

First, that might not be what is wanted if you want are reference type.

Second, that approach does not work for helping with:

  1. declare members separately. You still need to do this.
  2. initialize members in the most obvious manner possible. You still need to do this.
  3. define equality in the most obvious manner possible. You still need to do this. The default impl for structs is reflection based (and hte runtime will not change). So you pay a high price here.
  4. Same with GetHashCode.
  5. define the IEquatable contract. You still need to do this.
  6. add your operators. You still need to do this.

So you get barely any help here at all.

@CyrusNajmabadi
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Oct 25, 2020

This has to be done anyway, record or not.

This is not true. Records define operator== and operator !=.

@CyrusNajmabadi
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Oct 25, 2020

If saving that code is an issue, could we not create an official IRecord where T : struct {...} with a default implementation that reflects its fields?

I don't really know waht that is. But you can certainly feel free to start a discussion on the topic. If we like it, we could move forward with it in the language.

@CyrusNajmabadi
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Oct 25, 2020

All that aside, I thought there was a very, very high bar with adding a language keyword to C#.

Yes. Records met that bar.

records on the other hand are more domain- specific.

That's exactly not the case. Records are very domain-general. I.e. we expect to see records in effectively all domains. That's one of the reasons they warranted this treatment.

but I think we are running the real risk of having a feature that is not recommended or even disallowed in places,

I have no idea where this position is coming from. Why on earth would records be disallowed anywhere? If you're already wriitng out the 40 line version, why would someone stop you from using the 1 line equivalent? :)

@lrhoulemarde
Copy link

@lrhoulemarde lrhoulemarde commented Oct 26, 2020

@CyrusNajmabadi

I disagree, and I think many others will, that record meets the bar. All the use cases I've seen are related to non-mutable data processing: a very specific use case and especially tied to specific veins of functional programming. And... this is something that probably could be implemented in-language without much fuss as a combination of features.

Why would record usage ever be disallowed? Because equality comparisons of data inevitably have domain issues. If for instance if the myrecord.StreetAddress comparison is not to be case sensitive, the default comparison fails out-of-the box. To prevent these sort of potential expensive domain-data bugs, records will be avoided by policy unless the usage could be proven to be safe.

This is not to say records will be found to non-useful by many; just that in the spectrum of features being added to C# at the keyword level, this is the weakest and most problematic one to date, and one that does not have a "pit of success" under it. Again, time will probably tell.

@CyrusNajmabadi
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Oct 26, 2020

All the use cases I've seen are related to non-mutable data processing:

Data processing is literally part of practically every domain :)

and especially tied to specific veins of functional programming

There is nothing that ties this to functional programming. Working with data and values comes up everywhere.

And... this is something that probably could be implemented in-language without much fuss as a combination of features.

Yes... that's what 'records' are. The feature that lets you do this. As i mentioned already, trying to do this today with all the features we've shipped so far means 40+ lines for this very simple, domain-general, concept. We didn't like that, so we made a feature for it. And now it can be a single line that all devs can use for practically any problem space.

and one that does not have a "pit of success" under it.

I really don't get how you can say that. I've already outlined the problems people had prior to this. #39 (comment) shows all the hoops people need to jump through today. Part of those hoops are practically very hard to get right. This is especially true if you dn't have something like System.HashCode. It's also super difficult for people to get value-equality correct, among other things. On top of all of this, the maintenance cost of those problems is quite high.

The state prior to records is literally the opposite of a 'pit of success'. You have to put in a ton of effort, and it's a great pain to ensure correctness and keep maintained. Literally all of that goes away with 'records'. You can now write a single line, that just states the data you care about, and all of this is taken care of for you. It is definitionally a 'pit of success' because the core design around them was generating the appropriate defaults the ecosystem had already settled on for value types, instead of forcing the user to do that and maintain it.

this is the weakest and most problematic one to date

You haven't stated any weaknesses or problems with it. :-/

@PatrikBak
Copy link

@PatrikBak PatrikBak commented Nov 12, 2020

I would like to ask, will it be possible to have an immutable list of strings in a record? How about an immutable set of strings? Will it generate correct equals and hash code methods?

@HaloFour
Copy link
Contributor

@HaloFour HaloFour commented Nov 12, 2020

@PatrikBak

I would like to ask, will it be possible to have an immutable list of strings in a record? How about an immutable set of strings? Will it generate correct equals and hash code methods?

Records always generate the same equality and hash code methods. They defer how equality is determined to the types of the fields in the record. So if you're using an immutable list or set of Strings it's up to that collection class to evaluate equality.

@PatrikBak
Copy link

@PatrikBak PatrikBak commented Nov 12, 2020

@PatrikBak

I would like to ask, will it be possible to have an immutable list of strings in a record? How about an immutable set of strings? Will it generate correct equals and hash code methods?

Records always generate the same equality and hash code methods. They defer how equality is determined to the types of the fields in the record. So if you're using an immutable list or set of Strings it's up to that collection class to evaluate equality.

I see. That sounds a bit impractical, because even ImmutableList<T> does not implement Equals as SequenceEquals:

var a = ImmutableList.Create(42);
var b = ImmutableList.Create(42);

// This prints False
Console.WriteLine(a.Equals(b)); 

So, I would have to create a wrapper of a list / set that implements equals and hash code using its elements, and then use this wrapper in a record as a field, am I right?

@HaloFour
Copy link
Contributor

@HaloFour HaloFour commented Nov 12, 2020

@PatrikBak

Correct, you'd either have to provide wrappers and override the Equals/GetHashCode implementation of the element types, or you need to override the Equals/GetHashCode implementation of the record itself.

@PatrikBak
Copy link

@PatrikBak PatrikBak commented Nov 12, 2020

@HaloFour

or you need to override the Equals/GetHashCode implementation of the record itself.

This sounds interesting. Is there a way to combine overridding with the generated version? For example, if my record has 5 string fields and 1 list of strings, I'd like to write something like

public override bool Equals(MyType obj) => base.Equals(obj) && obj.List.SequenceEquals(List)

(Perhaps it would require some attribute on the string List indicating that it should not be including in the generated version?)

@HaloFour
Copy link
Contributor

@HaloFour HaloFour commented Nov 12, 2020

@PatrikBak

Is there a way to combine overridding with the generated version?

No, it's an all-or-none proposition. If you override the Equals and/or GetHashCode methods then the compiler will not generate them.

The team had considered some approaches to enabling some flexibility around equality but were considered that it would explode into a whole series of possible features. For these scenarios they instead recommend using source generators which could inspect attributes on the members of the record and emit custom equality members.

@DualBrain
Copy link

@DualBrain DualBrain commented Nov 18, 2020

but I think we are running the real risk of having a feature that is not recommended or even disallowed in places,

I have no idea where this position is coming from. Why on earth would records be disallowed anywhere? If you're already wriitng out the 40 line version, why would someone stop you from using the 1 line equivalent? :)

See dotnet/roslyn#49469 as to a very real reason why this feature (at least in part) may not be recommended or even disallowed in places. It's easier to recommend avoiding the feature than for many people to understand when, how and where it can be consumed. Use it internally, sure. As part of an API surface... sort of... and that is how you can end up with the recommendation/disallowing.

@Liander
Copy link

@Liander Liander commented Jan 2, 2021

This is a legal statement from a struct (or class) construction today:
new Message { Text = "Hello world!" }.PostOn(bus);

But this statement from a record construction is not:
Message.NewMessage() with { Text = "Hello world!" }.PostOn(bus);

Is there a rational for this difference?

Will there be opportunity to post-process a record? To instead support:
Message.PostMessage(bus) with { Text = "Hello world!" };

Will there be opportunity to pre-process a record? To support:
this.ModificationTime = DateTime.UtcNow; // set whenever changed (constructed/cloned)

@fubar-coder
Copy link

@fubar-coder fubar-coder commented Jan 2, 2021

@Liander Maybe you're looking for AOP (Aspect Oriented Programming) with - for example PostSharp? It should support the pre- or post-processing whenever a records gets created with then with expression.

@Liander
Copy link

@Liander Liander commented Jan 2, 2021

@fubar-coder No, I am looking for information of current plans for the record features. There have been discussions about overriding cloning, factories, validation, etc and I am not sure if the mentioned situation is within the scope of that.

@Liander
Copy link

@Liander Liander commented Feb 1, 2021

This is a legal statement from a struct (or class) construction today:
new Message { Text = "Hello world!" }.PostOn(bus);

But this statement from a record construction is not:
Message.NewMessage() with { Text = "Hello world!" }.PostOn(bus);

Is there a rational for this difference?

Will there be opportunity to post-process a record? To instead support:
Message.PostMessage(bus) with { Text = "Hello world!" };

Will there be opportunity to pre-process a record? To support:
this.ModificationTime = DateTime.UtcNow; // set whenever changed (constructed/cloned)

Does anyone know the plans for custom factories and cloning overrides (which I think is the way to address the above)? At some point I am pretty sure I have read about it in some spec but it does not seem to have made it, yet?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet