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

Record toString: useful, useless, or thrown(in implicit conversions) ? #136

Closed
ljharb opened this issue Jun 28, 2020 · 36 comments · Fixed by #354
Closed

Record toString: useful, useless, or thrown(in implicit conversions) ? #136

ljharb opened this issue Jun 28, 2020 · 36 comments · Fixed by #354

Comments

@ljharb
Copy link
Member

ljharb commented Jun 28, 2020

per #135 (comment)

At the very least, I'd expect Records to have a Symbol.toStringTag of "Record", which would Object.prototype.toString.call(record) produce [object Record].

However, String(record), `${record}`, etc, according to #135, will produce "[record]". This doesn't seem particularly useful at all; if someone wants to know it's a record, they'll typeof it.

Objects have always had a useless toString, but since everything inherits from Object, it's a tough sell to come up with something broadly useful for it to do. Arrays' toString has problems, and could be much better if legacy didn't hold it back, but is still useful since it stringifies its contents. I would hope that Records can have a better user story around stringification than objects.

@rickbutton
Copy link
Member

I don't have a strong opinion on what String(record) should output, I think that a static string or a JSON-ified version seem reasonable to me.

On Symbol.toStringTag, If the prototype is null, then that won't be possible.

@ljharb
Copy link
Member Author

ljharb commented Jun 29, 2020

It would be possible if all records had an own Symbol.toStringTag property.

@littledan
Copy link
Member

About @ljharb 's "at the very least" suggestion, I think it would be a reasonable starting point to make ToString(record) === Object.prototype.toString.call(record) === "[object Record]". I thought that might be confusing, since it's not an object, but probably JS programmers understand this more as a fixed pattern since Object.prototype.toString uses that for everything. We could do this even without making an own Symbol.toStringTag property, since Record wrappers could be checked for explicitly as other types are in lines 5-13 of Object.prototype.toString. It wouldn't be so crazy to make an own Symbol.toStringTag property, following module namespace exotic objects, but I also don't see the motivation.

On the question of whether ToString on Records and Tuples should be useful: I've sat on this some more, and I definitely see the developer experience benefit--that's the goal here, really. Ideally, we'd get another operation in JavaScript that's more like repr than str, but, well, we're working with what we have. So, if we wanted to make ToString on Records and Tuples useful, I'd suggest the following starting point for algorithm:

  • There's an enclosing #{/#[, a space after , and :, and double quotes around record keys
  • It's fully recursive, calling ToString on components until you get down to nothing
  • Symbols are allowed to be converted to strings when nested in Records and Tuples too

Thoughts? How should we make the consistency vs utility tradeoff? I was initially leaning towards "consistency" but now I feel myself leaning more towards "utility".

@ljharb
Copy link
Member Author

ljharb commented Jul 6, 2020

Your suggestion seems like the best mix of both:

  1. Object.prototype.toString explicitly returns [object Record] when given a Record primitive
  2. I think there should be an own Symbol.toStringTag property (altho a Record.prototype would be a much more sensible place for this) because Object.prototype.toString.call(Object(record)) === Object.prototype.toString.call(record) should be true, like it is for every other non-nullish primitive
  3. Stringifying records with String(record) or the equivalent should do what everyone will expect anyways, your suggestion :-)

@littledan
Copy link
Member

littledan commented Jul 6, 2020

because Object.prototype.toString.call(Object(record)) === Object.prototype.toString.call(record) should be true

I agree this is important to preserve. I'd suggest that the explicit case be for the [[RecordData]] internal slot, not for being a Record primitive. I think this will Just Work (TM) since Object.prototype.toString calls ToObject in step 3. So, I still think the Symbol.toStringTag is not necessary.

@rricard rricard added this to the stage 3 milestone Jul 7, 2020
@rickbutton
Copy link
Member

rickbutton commented Jul 7, 2020

There's an enclosing #{/#[, a space after , and :, and double quotes around record keys

+1, I was initially thinking that it would output JSON.parse-able {} but including the hash makes it more obvious that it is the "string representation" and not "the json version".

This would also generate a single line of "string representation". Does a better default format make sense, i.e. newlines and indenting object keys?

@littledan
Copy link
Member

I want to suggest we start by keeping it simple; prettyprinting gets very complicated to do well. Also note that my algorithm would still leave off the n on BigInts. I would leave fancier printing to DevTools or analogous libraries; this is just a default starting point for visibility.

@celmaun
Copy link

celmaun commented Sep 24, 2021

let obj = { __proto__: null, foo: 'bar' };
let rec = #{ foo: 'bar' };

assert(obj.__proto__ === null)
assert(rec.__proto__ === null);

String(obj) -> Uncaught TypeError: can't convert obj to string

therefore it makes sense that

String(rec) -> Uncaught TypeError: can't convert rec to string

It's a really annoying quirk of JS that there's random toString implementations alll over the place. Let's avoid making this mistake for future additions.

@ljharb
Copy link
Member Author

ljharb commented Sep 24, 2021

@Salmatron "annoying" is subjective, and I don't think that's universally agreed. It's actually quite annoying to me that null objects throw in a string context - to me, that is the mistake.

@celmaun
Copy link

celmaun commented Sep 24, 2021

Subjective, yes. I am of the opinion that developer mistakes should be pointed out early rather than swept under the rug. All objects without an explicit toString() implementation should throw IMO. How is '[object Object]' useful in any way? All that does is mask actual bugs. Putting a non-toString() type into a string context is a mistake that should be caught. (and Symbol.toStringTag should be read the proper way).

Like let obj2 = {}; obj2[obj] = 123 -> { '[object Object]': 123 } // <- Novices would appreciate being pointed to ES Map instead of being confused

Then there's array.sort() which casts everything to string. If Record has an expensive toString() method that dumps some JSON-like representation, that's gonna make things crawl on arrays with large data structures.

@mhofman
Copy link
Member

mhofman commented Sep 30, 2021

What would the behavior of String(box) be? Following the current path, I guess it could result in:

`Box(${String(content)})`

