Skip to content

Commit

Permalink
assert: refactor to use more primordials
Browse files Browse the repository at this point in the history
PR-URL: #36234
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
aduh95 authored and danielleadams committed Dec 7, 2020
1 parent 2c7358e commit e79bdc3
Showing 1 changed file with 37 additions and 22 deletions.
59 changes: 37 additions & 22 deletions lib/assert.js
Expand Up @@ -21,17 +21,29 @@
'use strict';

const {
ArrayPrototypeIndexOf,
ArrayPrototypeJoin,
ArrayPrototypePush,
ArrayPrototypeShift,
ArrayPrototypeSlice,
Error,
ErrorCaptureStackTrace,
FunctionPrototypeBind,
NumberIsNaN,
ObjectAssign,
ObjectIs,
ObjectKeys,
ObjectPrototypeIsPrototypeOf,
Map,
NumberIsNaN,
ReflectApply,
RegExpPrototypeTest,
SafeMap,
String,
StringPrototypeCharCodeAt,
StringPrototypeIncludes,
StringPrototypeIndexOf,
StringPrototypeReplace,
StringPrototypeSlice,
StringPrototypeSplit,
StringPrototypeStartsWith,
} = primordials;

Expand All @@ -54,7 +66,7 @@ const { EOL } = require('internal/constants');
const { NativeModule } = require('internal/bootstrap/loaders');
const { isError } = require('internal/util');

const errorCache = new Map();
const errorCache = new SafeMap();
const CallTracker = require('internal/assert/calltracker');

let isDeepEqual;
Expand Down Expand Up @@ -83,7 +95,7 @@ const meta = [
'\\u001e', '\\u001f'
];

const escapeFn = (str) => meta[str.charCodeAt(0)];
const escapeFn = (str) => meta[StringPrototypeCharCodeAt(str, 0)];

let warned = false;

Expand Down Expand Up @@ -234,7 +246,7 @@ function parseCode(code, offset) {
classFields,
staticClassFeatures
);
parseExpressionAt = Parser.parseExpressionAt.bind(Parser);
parseExpressionAt = FunctionPrototypeBind(Parser.parseExpressionAt, Parser);
}
let node;
let start = 0;
Expand All @@ -259,8 +271,9 @@ function parseCode(code, offset) {

return [
node.node.start,
code.slice(node.node.start, node.node.end)
.replace(escapeSequencesRegExp, escapeFn)
StringPrototypeReplace(StringPrototypeSlice(code,
node.node.start, node.node.end),
escapeSequencesRegExp, escapeFn)
];
}

Expand Down Expand Up @@ -324,23 +337,24 @@ function getErrMessage(message, fn) {
decoder.end();
} else {
for (let i = 0; i < line; i++) {
code = code.slice(code.indexOf('\n') + 1);
code = StringPrototypeSlice(code,
StringPrototypeIndexOf(code, '\n') + 1);
}
[column, message] = parseCode(code, column);
}
// Always normalize indentation, otherwise the message could look weird.
if (message.includes('\n')) {
if (StringPrototypeIncludes(message, '\n')) {
if (EOL === '\r\n') {
message = message.replace(/\r\n/g, '\n');
message = StringPrototypeReplace(message, /\r\n/g, '\n');
}
const frames = message.split('\n');
message = frames.shift();
const frames = StringPrototypeSplit(message, '\n');
message = ArrayPrototypeShift(frames);
for (const frame of frames) {
let pos = 0;
while (pos < column && (frame[pos] === ' ' || frame[pos] === '\t')) {
pos++;
}
message += `\n ${frame.slice(pos)}`;
message += `\n ${StringPrototypeSlice(frame, pos)}`;
}
}
message = `The expression evaluated to a falsy value:\n\n ${message}\n`;
Expand Down Expand Up @@ -606,7 +620,7 @@ function expectedException(actual, expected, message, fn) {
// Special handle errors to make sure the name and the message are
// compared as well.
if (expected instanceof Error) {
keys.push('name', 'message');
ArrayPrototypePush(keys, 'name', 'message');
} else if (keys.length === 0) {
throw new ERR_INVALID_ARG_VALUE('error',
expected, 'may not be an empty object');
Expand Down Expand Up @@ -649,7 +663,7 @@ function expectedException(actual, expected, message, fn) {
throwError = true;
} else {
// Check validation functions return value.
const res = expected.call({}, actual);
const res = ReflectApply(expected, {}, [actual]);
if (res !== true) {
if (!message) {
generatedMessage = true;
Expand Down Expand Up @@ -794,7 +808,7 @@ function hasMatchingError(actual, expected) {
if (ObjectPrototypeIsPrototypeOf(Error, expected)) {
return false;
}
return expected.call({}, actual) === true;
return ReflectApply(expected, {}, [actual]) === true;
}

function expectsNoError(stackStartFn, actual, error, message) {
Expand Down Expand Up @@ -866,20 +880,21 @@ assert.ifError = function ifError(err) {
// This will remove any duplicated frames from the error frames taken
// from within `ifError` and add the original error frames to the newly
// created ones.
const tmp2 = origStack.split('\n');
tmp2.shift();
const tmp2 = StringPrototypeSplit(origStack, '\n');
ArrayPrototypeShift(tmp2);
// Filter all frames existing in err.stack.
let tmp1 = newErr.stack.split('\n');
let tmp1 = StringPrototypeSplit(newErr.stack, '\n');
for (const errFrame of tmp2) {
// Find the first occurrence of the frame.
const pos = tmp1.indexOf(errFrame);
const pos = ArrayPrototypeIndexOf(tmp1, errFrame);
if (pos !== -1) {
// Only keep new frames.
tmp1 = tmp1.slice(0, pos);
tmp1 = ArrayPrototypeSlice(tmp1, 0, pos);
break;
}
}
newErr.stack = `${tmp1.join('\n')}\n${tmp2.join('\n')}`;
newErr.stack =
`${ArrayPrototypeJoin(tmp1, '\n')}\n${ArrayPrototypeJoin(tmp2, '\n')}`;
}

throw newErr;
Expand Down

3 comments on commit e79bdc3

@ChrisMiami
Copy link

Choose a reason for hiding this comment

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

"primordials" seems like massive overkill. If this code were accessed in a million-iteration loop, I could see bypassing the language's native invocation patterns, but for one or two invocations in a test assertion, it seems like one should just use the language as it was written. If performance here is that critical, why not just inline the raw code itself and avoid the stack altogether?

@Trott
Copy link
Member

@Trott Trott commented on e79bdc3 Dec 16, 2020

Choose a reason for hiding this comment

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

If performance here is that critical, why not just inline the raw code itself and avoid the stack altogether?

@ChrisMiami It's not about performance. In fact, primordials can sometimes hurt performance. It's about preventing Node.js internal code from being affected by userland monkey-patching of globals. Using primordials throughout Node.js internals is a security precaution.

@Trott
Copy link
Member

@Trott Trott commented on e79bdc3 Dec 16, 2020

Choose a reason for hiding this comment

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

I could see bypassing the language's native invocation patterns

Kind of restating what I said above, but it's not about bypassing the language's native invocation. It's about ensuring the language's native invocation regardless of whatever tampering some module deep in the dependency tree might be doing.

Please sign in to comment.