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

Discussion thread for records #10154

Closed
gafter opened this issue Mar 28, 2016 · 62 comments
Closed

Discussion thread for records #10154

gafter opened this issue Mar 28, 2016 · 62 comments

Comments

@gafter
Copy link
Member

gafter commented Mar 28, 2016

This is a discussion thread for records, as specified in https://github.com/dotnet/roslyn/blob/features/records/docs/features/records.md

Please check the spec before adding your comment - it might have been addressed in the latest draft.

@bbarry
Copy link

bbarry commented Mar 28, 2016

public Point With(int x = this.X, int y = this.Y) => new Point(x, y);

+1

will that be possible to extend with extension methods?

public static Point With(this Point p, int x = this.X, int y = this.Y) => new Point(x, y);

where this.X refers to Point::get_X()?

@orthoxerox
Copy link
Contributor

@gafter what's the point of making record_parameter_list optional here?:

record_parameters
    : '(' record_parameter_list? ')'
    ;

Won't an empty record parameter list simply create a regular class/struct with a useless Equals/GetHashCode implementation?

@HaloFour
Copy link

@orthoxerox Perhaps an empty abstract record from which a hierarchy can be defined with no shared members? Or patterns that match state by type rather than data objects? I can't speak to specific use cases but I don't think that it would be completely useless.

@MgSam
Copy link

MgSam commented Mar 28, 2016

  • I think caller-receiver parameters are great and will work really well.
  • The with keyword is still worth it; it makes the intent much more clear.
  • There are no examples of primary constructor bodies. Are these of the same form that were proposed for C# 6.0? I thought those were a disaster, as they saved almost no typing but added a lot of complexity to the language.
  • I still see auto-generation of Equals, HashCode, is, with as being a completely separable feature from records. Why do you have to have an immutable type to get this? I see code every single day that is mutable but necessarily overrides Equals and HashCode. Is it less safe? Yes, but often necessary. It seems totally bizarre and unintuitive that the sealed keyword + primary constructor all of a sudden adds all this extra compiler magic that there is no way to access otherwise. I think this auto-generation functionality should be enabled its own keyword or attribute.
  • == and != definitely need to be overridden. It is bizarre to have a type where == behaves differently from Equals.
  • ToString needs to be a part of the auto-generation proposal. The string representation of a type is a critical part of the debugging experience. I don't want to have to continue to use R# to generate ToString for every single type I create.

@alrz
Copy link
Member

alrz commented Mar 28, 2016

I think this auto-generation functionality should be enabled its own keyword or attribute.

F# has something similar to this, namely StructuralEqualityAttribute, NoEqualityAttribute, etc to control code generation. I think that would be nice to have it in C# as well. That said, if the only reason that records are required to be abstract or sealed is equality complication, I think when the compiler cannot provide a straightforward implementation, it should require the user to either implement it or apply the NoEqualityAttribute when reference equality is sufficient.

@HaloFour
Copy link

@MgSam

There are no examples of primary constructor bodies. Are these of the same form that were proposed for C# 6.0? I thought those were a disaster, as they saved almost no typing but added a lot of complexity to the language.

From the spec it looks like @gafter adopted the syntax I threw at the wall in #206 (comment), specifically the second example:

public class Point(int X, int Y) {
    // can apply constructor attributes here
    internal Point { // can apply different accessibility modifier here
        // validation logic here
        // record members assigned automatically
    }
}

@alrz
Copy link
Member

alrz commented Mar 28, 2016

@HaloFour You don't need to initialize record's properties in primary-constructor-body, according to the spec, this will be used to add additional code to the compiler generated primary constructor.

@HaloFour
Copy link

@alrz Ok, I'll update my comment to reflect. I wonder if the constructor can at least override setting the members if they aren't explicitly, such as:

public class Student(string Name) {
    public Student {
        this.Name = Name.ToUpper();
    }
}

@alrz
Copy link
Member

alrz commented Mar 28, 2016

Also, spec states that there can be more than one primary-constructor-body and they will be executed in source order. I don't know why this is required when we have replace/original.

sealed class Student(string Name) {
  public Student { }
}
partial class Student {
  replace public Student {
    original; // no arg list?
  }
}

Actually it is up to the replaced ctor to decide when original should be called.

@gafter
Copy link
Member Author

gafter commented Mar 29, 2016

@MgSam

There are no examples of primary constructor bodies. Are these of the same form that were proposed for C# 6.0? I thought those were a disaster, as they saved almost no typing but added a lot of complexity to the language.

We didn't have primary constructor bodies in C# 6. We had "primary constructors", which were the same syntax as declaring a record but didn't give you any of the members that records automatically declare.

I still see auto-generation of Equals, HashCode, is, with as being a completely separable feature from records. Why do you have to have an immutable type to get this?

Records do not have to be immutable. You can declare the properties yourself as mutable

public class Point(int X, int Y)
{
    public int X { get; set; } = X;
    public int Y { get; set; } = Y;
}
  • == and != definitely need to be overridden. It is bizarre to have a type where == behaves differently from Equals.

You mean like every interface ever written? This is something we will need to discuss.

  • ToString needs to be a part of the auto-generation proposal. The string representation of a type is a critical part of the debugging experience. I don't want to have to continue to use R# to generate ToString for every single type I create.

This too is something we will need to discuss.

@alrz

That said, if the only reason that records are required to be abstract or sealed is equality complication...

There is no such requirement in the current spec. Look for EqualityContract.

@alrz
Copy link
Member

alrz commented Mar 29, 2016

initializes compiler-generated backing fields for the properties corresponding to the value parameters (if these properties are compiler-provided)

Then @HaloFour's example could be written like this,

public class Student(string Name) {
    public string Name { get; } = Name.ToUpper();
}

to not assign it twice.

@HaloFour
Copy link

@alrz True, a better example might be one that involves both validation and initialization:

public class Student(string Name) {
    public Student {
        if (string.IsNullOrEmpty(Name)) {
            throw new ArgumentException("Name can't be empty.", nameof(Name));
        }
        this.Name = Name.ToUpper();
    }
}

Depending on when the initializers execute perhaps the parameter itself could be modified before property initialization, although that might really confuse some people especially since it differs from how constructors/initializers work in normal classes:

public class Student(string Name) {
    public Student {
        if (string.IsNullOrEmpty(Name)) {
            throw new ArgumentException("Name can't be empty.", nameof(Name));
        }
    }
    public string Name { get; } = Name.ToUpper();
}

@alrz
Copy link
Member

alrz commented Mar 29, 2016

@HaloFour Considering that primary constructor body is the last one in the list, yes that could be an issue, however, it would be perfectly fine if you initialize the property inside the primary constructor body:

public class Student(string Name) {
    public Student {
        if (string.IsNullOrEmpty(Name)) {
            throw new ArgumentException("Name can't be empty.", nameof(Name));
        }
        this.Name = Name.ToUpper();
    }
    // avoids compiler generated assignment
    public string Name { get; } 
}

Just to say, I still think Student {} syntax doesn't feel right especially with no access modifier. I'd prefer default Student() {} to add code to the primary constructor and also just one of these should be allowed. Then if the user intends to replace it, original shall be called with an empty arg list.

@HaloFour
Copy link

@alrz I'm having trouble finding where in the spec that it specifies that a single record class may have multiple primary constructor bodies?

I'm not completely sold on the primary constructor body syntax either, but I prefer it to Java-style initializers or having the code just appear within the record body. I do think that it's nicer than having to completely redefine the constructor, parameters and all, which is what was previously in the spec.

@alrz
Copy link
Member

alrz commented Mar 29, 2016

executes the body of each primary_constructor_body, if any, in source order

Open issue: We need to specify that order, particularly across compilation units for partials.

As for Java-like syntax, it would make sense if they allow it in non-record types.

Open issue: Should we allow something like a primary_constructor_body (presumably without attributes and modifiers) in a non-record type declaration, and treat it like we would the code of an instance field initializer?

However, you lose access modifiers and probably attributes with that syntax.

@MgSam
Copy link

MgSam commented Mar 29, 2016

@gafter I believe primary constructor bodies proposed for C# 6.0 were something of the form:

class Foo(string bar)
{
    //This is the primary constructor body
    {
        if(bar == null) throw new ArgumentNullException();
    }
}

which proved to be very controversial.

@ErikSchierboom
Copy link

I must say that this looks great. Records are very useful, and the current design feels very C#-ish, which makes it integrate nicely into the language. I'm also in favor of keeping the with syntax, which makes for some very readable code.

This was referenced Mar 29, 2016
@gafter
Copy link
Member Author

