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

Add ToString and PrintMembers to records #3762

Merged
merged 10 commits into from
Aug 7, 2020
Merged

Add ToString and PrintMembers to records #3762

merged 10 commits into from
Aug 7, 2020

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Jul 31, 2020

@dotnet/roslyn-compiler for review. This is a proposal for shape of ToString synthesized members.

@jcouv jcouv self-assigned this Jul 31, 2020
}
}
```

Copy link
Member

Choose a reason for hiding this comment

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

Extra newline.

It is an error if the method is declared explicitly.

The method:
1. appends a separator ", " to `builder` if `includeSeparator` is true and the record has public fields or properties,
Copy link
Member

Choose a reason for hiding this comment

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

this feels unneeded. why not just always include a trailing comma. then you have the consistent, copyable:

Person
{
  Name = "Julien",
  Age = 99,
}

@CyrusNajmabadi
Copy link
Member

proposals/csharp-9.0/records.md Outdated Show resolved Hide resolved
proposals/csharp-9.0/records.md Outdated Show resolved Hide resolved
proposals/csharp-9.0/records.md Outdated Show resolved Hide resolved
proposals/csharp-9.0/records.md Outdated Show resolved Hide resolved
proposals/csharp-9.0/records.md Outdated Show resolved Hide resolved

The record includes a synthesized method equivalent to a method declared as follows:
```C#
public override string ToString();
```

The method can be declared explicitly. It is an error if the explicit declaration does not match the expected signature or accessibility, or if the explicit declaration doesn't allow overiding it in a derived type and the record type is not `sealed`. It is an error if the method doesn't override a method with this signature in the record `Base` (for example, if the method is missing in the `Base`, or sealed, or not virtual, etc.).
The method can be declared explicitly. It is an error if the explicit declaration does not match the expected signature or accessibility, or if the explicit declaration doesn't allow overiding it in a derived type and the record type is not `sealed`. It is an error if either synthesized, or explicitly declared method doesn't override `object.ToString()` (for example, due to shadowing in intermediate base types, etc.).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The method can be declared explicitly. It is an error if the explicit declaration does not match the expected signature or accessibility, or if the explicit declaration doesn't allow overiding it in a derived type and the record type is not `sealed`. It is an error if either synthesized, or explicitly declared method doesn't override `object.ToString()` (for example, due to shadowing in intermediate base types, etc.).
The method can be declared explicitly. It is an error if the explicit declaration does not match the expected signature or accessibility, or if the explicit declaration doesn't allow overiding it in a derived type and the record type is not `sealed`. It is an error if either synthesized or explicitly declared method doesn't override `object.ToString()` (for example, due to shadowing in intermediate base types, etc.).

@jcouv
Copy link
Member Author

jcouv commented Aug 6, 2020

FYI, I'm going to revert to a speakable print method. Unspeakable print methods would throw a big wrench in our ability to derive records from classes. That seems unnecessary. I'll update the spec this afternoon.

Tagging @dotnet/roslyn-compiler

@333fred
Copy link
Member

333fred commented Aug 6, 2020

FYI, I'm going to revert to a speakable print method. Unspeakable print methods would throw a big wrench in our ability to derive records from classes. That seems unnecessary. I'll update the spec this afternoon.

That's a very good point, thanks Julien.

@jcouv
Copy link
Member Author

jcouv commented Aug 7, 2020

I'll merge this spec change shortly. The unresolved follow-up questions were added to the records test plan. Thanks for the review

  • ToString: should we always print a trailing comma R { Property1 = 42, Property2 = 43, }?
  • ToString: can we deal with stack overflow potential?
  • ToString: should we try to quote/escape values, for example R { IntProperty = 42, StringProperty = "hello" }?
  • ToString: should we omit ToString on abstract records?

@jcouv jcouv merged commit 7d3c77e into master Aug 7, 2020
@jcouv jcouv deleted the dev/jcouv/tostring branch August 7, 2020 17:38
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.