assert: improve simple assert

This improves the error message in simple asserts by using the
real call information instead of the already evaluated part.

PR-URL: nodejs#17581
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>

Author: BridgeAR

doc/api/assert.md

@@ -658,19 +658,50 @@ parameter is `undefined`, a default error message is assigned. If the `message`
 parameter is an instance of an [`Error`][] then it will be thrown instead of the
 `AssertionError`.
 
+Be aware that in the `repl` the error message will be different to the one
+thrown in a file! See below for further details.
+
 ```js
 const assert = require('assert').strict;
 
 assert.ok(true);
 // OK
 assert.ok(1);
 // OK
-assert.ok(false);
-// throws "AssertionError: false == true"
-assert.ok(0);
-// throws "AssertionError: 0 == true"
+
 assert.ok(false, 'it\'s false');
 // throws "AssertionError: it's false"
+
+// In the repl:
+assert.ok(typeof 123 === 'string');
+// throws:
+// "AssertionError: false == true
+
+// In a file (e.g. test.js):
+assert.ok(typeof 123 === 'string');
+// throws:
+// "AssertionError: The expression evaluated to a falsy value:
+//
+//   assert.ok(typeof 123 === 'string')
+
+assert.ok(false);
+// throws:
+// "AssertionError: The expression evaluated to a falsy value:
+//
+//   assert.ok(false)
+
+assert.ok(0);
+// throws:
+// "AssertionError: The expression evaluated to a falsy value:
+//
+//   assert.ok(0)
+
+// Using `assert()` works the same:
+assert(0);
+// throws:
+// "AssertionError: The expression evaluated to a falsy value:
+//
+//   assert(0)
 ```
 
 ## assert.strictEqual(actual, expected[, message])

lib/assert.js

@@ -20,10 +20,33 @@
 
 'use strict';
 
-const { isDeepEqual, isDeepStrictEqual } =
-  require('internal/util/comparisons');
+const { Buffer } = require('buffer');
+const {
+  isDeepEqual,
+  isDeepStrictEqual
+} = require('internal/util/comparisons');
 const errors = require('internal/errors');
+const { openSync, closeSync, readSync } = require('fs');
+const { parseExpressionAt } = require('internal/deps/acorn/dist/acorn');
 const { inspect } = require('util');
+const { EOL } = require('os');
+
+const codeCache = new Map();
+// Escape control characters but not \n and \t to keep the line breaks and
+// indentation intact.
+// eslint-disable-next-line no-control-regex
+const escapeSequencesRegExp = /[\x00-\x08\x0b\x0c\x0e-\x1f]/g;
+const meta = [
+  '\\u0000', '\\u0001', '\\u0002', '\\u0003', '\\u0004',
+  '\\u0005', '\\u0006', '\\u0007', '\\b', '',
+  '', '\\u000b', '\\f', '', '\\u000e',
+  '\\u000f', '\\u0010', '\\u0011', '\\u0012', '\\u0013',
+  '\\u0014', '\\u0015', '\\u0016', '\\u0017', '\\u0018',
+  '\\u0019', '\\u001a', '\\u001b', '\\u001c', '\\u001d',
+  '\\u001e', '\\u001f'
+];
+
+const escapeFn = (str) => meta[str.charCodeAt(0)];
 
 // The assert module provides functions that throw
 // AssertionError's when particular conditions are not met. The
@@ -74,20 +97,123 @@ assert.fail = fail;
 //                             expected: expected });
 assert.AssertionError = errors.AssertionError;
 
