assert: improve simple assert

1) If simple assert is called in the very first line of a file and
it causes an error, it used to report the wrong code. The reason
is that the column that is reported is faulty. This is fixed by
subtracting the offset from now on in such cases.

2) The actual code read is now limited to the part that is actually
required to visualize the call site. All other code in e.g. minified
files will not cause a significant overhead anymore.

3) The number of allocations is now significantly lower than it used
to be. The buffer is reused until the correct line in the code is
found. In general the algorithm tries to safe operations where
possible.

4) The indentation is now corrected depending on where the statement
actually beginns.

5) It is now possible to handle `.call()` and `.apply()` properly.

6) The user defined function name will now always be used instead of
only choosing either `assert.ok()` or `assert()`.

PR-URL: #21626
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

lib/assert.js

@@ -35,6 +35,10 @@ const { NativeModule } = require('internal/bootstrap/loaders');
 
 let isDeepEqual;
 let isDeepStrictEqual;
+let parseExpressionAt;
+let findNodeAround;
+let columnOffset = 0;
+let decoder;
 
 function lazyLoadComparison() {
   const comparison = require('internal/util/comparisons');
@@ -114,52 +118,141 @@ function fail(actual, expected, message, operator, stackStartFn) {
 assert.fail = fail;
 
 // The AssertionError is defined in internal/error.
-// new assert.AssertionError({ message: message,
-//                             actual: actual,
-//                             expected: expected });
 assert.AssertionError = AssertionError;
 
-function getBuffer(fd, assertLine) {
+function findColumn(fd, column, code) {
+  if (code.length > column + 100) {
+    try {
+      return parseCode(code, column);
+    } catch {
+      // End recursion in case no code could be parsed. The expression should
+      // have been found after 2500 characters, so stop trying.
+      if (code.length - column > 2500) {
+        // eslint-disable-next-line no-throw-literal
+        throw null;
+      }
+    }
+  }
+  // Read up to 2500 bytes more than necessary in columns. That way we address
+  // multi byte characters and read enough data to parse the code.
+  const bytesToRead = column - code.length + 2500;
+  const buffer = Buffer.allocUnsafe(bytesToRead);
+  const bytesRead = readSync(fd, buffer, 0, bytesToRead);
+  code += decoder.write(buffer.slice(0, bytesRead));
+  // EOF: fast path.
+  if (bytesRead < bytesToRead) {
+    return parseCode(code, column);
+  }
+  // Read potentially missing code.
+  return findColumn(fd, column, code);
+}
+
+function getCode(fd, line, column) {
+  let bytesRead = 0;
+  if (line === 0) {
+    // Special handle line number one. This is more efficient and simplifies the
+    // rest of the algorithm. Read more than the regular column number in bytes
+    // to prevent multiple reads in case multi byte characters are used.
+    return findColumn(fd, column, '');
+  }
   let lines = 0;
   // Prevent blocking the event loop by limiting the maximum amount of
   // data that may be read.
   let maxReads = 64; // bytesPerRead * maxReads = 512 kb
-  let bytesRead = 0;
-  let startBuffer = 0; // Start reading from that char on
   const bytesPerRead = 8192;
-  const buffers = [];
-  do {
-    const buffer = Buffer.allocUnsafe(bytesPerRead);
+  // Use a single buffer up front that is reused until the call site is found.
+  let buffer = Buffer.allocUnsafe(bytesPerRead);
+  while (maxReads-- !== 0) {
+    // Only allocate a new buffer in case the needed line is found. All data
+    // before that can be discarded.
+    buffer = lines < line ? buffer : Buffer.allocUnsafe(bytesPerRead);
     bytesRead = readSync(fd, buffer, 0, bytesPerRead);
+    // Read the buffer until the required code line is found.
     for (var i = 0; i < bytesRead; i++) {
-      if (buffer[i] === 10) {
-        lines++;
-        if (lines === assertLine) {
-          startBuffer = i + 1;
-        // Read up to 15 more lines to make sure all code gets matched
-        } else if (lines === assertLine + 16) {
-          buffers.push(buffer.slice(startBuffer, i));
-          return buffers;
+      if (buffer[i] === 10 && ++lines === line) {
+        // If the end of file is reached, directly parse the code and return.
+        if (bytesRead < bytesPerRead) {
+          return parseCode(buffer.toString('utf8', i + 1, bytesRead), column);
         }
+        // Check if the read code is sufficient or read more until the whole
+        // expression is read. Make sure multi byte characters are preserved
+        // properly by using the decoder.
+        const code = decoder.write(buffer.slice(i + 1, bytesRead));
+        return findColumn(fd, column, code);
       }
     }
-    if (lines >= assertLine) {
-      buffers.push(buffer.slice(startBuffer, bytesRead));
-      // Reset the startBuffer in case we need more than one chunk
-      startBuffer = 0;
+  }
+}
+
+function parseCode(code, offset) {
+  // Lazy load acorn.
+  if (parseExpressionAt === undefined) {
+    ({ parseExpressionAt } = require('internal/deps/acorn/dist/acorn'));
+    ({ findNodeAround } = require('internal/deps/acorn/dist/walk'));
+  }
+  let node;
+  let start = 0;
+  // Parse the read code until the correct expression is found.
+  do {
+    try {
+      node = parseExpressionAt(code, start);
+      start = node.end + 1 || start;
+      // Find the CallExpression in the tree.
+      node = findNodeAround(node, offset, 'CallExpression');
+    } catch (err) {
+      // Unexpected token error and the like.
+      start += err.raisedAt || 1;
+      if (start > offset) {
+        // No matching expression found. This could happen if the assert
+        // expression is bigger than the provided buffer.
+        // eslint-disable-next-line no-throw-literal
+        throw null;
+      }
     }
-  } while (--maxReads !== 0 && bytesRead !== 0);
-  return buffers;
+  } while (node === undefined || node.node.end < offset);
+
+  return [
+    node.node.start,
+    code.slice(node.node.start, node.node.end)
+        .replace(escapeSequencesRegExp, escapeFn)
+  ];
 }
 
-function getErrMessage(call) {
+function getErrMessage(message, fn) {
+  const tmpLimit = Error.stackTraceLimit;
+  // Make sure the limit is set to 1. Otherwise it could fail (<= 0) or it
+  // does to much work.
+  Error.stackTraceLimit = 1;
+  // We only need the stack trace. To minimize the overhead use an object
+  // instead of an error.
+  const err = {};
+  Error.captureStackTrace(err, fn);
+  Error.stackTraceLimit = tmpLimit;
+
+  const tmpPrepare = Error.prepareStackTrace;
+  Error.prepareStackTrace = (_, stack) => stack;
+  const call = err.stack[0];
+  Error.prepareStackTrace = tmpPrepare;
+
   const filename = call.getFileName();
+
   if (!filename) {
-    return;
+    return message;
   }
 
   const line = call.getLineNumber() - 1;
-  const column = call.getColumnNumber() - 1;
+  let column = call.getColumnNumber() - 1;
+
+  // Line number one reports the wrong column due to being wrapped in a
+  // function. Remove that offset to get the actual call.
+  if (line === 0) {
+    if (columnOffset === 0) {
+      const { wrapper } = require('internal/modules/cjs/loader');
+      columnOffset = wrapper[0].length;
+    }
+    column -= columnOffset;
+  }
+
   const identifier = `${filename}${line}${column}`;
 
   if (errorCache.has(identifier)) {
@@ -172,57 +265,49 @@ function getErrMessage(call) {
     return;
   }
 
-  let fd, message;
+  let fd;
   try {
+    // Set the stack trace limit to zero. This makes sure unexpected token
+    // errors are handled faster.
+    Error.stackTraceLimit = 0;
+
+    if (decoder === undefined) {
+      const { StringDecoder } = require('string_decoder');
+      decoder = new StringDecoder('utf8');
+    }
+
     fd = openSync(filename, 'r', 0o666);
-    const buffers = getBuffer(fd, line);
-    const code = Buffer.concat(buffers).toString('utf8');
-    // Lazy load acorn.
-    const { parseExpressionAt } = require('internal/deps/acorn/dist/acorn');
-    const nodes = parseExpressionAt(code, column);
-    // Node type should be "CallExpression" and some times
-    // "SequenceExpression".
-    const node = nodes.type === 'CallExpression' ? nodes : nodes.expressions[0];
-    const name = node.callee.name;
-    // Calling `ok` with .apply or .call is uncommon but we use a simple
-    // safeguard nevertheless.
-    if (name !== 'apply' && name !== 'call') {
-    // Only use `assert` and `assert.ok` to reference the "real API" and
-    // not user defined function names.
-      const ok = name === 'ok' ? '.ok' : '';
-      const args = node.arguments;
-      message = code
-        .slice(args[0].start, args[args.length - 1].end)
-        .replace(escapeSequencesRegExp, escapeFn);
+    // Reset column and message.
+    [column, message] = getCode(fd, line, column);
+    // Flush unfinished multi byte characters.
+    decoder.end();
+    // Always normalize indentation, otherwise the message could look weird.
+    if (message.indexOf('\n') !== -1) {
       if (EOL === '\r\n') {
         message = message.replace(/\r\n/g, '\n');
       }
-      // Always normalize indentation, otherwise the message could look weird.
-      if (message.indexOf('\n') !== -1) {
-        const tmp = message.split('\n');
-        message = tmp[0];
-        for (var i = 1; i < tmp.length; i++) {
-          let pos = 0;
-          while (pos < column &&
-              (tmp[i][pos] === ' ' || tmp[i][pos] === '\t')) {
-            pos++;
-          }
-          message += `\n  ${tmp[i].slice(pos)}`;
+      const frames = message.split('\n');
+      message = frames.shift();
+      for (const frame of frames) {
+        let pos = 0;
+        while (pos < column && (frame[pos] === ' ' || frame[pos] === '\t')) {
+          pos++;
         }
+        message += `\n  ${frame.slice(pos)}`;
       }
-      message = 'The expression evaluated to a falsy value:' +
-        `\n\n  assert${ok}(${message})\n`;
     }
+    message = `The expression evaluated to a falsy value:\n\n  ${message}\n`;
     // Make sure to always set the cache! No matter if the message is
     // undefined or not
     errorCache.set(identifier, message);
 
     return message;
-
   } catch (e) {
   // Invalidate cache to prevent trying to read this part again.
     errorCache.set(identifier, undefined);
   } finally {
+    // Reset limit.
+    Error.stackTraceLimit = tmpLimit;
     if (fd !== undefined)
       closeSync(fd);
   }
@@ -236,25 +321,8 @@ function innerOk(fn, argLen, value, message) {
       generatedMessage = true;
       message = 'No value argument passed to `assert.ok()`';
     } else if (message == null) {
-      // Use the call as error message if possible.
-      // This does not work with e.g. the repl.
-      // eslint-disable-next-line no-restricted-syntax
-      const err = new Error();
-      // Make sure the limit is set to 1. Otherwise it could fail (<= 0) or it
-      // does to much work.
-      const tmpLimit = Error.stackTraceLimit;
-      Error.stackTraceLimit = 1;
-      Error.captureStackTrace(err, fn);
-      Error.stackTraceLimit = tmpLimit;
-
-      const tmpPrepare = Error.prepareStackTrace;
-      Error.prepareStackTrace = (_, stack) => stack;
-      const call = err.stack[0];
-      Error.prepareStackTrace = tmpPrepare;
-
-      // Make sure it would be "null" in case that is used.
-      message = getErrMessage(call) || message;
       generatedMessage = true;
+      message = getErrMessage(message, fn);
     } else if (message instanceof Error) {
       throw message;
     }

test/fixtures/assert-first-line.js

@@ -0,0 +1,2 @@
+'use strict'; const ässört = require('assert'); ässört(true); ässört.ok(''); ässört(null);
+// aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(false);