dotnet / csharplang

The official repo for the design of the C# programming language
11.62k stars 1.03k forks source link

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

Open MadsTorgersen opened 7 years ago

MadsTorgersen commented 7 years ago

LDM notes:

CyrusNajmabadi commented 4 years ago

Honestly, I haven't thought much about it. But as you can restrain with class and struct keywords

The class and struct constraints do not limit one to only passing classes or structs in. They limit one to passing reference types or value types.

For example, with the class constraint, one can pass in classes, interfaces, arrays, delegates, etc. With the structconstraint one can pass in structs, enums, tuples, etc.

So the addition of a new reference to does not warrant changes with constraints here.

CyrusNajmabadi commented 4 years ago

Another use case I can see is if you want to use the with keyword on that generic type, the compiler will need to know that it's a record type.

Nope. with is not limited to records.

HaloFour commented 4 years ago

@CyrusNajmabadi

Nope. with is not limited to records.

Is that happening in the C# 9.0 timeframe? I thought with currently required the clone method with an unspeakable name that is currently only emitted as a part of a record.

333fred commented 4 years ago

CyrusNajmabadi

Nope. with is not limited to records.

Is that happening in the C# 9.0 timeframe? I thought with currently required the clone method with an unspeakable name that is currently only emitted as a part of a record.

Correct. The intention is to remove the restriction, but currently with is restricted.

PathogenDavid commented 4 years ago

Is there a reason that the synthesized Equals method doesn't check for reference equality as an early-out before comparing all of the fields?

For well-behaved Equals implementations for all fields, it think it should be expected that r.Equals(r) will be true when ReferenceEquals(r, r) is also true.

The synthesized operator == does check reference equality before calling Equals. The intent in that case may have been for handling null == null, but it also means that there is a measurable performance disparity between x.Equals(x) and x == x:

Method Mean Error StdDev Ratio RatioSD
x.Equals(x) 27.0569 ns 0.2132 ns 0.1890 ns 84.25 8.87
'x == x' 0.3225 ns 0.0378 ns 0.0354 ns 1.00 0.00
x.Equals(x2) 26.4226 ns 0.1043 ns 0.0925 ns 82.30 8.88
'x == x2' 26.4400 ns 0.3351 ns 0.2616 ns 81.22 9.44
x.Equals(y) 10.2818 ns 0.1136 ns 0.1063 ns 32.22 3.36
'x == y' 9.7670 ns 0.0641 ns 0.0568 ns 30.42 3.23

(x2 is a record with the same values as x, but a different reference. y is a different record entirely.)

(This benchmark was built with the latest Roslyn nightly 3.8.0-3.20420.3 (f2e2f2a2) and ran on .NET 5 preview 7)

CyrusNajmabadi commented 4 years ago

This is not really a language question. Can you raise this at dotnet/Roslyn ? Thanks @PathogenDavid

PathogenDavid commented 4 years ago

@CyrusNajmabadi Are you sure? The spec is pretty explicit about what gets synthesized for Equals:

The synthesized Equals(R?) returns true if and only if each of the following are true:

  • other is not null, and
  • For each instance field fieldN in the record type that is not inherited, the value of System.Collections.Generic.EqualityComparer<TN>.Default.Equals(fieldN, other.fieldN) where TN is the field type, and
  • If there is a base record type, the value of base.Equals(other) (a non-virtual call to public virtual bool Equals(Base? other)); otherwise the value of EqualityContract == other.EqualityContract.

The definition for == is even more specific and provides code, which includes the reference comparison:

pubic static bool operator==(R? r1, R? r2)
    => (object)r1 == r2 || (r1?.Equals(r2) ?? false);

If the compiler were to start emitting a reference comparison for Equals, I would expect that the spec here would need to change since it would change the behavior in weird edge-cases. (Both would be true if Equals had a reference equality check.)

CyrusNajmabadi commented 4 years ago

That's a fair point. As this would not just be an internal optimization, but potentially a visible external behavior, it works best to be spec'ed.

@jcouv can you bring to meeting, or fast track an email discussion on this? I think we could resolve within the day. Thanks!

333fred commented 4 years ago

Yes, that's a language question. I'll bring it up with the team later today.

Happypig375 commented 4 years ago

Would it also make sense to test reference equality on all reference type fields before testing value equality to speed up?

CyrusNajmabadi commented 4 years ago

Would it also make sense to test reference equality on all reference type fields before testing value equality to speed up?

I think we want value semantics. Which means checking if the fields have value equality. Those types htemselves can speed this up by doing a ref-equals if they want.

darcythomas commented 4 years ago

@mbernard

Are you planning on supporting generic constraints with the record keyword?

Out of curiosity when would you want to constraint the generic type argument to a record?

I would like to have something like IRepository<T>: where T is record

That would then mean that, across a layer boundary, I know I will always get record types back.

333fred commented 4 years ago

That would then mean that, across a layer boundary, I know I will always get record types back.

But what would that actually mean? The intent with records is that, hopefully in C# 10, there won't be a meaningful semantic difference between a record and a class. You'll be able to get all the benefits of records (such as withability) in either the full form of a class or in the smaller record declaration. I'm just not sure what you actually want to to constrain to with that.

darcythomas commented 4 years ago

That would then mean that, across a layer boundary, I know I will always get record types back.

But what would that actually mean? The intent with records is that, hopefully in C# 10, there won't be a meaningful semantic difference between a record and a class. You'll be able to get all the benefits of records (such as withability) in either the full form of a class or in the smaller record declaration. I'm just not sure what you actually want to to constrain to with that.

It means that the 'pit of success' is to only use simple records as dtos / message parsing objects. Making it harder to put business logic into them (BL should live in service classes imho, or perhaps in actor (i.e., akka.net) classes for advanced use-cases)

It would be an obvious 'code smell' if you see a record class with methods, not just simple properties.

333fred commented 4 years ago

It would be an obvious 'code smell' if you see a record class with methods, not just simple properties.

I don't know if I believe that. I might, but I do think that computed properties/small computed methods might also make sense on a record type. Further, I don't think that using record as a constraint there actually implies that fact, or really does anything to help a consumer of that method. It's not like the requirement that a record type be simple is encoded in the type system. It seems like equal amounts of user education would be necessary with or without that constraint.

Making it harder to put business logic into them

See, this is why I'm leary: there's nothing in a record constraint that would actually make this true.

darcythomas commented 4 years ago

It would be an obvious 'code smell' if you see a record class with methods, not just simple properties.

I don't know if I believe that. I might, but I do think that computed properties/small computed methods might also make sense on a record type.

Computed properties are ok (imho) because they wont change the state of the record.

See, this is why I'm leary: there's nothing in a record constraint that would actually make this true.

The semantic meaning of a record is well, arecord/snapshot/dto/message. So it then would be reasonable to create a roslyn analyser, which will give warnings/errors if a record has methods (computed properties would get a pass).

You could also make an analyser to give warnings if a method on a service class (from the BL project) accepts a non record type as a parameter.

These are obviously more specialized usages. Not everyone would want them, but by having the option to have a generic constraint of record we give the option for people to implement those usages.

This could be implemented quite easily, if the record code generation just added something like a System.IRecord interface to the type (the interface would be empty or an interface with a simple default implementation). Having a context aware record <==> System.IRecord would be nice semantic sugar

333fred commented 4 years ago

I would argue that a much better solution would be to implement such an analyzer yourself, defining your own DTO-base-interface and warning when a type that implements this interface does complicated things. This logic that you would want to perform is predicated on a very specific interpretation of records as a feature, one that the language just doesn't share. The way to actually get what you're looking for is through an analyzer, not a record constraint.

chrisoverzero commented 4 years ago

The semantic meaning of a record is well, arecord/snapshot/dto/message.

I don’t think the design of records is that they have any semantic meaning whatsoever. It’s effectively a macro for defining a type with many commonly used features in idiomatic and correct ways using a compact syntax.

darcythomas commented 4 years ago

The semantic meaning of a record is well, arecord/snapshot/dto/message.

I don’t think the design of records is that they have any semantic meaning whatsoever. It’s effectively a macro for defining a type with many commonly used features in idiomatic and correct ways using a compact syntax.

A record is a 'macro' for compactly defining a class, which is (common case) a collection of properties (with correctly created commonly used features).

A class which is primarily a collection if properties, is semantically a dto/message. So yes a record is flexible to be more than that, but the common case is a dto/message. So to me it does seem to have a strong semantic meaning.

If a developer doesn't want to stick with that, that is Ok. However those that do, having a generic constraint (as simple as IRecord) means it is easy, for those that do write analyzers, to be able to target records.

So:

  1. There is an (idiomatic) use case
  2. It would be simple to implement,
  3. It would not be causing language design lock-in issues
  4. It would not have a big drawback for those who don't want to use this idiomatic feature.
  5. A work around of defining your own DTO-base-interface is possible but requires unnecessary boiler plate

Thoughts? How would you use an IRecord generic constraint if it were available?

CyrusNajmabadi commented 4 years ago

However those that do, having a generic constraint (as simple as IRecord) means it is easy, for those that do write analyzers, to be able to target records.

I think the point is this:

Because records don't actually have any restrictions (as you pointed out "If a developer doesn't want to stick with that, that is Ok"), then this constraint has no restrictions.

Note: you can always add your own attribute and analyzer to add this restriction if you want. i.e.:

void Foo<[DarcyRecordsOnly]T>() { ... }

333fred commented 4 years ago
  1. It would not be causing language design lock-in issues
  2. It would not have a big drawback for those who don't want to use this idiomatic feature.

No, as I mentioned previously, it would indeed have both of these issues. The design for records is that, from a consumption point of view, you cannot tell if a class is a record type or not. We didn't quite get there in C# 9, but the goal is still to get to that point. Adding a record constraint means that this would no longer be an achievable goal.

darcythomas commented 4 years ago

@CyrusNajmabadi @333fred

I guess my point is I see records as a great way to write DTO,s/messages (the 'right' way if there were such a thing?!) Now I could go and decorate all of my records with a DarcyRecords attribute or interface, but as a lazy developer (in a productive way) I don't want to id there were another way.

I guess it come around to:

Why would you not want to be able to tell (if you care about it) that a class was generated from a record or a 'normal' class?

CyrusNajmabadi commented 4 years ago

Why would you not want to be able to tell (if you care about it) that a class was generated from a record or a 'normal' class

THe same reason i don't care if a Task returning method uses async or not. It's an implementation detail. It's not part of hte public contract of the thing. i.e. if somone writes class Foo { public string Name { get; } }, i see no reason why that couldn't be used anywhere where record Foo(string Name); could be used.

GGG-KILLER commented 4 years ago

I might be a bit late to the party (and on the wrong issue) but, considering init properties set the value of a readonly backing field, shouldn't they be allowed in readonly structs? The backing field and the property are essentialy readonly aren't they?

richardpawson commented 4 years ago

I see a real need for the ability to define that a generic type is a record e.g. T: record and nothing to do with encapsulated methods (above). Because you can't use the with keyword with a class that is not a record.

I have a large number of records that implement:

public interface IHasModifiedDate
{
    DateTime ModifiedDate {get; init;}
}

but don't and can't have a common superclass.

So I want a freestanding generic function that can apply with to any record of type IHasModifiedDate e.g.:

public static T UpdateModifiedDate<T>(T obj, DateTime whenTo) where T: IHasModifiedDate 
{
    return obj with {ModifiedDate = whenTo};
}

but that won't compile because The receiver type 'T' is not a valid record type

where T : record, IHasModifiedDate

would permit this. Not allowing functions to work on records defined by interfaces is a huge constraint on the value of records, IMO.

chrisoverzero commented 4 years ago

@richardpawson The plan is for with to be pattern-based, so available to non-records: https://github.com/dotnet/csharplang/issues/162

richardpawson commented 4 years ago

@chrisoverzero That's good to hear. So you can have init-only properties on any class, and you will be able to use with on any class with init-only properties - is that right? And (strangely, to me) you can have regular properties on a record. And I'm aware that a record is a class. So what will be the distinguishing characteristic of a record over any other form of class?

lcsondes commented 4 years ago

If I understand correctly, nothing, it will be a shortcut to implement a kind of class not unlike how iterator methods are a shortcut to implement IEnumerable<T>.

chrisoverzero commented 4 years ago

So what will be the distinguishing characteristic of a record over any other form of class?

I don’t believe there is one – there’s nothing to distinguish. record is not a new thing, it’s a new way of defining an existing thing.

Serentty commented 4 years ago

Since it looks like the data modifier for auto-properties in records looks like it will be pushed back to C# 10, is there any chance that it can use the required property feature that is also planned for C# 10?

333fred commented 4 years ago

Yes, there is certainly a possibility that it could.

Serentty commented 4 years ago

I would absolutely love it if records went in that direction. The possibility that someone might not initialize one of the fields is one of my biggest issues with structs in C#. If data properties aren't must-init by default, I'll probably end up using positional records with named argument constructors instead. Speaking of which, it does seem like choosing between those two might be somewhat hard.

seanterry commented 4 years ago

I hope this is the right place to ask this:

When using the compact syntax to declare a record, how do you add XML documentation for the properties?

/// <summary>
/// Description of Foo. 
/// </summary>

public record Foo( int Id, string Value );

// warning CS1591: Missing XML comment for publicly visible type or member 'Foo.Id'
// warning CS1591: Missing XML comment for publicly visible type or member 'Foo.Value'
jnm2 commented 4 years ago

I think you have to manually declare them (public int Id { get; } = Id;). It's a pity because my XML docs are often identical for parameters and properties. Maybe if the markdown documentation comment overhaul goes through, something nice could happen for records.

andre-ss6 commented 4 years ago

I think you have to manually declare them (public int Id { get; } = Id;). It's a pity because my XML docs are often identical for parameters and properties. Maybe if the markdown documentation comment overhaul goes through, something nice could happen for records.

What overhaul? 🤔

PathogenDavid commented 4 years ago

What overhaul? 🤔

Nothing concrete, but it's been a topic of discussion for quite some time:

jnm2 commented 4 years ago

@sharwell also has a nice proposal draft: https://gist.github.com/sharwell/ab7a6ccab745c7e0a5b8662104e79735.

heshuimu commented 4 years ago

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.

That being said, I support that record should be available as a generic constraint, as its relation to class is not literally obvious and its extra capabilities would not come through if the generic constraint is specified as class. I assume some of us could live with where T: class, IEquatable<T>, IClonable, but hey we already have a keyword for it, why not use it?

And this leads to my main problem with this feature: It's too specific to one set of capabilities. There may be situations I don't want a record to be equatable, maybe I want my non-record class to conform to IEquatable automatically, and what if a record may do something extra in the future. I personally prefer Swift's auto-conformance to protocols like Codable and Equatable if the type specifies them, so that the type author could opt-in. This auto-synthesis for record should have been broken down into smaller pieces.

Hermholtz commented 4 years ago

I've noticed that the Equals method generated for records uses default equality comparers for fields by leveraging EqualityComparer<T>.Default or EqualityComparer<Nullable<T>>.Default.

Is it possible to have the autogenerated Equals method use case insensitive comparison for string fields in records?

HaloFour commented 4 years ago

@Hermholtz

Is it possible to have the autogenerated Equals method use case insensitive comparison for string fields in records?

No, as that would likely be the incorrect comparer in the majority of cases. The default comparer gets you the equality behavior of value types and if you have a need to customize equality for your own domain types then you can override the equality of that record.

darcythomas commented 4 years ago

