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 2 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.
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.
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.
How do we handle inherited properties that are inaccessible, or
get
orset
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?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.
Lemme know if this needs more specification but hopefully the "matching" rule resolves everything.
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 if no arguments were provided?
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.
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.
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 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)
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.
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.