util: inspect: enumerable Symbols no longer have square brackets

[ljharb]

The intention of this change is to increase consistency. Before this PR, non-enumerable string properties have square brackets, and all Symbol properties have square brackets - meaning that without color cues, there's no way to determine whether a Symbol is enumerable or not, and it can be confusing whether [foo] is indeed bracketed because it's non-enumerable.

Due to the following considerations:

1. minimizing disruption/changes to output
2. square brackets are already used for a number of "special" things, including a data proto property, Symbol.toStringTag values, null objects, etc, so we already wouldn't be able to have square brackets mean only "computed properties"

… we decided to go with "enumerable Symbols do not have square brackets", so that all non-enumerables use square brackets.

Implements https://github.com/orgs/nodejs/discussions/41283#discussioncomment-11188239

[ljharb]

cc @BridgeAR

[avivkeller]

IMO this is a semver-major breaking change, as it breaks the existing behavior of brackets existing on all symbols

[BridgeAR]

Generally LGTM. Please also add a few tests for the now escaped non-enumerable key names that need escaping.

I am fine to consider it as semver-major, while we normally do not handle these changes as being part of semver. The output changes from time to time and it is considered a debugging utility that does not have a stable output. I suggest to just add a few do not land labels for now and consider it a semver minor change? It could theoretically be back parted at a later point that way.

[BridgeAR]

@ljharb I would probably just take the gist out of the discussion and use that as commit description. That way it's clear that this is done for consistency reasons to distinguish non-enumerable properties easier.

[avivkeller]

@BridgeAR I've added semver-major PRs that contain breaking changes and should be released in the next major version. for now, but if something changes, we can always adjust it to your suggestion.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.40%. Comparing base (58a8eb4) to head (577b7e7).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55778      +/-   ##
==========================================
- Coverage   88.40%   88.40%   -0.01%     
==========================================
  Files         654      654              
  Lines      187815   187810       -5     
  Branches    36136    36140       +4     
==========================================
- Hits       166045   166035      -10     
- Misses      15001    15014      +13     
+ Partials     6769     6761       -8     

Files with missing lines	Coverage Δ
lib/internal/util/inspect.js	99.95% <100.00%> (-0.05%)	⬇️

... and 37 files with indirect coverage changes

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[ljharb]

Of the 2 failures, both appear to be unrelated to the PR, and seem to be issues with git itself. Can someone take a look?

[nodejs-github-bot]

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

[ljharb]

CI doesn't seem to be running, despite someone resuming it multiple times. Can someone look into it?

[RafaelGSS]

It seems nodejs/build#3959

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[ljharb]

yay, all green!

[nodejs-github-bot]

Landed in 9f2885a...e577618