What if the object contained in a Box has a custom toString which attempts to call String(containingBox) again, aka creates a cycle (possibly through a record or tuple.

const obj = {
  toString() {
    return `{${Object.entries.map(([key, value]) => `${key}: ${String(value)}`).join(', ')}}`;
  }
};
obj.box = Box(obj);

String(obj); // ?

@ljharb
Copy link
Member Author

ljharb commented Sep 30, 2021

Same thing a toString would do that has an infinite loop - halt the program.

@acutmore
Copy link
Collaborator

acutmore commented Sep 30, 2021

What would the behavior of String(box) be?

If we want to constrain access to unbox then potentially no other built-in should use that operation. JSON.stringify with a custom replacer would effectively be another way to unbox. So maybe for consistency String also shouldn’t be an implicit call to unbox? Though it does not appear to be the same level of risk if it did.

@mhofman
Copy link
Member

mhofman commented Sep 30, 2021

If we want to constrain access to unbox then potentially no other built-in should use that operation.

That's a very good point I totally forgot about, and probably the biggest drawback to allowing virtualization through Box.unbox instead of leaving it on the prototype.

@rricard rricard changed the title Record toString: useful or useless? Record toString: useful, useless or thrown? Jul 8, 2022
@rricard rricard changed the title Record toString: useful, useless or thrown? Record toString: useful, useless or thrown(in implicit conversions) ? Jul 8, 2022
@rricard
Copy link
Member

rricard commented Jul 8, 2022

We are considering the option of throwing here for implicit conversions. This would have the effect of solving #289 by throwing on operators.

If passing explicitely to the String constructor, we would prefer a useless string as anyone passing to that constructor could also pass it to JSON.stringify where they would have more control over the conversion.

What do you think @ljharb?

@ljharb
Copy link
Member Author

ljharb commented Jul 8, 2022

I would hope that Records can have a better user story around stringification than objects.

Why wouldn’t toString just defer to JSON.stringify, if toJSON ensures that JSON.stringify prints something useful?

@acutmore
Copy link
Collaborator

acutmore commented Jul 8, 2022

Why wouldn’t toString just defer to JSON.stringify, if toJSON ensures that JSON.stringify prints something useful?

Tuples follow Arrays behavior for toString and JSON.stringify (following the general design theme of basing Tuple methods on Arrays).

String( [1,  [ [ [2]]], 3]); // '1,2,3'
String(#[1, #[#[#[2]]], 3]); // '1,2,3'
JSON.stringify( [1,  [ [ [2]]], 3]); // '[1,[[[2]]],3]'
JSON.stringify(#[1, #[#[#[2]]], 3]); // '[1,[[[2]]],3]'

It is potentially odd that a Tuple would serialize differently when passed to String directly compared to when its nested in a Record.

String(#[1, #[#{ t: 2 }]]); // `1,{"t": 2}`
String(#{ t: #[#[2]] }); // `{"t":[[2]]}`

@ljharb
Copy link
Member Author

ljharb commented Jul 8, 2022

The usefulness is for debugging, and i don't think list-likes and dictionary-likes need to be consistent with each other.

@acutmore
Copy link
Collaborator

Summary post for reference


Context

Without explicit handling either by the application, or in the spec itself, treating a record as a string can't produce a useful result - in fact right now implicit conversion will throw an error. This is because Records do not have any of the methods that are used in the string conversion routine: toString, valueOf, and Symbol.toPrimitive and they can't do because they do not have a prototype to put these on.

const rec = #{ prop: 42 };
rec.toString === undefined;
Object.prototype.toString.call(rec); // `[object Record]`
String(rec); // [object Record]
JSON.stringify(rec); // `{"prop":42}`

const msg = `my record is: ${rec}`; // throws TypeError
// instead of `my record is: [object Record]`

Returning "[object Record]" is consistent with how objects behave by default in JavaScript. If objects want a custom string representation they need to explicitly implement toString.

const obj = { prop: 42 };
const set = new Set([1, 2, 3]);

console.log(`obj: ${obj} set: ${set}`); // "obj: [object Object] set: [object Set]"

For this reason, when logging, it can be preferable to leave the stringification of values to the platform/library rather than performed at the individual call sites:

$ node -e "console.log('logging:', { prop: 42 })"
logging: { prop: 42 }

We expect a similar expierence for Records, so when passed directly to debugging mechanisms a rich representation is produced.

Throwing

We hypothesize that implicit conversion of a record to a string is a programming error based on the fact that the string would display no 'helpful' information about the record's contents. Similar to when someone accidentally gets "[object Object]". This is why it throws an error, to help catch this.

There is some precedent for throwing on implicit string conversion of a primitive as this is what happens with symbol.

If we imagine an application that captures logs during execution, perhaps it is a webserver. An issue is reported, when the developers go to check the logs around the time of the issue they see: 2022-07-12T08:09:38.747Z I: data: [object Record] and only then realise that they have not been logging useful information. Instead what will happen is they would have immediately had an error produced pointing out the accidental implicit string conversion.

A counter-argument to this is if the application only logs when there is an error, the log is in a catch block for example. In this situation the accidental implicit string conversion could go unnoticed until the error happens. The implicit string conversion error could then mask the original error. Note: This is a problem in general with catch blocks, the catch handler should ideally be resilient to this category of problem.

A safer catch block
function safeHandler(originalError, handler) {
    try {
        return handler(originalError);
    } catch (handlerError) {
        if (handlerError === originalError) {
            throw originalError;
        } else {
            throw new AggregateError([originalError, handlerError]);
        }
    }
}

try {
    ...
} catch (err) {
    // the safeHandler ensures we don't lose `err` when our logging goes wrong
    safeHandler(err, () => {
        // whoops, this will throw when the record is converted to a string
        console.log(`error during processing of record: ${record}`);
    });
}

A useful string

Instead of producing "[object Record]" the spec could include logic to produce a detailed string based on the contents of the record.

// Hypothetical:
const rec = #{ foo: #{bar: 42} };
`rec is ${rec}`; // `rec is #{"foo": #{"bar": 42}}`

This is similar to languages like Python and Java

from collections import namedtuple

Point = namedtuple('Point', 'x y')
pt1 = Point(1.0, 5.0)

print(f'pt1 is: {pt1}') # "pt1 is: Point(x=1.0, y=5.0)"

# String interpolation is different to string concatenation in Python:
'pt1 is: ' + pt1 # TypeError: can only concatenate str (not "Point") to str
public class Main {
  private record Point(float x, float y) { }

  public static void main(String args[]) {
    Point p = new Point(1.0f, 5.0f);

    String message = String.format("p is %s", p);
    System.out.println(message); // "p is Point[x=1.0, y=5.0]"
  }
}

The easiest (least amount of spec text) would be to reuse JSON.stringify. However this has downsides, not all primitives can be directly reproduced in JSON, specifically undefined, symbol, bigint, NaN. These values are either converted to null, skipped or throw. While these could be worked around, e.g. displaying bigint as a string this could cause confusion and/or errors where someone is unaware of these silent conversions and is using the output as a JSON parsable input to something else. It would be clearer if the string was immediately clear that it is not JSON so that it is very unlikely to be mistaken for JSON. This can be achieved by retaining the #{ and #[ opening brackets or records and tuples.

// Hypothetical JSON-inspired but bespoke string representation, that recursively stringifies its elements
const rec = #{ foo: #[undefined, NaN, Symbol("s"), 1n] };
`rec is ${rec}`; // `rec is #{"foo": #[undefined, NaN, Symbol(s), 1]}`

The downsides to this are:

  • No reliable built-in way to parse this back to the original record.
    • e.g. The string of a bigint does not include the n suffix and thus would be indistinguishable from a number
    • Identity would not always be preserved (if there are symbols), or records containing symbols could continue throwing because symbols throw when implicitly stringified
  • Code may couple itself to this string format
    • this prevents it from evolving
  • Large subjective design space (should it have indentation? trailing commas? only keys and no values?).
  • The only limit on how large this string could become is the string limits themselves
    • Unless an arbitrary limit (e.g. on recursion depth) is also part of the spec
  • Not possible to customise when defining the record.
    • in Python and Java there are __str__ and toString methods that can be implemented
  • Not possible to configure globally.
    • e.g. Record.setStringFormatting(indentation) would introduce global state, clashes and a communications channel
  • Tuples inside Records get special treatment that differs from how they would usually toString
    • Tuples follow Array behavior of using join(",") when converted to a string
    • Though this is the same as explicit JSON.stringify(arr) vs arr.toString()

Conclusion

While producing a "useful" string was an interesting thought experiment, the champion group's position is to retain consistency with default object behavior of returning the static string "[object Record]" for explicit conversion, and to help avoid mistakes by throwing for implicit conversion. Leaving the stringification to userland provides more flexibility and time&space to evolve.

@ljharb
Copy link
Member Author

ljharb commented Jul 14, 2022

I don't think that consistency should be sought. If Objects hadn't made the mistake of being everything's base class, I think Object.prototype.toString would do exactly what we're asking for here. Object is special; things shouldn't strive to be consistent with it.

@acutmore
Copy link
Collaborator

I think this precedent is more recent than Object.prototype.toString. There could have been a Map.prototype.toString which produced a detailed representation similar to Date.prototype.toString. I’m not sure if that was discussed during ES6.

@ljharb
Copy link
Member Author

ljharb commented Jul 14, 2022

Map's purpose is to have object keys, and objects don't have a useful string serialization for the reasons discussed, so I think it's a consequence of the same precedent.

Records can't have objects, so they're free of that constraint.

@rricard rricard mentioned this issue Jul 25, 2022
25 tasks
@rkirsling
Copy link
Member

rkirsling commented Aug 10, 2022

Thinking purely as a user:

  1. Records are basically just Tuples where the keys can be arbitrary strings instead of integer indices.
    In particular, we know that keys won't be Symbols and values won't be Objects, so that screams "easy to stringify!"
  2. Tuple stringification throws on Symbol values while happily dealing with all other primitives.
    Since the value space for Records is exactly the same, I am led to expect that Record stringification will do similarly.
  3. However, we immediately have two problems due to Tuple's hard-alignment with Array: #[undefined,null,3] stringifies as ,,3, yet #{ x: undefined, y: null, z: 3 } cannot reasonably omit braces or use '' for undefined and null.

As such, I believe it is inappropriate to view this lopsidedly as an issue with Record and not with Tuple.

  • If Tuple hard-aligns with Array, then Record probably should hard-align with Object.
    (This would at least avoid the nasty situation where a Record value prevents a Tuple from being stringified.)
  • If we want Records to stringify nicely (which I do believe users would prefer!), then Tuple should not simply defer to Array.prototype.join. We should be designing a coherent solution for this proposal as a whole.

@acutmore
Copy link
Collaborator

Thanks @rkirsling. How much of a risk do you think there would be if Tuple.prototype.toString diverged from Array.prototype.toString? For example when refactoring some code from arrays to tuples.

@ljharb
Copy link
Member Author

ljharb commented Aug 11, 2022

I would hope nobody is relying on array toString when join exists - and i doubt that anyone doing such a refactor would miss the difference.

@rkirsling
Copy link
Member

I agree with what @ljharb said, but moreover, I feel like the only reason we'd be aligning with Array to begin with is because its behavior is "just good enough" to accept. But it too has clear room for improvement, and if Record and Tuple are in clear alignment on a "better" solution, I think the divergence from Array would be enthusiastically forgiven.

@kninnug
Copy link

kninnug commented Aug 19, 2022

I agree that returning "[object Record]" is not useful and that throwing on implicit conversion to string would be better, so that mistakes can be found early.

However, in the interest of developer ergonomics, I suggest a new method: Record.stringify(value[, replacer[, options]]). It is not called toString to signal that this method is not called on implicit conversion, but explicitly by the developer.

While it doesn't avoid all of the downsides in acutmore's comment, since it's not called implicitly these are IMHO less dangerous.

This also opens up the opportunity for adding a Record.parse method if/when a reliable format can be determined. Until such time, the format produced by Record.stringify should be restricted enough to allow for reliable parsing in the future. Unless it can be fully worked out in this proposal. Alternatively, it could be decided right now that the format produced by Record.stringify is never meant to be parsable, and have it be (largely) implementation-defined. In that case, it should probably have a different name in order to avoid expected symmetry JSON.stringify & JSON.parse.

Naturally, I would make the same suggestion for a Tuple.stringify.

In any case, having a built-in way to represent Records (and Tuples) as strings is more useful than consistency in the form of "[object Record]". But maybe it doesn't have to be by way of an implicitly called toString.

@littledan
Copy link
Member

I do see some value in having a nicer toString behavior for Records and Tuples, but it becomes a bit hard to limit the scope of such a project. In prettyprinters for data structures in several languages, there are mechanisms to limit the depth and the length traversed, as well as possibly control whitespace. This is exactly what console.log gives you. Here, we need something much more simple and minimal, but a useful toString for records opens up exactly this can of worms.

@acutmore
Copy link
Collaborator

acutmore commented Sep 5, 2022

PR #354 opened so implicit toString conversion of a Record/Tuple to a string gives a useful string representation.

One small thing we would lose with this is that it's no longer an error if someone attempts to use a Record as an object property key. There is the potential to add a check to toPropertyKey that would throw if the type is record or tuple. So that would be one place where implicit conversion would still not be allowed. What do people think?

@rkirsling
Copy link
Member

I think a ToPropertyKey check would be reasonable; since my overarching feeling is that Record and Tuple should have similar behavior, it would also make sense not to have a tuple property key just work.

@ljharb ljharb changed the title Record toString: useful, useless or thrown(in implicit conversions) ? Record toString: useful, useless, or thrown(in implicit conversions) ? Sep 14, 2022
@bathos
Copy link
Contributor

bathos commented Oct 26, 2022

I was pretty surprised by the BigInt behavior. I shouldn’t have been — it does make sense — but somehow a ToString behavior I already knew to expect for BigInts on their lonesome seemed like a bug in the context of a serialization which (having no symbol values) would have roundtripped with one more character. It likewise surprised me that symbol members at any depth don’t just cause ToString to throw.

An implicit ToString behavior introduced solely(?) as a debugging aid (vs one which incidentally happens to serve as one sometimes) seems pretty exotic to me. ToPrimitive/ToString are part of so many regular runtime contracts that aren’t related to debugging or introspection. I supposed I’d have imagined the “if we could go back...” story for O.p.toString would be to not have it in the first place rather than to deck it out.

I may have missed some of the motivations here & these aren’t super carefully considered opinions or anything: this is just “field report” feedback based on exploring the proposal from the trying-to-use-it end. However if other folks do feel doubts about the currently specified behavior, I would suggest unilateral throwing behavior could be a reasonable MVP approach with the expectation that serialization be addressed in a ride-on proposal. My suspicion is that getting the full what-do-people-who-are-now-using-this-for-real-need-here feedback loop running before answering this question might produce different conclusions (perhaps different from anything which was discussed so far).

@acutmore
Copy link
Collaborator

acutmore commented Oct 26, 2022

Hi @bathos, thanks for the feedback.

When the change was presented at the september plenary this year (slide), one thing we said is that we are purposefully not attempting to create an ad-hoc serialization format and that providing the detailed string is to help DX when the values are implicitly converted to strings. Previously we did throw an error, but the feedback we got was that this would not provide a good DX.

As you say we could have special handling for bigint but symbols would still not work if people are intending a round-trip. If people do require serialization in the future this seems better left to a dedicated API rather than the String constructor or toString.

@bathos
Copy link
Contributor

bathos commented Oct 26, 2022

@acutmore I wouldn’t want an ad-hoc serialization format, either. That’s how I’d describe what it is producing, though.

@acutmore
Copy link
Collaborator

Interesting. The reason we include the # in the string is that things like JSON.parse would never work on the output. I guess someone could still use eval or new Function to parse the string we currently produce.

Do you think there are other ways we could produce a detailed string while making it as clear as possible that it is not intended to be a serialization format?

@bathos
Copy link
Contributor

bathos commented Oct 26, 2022

Do you think there are other ways we could produce a detailed string while making it as clear as possible that it is not intended to be a serialization format?

Perhaps repurposing the bracketed O.p.toString “notation” to wrap the description “format”, like [record #{ ... }]? In addition to ensuring it never evaluates back to the record, it would maybe be just familiar enough to get the “oh ... got it” across to someone encountering it for the first time.

@ljharb
Copy link
Member Author

ljharb commented Oct 26, 2022

I don't think that's a reasonable restriction to have - functions' toString doesn't do that, for example.

Just because we're not trying to provide a serialization mechanism doesn't mean that having one by accident is bad, or needs preventing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants