search

whatwg / console

Console Standard
https://console.spec.whatwg.org/
Other
272 stars 69 forks source link

Handling of Formatter exceptions #113

Closed terinjokes closed 6 years ago

terinjokes commented 7 years ago

Some of the algorithms defined in the Formatter section can throw exceptions, depending on the input. This specification doesn't state how to handle these exceptions.

console.log("symbol: %s", Symbol.for("foo"));

In the Formatter algorithm we define how to handle the %s formatter:

If specifier is %s, let converted be the result of ToString(current).

The ToString documentation from ECMA-262 states that for Symbol input, a TypeError should be thrown. I would expect, based on the text, that this TypeError is thrown from the Console methods.

Implementations differ in their behavior here, though they do not seem throw the TypeError:

domfarolino commented 6 years ago

This seemed closely related to https://github.com/whatwg/console/issues/26 and https://github.com/whatwg/console/issues/88 regarding ToString conversion stuff. Therefore I felt like implementations might start throwing here just as a result of fixing some of the previous issues (like re-throwing other ToString conversions https://bugzilla.mozilla.org/show_bug.cgi?id=1346336), however latest FF does not throw in your example (or any ToString failure with the %s specifier).

Given the decisions we've previously made (to re throw other ToString errors) I think we should add spec text to re-throw the error even though no implementations do that now. If agreed, I can add a PR for this and file some more browser issues.

Edit

I've pinged the following chrome issue here to ask whether this behavior could be fixed in Chrome by fixing one of the old issues too, though I suspect the answer is no.

domenic commented 6 years ago

Although I agree with the general principle, I think in the particular case here, throwing for all symbols would be bad. Instead we might want to special case them.

JosephPecoraro commented 6 years ago

Given the decisions we've previously made (to re throw other ToString errors) I think we should add spec text to re-throw the error even though no implementations do that now.

Can you point me to that discussion?

WebKit (and Blink as I understand it) don't currently evaluate the format string ("Formatter") operations in the page's context. Instead each argument is captured and serialized to a debugger. The debugger then applies the format string operations as needed.

One advantage to this approach is that console operations when there is no debugger opened can do very little work (just hold on to the values). Another advantage is that there are no exceptions using console.log, so developer feel free to sprinkle them around and not break program control flow.

Also, changing console.log behavior to now throw exceptions has the potential to break existing web pages with log statements that wouldn't have thrown before. Is that a concern?

domfarolino commented 6 years ago

Can you point me to that discussion?

I had a couple previous threads regarding IDL validation in mind (https://github.com/whatwg/console/issues/26, and https://github.com/whatwg/console/issues/88) and this thread/comment (https://github.com/whatwg/console/issues/54#issuecomment-216631477) (though that last one isn't totally specific to ToString() conversions it seems, as I skimmed).

That makes sense, and yes I think that it is a concern, but I think an observable difference for console methods between the developer tools being open and closed seems a little messy.

Edit:

Regardless, I think my comment is a little obsolete since we've opted not to throw when the %s format specifier is applied to Symbols.

domenic commented 6 years ago

Note that Edge does throw in these cases, so the compat risk should be at least somewhat mitigated.

domfarolino commented 6 years ago

@JosephPecoraro it seems we've decided not to throw when the %s specifier is applied to Symbols (https://github.com/whatwg/console/pull/123 + https://github.com/w3c/web-platform-tests/pull/9008), however as the spec currently stands, we do throw errors when we apply the %d, %f, and %i specifiers. So far neither Chrome, Firefox, nor Safari throw in these case. Chrome and Safari print NaN, and Firefox does absolutely nothing. Not sure about Edge.

If we decide to keep this behavior (and merge https://github.com/w3c/web-platform-tests/pull/9488), of course Console methods would be able to throw in more cases, and as I've previously mentioned I definitely think we should not have observable differences depending on whether the DevTools are open or closed (like throwing errors or not). The fact that some Console implementations don't evaluate the format string when the DevTools is closed definitely makes sense for speed and what not, but I think having no observable differences is important.

Alternatively, for the %d, %f, and %i specifiers we could do a Type check before calling parseInt (and similar algorithms) to see if we're handling a Symbol, and opt to Print NaN in this case instead of continuing with parseInt/Float (eventually throwing). That matches the behavior of Chrome and Safari and of course has the added benefit of not breaking already-existing things.

domfarolino commented 6 years ago

Moving forward I think the best path would be to not throw, and instead show NaN. As mentioned, this:

Looping in @bgrins and @nchevobbe for Firefox here to get their thoughts on this proposed path, as well as @xirzec for Edge.

I can update the WPTs for this (https://github.com/w3c/web-platform-tests/pull/9488) to reflect this proposed path if it is agreed upon.

Bugs for tracking:

xirzec commented 6 years ago

@thelarkinn looks like the bug is already on Yong, but FYI in case it impacts your spec.

On the Edge DevTools team, we've been looking into what we want to do with console when the tools are not open. Today due to behavior gaps you can tell if the tools are open or not by interrogating the Console object, which is not great. Furthermore, there are unfortunate differences in behavior when you do open the tools, such as the serialization being different for messages logged after opening.

The tl;dr answer is we're looking at aligning with a model closer to what Chrome/Blink does, namely storing things as-is and letting the console UI sort it out when it opens. So I think that in this case (format specifiers triggering errors only when they are realized in the UI) we would be moving towards a model that would not throw, since we wouldn't be interpreting it synchronously anymore.

nchevobbe commented 6 years ago

Moving forward I think the best path would be to not throw, and instead show NaN

So this is only for %d, %f, and %i specifiers right ? I agree this would be the right move.

For %s, given console.log("symbol: %s", Symbol.for("foo")); what should be printed ? It seems that there is a decision to not throw (which I agree on, to not break existing usages), but I can't find a decision on what to actually print ?

domfarolino commented 6 years ago

Correct, this is only for those specifiers. The decision for what to display when applying %s on Symbols can be found here: https://github.com/whatwg/console/pull/123. We opted to use the String() constructor for the %s specifier, which delegates to SymbolDescriptiveString for Symbols.