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

Bring records proposal up to date #3527

Merged
merged 10 commits into from
Jun 4, 2020
Merged

Bring records proposal up to date #3527

merged 10 commits into from
Jun 4, 2020

Conversation

agocke
Copy link
Member

@agocke agocke commented May 31, 2020

Try to bring the proposal in line with LDM decisions. Inheritance is somewhat described, but there
have been no affirmative decisions by LDM so this is still in flux

agocke and others added 2 commits May 30, 2020 17:28
Try to bring the proposal in line with LDM decisions. Inheritance is somewhat described, but there
have been no affirmative decisions by LDM so this is still in flux
@agocke agocke marked this pull request as ready for review May 31, 2020 23:56
proposals/records.md Outdated Show resolved Hide resolved
proposals/records.md Outdated Show resolved Hide resolved
proposals/records.md Outdated Show resolved Hide resolved
proposals/records.md Outdated Show resolved Hide resolved

The protected constructor is referred to as the "copy constructor" and the synthesized
body copies the values of all instance fields declared in the input type to the corresponding
body copies the values of all instance fields declared in the input type to the corresponding
fields of `this`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this`. [](start = 11, length = 6)

Just to be clear: I assume it is intentional that fields of the base are not copied. This will be extremely confusing to users.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither intentional nor unintentional -- I see two viable specifications here. One way is to try to set up a pattern of delegation, where if the base type is a record, we call base.Equals, and the other where we always override and check all types regardless.

LDM hasn't decided on anything so I haven't spent time detailing either one.

proposals/records.md Outdated Show resolved Hide resolved
proposals/records.md Outdated Show resolved Hide resolved
proposals/records.md Outdated Show resolved Hide resolved
proposals/records.md Outdated Show resolved Hide resolved
proposals/records.md Outdated Show resolved Hide resolved
## Positional record members

In addition to the above members, records with a parameter list ("positional records") synthesize
the following members, if a concrete (i.e. non-abstract) member with the same signature (or name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same signature [](start = 73, length = 14)

How do we handle inherited properties that are inaccessible, or get or set only.

And can the types, of inherited or explicit properties and record parameters, differ by dynamic/object, tuple element names, n[u]int/System.[U]IntPtr, or nullability?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lemme know if this needs more specification but hopefully the "matching" rule resolves everything.

proposals/records.md Outdated Show resolved Hide resolved
At runtime the primary constructor

1. executes the instance initializers appearing in the class-body; and then
invokes the base class constructor with the arguments provided in the `record_base` clause, if present
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arguments provided [](start = 47, length = 18)

What if no arguments were provided?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I'm not sure if anything else needs to be spec'd but my read is: reference types must call a base constructor. If there are no arguments, no base constructor is called. Thus, an error should be produced.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if anything else needs to be spec'd but my read is: reference types must call a base constructor. If there are no arguments, no base constructor is called. Thus, an error should be produced.

I don't think there is anything special in this scenario by comparison to regular classes. A default constructor initializer is going to be synthesized. That can cause an error if, for example, base class doesn't have an accessible parameter-less constructor.


In reply to: 433603013 [](ancestors = 433603013)

proposals/records.md Outdated Show resolved Hide resolved
proposals/records.md Outdated Show resolved Hide resolved

The protected constructor is referred to as the "copy constructor" and the synthesized
body copies the values of all instance fields declared in the input type to the corresponding
body copies the values of all accessible instance fields in the input type to the corresponding
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all accessible instance fields [](start = 26, length = 30)

Consider clarifying this, obviously all fields within a record are accessible.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about inherited private fields?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about inherited private fields?

Shouldn't base take care of this?


In reply to: 434061428 [](ancestors = 434061428)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for the very first record in a hierarchy, inheriting from a user-written class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for the very first record in a hierarchy, inheriting from a user-written class.

That is why I think it would be good to clearly describe two distinct scenarios and what is the behavior of the constructor in each of them.


In reply to: 434063198 [](ancestors = 434063198)

For a record:

* A public `get` and `init` auto-property is created (see separate `init` accessor specification).
Each "matching" inherited abstract accessor is overridden. The auto-property is initialized to
Copy link
Member

@cston cston Jun 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we require the abstract property to match the expected signature? Could an accessor be missing, or non-public? Could the property be a type that is assignable from the record parameter type rather than an exact match?


### Properties

For each record parameter of a record type declaration there is a corresponding public property member whose name and type are taken from the value parameter declaration. If no concrete (i.e. non-abstract) property with a get accessor and with this name and type is explicitly declared or inherited, it is produced by the compiler as follows:
For each record parameter of a record type declaration there is a corresponding public property
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public [](start = 80, length = 6)

synthesized?


* A public `get` and `init` auto-property is created (see separate `init` accessor specification). Its value is initialized during construction with the value of the corresponding primary constructor parameter. Each "matching" inherited abstract property's get accessor is overridden.
* A public `get` and `init` auto-property is created (see separate `init` accessor specification).
Each "matching" inherited abstract accessor is overridden. The auto-property is initialized to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overridden [](start = 49, length = 10)

But virtual (non-abstract) ones are not overridden?


## Members of a record type

In addition to the members declared in the class or struct body, a record type has the following additional members:
In addition to the members declared in the record body, a record type has additional synthesized members.
Members are synthesized unless an accessible concrete (non-abstract) member with a "matching" signature is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"matching" signature [](start = 83, length = 20)

The spec doesn't have a definition of signature for a property.

In addition to the members declared in the class or struct body, a record type has the following additional members:
In addition to the members declared in the record body, a record type has additional synthesized members.
Members are synthesized unless an accessible concrete (non-abstract) member with a "matching" signature is
either inherited or declared in the record body. Two members are considered matching if they have the same
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inherited [](start = 7, length = 9)

An abstract member in a grandparent class is still inherited into the child class even if overridden (implemented) in the parent class due to transitivity of inheritance. You may want to be more precise about what conditions cause the synthesiis or its suppression.

parameter declaration for each parameter of the primary constructor declaration. Each parameter
of the Deconstruct method has the same type as the corresponding parameter of the primary
constructor declaration. The body of the method assigns each parameter of the Deconstruct method
to the value from an instance member access to a member of the same name.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if there is a positional parameter with the same name as a property or field that is already present in the type? e.g.

record C(int X)
{
    public int X { get; init; }
}

It feels like we shouldn't generate a Deconstruct method in this case, for the same reason we don't automatically assign from the positional parameter to the explicitly declared property.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that mean that patterns like case Some(var value) would not be possible?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I completely follow, but users are free to write whatever Deconstruct method is appropriate for their type. The compiler should refrain from generating the Deconstruct method if a method with the same signature exists already on the record type. Not sure what should happen if the Deconstruct() method is on the base type. I don't think that is specified here, so let's make sure we come to a conclusion on that question.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, I read that as not generating a Deconstruct for a positional record with a single parameter. Nevermind.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should generate a Deconstruct regardless. If the user doesn't want one, they can declare a private method with the same signature.


## Members of a record type

In addition to the members declared in the class or struct body, a record type has the following additional members:
In addition to the members declared in the record body, a record type has additional synthesized members.
Members are synthesized unless an accessible concrete (non-abstract) member with a "matching" signature is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should an abstract property declared on the record type be allowed without error?

abstract data class C(object P)
{
    public abstract object P { get; }
}


`T Equals(T)` is specified to perform value equality such that `Equals` is true if and only if
all accessible instance fields in the receiver are equal to the fields of the parameter
and `this.EqualityContract` equals `other.EqualityContract`.
Copy link
Member

@cston cston Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The base class Equals(T) will ignore derived class fields. Is that expected?

For instance, the following will print False, True:

using System;

data class A(int X);

data class B(int X, int Y) : A(X);

class Program
{
    static void Main()
    {
        var b1 = new B(1, 2);
        var b2 = new B(1, 3);
        Console.WriteLine(b1.Equals(b2));
        Console.WriteLine(((A)b1).Equals(b2));
    }
}

Copy link

@theunrepentantgeek theunrepentantgeek Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will be different EqualityContract values for A & B - so even if you are calling A.Equals(), it can detect that they are not the same type, and therefore your example would write true both times.

@agocke
Copy link
Member Author

agocke commented Jun 4, 2020

Merging what I have so far, we can continue iterating

@agocke agocke merged commit 100d3f7 into dotnet:master Jun 4, 2020
@agocke agocke deleted the ff-records branch June 4, 2020 19:18
Comment on lines +27 to +28
Record types are reference types, similar to a class declaration. It is an error for a record to provide
a `record_base` `argument_list` if the `record_declaration` does not contain a `parameter_list`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agocke could you explain the rationale behind this restriction? (related discussion: #3795)

Thanks!

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

Successfully merging this pull request may close these issues.

8 participants