Skip to content

Disallow "Clone" members#3652

Merged
jcouv merged 2 commits intomasterfrom
dev/jcouv/disallow-clone
Jul 9, 2020
Merged

Disallow "Clone" members#3652
jcouv merged 2 commits intomasterfrom
dev/jcouv/disallow-clone

Conversation

@jcouv
Copy link
Member

@jcouv jcouv commented Jul 7, 2020

No description provided.

@jcouv jcouv self-assigned this Jul 7, 2020
@jcouv jcouv marked this pull request as ready for review July 8, 2020 07:22
an accessible concrete non-virtual member with a "matching" signature is inherited.
Two members are considered matching if they have the same
signature or would be considered "hiding" in an inheritance scenario.
It is an error for a member of a record to be named "Clone".
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we really decide to disallow all members named Clone, or should we allow methods named Clone as long as they have parameters?

Copy link
Member Author

Choose a reason for hiding this comment

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

Notes: https://github.com/dotnet/csharplang/blob/master/meetings/2020/LDM-2020-07-01.md#Confirming-unspeakable-Clone-method-implications

We explicitly discussed methods and fields in LDM ("Clone" fields should be disallowed). For nested types, I chatted with Jared offline and I think we'll want the same behavior.

Copy link
Member

@333fred 333fred Jul 9, 2020

Choose a reason for hiding this comment

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

I don't believe that particular variant of the question was raised in LDM. Personally, I'd prefer to disallow all members (named Clone), just in case we end up wanting to add some kind of parameter list to possibly support only cloning if an initer will actually change a field, but it's a followup we could raise with the LDM if we want to clarify.

@HaloFour
Copy link
Contributor

HaloFour commented Jul 8, 2020

Out of curiosity is there a reason to disallow Clone methods? Aren't records relying on an unspeakable name to clone the instance? Is that changing or is this to prevent confusion or collision with a more general-purpose clone/wither solution intended for non-records?

@jcouv
Copy link
Member Author

jcouv commented Jul 8, 2020

@HaloFour We're reserving it as we may allow customization of the clone method, or implementation on non-record types in the future. Reserving that name keeps some options open. Notes: https://github.com/dotnet/csharplang/blob/master/meetings/2020/LDM-2020-07-01.md#Confirming-unspeakable-Clone-method-implications

@HaloFour
Copy link
Contributor

HaloFour commented Jul 8, 2020

ah ok, gotcha. I assume that this means that a record cannot implement ICloneable unless they do so explicitly?

@jcouv
Copy link
Member Author

jcouv commented Jul 8, 2020

Correct. That's a good test scenario (I'll add to my PR).

@jcouv jcouv merged commit 78a7c37 into master Jul 9, 2020
@jcouv jcouv deleted the dev/jcouv/disallow-clone branch July 9, 2020 17:11
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.

4 participants