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

util: fix util.getCallSites plurality #55626

Merged
merged 3 commits into from
Nov 2, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 15 additions & 15 deletions benchmark/util/get-callsite.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
'use strict';

const common = require('../common');
const { getCallSite } = require('node:util');
const { getCallSites } = require('node:util');
const assert = require('node:assert');

const bench = common.createBenchmark(main, {
n: [1e6],
method: ['ErrorCallSite', 'ErrorCallSiteSerialized', 'CPP'],
method: ['ErrorCallSites', 'ErrorCallSitesSerialized', 'CPP'],
});

function ErrorGetCallSite() {
function ErrorGetCallSites() {
const originalStackFormatter = Error.prepareStackTrace;
Error.prepareStackTrace = (_err, stack) => {
if (stack && stack.length > 1) {
Expand All @@ -25,15 +25,15 @@ function ErrorGetCallSite() {
return err.stack;
}

function ErrorCallSiteSerialized() {
const callsite = ErrorGetCallSite();
function ErrorCallSitesSerialized() {
const callSites = ErrorGetCallSites();
const serialized = [];
for (let i = 0; i < callsite.length; ++i) {
for (let i = 0; i < callSites.length; ++i) {
serialized.push({
functionName: callsite[i].getFunctionName(),
scriptName: callsite[i].getFileName(),
lineNumber: callsite[i].getLineNumber(),
column: callsite[i].getColumnNumber(),
functionName: callSites[i].getFunctionName(),
scriptName: callSites[i].getFileName(),
lineNumber: callSites[i].getLineNumber(),
column: callSites[i].getColumnNumber(),
});
}
return serialized;
Expand All @@ -42,14 +42,14 @@ function ErrorCallSiteSerialized() {
function main({ n, method }) {
let fn;
switch (method) {
case 'ErrorCallSite':
fn = ErrorGetCallSite;
case 'ErrorCallSites':
fn = ErrorGetCallSites;
break;
case 'ErrorCallSiteSerialized':
fn = ErrorCallSiteSerialized;
case 'ErrorCallSitesSerialized':
fn = ErrorCallSitesSerialized;
break;
case 'CPP':
fn = getCallSite;
fn = getCallSites;
break;
}
let lastStack = {};
Expand Down
14 changes: 14 additions & 0 deletions doc/api/deprecations.md
Original file line number Diff line number Diff line change
Expand Up @@ -3761,6 +3761,19 @@ Instantiating classes without the `new` qualifier exported by the `node:repl` mo
It is recommended to use the `new` qualifier instead. This applies to all REPL classes, including
`REPLServer` and `Recoverable`.

### DEP0186: `util.getCallSite`

<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/55626
description: Runtime deprecation.
-->

Type: Runtime

The `util.getCallSite` API has been removed. Please use [`util.getCallSites()`][] instead.
Comment on lines +3764 to +3775
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this have to be doc deprecated first?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an API in active development status, not stable yet. I don't think it is necessary to document deprecated.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should not deprecate experimental APIs, deprecation is for stable features we want to phase out.

Copy link
Member Author

Choose a reason for hiding this comment

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

There were deprecation for experimental APIs like https://github.com/nodejs/node/blob/main/doc/api/deprecations.md#dep0173-the-assertcalltracker-class.

I was hesistant about adding a deprecation code for this. But all our toolings like expectWarning expects a deprecation code.


[NIST SP 800-38D]: https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf
[RFC 6066]: https://tools.ietf.org/html/rfc6066#section-3
[RFC 8247 Section 2.4]: https://www.rfc-editor.org/rfc/rfc8247#section-2.4
Expand Down Expand Up @@ -3886,6 +3899,7 @@ It is recommended to use the `new` qualifier instead. This applies to all REPL c
[`url.parse()`]: url.md#urlparseurlstring-parsequerystring-slashesdenotehost
[`url.resolve()`]: url.md#urlresolvefrom-to
[`util._extend()`]: util.md#util_extendtarget-source
[`util.getCallSites()`]: util.md#utilgetcallsitesframecount
[`util.getSystemErrorName()`]: util.md#utilgetsystemerrornameerr
[`util.inspect()`]: util.md#utilinspectobject-options
[`util.inspect.custom`]: util.md#utilinspectcustom
Expand Down
14 changes: 7 additions & 7 deletions doc/api/util.md
Original file line number Diff line number Diff line change
Expand Up @@ -364,31 +364,31 @@ util.formatWithOptions({ colors: true }, 'See object %O', { foo: 42 });
// when printed to a terminal.
```

## `util.getCallSite(frames)`
## `util.getCallSites(frameCount)`

> Stability: 1.1 - Active development

<!-- YAML
added: v22.9.0
-->

* `frames` {number} Number of frames returned in the stacktrace.
* `frameCount` {number} Number of frames to capture as call site objects.
**Default:** `10`. Allowable range is between 1 and 200.
* Returns: {Object\[]} An array of stacktrace objects
* `functionName` {string} Returns the name of the function associated with this stack frame.
* Returns: {Object\[]} An array of call site objects
* `functionName` {string} Returns the name of the function associated with this call site.
* `scriptName` {string} Returns the name of the resource that contains the script for the
function for this StackFrame.
function for this call site.
* `lineNumber` {number} Returns the number, 1-based, of the line for the associate function call.
* `column` {number} Returns the 1-based column offset on the line for the associated function call.

Returns an array of stacktrace objects containing the stack of
Returns an array of call site objects containing the stack of
the caller function.

```js
const util = require('node:util');

function exampleFunction() {
const callSites = util.getCallSite();
const callSites = util.getCallSites();

console.log('Call Sites:');
callSites.forEach((callSite, index) => {
Expand Down
15 changes: 10 additions & 5 deletions lib/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -330,13 +330,13 @@ function parseEnv(content) {

/**
* Returns the callSite
* @param {number} frames
* @param {number} frameCount
* @returns {object}
*/
function getCallSite(frames = 10) {
function getCallSites(frameCount = 10) {
// Using kDefaultMaxCallStackSizeToCapture as reference
validateNumber(frames, 'frames', 1, 200);
return binding.getCallSite(frames);
validateNumber(frameCount, 'frameCount', 1, 200);
return binding.getCallSites(frameCount);
};

// Keep the `exports =` so that various functions can still be monkeypatched
Expand All @@ -353,7 +353,12 @@ module.exports = {
format,
styleText,
formatWithOptions,
getCallSite,
// Deprecated getCallSite. No deprecation code as `util.getCallSite` was in active development.
legendecas marked this conversation as resolved.
Show resolved Hide resolved
// This API can be removed in next semver-minor release.
getCallSite: deprecate(getCallSites,
'The `util.getCallSite` API is deprecated. Please use `util.getCallSites()` instead.',
'DEP0186'),
getCallSites,
getSystemErrorMap,
getSystemErrorName,
getSystemErrorMessage,
Expand Down
6 changes: 3 additions & 3 deletions src/node_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ static void ParseEnv(const FunctionCallbackInfo<Value>& args) {
args.GetReturnValue().Set(dotenv.ToObject(env));
}

static void GetCallSite(const FunctionCallbackInfo<Value>& args) {
static void GetCallSites(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Isolate* isolate = env->isolate();

Expand Down Expand Up @@ -345,7 +345,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
registry->Register(GetProxyDetails);
registry->Register(GetCallerLocation);
registry->Register(PreviewEntries);
registry->Register(GetCallSite);
registry->Register(GetCallSites);
registry->Register(GetOwnNonIndexProperties);
registry->Register(GetConstructorName);
registry->Register(GetExternalValue);
Expand Down Expand Up @@ -451,7 +451,7 @@ void Initialize(Local<Object> target,
SetMethodNoSideEffect(
context, target, "getConstructorName", GetConstructorName);
SetMethodNoSideEffect(context, target, "getExternalValue", GetExternalValue);
SetMethodNoSideEffect(context, target, "getCallSite", GetCallSite);
SetMethodNoSideEffect(context, target, "getCallSites", GetCallSites);
SetMethod(context, target, "sleep", Sleep);
SetMethod(context, target, "parseEnv", ParseEnv);

Expand Down
4 changes: 2 additions & 2 deletions test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const net = require('net');
// Do not require 'os' until needed so that test-os-checked-function can
// monkey patch it. If 'os' is required here, that test will fail.
const path = require('path');
const { inspect, getCallSite } = require('util');
const { inspect, getCallSites } = require('util');
const { isMainThread } = require('worker_threads');
const { isModuleNamespaceObject } = require('util/types');

Expand Down Expand Up @@ -550,7 +550,7 @@ function canCreateSymLink() {
}

function mustNotCall(msg) {
const callSite = getCallSite()[1];
const callSite = getCallSites()[1];
return function mustNotCall(...args) {
const argsInfo = args.length > 0 ?
`\ncalled with arguments: ${args.map((arg) => inspect(arg)).join(', ')}` : '';
Expand Down
4 changes: 0 additions & 4 deletions test/fixtures/get-call-site.js

This file was deleted.

4 changes: 4 additions & 0 deletions test/fixtures/get-call-sites.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
const util = require('node:util');
const assert = require('node:assert');
assert.ok(util.getCallSites().length > 1);
process.stdout.write(util.getCallSites()[0].scriptName);
9 changes: 9 additions & 0 deletions test/parallel/test-util-getcallsite.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
'use strict';

require('../common');
const { getCallSite } = require('node:util');
const { expectWarning } = require('../common');

const warning = 'The `util.getCallSite` API is deprecated. Please use `util.getCallSites()` instead.';
expectWarning('DeprecationWarning', warning, 'DEP0186');
getCallSite();
Original file line number Diff line number Diff line change
Expand Up @@ -3,68 +3,68 @@
const common = require('../common');

const fixtures = require('../common/fixtures');
const file = fixtures.path('get-call-site.js');
const file = fixtures.path('get-call-sites.js');

const { getCallSite } = require('node:util');
const { getCallSites } = require('node:util');
const { spawnSync } = require('node:child_process');
const assert = require('node:assert');

{
const callsite = getCallSite();
assert.ok(callsite.length > 1);
const callSites = getCallSites();
assert.ok(callSites.length > 1);
assert.match(
callsite[0].scriptName,
/test-util-getCallSite/,
callSites[0].scriptName,
/test-util-getcallsites/,
'node:util should be ignored',
);
}

{
const callsite = getCallSite(3);
assert.strictEqual(callsite.length, 3);
const callSites = getCallSites(3);
assert.strictEqual(callSites.length, 3);
assert.match(
callsite[0].scriptName,
/test-util-getCallSite/,
callSites[0].scriptName,
/test-util-getcallsites/,
'node:util should be ignored',
);
}

// Guarantee dot-left numbers are ignored
{
const callsite = getCallSite(3.6);
assert.strictEqual(callsite.length, 3);
const callSites = getCallSites(3.6);
assert.strictEqual(callSites.length, 3);
}

{
const callsite = getCallSite(3.4);
assert.strictEqual(callsite.length, 3);
const callSites = getCallSites(3.4);
assert.strictEqual(callSites.length, 3);
}

{
assert.throws(() => {
// Max than kDefaultMaxCallStackSizeToCapture
getCallSite(201);
getCallSites(201);
}, common.expectsError({
code: 'ERR_OUT_OF_RANGE'
}));
assert.throws(() => {
getCallSite(-1);
getCallSites(-1);
}, common.expectsError({
code: 'ERR_OUT_OF_RANGE'
}));
assert.throws(() => {
getCallSite({});
getCallSites({});
}, common.expectsError({
code: 'ERR_INVALID_ARG_TYPE'
}));
}

{
const callsite = getCallSite(1);
assert.strictEqual(callsite.length, 1);
const callSites = getCallSites(1);
assert.strictEqual(callSites.length, 1);
assert.match(
callsite[0].scriptName,
/test-util-getCallSite/,
callSites[0].scriptName,
/test-util-getcallsites/,
'node:util should be ignored',
);
}
Expand All @@ -77,8 +77,8 @@ const assert = require('node:assert');
'-e',
`const util = require('util');
const assert = require('assert');
assert.ok(util.getCallSite().length > 1);
process.stdout.write(util.getCallSite()[0].scriptName);
assert.ok(util.getCallSites().length > 1);
process.stdout.write(util.getCallSites()[0].scriptName);
`,
],
);
Expand All @@ -100,7 +100,7 @@ const assert = require('node:assert');
{
const originalStackTraceLimit = Error.stackTraceLimit;
Error.stackTraceLimit = 0;
const callsite = getCallSite();
assert.notStrictEqual(callsite.length, 0);
const callSites = getCallSites();
assert.notStrictEqual(callSites.length, 0);
Error.stackTraceLimit = originalStackTraceLimit;
}
Loading