lib: fix inspect deduplication logic

[miguelmarcondesf]

Fix deduplication logic for inspect output when dealing with constructor names that contain custom tags, but checking for tag followed by lowercase characters, e.g: avoiding unnecessary tag Set from a constructor extended like FooSet(3) { 1, 2, 3 } from a util.inspect(new FooSet([1, 2, 3])) but keeping it for Settings(3) [Set] { 1, 2, 3 } from a util.inspect(new Settings([1, 2, 3]))

cc @BridgeAR

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.44%. Comparing base (4612c79) to head (a3816c8).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59687      +/-   ##
==========================================
+ Coverage   88.25%   88.44%   +0.19%     
==========================================
  Files         703      703              
  Lines      207412   207408       -4     
  Branches    39893    39999     +106     
==========================================
+ Hits       183052   183444     +392     
+ Misses      16313    15944     -369     
+ Partials     8047     8020      -27     

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

... and 48 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[BridgeAR]

LGTM with reverting the two cases / Highlighting the exact change where the change would be beneficial.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[ljharb]

I won't block this change, but I think that "sometimes omitting the [Set] output when the subclass name has 'Set' in it" is magic implicit behavior that makes the output less readable and more confusing. (also, that it doesn't apply for Map or other builtins adds to the inconsistency).

[BridgeAR]

The intention when I added the [ClassName] was about making sure that we have that available somewhere for the user.
I think deduping is actually good :) (this only applies to built-ins anyway and as such also for Maps).

[ljharb]

I agree deduping is good - but I don't think strings should be divisible, and FooSet and Set aren't duplicates, they're two distinct strings. If i name it class Settings extends Set it doesn't actually have "Set" in it in a human sense, but it would hit this logic too.

[BridgeAR]

@ljharb it's a heuristic for the best behavior. We could guard against the case you mention, if you would like that (checking that the match is either at the end of the class name or that the next character is a capital letter).

[ljharb]

That makes the heuristic less likely to be wrong, but also more magical and implicit, which doesn't seem worth it to me. I defer to you.

[github-actions]

Failed to start CI

   ⚠  Commits were pushed since the last approving review:
   ⚠  - lib: update inspect output format for subclasses
   ⚠  - test: add inspect tests for null constructor scenarios
   ⚠  - test: update syntax error assertion in ESM loader test
   ⚠  - lib: revert tag exclusion for getClassBase
   ⚠  - lib: revert tag exclusion for getFunctionBase
   ⚠  - test: rename subclasses for Map and Promise in inspect tests
   ⚠  - lib: Update lib/internal/util/inspect.js
   ⚠  - test: enhance util.inspect assertions
   ✘  Refusing to run CI on potentially unsafe PR

https://github.com/nodejs/node/actions/runs/17830830316

[BridgeAR]

I am struggling understanding the test cases.

I believe we need a few scenarios:

1. class SetAbc extends Set {}
2. class AbcMap extends Map {}
3. class MiddleErrorPart extends Error {}
4. class Settings extends Set {}

These cases should cover all possible scenarios and the outcome would be:

1., 2., and 3. are all not having the fallback.
4. Shows the Set next to the constructor name.

Null prototypes work differently, since we can not reconstruct the class fully and they are not impacted.

[miguelmarcondesf]

I am struggling understanding the test cases.

I believe we need a few scenarios:

1. class SetAbc extends Set {}
2. class AbcMap extends Map {}
3. class MiddleErrorPart extends Error {}
4. class Settings extends Set {}

These cases should cover all possible scenarios and the outcome would be:

1., 2., and 3. are all not having the fallback. 4. Shows the Set next to the constructor name.

Null prototypes work differently, since we can not reconstruct the class fully and they are not impacted.

Hey, I removed the null tests because it was related to our first implementation.

And also added the cases you suggested, we were indeed missing a middle check one, thanks!

Also, now we have problems with the class MiddleErrorPart extends Error {} and I needed to add an extra check for it, because Error subclasses with meaningful Symbol.toStringTag abbreviations would always show the
full constructor name with stack 🤔

edit: I completely misunderstood what had to be done with that error case, already fixed that.

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in aa6838c