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

Fix method for getting function name for stack frames #40

Open
cspotcode opened this issue May 2, 2022 · 1 comment · May be fixed by #41
Open

Fix method for getting function name for stack frames #40

cspotcode opened this issue May 2, 2022 · 1 comment · May be fixed by #41

Comments

@cspotcode
Copy link
Owner

cspotcode commented May 2, 2022

From jestjs/jest#12786 (comment)

// hubba.test.js
function BadCode() {
  throw new Error('noo');
}

function run(fn) {
  fn();
}

run(BadCode);

compiled to:

"use strict";

function BadCode() {
  throw new Error('noo');
}

function run(fn) {
  fn();
}

run(BadCode);
//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJuYW1lcyI6WyJCYWRDb2RlIiwiRXJyb3IiLCJydW4iLCJmbiJdLCJzb3VyY2VzIjpbImh1YmJhLnRlc3QuanMiXSwic291cmNlc0NvbnRlbnQiOlsiZnVuY3Rpb24gQmFkQ29kZSgpIHtcbiAgdGhyb3cgbmV3IEVycm9yKCdub28nKTtcbn1cblxuZnVuY3Rpb24gcnVuKGZuKSB7XG4gIGZuKCk7XG59XG5cbnJ1bihCYWRDb2RlKTtcbiJdLCJtYXBwaW5ncyI6Ijs7QUFBQSxTQUFTQSxPQUFULEdBQW1CO0VBQ2pCLE1BQU0sSUFBSUMsS0FBSixDQUFVLEtBQVYsQ0FBTjtBQUNEOztBQUVELFNBQVNDLEdBQVQsQ0FBYUMsRUFBYixFQUFpQjtFQUNmQSxFQUFFO0FBQ0g7O0FBRURELEdBQUcsQ0FBQ0YsT0FBRCxDQUFIIn0=

Stack trace with source-mapping is wrong:

Error: noo
    at fn (/Users/simen/repos/jest/hubba.test.js:2:9) <----- this stack frame should say `BadCode`
    at run (/Users/simen/repos/jest/hubba.test.js:6:3)
    at Object.<anonymous> (/Users/simen/repos/jest/hubba.test.js:9:1)

source-map-support's algorithm for getting the name of a stack frame: it checks the location of the next stack frame. That is why the bug is showing us at fn when it should be showing us at BadCode. The stack frame happens within BadCode, but the location of the next stack frame is the invocation site: fn()

We would see the same issue with the second stack frame if we did const runIt = run; runIt(BadCode) Because it's checking the name at the callsite, it's not checking the invokee's name.

Pretty sure this is not correct and stack frames actually expose additional information we can use to get the correct answer.

In addition to frame.getLineNumber() and frame.getColumnNumber(), we have frame.getEnclosingLineNumber() and frame.getEnclosingColumnNumber() which are the location of the invoked function. For example:

// getEnclosing*Number() gives us location A
// get*Number() gives us location B
/*A*/function run() {}
/*B*/run()
@cspotcode
Copy link
Owner Author

Copied from jestjs/jest#12786 (comment)

I tried teaching source-map-support to map names at getEnclosingLineNumber and getEnclosingColumnNumber the way node does. This highlighted another problem with node's sourcemap implementation -- another place where we should be deviating from node. Thankfully, we're already there, because stable @cspotcode/source-map-support never attempts to map enclosing position.

In this example:

it('should fail', done => {
  throw new Error();
});

The first stack frame's getEnclosing*Number() gives us the location of the done identifier, because that is where the function declaration starts. If we attempt to source-map the name at that position, we erroneously believe that the function's name is done. It is not; that is the name of the first argument to the function.

This example is great, because it shows how bad it can be to attempt these imperfect sourcemap checks. In the examples we looked at several days ago, we were mapping to the name of a function at its callsite, not its declared name. Both names referred to the same function. So it was imperfect, but semantically it sorta made sense.

But today's example is much worst: we are mapping to the name of an entirely different function. This isn't merely imperfect, it's flat-out wrong and misleading. done refers to a different function with a different purpose.

The fix is: never attempt to map names at the "enclosing" position. The sourcemap spec, as it is today, can't do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant