Records: Metadata determination of whether a type is a record

[333fred]

Tracking issue for discussion in LDM. We have several customers (such as EF, F#, the IDE) who have taken implicit dependency on the <Clone>$ method name as a valid way to determine whether a type is a record or not, and there have been performance issues with this approach (as loading method metadata can be expensive unless special codepaths are added to look for a specific method, see dotnet/roslyn#48935). This is not a forward-compatible way of determining whether a type is a record, so we need to discuss adding some real metadata in the 16.9 timeframe to record generation for future determination of whether a type is a record or not. Some points to discuss:

• Our initial plan was that moving from record->non-record would be an implementation detail (though we did not quite achieve this in C# 9). Exposing this information would mean we can no longer achieve this goal, and would instead have to strive for making the record->non-record transition be a fully compatible change.
• 16.9 is likely an ok timeframe for this, particularly since 16.8 is not an LTS, but we need to make sure that we are ok with this.

/cc @jaredpar

LDM Discussions:
https://github.com/dotnet/csharplang/blob/master/meetings/2020/LDM-2020-11-11.md#isrecord-in-metadata

[jaredpar]

This is not a forward-compatible way of determining whether a type is a record,

This is not the conclusion of LDM when we discussed this previously.

[333fred]

This is not the conclusion of LDM when we discussed this previously.

The previous conclusion of the LDM was that it was possible we'd need to keep this method around, regardless of whether we end up implementing generalized with by allowing any type to have this method. That has nothing to do with whether or not using this method will always be a good way to determine if a type is a record, and is what we need to discuss on Wednesday.

[333fred]

Alright, talked with Jared offline. We were talking past each other about part of my wording above: regardless of whether we implement generalized with support via <Clone>$, that method will not be going away. My point was more about whether it would be vestigial or not, which was not clear: we definitely need to keep the method regardless.

As to implementation strategy, we can take a page out of how we do extension methods: in 16.9, we emit an attribute on both the module and on the record itself. If the attribute exists on the module, then we only consider the attribute when determining whether the type is a record. If the attribute does not exist on the module, we only consider the presence of a <Clone>$ method, since the the only way that method can exist is if someone manually defined it in F# or IL, or if the type is actually a C# record (with the latter being much more likely).

[bchavez]

FWIW, just to add some context: an issue recently came up in Bogus to support C# 9 record types here: bchavez/Bogus#334.

Currently, given a positional record of type T, the following is not possible:

record Dog(string Name);

var dog = new Faker<Dog>()
     .RuleFor(d => d.Name, f => f.Name.FindName())
     .Generate();

System.MissingMethodException: 'No parameterless constructor defined for type 'Dog'.

To get a positional record type working with Faker<T> the user is required to supply a .CustomInstantiator() as shown below:

record Dog(string Name);

var dog = new Faker<Dog>()
               .CustomInstantiator( f => new Dog( f.Name.FindName() ) )
               .Generate();

To ease some of the developer pain and help ergonomics, I was looking to reflect over T inside Faker<T> for something like typeof(T).IsRecord and call .GetUninitializedObject() for T; but I recently found that typeof(T).IsRecord is not a thing and that reflecting for <Clone>$ is probably what needs to be done.

It's a hack for sure and probably not the greatest, but seems like could work.

[jnm2]

@bchavez Wouldn't it be better to treat records the same as ordinary classes, since that's what they output? How do you handle classes that have no parameterless constructor?

[YairHalberstadt]

There should also be some way to tell which constructor is the primary constructor.

[kofifus]

ATM I detect records like this:

 var isRecord = (((TypeInfo)t).DeclaredProperties.Where(x => x.Name == "EqualityContract").FirstOrDefault()?.GetMethod?.GetCustomAttribute(typeof(CompilerGeneratedAttribute)) is object) && (t.GetMethod("<Clone>$") is object);

Another way to detect records can be to check if Equals and GetHashCode are marked with the CompilerGenerated attribute see dotnet/roslyn#47613

If one can determine that a type is a) a record and b) has compiler generated Equals and GetHashCode then one can assert that the type has value semantics. That is incredibly important (to me) information that is impossible to deduce for a regular class. In fact it is to me disappointing the record feature is not restricted to value semantics. All this push to make records indistinguishable from classes is really confusing, if they're the same why do we need records at all ? just for some trivial syntactic sugar ?

[HaloFour]

@kofifus

All this push to make records indistinguishable from classes is really confusing, if they're the same why do we need records at all ? just for some trivial syntactic sugar ?

Why is it important to distinguish them as records? It's more important to recognize each type for the functionality they offer. Non-records may definitely offer value equality, and records are capable of not having value equality.

IMO it's like iterators. They're an implementation detail, an aid to the developer. All you know is that you have an IEnumerable<T>, but you have no idea that was created by an iterator. And it's not all that important that it was.

[HaloFour]

@YairHalberstadt

There should also be some way to tell which constructor is the primary constructor.

Out of curiosity, why? What would make a record with a primary constructor different from a regular class with a single non-parameterless constructor? Or a record with both a primary constructor and a secondary constructor different from a regular class that happens to have two constructors?

[kofifus]

@HaloFour again the motivation is asserting a type has value semantics, then you can ie use it as a dictionary key and composing it in other value types .. anyway that ship has sailed but at least there should be a way to tell if a type is a record with no user defined equals/gethashcode as a second best

[HaloFour]

@kofifus

the motivation is asserting a type has value semantics, then you can ie use it as a dictionary key and composing it in other value types

IMO IEquatable<T> is the best indicator of that for reference types. Records implement that interface automatically.

[AartBluestoke]

again the motivation is asserting a type has value semantics, then you can ie use it as a dictionary key

There is nothing about being a record that guarantees that. Records are only default readonly, but anyone can define a set operator on any record property.

[theunrepentantgeek]

@kofifus asked

All this push to make records indistinguishable from classes is really confusing, if they're the same why do we need records at all ? just for some trivial syntactic sugar ?

The syntactic sugar is far from trivial.

There are a bunch of extremely common mistakes that records eliminate, preventing whole classes of subtle bugs. These include (not an exhaustive list)

• Poor implementations of GetHashCode() can kill the performance of dictionaries and sets
• Not ensuring the IEquatable<T> implementation is sensitive to all the members of the type, or not ensuring that its symmetric
• Not ensuring GetHashCode() is sensitive to all the members of the type, or at least to the same members as IEquatable<T>
• Inconsistency between IEquatable<T> and GetHashCode() (if the former returns true, the two hashcodes had better be the same, else chaos ensues)

[kofifus]

thanks @HaloFour that was useful

[AartBluestoke]

I think the documentation about this should be clearer.
There is no such thing as a record at runtime. It is just a compile time option for a class with a few prebuilt operators. Some of them have complicated implementations not currently available to default language constructs, but that didn't stop them being a class.

[vritant24]

To add another use case here: for Microsoft Fakes we generate Stub classes for all types in an assembly, and it looks like so:

public class StubClass : ClassBeingStubbed
{
}

The addition of records made this cause failures because we did not account for checking if ClassBeingStubbed is a record, and classes cannot inherit records.
At the moment we've decided to use the presence of the <Clone>$ method to determine if a type is a record to handle this unique case, but it would be nice to have a more concrete way of determining if a type is a record for future proofing.

> Our initial plan was that moving from record->non-record would be an implementation detail

I don't think it's entirely correct to say this. Maybe in the runtime it may be so but in cases of code generation / compile time it matters because of the inheritance behavior of records (records cannot inherit classes and classes cannot inherit records). In these cases, a metadata property like IsRecord or the presence of an attribute (like it is done for ref struct in .NET Framework) would be much better to make available rather than having users rely on the existence of a method which doesn't have a guarantee to remain the "unique identifier".

[333fred]

I don't think it's entirely correct to say this.

It is correct to say this, as long as you include the second part of that sentence: (though we did not quite achieve this in C# 9) 🙂.

[vritant24]

I don't think it's entirely correct to say this.

It is correct to say this, as long as you include the second part of that sentence: (though we did not quite achieve this in C# 9) 🙂.

You're right. I skimmed over that and didn't register it.

Is there planned work to remove the inheritance constraints and/or to consider adding a metadata indicator?

[333fred]

Not yet.

[333fred]

We have no plans to do anything more here. Closing out.