+function getBuffer(fd, assertLine) {
+  var lines = 0;
+  // Prevent blocking the event loop by limiting the maximum amount of
+  // data that may be read.
+  var maxReads = 64; // bytesPerRead * maxReads = 512 kb
+  var bytesRead = 0;
+  var startBuffer = 0; // Start reading from that char on
+  const bytesPerRead = 8192;
+  const buffers = [];
+  do {
+    const buffer = Buffer.allocUnsafe(bytesPerRead);
+    bytesRead = readSync(fd, buffer, 0, bytesPerRead);
+    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 (lines >= assertLine) {
+      buffers.push(buffer.slice(startBuffer, bytesRead));
+      // Reset the startBuffer in case we need more than one chunk
+      startBuffer = 0;
+    }
+  } while (--maxReads !== 0 && bytesRead !== 0);
+  return buffers;
+}
+
+function innerOk(args, fn) {
+  var [value, message] = args;
 
-// Pure assertion tests whether a value is truthy, as determined
-// by !!value.
-function ok(value, message) {
   if (!value) {
+    if (message == null) {
+      // Use the call as error message if possible.
+      // This does not work with e.g. the repl.
+      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;
+
+      const filename = call.getFileName();
+      const line = call.getLineNumber() - 1;
+      const column = call.getColumnNumber() - 1;
+      const identifier = `${filename}${line}${column}`;
+
+      if (codeCache.has(identifier)) {
+        message = codeCache.get(identifier);
+      } else {
+        var fd;
+        try {
+          fd = openSync(filename, 'r', 0o666);
+          const buffers = getBuffer(fd, line);
+          const code = Buffer.concat(buffers).toString('utf8');
+          const nodes = parseExpressionAt(code, column);
+          // Node type should be "CallExpression" and some times
+          // "SequenceExpression".
+          const node = nodes.type === 'CallExpression' ?
+            nodes :
+            nodes.expressions[0];
+          // TODO: fix the "generatedMessage property"
+          // Since this is actually a generated message, it has to be
+          // determined differently from now on.
+
+          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);
+            message = 'The expression evaluated to a falsy value:' +
+              `${EOL}${EOL}  assert${ok}(${message})${EOL}`;
+          }
+          // Make sure to always set the cache! No matter if the message is
+          // undefined or not
+          codeCache.set(identifier, message);
+        } catch (e) {
+          // Invalidate cache to prevent trying to read this part again.
+          codeCache.set(identifier, undefined);
+        } finally {
+          if (fd !== undefined)
+            closeSync(fd);
+        }
+      }
+    }
     innerFail({
       actual: value,
       expected: true,
       message,
       operator: '==',
-      stackStartFn: ok
+      stackStartFn: fn
     });
   }
 }
+
+// Pure assertion tests whether a value is truthy, as determined
+// by !!value.
+function ok(...args) {
+  innerOk(args, ok);
+}
 assert.ok = ok;
 
 // The equality assertion tests shallow, coercive equality with ==.
@@ -318,16 +444,8 @@ assert.doesNotThrow = function doesNotThrow(block, error, message) {
 assert.ifError = function ifError(err) { if (err) throw err; };
 
 // Expose a strict only variant of assert
-function strict(value, message) {
-  if (!value) {
-    innerFail({
-      actual: value,
-      expected: true,
-      message,
-      operator: '==',
-      stackStartFn: strict
-    });
-  }
+function strict(...args) {
+  innerOk(args, strict);
 }
 assert.strict = Object.assign(strict, assert, {
   equal: assert.strictEqual,

lib/internal/errors.js

@@ -138,7 +138,7 @@ class AssertionError extends Error {
       throw new exports.TypeError('ERR_INVALID_ARG_TYPE', 'options', 'Object');
     }
     var { actual, expected, message, operator, stackStartFn } = options;
-    if (message) {
+    if (message != null) {
       super(message);
     } else {
       if (actual && actual.stack && actual instanceof Error)

test/parallel/test-assert.js

@@ -25,6 +25,7 @@
 
 const common = require('../common');
 const assert = require('assert');
+const { EOL } = require('os');
 const a = assert;
 
 function makeBlock(f) {
@@ -753,22 +754,133 @@ common.expectsError(
   assert.equal(Object.keys(assert).length, Object.keys(a).length);
   /* eslint-enable no-restricted-properties */
   assert(7);
+
+  // Test setting the limit to zero and that assert.strict works properly.
+  const tmpLimit = Error.stackTraceLimit;
+  Error.stackTraceLimit = 0;
   common.expectsError(
-    () => assert(),
+    () => {
+      assert.ok(
+        typeof 123 === 'string'
+      );
+    },
     {
       code: 'ERR_ASSERTION',
       type: assert.AssertionError,
-      message: 'undefined == true'
+      message: `The expression evaluated to a falsy value:${EOL}${EOL}  ` +
+               `assert.ok(typeof 123 === 'string')${EOL}`
     }
   );
+  Error.stackTraceLimit = tmpLimit;
 }
 
 common.expectsError(
   () => assert.ok(null),
   {
     code: 'ERR_ASSERTION',
     type: assert.AssertionError,
-    message: 'null == true'
+    message: `The expression evaluated to a falsy value:${EOL}${EOL}  ` +
+             `assert.ok(null)${EOL}`
+  }
+);
+common.expectsError(
+  () => assert(typeof 123 === 'string'),
+  {
+    code: 'ERR_ASSERTION',
+    type: assert.AssertionError,
+    message: `The expression evaluated to a falsy value:${EOL}${EOL}  ` +
+             `assert(typeof 123 === 'string')${EOL}`
+  }
+);
+
+{
+  // Test caching
+  const fs = process.binding('fs');
+  const tmp = fs.close;
+  fs.close = common.mustCall(tmp, 1);
+  function throwErr() {
+    // eslint-disable-next-line prefer-assert-methods
+    assert(
+      (Buffer.from('test') instanceof Error)
+    );
+  }
+  common.expectsError(
+    () => throwErr(),
+    {
+      code: 'ERR_ASSERTION',
+      type: assert.AssertionError,
+      message: `The expression evaluated to a falsy value:${EOL}${EOL}  ` +
+               `assert(Buffer.from('test') instanceof Error)${EOL}`
+    }
+  );
+  common.expectsError(
+    () => throwErr(),
+    {
+      code: 'ERR_ASSERTION',
+      type: assert.AssertionError,
+      message: `The expression evaluated to a falsy value:${EOL}${EOL}  ` +
+               `assert(Buffer.from('test') instanceof Error)${EOL}`
+    }
+  );
+  fs.close = tmp;
+}
+
+common.expectsError(
+  () => {
+    a(
+      (() => 'string')()
+      // eslint-disable-next-line
+      ===
+      123 instanceof
+          Buffer
+    );
+  },
+  {
+    code: 'ERR_ASSERTION',
+    type: assert.AssertionError,
+    message: `The expression evaluated to a falsy value:${EOL}${EOL}  ` +
+             `assert((() => 'string')()${EOL}` +
+             `      // eslint-disable-next-line${EOL}` +
+             `      ===${EOL}` +
+             `      123 instanceof${EOL}` +
+             `          Buffer)${EOL}`
+  }
+);
+
+common.expectsError(
+  () => assert(null, undefined),
+  {
+    code: 'ERR_ASSERTION',
+    type: assert.AssertionError,
+    message: `The expression evaluated to a falsy value:${EOL}${EOL}  ` +
+             `assert(null, undefined)${EOL}`
+  }
+);
+
+common.expectsError(
+  () => assert.ok.apply(null, [0]),
+  {
+    code: 'ERR_ASSERTION',
+    type: assert.AssertionError,
+    message: '0 == true'
+  }
+);
+
+common.expectsError(
+  () => assert.ok.call(null, 0),
+  {
+    code: 'ERR_ASSERTION',
+    type: assert.AssertionError,
+    message: '0 == true'
+  }
+);
+
+common.expectsError(
+  () => assert.ok.call(null, 0, 'test'),
+  {
+    code: 'ERR_ASSERTION',
+    type: assert.AssertionError,
+    message: 'test'
   }
 );