gafter commented Mar 29, 2016

@bbarry Re "will that be possible to extend with extension methods?"

There is nothing in the current specification that would enable that.

@bbarry
Copy link

bbarry commented Mar 29, 2016

Fine by me; I think the syntax seems a bit unnatural for that case (thinking about the default parameter value, not the fact that it is the with method; I could see paramname = this.VisibleIdentifier being useful outside methods supporting with expressions). I was just wondering if it was considered and rejected or not considered at all so far.

To the main topic, It seems odd to create a language feature where adding a property to the type cannot be done without forcing a rebuild of any dependent code. Perhaps some tooling to convert a record class into a fully specified regular class might be necessary (would the reverse tooling even be possible in the general case?). In this spec:

public class Point(int X, int Y);

Adding a parameter:

public class Point(int X, int Y, int Z);

would require full rebuilds from source and potentially code fixes for almost any code that depends on Point. For example:

var w = p.With(Y: 0);

But if the class were converted back away from the record type, the necessary overloads could be added and existing code could continue to function:

public class Point
{
    public Point(int X, int Y) : this (X, Y, 0) {}
    public Point(int X, int Y, int Z) { ... }
    ...
    //public Point With(int X = this.X, int Y = this.Y) => new Point(X, Y, this.Z);
    public Point With(int X, int Y) => new Point(X, Y, this.Z);
    public Point With(int X = this.X, int Y = this.Y, int Z = this.Z) => new Point(X, Y, Z);
    ...
}

(Ignore Point being a bad example type for this kind of problem)

Edit: would the optional parameters be necessary on the 2 parameter version? You would want a recompile to switch immediately to the 3 param version and existing calls are already specified...

@HaloFour
Copy link

@bbarry

I believe that's part of the nature of records. Their shape and their construction are intrinsically linked. F# records have the same behavior.

🍝

Perhaps adding an optional parameter to the end of the primary constructor argument list could result in a compatible type?

public class Point(int X, int Y, int Z = 0);
// results in:
public class Point : IEquatable<Point> {
    public int X { get; }
    public int Y { get; }
    public int Z { get; }

    public Point(int X, int Y) : this(X, Y, 0) { }
    public Point(int X, int Y, int Z = 0) {
        this.X = X;
        this.Y = Y;
        this.Z = Z;
    }

    public virtual Point With(int X = this.X, int Y = this.Y) => With(X, Y, this.Z);
    public virtual Point With(int X = this.X, int Y = this.Y, int Z = this.Z) => new Point(X, Y, Z);

    public static void operator is(Point point, out int X, out int Y) {
        X = point.X;
        Y = point.Y;
    }
    public static void operator is(Point point, out int X, out int Y, out int Z) {
        X = point.X;
        Y = point.Y;
        Z = point.Z;
    }

    // other members omitted
}

@alrz
Copy link
Member

alrz commented Mar 31, 2016

Currently, deriving from a base type also causes to re-assignment of inherited properties.

public Student(string Name, decimal Gpa) : base(Name)
{
  this.Name = Name; // !
  this.Gpa = Gpa;
}

Since moving record element declarations into the body of the type doesn't seem to be an option here, it would be nice to be able to call this from the primary constructor and also override auto-generated assignments depending on the usage in any constructor initializer or presence of a primary constructor body. For example,

// causes to not generate assignment for `Name`
// or any property that is used in `BaseType(...)`
// obviously, either 'this' or 'base' can be called
public class Student(string Name, int Gpa) : this(Name), BaseType(...) {
  private Student(string Name) {
    if (string.IsNullOrEmpty(Name)) 
      throw new ArgumentException("Name can't be empty.", nameof(Name));
    this.Name = Name.ToUpper();
  }
}

public class Student(string Name, int Gpa) {
  // causes to not generate assignments for all properties
  public Student {
    if (string.IsNullOrEmpty(Name)) 
      throw new ArgumentException("Name can't be empty.", nameof(Name));
    if (Gpa > 0)
      throw new ArgumentException(...);
    this.Name = Name.ToUpper();
    this.Gpa = Gpa;
  }
}

@gafter
Copy link
Member Author

gafter commented Mar 31, 2016

@alrz According to the spec, if a suitable property is inherited, then it is not generated.

@alrz
Copy link
Member

alrz commented Mar 31, 2016