That may be nice for your use case. But it may not be what others want. What if you want case sensitive comparisons on one property but case insensitive on another? What to do in non latin cultures (e.g., in german ss and ß are equivalent. but what happens with Ss and ß or sS and ß. Then what to do with the 4 different Turkish I's)

There was an issue for making a better string equals syntax but it all got a bit hard; if I recall correctly. It could be 10 times worse sadly with records.

It would be possible for you to either use your own IEqualityComparer<TMyRecord> when checking for equality. Or you could override the Equals method for your specific use case/ record.

That being said if there were an attribute we could put on a property to mark what kind of equality you were wanting that may be doable?!

On Tue, Oct 20, 2020, 10:27 Hermholtz notifications@github.com wrote:

I've noticed that the Equals method generated for records uses default equality comparers for fields by leveraging EqualityComparer.Default or EqualityComparer<Nullable>.Default.

Is it possible to have the autogenerated Equals method use case insensitive comparison for string fields in records?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/dotnet/csharplang/issues/39#issuecomment-712452113, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQIWVQBML4J4AVDHN7AZT3SLSVKZANCNFSM4C7SB5ZA .

Hermholtz commented 4 years ago

Thanks. Just wondering, because I found no answer on this. No offense intended.

CyrusNajmabadi commented 4 years ago

No offense intended.

No offense taken. it's a great question :)

Is it possible to have the autogenerated Equals method use case insensitive comparison for string fields in records?

As others have mentioned, this is out of scope. We did consider this sort of thing at one point, but decided against it due to the complexity. As above, the only way to get this is to take control of all of equality/hashing yourself. Sorry there's no other mechanism currently.

Note: with SourceGenerators, it might be possible to get some of the way here. i.e. you could define some attributes you'd put on the member. The SG would then spit out those methods with the semantics you want.

Thanks!

LokiMidgard commented 4 years ago

An Equals & HashCode generator would generally be pretty cool.

By the way, I wasn't aware that c# treats ß and ss as equals in de-DE culture. It's a little bit a shock. I think we had many lessons in school where to use ß and where to use `ss´ and I'm pretty sure you can't just interchange them.

CyrusNajmabadi commented 4 years ago

By the way, I wasn't aware that c# treats ß and ss as equals in de-DE culture.

Can you explain what you mean by that? C# shouldn't really care about this. Do you mean the .net string equality APIs?

svick commented 4 years ago

@LokiMidgard It doesn't, .Net (which is distinct from C#), sometimes says they are not equal, but that they sort the same. I.e., the following code:

CultureInfo.CurrentCulture = new CultureInfo("de-DE");
Console.WriteLine("ss".Equals("ß"));
Console.WriteLine("ss".CompareTo("ß"));

outputs the following on .Net Framework or .Net Core 3.1 on Windows:

False
0

Though on .Net 5.0 RC 2, I get:

False
-1

This is because .Net 5.0 changes the default globalization library on Windows.

LokiMidgard commented 4 years ago

@CyrusNajmabadi Sorry the sentence was poorly phrased. If you use equals using the culture explicite. The default equals does not do that.

"ss".Equals("ß",StringComparison.CurrentCulture) => true

But still unlike ä to ae which I think is normally equivalent, it's not the case for ß and ss. The words die Buße and die Busse have different meaning and are differently pronounced. The first one is atonement while the later one is the plural of bus.

@svick So will .Net 5 result in the above statement to false? I tested it in the Visual Studio Preview Interactive console. But I'm not sure if that uses .Net 5

Hermholtz commented 4 years ago

@LokiMidgard I think you need to report a proper bug. I'm not a team member, so no suggestions where, but I'm sure it's not the right place to discuss it, and it's going to be ignored here... ;)

PathogenDavid commented 4 years ago

dotnet/runtime is probably the better place to discuss this sort of thing.

CyrusNajmabadi commented 4 years ago

@CyrusNajmabadi Sorry the sentence was poorly phrased. If you use equals using the culture explicite. The default equals does not do that.

Correct. The runtime defines default equality semantics for strings. C# has no opinion on that particular decision. What we do have an opinion on is that record default-equality should map to field default equality (the same as how it's worked since 1.0 for structs). So, in this case, records that contain strings will be default-equal if the strings themselves are default-equal.

To me, that's really the only sane position to take by default (no pun intended). If we were different, then records and structs would behave differently. This would also make things super confusing once we add 'value records'. You would also just have to encode this idea that C# has special string handling in these corners of the language. All in all, not a good or understandable design for this core feature.