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

Exception message for assert(0) depends on whitespace #30872

Closed
AndrewFinlay opened this issue Dec 9, 2019 · 5 comments · Fixed by #46760
Closed

Exception message for assert(0) depends on whitespace #30872

AndrewFinlay opened this issue Dec 9, 2019 · 5 comments · Fixed by #46760
Labels
assert Issues and PRs related to the assert subsystem. confirmed-bug Issues with confirmed bugs.

Comments

@AndrewFinlay
Copy link

  • Version: 10, 12
  • Platform: macOS High Sierra 10.13.6
  • Subsystem: Assert

Under Node 10 and Node 12, generating an exception from an assertion failure returns a different assertion failure message depending on differences in whitespace. It seems that with a certain whitespace configuration we see the Node 8 assertion failure message 0 == true, other configurations will generate the Node 10 message The expression evaluated to a falsy value. This issue only seems to affect the exception message, all other behaviour seems consistent.

This will affect anything that runs minified source.

I have included a simple reproduction of the issue here

@BridgeAR BridgeAR added assert Issues and PRs related to the assert subsystem. confirmed-bug Issues with confirmed bugs. labels Dec 9, 2019
@BridgeAR
Copy link
Member

BridgeAR commented Dec 9, 2019

@AndrewFinlay thank you for the report. This should indeed not fail to create the better error message.

@pd4d10
Copy link
Contributor

pd4d10 commented Dec 10, 2019

The left curly bracket without a line break before assert would cause this issue:

try { assert(0)
} catch (err) {}

function test() { assert(0)
}

It looks like the expression parse is intentionally started at the line start. Modifying it from start to the actual offset seems to fix these cases, but not sure if it breaks others.

node/lib/assert.js

Lines 235 to 239 in 7629fb2

let start = 0;
// Parse the read code until the correct expression is found.
do {
try {
node = parseExpressionAt(code, start, { ecmaVersion: 11 });

// line 235
-let start = 0
+let start = offset

@himself65
Copy link
Member

@pd4d10 no

node/lib/assert.js

Lines 239 to 242 in 7629fb2

node = parseExpressionAt(code, start, { ecmaVersion: 11 });
start = node.end + 1 || start;
// Find the CallExpression in the tree.
node = findNodeAround(node, offset, 'CallExpression');

 node = parseExpressionAt(code, start, { ecmaVersion: 11 });

this line will throw error with message Unexpected token (1:2) and the code are

try {         assert(condition, message)
  } catch (e) {
    throw e
  }
}

// Now run tests

console.log('Call assert proxy with slightly minified whitespace')
try {
  assertProxySlightlyMinified(0)
  console.error(`assertProxySlightlyMinified(0); failed to cause an exception`)
} catch (e) {
  console.log(`assertProxySlightlyMinified(0); generated exception: ${e}`)
}

console.log('Finished tests')

@himself65
Copy link
Member

and this code will not throw error

    assert(condition, message)
  } catch (e) {
    throw e
  }
}

const assertProxySlightlyMinified = (condition, message) => {
  try {         assert(condition, message)
  } catch (e) {
    throw e
  }
}

// Now run tests

console.log('Call assert proxy with slightly minified whitespace')
try {
  assertProxy(0)
  console.error(`assertProxySlightlyMinified(0); failed to cause an exception`)
} catch (e) {
  console.log(`assertProxySlightlyMinified(0); generated exception: ${e}`)
}

console.log('Finished tests')

@BridgeAR
Copy link
Member

BridgeAR commented Dec 10, 2019

@himself65 @pd4d10 is correct about that. The start is set to zero to include assert or what ever name the user named it. It would otherwise only show ok().

I am currently looking into it (there is an easy solution but that would waste a lot of CPU time, so I am trying to find a proper fix).

@BridgeAR BridgeAR self-assigned this Dec 10, 2019
hidecology pushed a commit to hidecology/node that referenced this issue Feb 22, 2023
@BridgeAR BridgeAR removed their assignment Feb 22, 2023
nodejs-github-bot pushed a commit that referenced this issue Feb 26, 2023
Fixes: #30872
PR-URL: #46760
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Kohei Ueno <[email protected]>
targos pushed a commit that referenced this issue Mar 13, 2023
Fixes: #30872
PR-URL: #46760
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Kohei Ueno <[email protected]>
targos pushed a commit that referenced this issue Mar 14, 2023
Fixes: #30872
PR-URL: #46760
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Kohei Ueno <[email protected]>
danielleadams pushed a commit that referenced this issue Apr 11, 2023
Fixes: #30872
PR-URL: #46760
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Kohei Ueno <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. confirmed-bug Issues with confirmed bugs.
Projects
None yet
4 participants