@gafter I was quoting the example from spec. So probably that is mistyped.

@qrli
Copy link

qrli commented Apr 1, 2016

One question: What's the reason for .With() and operator is taking different ways? Why cannot they both be instance methods, or both be static functions?

@HaloFour
Copy link

HaloFour commented Apr 1, 2016

@qrli

One question: What's the reason for .With() and operator is taking different ways? Why cannot they both be instance methods, or both be static functions?

The With method must be an instance method and virtual in order to avoid decapitation of the type. You want the most derived version to handle the method in order to ensure that the instance returned is the derived version and that any new properties are also copied intact:

public class Person {
    public string Name { get; set; }

    public virtual Person With(string Name) => new Person() { Name = Name };
}

public class Student : Person {
    public double Gpa { get; set; }

    public override Person With(string Name) => new Student() { Name = Name, Gpa = this.Gpa };
    public virtual Student With(string Name, double Gpa) => new Student() { Name = Name, Gpa = Gpa };
}

Person person = new Student() { Name = "Billy", Gpa = 3.5 };
Person other = person.With(Name: "William");
Debug.Assert(other is Student && ((Student)other).Gpa == 3.5);

As for operator is, the actual operator is specified by the type of the pattern used, not the type of the operand. It's quite possible for the type of the is operator to be completely unrelated to the type of the operand. So the compiler resolution has to be more like that of conversion operators (or even extension methods). The Cartesian/Polar example in the pattern matching spec is a good demonstration. You'll note there that Cartesian has no relation to Polar, the relationship is purely logical and conditional based on the values of the properties.

@azhoshkin
Copy link

Just another way to combine record types (to define right name and position of properties in Student for Person):

public class Person(string First, string Last);
// These will not compile if in the constructor are not the all parameters of Person
public class Student(Person.First, Person.Last, double GPA): Person;
// Or to change positions
public class Student(Person.First, double GPA, Person.Last): Person;

@mjp41
Copy link
Member

mjp41 commented May 18, 2016

Are you expecting to allow generic records? I couldn't find anything about generics in the document. The main reason I ask is that I found the F# version of with is less general than ideal see. After discussing this with Don, it is probably unlikely to get fixed for F# as it would be a breaking change, but for C# this could potentially be avoided.

So based on what you have so far I guess a generic pair would be

 class Pair<A,B>
    {
        A X;
        B Y;

        Pair(A x, B y) { this.X = x;  this.Y = y; }

        Pair<A, B> With(A x = this.X, B y = this.Y) { return new Pair<A, B>(x, y); }
    }

Now this would not allow with to change the generic parameters. If you had a generic method With, e.g.

        Pair<C,D> With<C,D>(C x = this.X, D y = this.Y) { return new Pair<C, D>(x, y); }

Then this would be nice, but would require careful design. If you omit either parameter, then you effectively constrain C = A or B=D. So you can kind of view it as really being the following four methods.

        Pair<C, D> With1<C, D>(C x, D y) { return new Pair<C, D>(x, y); }
        Pair<A, D> With2<A, D>(D y) { return new Pair<A, D >(this.X, y); }
        Pair<C, B> With3<C, B>(C x) { return new Pair<C, B>(x, this.Y); }
        Pair<A, B> With4<A, B>() { return new Pair<A, B>(this.X, this.Y); }

There are then worse cases if multiple types overlap in there usage of generic parameters, and really you want full type inference. So I guess this is going to be out of scope for C#?

@aelij
Copy link
Contributor

aelij commented Jun 15, 2016

@gafter

That's a pretty great spec. I love how easy it is to create immutable types!

I do have one question - can records be serialized and deserialized using existing serializers? (Data Contract, JSON, etc.) I couldn't understand from the spec whether you could apply attributes to the generated fields/properties. For example:

[DataContract]
class Person(
    [field: DataMember(Name = nameof(FirstName))]
    string FirstName,
    [field: DataMember(Name = nameof(LastName))]
    string LastName
);

(I'm using the field since the property only has a getter)

Thanks.

@qrli
Copy link

qrli commented Jun 16, 2016

@aelij I don't think DataContractSerializer works with immutable types by default.
However, Json.NET supports immutable types by default. It will map properties to constructor parameters. As of today, I don't think there is a good reason to use DCS for new code, unless for communication with old DCS endpoints.

@aelij
Copy link
Contributor

aelij commented Jun 16, 2016

@qrli DCS is still in use. It's shipped with .NET Core (unlike other older serializers). It's also the default serializer for Service Fabric's reliable collections.

Besides, the question whether attributes can be applied to fields/properties has broader implications (for example, you could use the newly proposed Roslyn source generators to add functionality to some auto-generated members by applying attributes to them).

@kostrse
Copy link

kostrse commented Jun 29, 2016

Please consider generation of ToString.
This is essential for logging and REPL/debugging scenarios.

Other languages which have concept of recods (case classes, data classes) implement it (at least I saw in Scala, Kotlin).

When you logging messages passing through your code in actor system / messaging bus etc. or in other distributed scenarios, you need logging.

Also consider things like REPL - you have no Visual Studio experience here.

@sirgru
Copy link

sirgru commented Oct 24, 2016

I just saw the features this afternoon so I may have missed something critical in the discussion, for that I apologize in advance.
Here is my personal opinion: the positional pattern matching feels wrong to me.

var p = new Person { FirstName = "Mickey", LastName = "Mouse" }; // object initializer
if (p is Person("Mickey", *)) // positional deconstruction
{
  return p with { FirstName = "Minney" }; // with-expression
}

it just feels very unintuitive, with something that is an 'is' operation I am not expecting assignements. Something like this is perfectly readable to me though:

var person = new Person { FirstName = "Mickey", LastName = "Mouse" }; // object initializer
if (person is Person p when p.FirstName == "Mickey") 
{
  return p with { FirstName = "Minney" }; // with-expression
}

I also have a slight problem with readability of the already established convention person is Person p. To me it reads almost as 'if person is type of Person and is p'. I wish the syntax was a bit different. For me, this would read the best.

var person = new Person { FirstName = "Mickey", LastName = "Mouse" };
if (person is Person using p where (p.FirstName == "Mickey")) 
{
  return p with { FirstName = "Minney" }; // with-expression
}

@ViIvanov
Copy link

@sirgru If I understand correctly, you can use next syntax:

if(person is Person p && p.FirstName == "Mickey")

In addition, in some cases, you can use PM syntax.

@sirgru
Copy link

sirgru commented Oct 26, 2016

After some looking around I have concluded I like that syntax, despite it being a bit strange at first. I guess even more benefits will come when looking at the bigger picture. Looking forward to the next release!

@OJacot-Descombes
Copy link

The current proposal (Language Feature Status) for the compiler generated GetHashCode proposes the following implementation:

public override int GetHashCode()
{
    return (Name?.GetHashCode()*17 + Gpa?.GetHashCode()).GetValueOrDefault();
}

This produces a hash code 0 as soon as one property is null. The following implementation I am proposing produces a useful hash code in any case:

public override int GetHashCode()
{
    return (Name == null ? 0 : Name.GetHashCode() * 17) +
           (Gpa  == null ? 0 : Gpa.GetHashCode());
}

@gulshan
Copy link

gulshan commented Feb 7, 2017

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.

@SirDragonClaw
Copy link

Personally I think that this should be more than just a POCO. Equals, HashCode should both be auto implemented otherwise what is the point really?

@lachbaer
Copy link
Contributor

My 5 cents: parts of the record type could be automatically implemented by interfaces, if requested by the user with the auto keyword. Otherwise it is a POCO.

class Point(int X, int Y) : auto IWithable, auto IEquatable<Point> {}

The standard object method overrides should always be created automatically, as should the operator is.

@lachbaer
Copy link
Contributor

I've got to explain the advantages of my previous suggestion. You can incrementally add functionality to the record types when the compiler and framework evolves. The record types are downward compatible, because the user can cast them to the necessary interface.

To ease with the available interfaces, there can be a summarizing interface, like 'IRecordTypeBase' that implements basic functionality and 'IRecordTypeNet50' for additional functionality provided by .Net 5.0.

@jinujoseph jinujoseph modified the milestones: 16.0, Backlog Jan 21, 2019
@gafter
Copy link
Member Author

gafter commented Jan 28, 2019

Any further discussion should be moved to the csharplang repo.

@gafter gafter closed this as completed Jan 28, 2019
@gafter
Copy link
Member Author

gafter commented Jan 28, 2019

/cc @agocke You might want to mine this thread for relevant comments to your current prototyping work.

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