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

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

Merged
merged 1 commit into from
May 12, 2024

Conversation

Ch3nYuY
Copy link

@Ch3nYuY Ch3nYuY commented Dec 1, 2023

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

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Dec 1, 2023
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Ch3nYuY
Copy link
Author

Ch3nYuY commented Dec 1, 2023

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;
        }

@Ch3nYuY Ch3nYuY requested a review from BridgeAR December 4, 2023 01:32
@BridgeAR
Copy link
Member

BridgeAR commented Dec 4, 2023

@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.

@Ch3nYuY Ch3nYuY force-pushed the fix/console-log branch 2 times, most recently from baf1b8c to 79df127 Compare March 18, 2024 07:09
@Ch3nYuY
Copy link
Author

Ch3nYuY commented Mar 18, 2024

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

@Ch3nYuY
Copy link
Author

Ch3nYuY commented Mar 19, 2024

@legendecas PTAL

@legendecas
Copy link
Member

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!

@Ch3nYuY
Copy link
Author

Ch3nYuY commented Mar 24, 2024

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

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 26, 2024
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@legendecas
Copy link
Member

@BridgeAR would you mind taking a look again?

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 26, 2024
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for following up upon this!
LGTM with the comment removed.

test/parallel/test-util-format.js Outdated Show resolved Hide resolved
@Ch3nYuY
Copy link
Author

Ch3nYuY commented Mar 28, 2024

@BridgeAR Would you mind reviewing it again?

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

We have to change the commit message when merging. It should be e.g., util: fix %s format for objects with Symbol.toPrimitive.

@BridgeAR
Copy link
Member

@Ch3nYuY thank you for following up on the comments!

@BridgeAR BridgeAR added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 24, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 24, 2024
@nodejs-github-bot
Copy link
Collaborator

@BridgeAR
Copy link
Member

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

@Ch3nYuY
Copy link
Author

Ch3nYuY commented Apr 28, 2024

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

@Ch3nYuY
Copy link
Author

Ch3nYuY commented Apr 29, 2024

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

@himself65 himself65 added the request-ci Add this label to start a Jenkins CI on a PR. label May 2, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 2, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR
Copy link
Member

BridgeAR commented May 8, 2024

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

@Ch3nYuY
Copy link
Author

Ch3nYuY commented May 9, 2024

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

This commit ensures `console.log("%s", obj)` correctly invokes
`obj[Symbol.toPrimitive]` for string conversion, fixing unexpected
object display issue.

Fixes: nodejs#50909
@aduh95 aduh95 force-pushed the fix/console-log branch from 58e234b to 796a3ab Compare May 11, 2024 14:48
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 11, 2024
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 merged commit 7129915 into nodejs:main May 12, 2024
53 checks passed
@aduh95
Copy link
Contributor

aduh95 commented May 12, 2024

Landed in 7129915 Sorry, wrong commit message!

Landed in dde2965

aduh95 pushed a commit that referenced this pull request May 12, 2024
This commit ensures `console.log("%s", obj)` correctly invokes
`obj[Symbol.toPrimitive]` for string conversion, fixing unexpected
object display issue.

PR-URL: #50992
Fixes: #50909
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
targos pushed a commit that referenced this pull request May 12, 2024
This commit ensures `console.log("%s", obj)` correctly invokes
`obj[Symbol.toPrimitive]` for string conversion, fixing unexpected
object display issue.

PR-URL: #50992
Fixes: #50909
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
This commit ensures `console.log("%s", obj)` correctly invokes
`obj[Symbol.toPrimitive]` for string conversion, fixing unexpected
object display issue.

PR-URL: #50992
Fixes: #50909
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
This commit ensures `console.log("%s", obj)` correctly invokes
`obj[Symbol.toPrimitive]` for string conversion, fixing unexpected
object display issue.

PR-URL: #50992
Fixes: #50909
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
This commit ensures `console.log("%s", obj)` correctly invokes
`obj[Symbol.toPrimitive]` for string conversion, fixing unexpected
object display issue.

PR-URL: #50992
Fixes: #50909
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
sophoniie pushed a commit to sophoniie/node that referenced this pull request Jun 20, 2024
This commit ensures `console.log("%s", obj)` correctly invokes
`obj[Symbol.toPrimitive]` for string conversion, fixing unexpected
object display issue.

PR-URL: nodejs#50992
Fixes: nodejs#50909
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
This commit ensures `console.log("%s", obj)` correctly invokes
`obj[Symbol.toPrimitive]` for string conversion, fixing unexpected
object display issue.

PR-URL: nodejs#50992
Fixes: nodejs#50909
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

console.log("%s", { [Symbol.toPrimitive]: () => "hello" }) shows the object, not "hello"
7 participants