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

console.assert(false, Symbol()) throws #49680

Closed
jcbhmr opened this issue Sep 17, 2023 · 7 comments · Fixed by #49722
Closed

console.assert(false, Symbol()) throws #49680

jcbhmr opened this issue Sep 17, 2023 · 7 comments · Fixed by #49722
Labels
console Issues and PRs related to the console subsystem.

Comments

@jcbhmr
Copy link
Contributor

jcbhmr commented Sep 17, 2023

Version

v20.6.1

Platform

Linux PIG-2016 5.15.90.1-microsoft-standard-WSL2 #1 SMP Fri Jan 27 02:56:13 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

console

What steps will reproduce the bug?

node -e 'console.assert(false, Symbol())'

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior? Why is that the expected behavior?

image

What do you see instead?

internal/console/constructor.js:401
      args[0] = `Assertion failed${args.length === 0 ? '' : `: ${args[0]}`}`;
                                                                     ^

TypeError: Cannot convert a Symbol value to a string
    at console.assert (internal/console/constructor.js:401:70)
    at Object.<anonymous> (/home/cg/root/6509ba68c2e82/main.js:1:9)
    at Module._compile (internal/modules/cjs/loader.js:999:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
    at Module.load (internal/modules/cjs/loader.js:863:32)
    at Function.Module._load (internal/modules/cjs/loader.js:708:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:60:12)
    at internal/main/run_main_module.js:17:47

Additional information

No response

@bnoordhuis
Copy link
Member

Working as expected/documented. Try this instead:

console.assert(false, "%o", { hello: "world" })
// or even just:
console.assert(false, "", { hello: "world" })

It wouldn't be hard to use util.inspect() by default but:

  1. it's the documented behavior
  2. is mandated by the console spec
  3. would be a rather visible change (can spam large amounts of text to the console) and therefore at least a semver-major change

You're welcome to open a pull request and see how it's received if you still think it's a good idea but be prepared for rejection.

@bnoordhuis bnoordhuis closed this as not planned Won't fix, can't repro, duplicate, stale Sep 17, 2023
@bnoordhuis bnoordhuis added the console Issues and PRs related to the console subsystem. label Sep 17, 2023
@jcbhmr

This comment was marked as outdated.

@bnoordhuis
Copy link
Member

Yes, I'm pretty sure. The more human-readable prose from the MDN lemma (emphasis mine):

A list of JavaScript objects to output. The string representations of each of these objects are appended together in the order listed and output.

It's only when you use a message format string that object formatting comes into play:

JavaScript objects with which to replace substitution strings within msg.

@jcbhmr
Copy link
Contributor Author

jcbhmr commented Sep 19, 2023

After further investigation, here's the erroneous line:

args[0] = `Assertion failed${args.length === 0 ? '' : `: ${args[0]}`}`;

👆 this isn't precisely spec compliant. Why? Because there's supposed to be this:
image

which means something more like this:

  assert(expression, ...args) {
    if (!expression) {
      if (typeof args[0] === "string") {
        // if you provide a fmt string, it still gets used (first arg)
        args[0] = `Assertion failed: ${args[0]}`
      } else {
        // but if it's anything else, it's not stringified as a format string;
        // its just an object to be printed. so we prepend a fmt string arg (with no specifiers
        // that would eat any user-supplied objects) as a nice "Assertion failed" message
        ArrayPrototypeUnshift(args, "Assertion failed")
      }
      // The arguments will be formatted in warn() again
      ReflectApply(this.warn, this, args);
    }
  },

you can see a bug here from the current Node.js impl that highlights what's happening:

const mySymbol = Symbol()
console.assert(false, mySymbol)
// THROWS because Symbol() cant be stringified like `symbol string: ${Symbol()}`!

the mdn thing you cited:

A list of JavaScript objects to output. The string representations of each of these objects are appended together in the order listed and output.

this same language "string representations" is also used in console.info() and other console methods to mean the formatted pretty output not the .toString() version:
https://developer.mozilla.org/en-US/docs/Web/API/console/info
image

also backed up by someone from the whatwg/console repo:

My understanding of the spec is the object {hello: "world"} would get passed as a member of args (eg, ["Assertion failed", {hello: "world"}]) to Printer, which is implementation defined. Converting a POJO to the string "[object Object]" is certainly a valid way of being implementation defined, but I wouldn't judge it "to be maximally useful and informative."

whatwg/console#226 (comment)

so in summary it would seem:

  • [object Object] is a valid string representation of the object
  • ...but it's not very helpful
  • there's the edge case with Symbol() not being stringifiable in the way console.assert() is implemented
  • most other runtimes Chrome/Firefox/Deno/Bun do it where console.assert(false, {a:1}) is properly inspected; Node.js is the outlier.

i also am making an effort to clarify the mdn wording that had us all confused 🤣 mdn/content#29172

@jcbhmr jcbhmr changed the title console.assert(false, { hello: "world" }) logs [object Object] instead of inspecting the object console.assert(false, Symbol()) throws Sep 19, 2023
@jcbhmr
Copy link
Contributor Author

jcbhmr commented Sep 19, 2023

Changed title and body to reflect exact buggy impl:

console.assert(false, Symbol())

@BridgeAR
Copy link
Member

@bnoordhuis I believe it's fine to handle it that way. It aligns well with the other APIs where the object would be printed in a human readable way. There's also already an open PR and I guess we could reopen the issue as such?

@bnoordhuis
Copy link
Member

Yeah, I'm fine with that, I don't really have a strong opinion either way except that changing the default should be a semver-major change.

@bnoordhuis bnoordhuis reopened this Sep 20, 2023
nodejs-github-bot pushed a commit that referenced this issue Nov 1, 2023
fixes #49680

PR-URL: #49722
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
anonrig pushed a commit to anonrig/node that referenced this issue Nov 9, 2023
fixes nodejs#49680

PR-URL: nodejs#49722
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
console Issues and PRs related to the console subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants