Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Bring records proposal up to date #3527
Changes from 9 commits
5dcd56b
1b2be19
3a97005
bdbfc8d
fd5e558
c5888c5
470f754
e3d2525
61eeae6
53f957b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec doesn't have a definition of signature for a property.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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
:There was a problem hiding this comment.
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 forA
&B
- so even if you are callingA.Equals()
, it can detect that they are not the same type, and therefore your example would write true both times.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider clarifying this, obviously all fields within a record are accessible.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't base take care of this?
In reply to: 434061428 [](ancestors = 434061428)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear: I assume it is intentional that fields of the base are not copied. This will be extremely confusing to users.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
synthesized?
There was a problem hiding this comment.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But virtual (non-abstract) ones are not overridden?
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.