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

Scopes: Make Original Source's column relative #75

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jridgewell
Copy link
Member

@jridgewell jridgewell commented Mar 7, 2024

Take the following code (while large nesting isn't super common, it does happen):

function foo() {
    function bar() {
        function baz() {
            function qux() {
                function example() {
                }
            }
        }
    }
}

This would have scopes:

[ 
  {
    start: { line: 0, column: 0 },
    end: { line: 9, column: 1 },
  },
  {
    start: { line: 1, column: 4 },
    end: { line: 8, column: 5 },
  },
  {
    start: { line: 2, column: 8 },
    end: { line: 7, column: 9 },
  },
  {
    start: { line: 3, column: 12 },
    end: { line: 6, column: 13 },
  },
  {
    start: { line: 4, column: 16 },
    end: { line: 5, column: 17 },
  },
]

The absolute columns that we need to encode would give us: [0, 4, 8, 12, 16, 17, 13, 9, 5, 1]. As VLQ, we have A,I,Q,Y,gB,iB,a,S,K,C. Notice the gB and iB 2-byte VLQ encodings for 16 and 17 columns. Because VLQ can only use 5 bits of every byte, and the first byte also needs to encode the sign, we only have 4 bits of data to encode the column in one byte.

There's an easy alternative that can use 1 byte for all of these: relative encodings, just like line. That would mean we just need to encode [0, 4, 4, 4, 4, 1, -4, -4, -4, -4], or A,I,I,I,I,C,J,J,J,J.

@jridgewell jridgewell changed the title Make Original Source's column relative Scopes: Make Original Source's column relative Mar 7, 2024
@jkup jkup requested review from szuend, hbenl and jkup March 7, 2024 14:19
Copy link
Collaborator

@jkup jkup left a comment

Choose a reason for hiding this comment

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

This looks great to me! I'll let Simon and Holger weigh in as they've been leading the proposal and own the proof of concept implementations!

Copy link
Collaborator

@szuend szuend left a comment

Choose a reason for hiding this comment

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

Sounds like a great idea given how most authored code is formatted.

Copy link
Collaborator

@hbenl hbenl left a comment

Choose a reason for hiding this comment

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

I think the idea behind absolute columns was that the column would be "reset" for every line, just as it's done in the mappings field, and then it will be absolute most of the time anyway (because original code rarely has more than one scope starting in the same line).
But while "resetting" the column for every line makes sense for locations in the generated code (because it will usually be minified), it doesn't make sense for locations in the original code, as demonstrated in the PR description.
So I agree with the change in this PR.

@hbenl
Copy link
Collaborator

hbenl commented Mar 8, 2024

I tried using relative columns here and found that the encoded original scopes got larger rather than smaller.
Here's an example:

import {Logger} from './module.js';

function inner(x) {
  Logger.log(x);
}

function outer(x) {
  inner(x);
}

outer(42);
outer(null);

has the following scopes (leaving out the global scope; note that I put the scope start at the { character, see #76 ):

[ 
  {
    start: { line: 2, column: 18 },
    end: { line: 4, column: 1 },
  },
  {
    start: { line: 6, column: 18 },
    end: { line: 8, column: 1 },
  },
]

So the absolute columns to encode would be [18,1,18,1] and the relative columns would be [18,-17,18,-17].
We could

  • make each start column relative to the previous start column and the same for end columns or
  • put the scope start at the beginning of function

@jridgewell
Copy link
Member Author

jridgewell commented Mar 9, 2024

put the scope start at the beginning of function

This was my expectation, and that it fixes this issue makes it even better. It also matches what V8 does when using the prepareCallStack API:

Error.prepareStackTrace = function (_error, frames) {
  for (const f of frames) {
    console.log({
      lineNumber: f.getLineNumber(),
      columnNumber: f.getColumnNumber(),
      enclosingLineNumber: f.getEnclosingLineNumber(),
      enclosingColumnNumber: f.getEnclosingColumnNumber(),
      fileName: f.getFileName(),
    });
  }
};

function foo() {
  throw new Error("bar");
}
foo();

The getEnclosing* methods return the function around the call site, and this logs:

{
  lineNumber: 14,
  columnNumber: 9,
  enclosingLineNumber: 13,
  enclosingColumnNumber: 1,
  fileName: '/Users/jridgewell/tmp/callsite/index.js'
}
{
  lineNumber: 16,
  columnNumber: 1,
  enclosingLineNumber: 1,
  enclosingColumnNumber: 1,
  fileName: '/Users/jridgewell/tmp/callsite/index.js'
}

(Chrome uses 1-based line/columns)

@jridgewell
Copy link
Member Author

A little more to support this: function foo(x, y = x) {} default parameters evaluate within the function scope, so the start column should at most be the ( and definitely before the {.

@hbenl
Copy link
Collaborator

hbenl commented Mar 11, 2024

The getEnclosing* methods return the function around the call site

I couldn't find any documentation for these methods, but it looks like they just return the beginning of the function declaration, I don't know if this is supposed to be the beginning of the function's scope.
It would be interesting to get the locations from the CDP but I don't know how to do that.

default parameters evaluate within the function scope

It's not that simple, from MDN:

The default parameter initializers live in their own scope, which is a parent of the scope created for the function body.

and

functions and variables declared in the function body cannot be referred to from default value parameter initializers

So in let x = 1; function foo(y = x) { let x = 2; }, the default value for y is 1, not 2. To handle default parameter initializers correctly, we'd need #78.

@szuend
Copy link
Collaborator

szuend commented Mar 12, 2024

If you wanna check the locations that we send over CDP from V8, one easy way to do this in DevTools directly:

  1. Enable the "Protocol Monitor" experiment in Chrome DevTools (Settings > Experiments)
  2. Open the protocol monitor (either via Ctrl + Shift + P, or the "More tools" menu)
  3. Create a snippet with debugger statement or a breakpoint
  4. Run the snippet
  5. Look/filter for "Debugger.paused" message in the protocol monitor. Note that we send the scope chain per call frame, so the "Debugger.paused" message is very verbose.

For Holgers example V8 indeed emits 2 scopes:

  1. The normal function scope that starts at the opening paren
  2. An additional block scope that starts at the opening brace (This one is omitted if you remove the default parameter).

image

@jridgewell
Copy link
Member Author

Why does the parameter scope end at the closing } instead of the closing )?

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Mar 16, 2024

Because parameters are still visible inside the function body — the function body scope is nested inside the scope that also includes params.

@jkup
Copy link
Collaborator

jkup commented Jun 26, 2024

Is this good to merge? It looks like it got 👍🏻 but then there was a side question?

@hbenl
Copy link
Collaborator

hbenl commented Jun 28, 2024

Perhaps we should do #76 first and then check if this change would make the encoding more efficient on average.

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

Successfully merging this pull request may close these issues.

5 participants