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

What does toString on a record do? #2389

Closed
leafpetersen opened this issue Aug 5, 2022 · 6 comments · Fixed by #2422
Closed

What does toString on a record do? #2389

leafpetersen opened this issue Aug 5, 2022 · 6 comments · Fixed by #2422
Assignees
Labels
records Issues related to records.

Comments

@leafpetersen
Copy link
Member

The record proposal says:

The toString() method's behavior is unspecified.

This is all well and good, but we have to implement something. So, what do we do here?

cc @munificent @eernstg @lrhn @jakemac53 @natebosch @stereotype441 @rakudrama @mraleph

@leafpetersen leafpetersen added the patterns Issues related to pattern matching. label Aug 5, 2022
@munificent
Copy link
Member

munificent commented Aug 5, 2022

Ugh, yeah. Honestly, I think the right thing is to print the fields and all of their names. Give the users what they want to see when they are debugging their app. We keep the shape and names around for printing function types, so it doesn't seem unreasonable to do the same thing for records.

But I know some people would really like to be able to tree-shake those names. I sympathize but... it's really painful to make debugging harder. I'm happy to defer to letting the core lib and compiler folks hash this one out. :)

@lrhn
Copy link
Member

lrhn commented Aug 8, 2022

tl;dr: The specification should not force retaining information that isn't necessary at runtime.

Records are structural data, and we can see from the program code how they are destructured.

If a compiler can see that a record type contains an element which is never used, it should be able to completely eliminate that element and never store a value.

If the toString is required to return (${this.0}, ${this.1}, foo: {$this.foo}) for (int, int, foo: int), even though that toString is the only code which ever uses the .1 value for anything, then we are preventing that optimization.

I'd at least prefer to keep the toString unspecified or under-specified, so it's possible to return (${this.0}, <optimized away>, foo: ${this.foo}) and stay within the specification.

We probably will retain enough information to know the named element names of records, in order to facilitate dynamic member access. It's OK to use that in the output, but it should also be fine if it ends up with minified member names instead of the source identifier.
As long as the toString can be computed entirely from the information in the record shape (which should only exist as one object per shape in the program) and the stored element values, we should be fine. We should not mandate what that record shape information contains at runtime, the implementations should just use what they have, and not retain anything solely for implementing toString.

We should then also document, very clearly, on Record.toString, that THE FORMAT IS UNSPECIFIED AND ANYTHING YOU SEE IS BEST EFFORT ONLY, and ONLY INTENDED FOR DEBUGGING.
Tell all code reviewers that code parsing record toString is broken and should be rejected.
Put questions on stack overflow about it, and answer them with "never do that!"

(Which also means that a result of (<optimized away>) should be completely acceptable. And that we can choose to make the toString act differently between debug and production mode.)

@mit-mit
Copy link
Member

mit-mit commented Aug 16, 2022

Could we:

  1. leave it unspecified
  2. in debug mode print (${this.0}, ${this.1}, foo: {$this.foo}) for (int, int, foo: int)
  3. in release mode print (${this.0}, ${this.1}, _: {$this.foo}) for (int, int, foo: int)

?

@munificent
Copy link
Member

@mit-mit in principle, yes, and I don't think that's necessarily a bad idea. But it's always somewhat risky to have code that behaves one way when you run your tests and another way when out in the wild. Any code that parses the result of toString() on a record will pass its tests but then fail in the shipped app. How about we:

  1. Specify that toString() prints it nicely, including the names of named fields.
  2. Also specify that an optimizing compiler may choose to eliminate the field names.

?

@natebosch
Copy link
Member

2. Also specify that an optimizing compiler may choose to eliminate the field names.

Can we also specify that it may choose to eliminate field values?

@munificent
Copy link
Member

Oof, maybe?

@munificent munificent added records Issues related to records. and removed patterns Issues related to pattern matching. labels Aug 18, 2022
@munificent munificent changed the title Records: What does toString do? What does toString do? Aug 18, 2022
@munificent munificent changed the title What does toString do? What does toString on a record do? Aug 18, 2022
munificent added a commit that referenced this issue Aug 19, 2022
- Support constant records. Fix #2337.
- Support empty and one-positional-field records. Fix #2386.
- Re-add support for positional field getters Fix #2388.
- Specify the behavior of `toString()`. Fix #2389.
- Disambiguate record types in `on` clauses. Fix #2406.
munificent added a commit that referenced this issue Aug 25, 2022
* Address a bunch of records issues.

- Support constant records. Fix #2337.
- Support empty and one-positional-field records. Fix #2386.
- Re-add support for positional field getters Fix #2388.
- Specify the behavior of `toString()`. Fix #2389.
- Disambiguate record types in `on` clauses. Fix #2406.

* Clarify the iteration order of fields in `==`.

* Copy-edit the sections on const records and canonicalization.

There should be no meaningful changes. I just:

- Fixed some misspellings.
- Used Markdown style consistent with the rest of the doc.
- Re-worded things to, I hope, read a little more naturally.
- Removed the parenthetical on identical() in a const context because
  that felt a little too academic.

* Leave the order that positional fields are checked in == unspecified.

* Clarify that positional fields are not sugar for named fields.

Specify the evaluation order of fields.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
records Issues related to records.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants