node inspect: fix console.log(%s, { [Symbol.toPrimitive]: () => hello })

console.log("%s", o) invokes the inspect method to retrieve the object. This results in console.log("%s", { [Symbol.toPrimitive]: () => "hello" }) displaying the object itself, rather than 'hello'.

Fixes: #50909

Dec 01 '23 08:12 Ch3nYuY

The former implementation had it's reason as well as the internal toString method is mostly not ideal. The main problem is that hasBuiltInToString does not know about the toPrimitive symbol. By checking for that, it would handle both cases right.

Thank you. Are you suggesting to modify it this way?

        case 115: { // 's'
          const tempArg = args[++a];
          if (typeof tempArg === 'number') {
            tempStr = formatNumberNoColor(tempArg, inspectOptions);
          } else if (typeof tempArg === 'bigint') {
            tempStr = formatBigIntNoColor(tempArg, inspectOptions);
          } else if (hasBuiltInToString(tempArg)) {
            tempStr = String(tempArg);
          } else {
            tempStr = inspect(tempArg, {
              ...inspectOptions,
              compact: 3,
              colors: false,
              depth: 0,
            });
          }
          break;
        }

Dec 01 '23 09:12 Ch3nYuY

@chenyuyang2022 no, the hasBuiltInToString method has to check for for the toPrimitive symbol as well. Please always run the tests as well. There should be test failures related to the current change.

Dec 04 '23 10:12 BridgeAR

@BridgeAR Thank you for the reminder. I have modified the implementation and passed all the tests with 'make test'.

Mar 18 '24 07:03 Ch3nYuY

@legendecas PTAL

Mar 19 '24 09:03 Ch3nYuY

Would you mind updating the hasBuiltInToString function instead as @BridgeAR suggested? Also, please add a test case in https://github.com/nodejs/node/blob/main/test/parallel/test-util-format.js as well. Thank you!

Mar 19 '24 10:03 legendecas

@legendecas Thank you, I've made the changes according to your suggestions. Could you please take a look?

Mar 24 '24 18:03 Ch3nYuY

@BridgeAR would you mind taking a look again?

Mar 26 '24 03:03 legendecas

CI: https://ci.nodejs.org/job/node-test-pull-request/57954/

Mar 26 '24 03:03 nodejs-github-bot

@BridgeAR Would you mind reviewing it again?

Mar 28 '24 10:03 Ch3nYuY

@Ch3nYuY thank you for following up on the comments!

Apr 24 '24 12:04 BridgeAR

CI: https://ci.nodejs.org/job/node-test-pull-request/58665/

Apr 24 '24 12:04 nodejs-github-bot

@Ch3nYuY seems like there is a linter error that has to be resolved before it's possible to land this

Apr 27 '24 14:04 BridgeAR

Thank you, @BridgeAR . I'll see how to solve it

Apr 28 '24 15:04 Ch3nYuY

@BridgeAR I've solved all the linter error. Could you please add the 'request-ci' tag again?

Apr 29 '24 09:04 Ch3nYuY

CI: https://ci.nodejs.org/job/node-test-pull-request/58873/

May 02 '24 18:05 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/58920/

May 04 '24 12:05 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/58925/

May 04 '24 15:05 nodejs-github-bot

Hey @Ch3nYuY, the recent commits block the PR from being merged. Can you please rebase? :)

May 08 '24 09:05 BridgeAR

Thanks! @BridgeAR I resolved the merge conflicts and it should look ok now.

May 09 '24 01:05 Ch3nYuY

CI: https://ci.nodejs.org/job/node-test-pull-request/59110/

May 11 '24 14:05 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/59152/

May 12 '24 07:05 nodejs-github-bot

~Landed in 7129915068b3613f8bb525c84ca4feaa7c9c2077~ Sorry, wrong commit message!

Landed in dde29657658a8e78aa09bb7bbaaf6ba25008359d

May 12 '24 10:05 aduh95

node node copied to clipboard

inspect: fix console.log(%s, { [Symbol.toPrimitive]: () => hello })

node
node copied to clipboard