feat: Generate sourcemaps of friendly call frames

[liuxingbaoyu]

Q	A
Fixed Issues?	Fixes #14907, Fixes #14635, Fixes #5408
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	√
Documentation PR Link	babel/website#2720
Any Dependency Changes?
License	MIT

There is a small performance optimization, since indentation will never have an identifier Name.
I've only updated some of the necessary tests, because a rebase is definitely required before merging.

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/53972/

[jridgewell]

Can we have minimal tests for (some of):

• Function decl with id

• Function decl without id (default export)

• Method with identifier name

• Method with computed identifier name

• Method with computed string name

• Function expression

• Function expression as property

• Arrow function and no params

• Arrow function and 1 param without quotes

• Arrow function and 1 param with quotes

• Arrow function and multiple params

• Arrow function as property

[jridgewell]

These closure allocations are probably more expensive that the property access, no? Could we just queue the name, and have it unset after making the next mark?

[liuxingbaoyu]

Is it such a test? The result is as follows, I'm not sure if the method needs to do anything.

link

[jridgewell]

Technically, we can mark this one as fn

[jridgewell]

These should be var x = … form, since we could mark the arrows with names (but only if it has a ().

[liuxingbaoyu]

Directly generated looks fine, not sure how it goes when merging source maps, probably not so good.

[jridgewell]

not sure how it goes when merging source maps, probably not so good.

Merging will will take the original's name if it exists, but will take the generated's name if it does not. Should be fine.

[nicolo-ribaudo]

Can't we just delete it if it's always undefined?

[liuxingbaoyu]

I pass it as Loc and ts will complain, plus I personally tend to keep the object shape.

[liuxingbaoyu]

What if the input source map has no identifier name for (?
But this should be rare, babel is always in front of the minimization.

[jridgewell]

What if the input source map has no identifier name for (?

It falls back to the generated sourcemap's name.

[liuxingbaoyu]

Will this lead to inconsistencies? If the input source map has identifier names for function names, but not for (, it might end up generating different names, which is a bit confusing. I can't think of a good way to fix this though.

[jridgewell]

Yes, that's a possibility. If the identifier is marked in the sourcemap with a name, and the ( is not, then the identifier will be mapped to the original name but the ( will be mapped to the generated name.

This might mean we should not create a mark for the (, and instead allow the identifier's segment to span both the identifier and the (. That will work for stack frames, and merging won't see the ( mark explicitly, only the identifier. Because of that, the merged output won't have a ( mark, which again works fine for stack frames.

[liuxingbaoyu]

For vars and methods though, this might not look good, like ["fn"]: function () {}, maybe moving the source merge from core to generator would be better for this problem.

Or we could pass the source map to the generator to get the identifier names, but not sure how that would affect performance.

[nicolo-ribaudo]

How do the devtools look like in these cases?

• Original source map without identfier name for (, behavior before this PR
• Original source map without identfier name for (, behavior with this PR
• Original source map with identfier name for (, behavior with this PR

[jridgewell]

For vars and methods though, this might not look good, like ["fn"]: function () {}, maybe moving the source merge from core to generator would be better for this problem.

That's a great idea. It'll actually help with memory, because we don't need to create a full sourcemap just to throw it away during merge. We can perform the lookups for original source/line/column/name before marking our map, making it just a single map.

How do the devtools look like in these cases?

Devtools don't handle multiple source maps of a single file, so "original" is confusing here. There's only the map that's liked via sourceMappingUrl comment, and it uses whatever identifier name is provided for the current mapping.

[liuxingbaoyu]

I'm trying to do what I said above, but I'm running into some issues.

https://github.com/babel/babel/blob/main/packages/babel-core/src/transformation/file/merge-map.ts#L4-L48
Here we removed sourceRoot, I'm not sure what to do in TraceMap.

packages\babel-cli\test\fixtures\babel\filename-sourcemap --out-file --source-maps inline\out-files\script2.js In this test, the previous behavior seems to be according to the input sourcemap to create the fragment, and now it's based on what's generated after, which looks fine to me, but I'm not sure.(#14209 (review) looks like it was introduced here)

input
before output
after output1

As above, what should we do with what cannot be found in the input source map? I'm completely clueless about the scenario when code is an object. Currently I'm adding the input code to the sourcemap as well, what could be wrong?
microsoft/vscode-js-debug#1419
Also I'm using a fragment as an identifier in this PR, it seems that if we map as per the input source map, the code of this PR won't work as expected.

I'm submitting what I've already done, any guidance would be greatly appreciated!

.

[jridgewell]

Here we removed sourceRoot, I'm not sure what to do in TraceMap.

We don't need to do anything special. Because the GenMapping holds it separate, it'll just work.

In this test, the previous behavior seems to be according to the input sourcemap to create the fragment, and now it's based on what's generated after, which looks fine to me, but I'm not sure.(#14209 (review) looks like it was introduced here)

The new output is fine. The input's map only has 4 (badly placed) mappings. The old behavior during merging would only keep mappings that could map directly into one of those 4 locations, and then perform deduplication with contiguous segments that point to the same.

Eg, arr.map, there should be 3 mappings (arr, ., map). With the old code, we performed a lookup and they all pointed to the same segment. There's no point in having 3 mappings to that same position, so we ignored the latter.

This is only a problem because the input map was poorly generated (I don't actually know where it came from). If the input is high-fidelity, then this would have given better results before.

As above, what should we do with what cannot be found in the input source map? I'm completely clueless about the scenario when code is an object. Currently I'm adding the input code to the sourcemap as well, what could be wrong?

I mentioned this during one of the inline comments.

Also I'm using a fragment as an identifier in this PR, it seems that if we map as per the input source map, the code of this PR won't work as expected.

Fragment?

[jridgewell]

Can't we just leave this till the mark call? We're already performing the originalPositionFor call there. I think we can just store the name name we think, then prioritize the original's name during mark. I think we could store the loc?

[jridgewell]

I think these calls are unnecessary. The ( won't map to the identifier name. Could we instead store the loc?

[jridgewell]

There are two paths to hit this:

1. The original mapping is not contained the input map.
2. There's no input map.

I think we should only do this for 2). For 1), if the original mapping couldn't be found, then that means that this position is sourceless, and can't point to a real location. So, it's just generated, not original.

See also, Terser's behavior

[liuxingbaoyu]

Thanks for your review, very helpful!

Also I'm using a fragment as an identifier in this PR, it seems that if we map as per the input source map, the code of this PR won't work as expected.

Fragment?

I use computeColumnSpans in source-map to calculate the generated identifier code text length.
It's going to go wrong in the before output case.

The x identifier name is mapped to the entire generated expression.
When the debugger renames an identifier it treats the entire expression as one identifier, which is not expected.
Is this form of source map common? I'm not sure if I should use computeColumnSpans.

[jridgewell]

When the debugger renames an identifier it treats the entire expression as one identifier, which is not expected.

This is because the input sourcemap is very low fidelity. A good input map would have given proper outputs. The input var foo = () => 4 has no relation with the output arr.map(…), they share nothing that could reasonably be mapped. Honestly, the output here looks entirely generated, and there isn't anywhere for the map to point to in the original. It should probably be empty.

Is this form of source map common? I'm not sure if I should use computeColumnSpans.

I don't implement it for my packages because I've never needed it. And it's not guaranteed to work to work the way you want. The only information available in a sourcemap are the segment positions. This essentially gives you the range that a segment covers by looking at the generated column of the next segment in the map, but who knows what that segment represents?

Eg, esbuild's output would have a foo = range, and not a foo. Terser would give foo=.

The easiest sourcemaps to generate just mark the beginnings of identifiers, and completely ignore any syntax. The debugging experience will be better with } and now the ( marked. But really, nothing else is strictly necessary. So there's no guarantee that the whitespace/syntax/X directly following an identifier name will be marked.

[jridgewell]

Shouldn't this be using identifierNamePos.line?

[liuxingbaoyu]

Oops this is a bug.

[jridgewell]

I think source implies originalMapping.line != null and vice-versa.

[jridgewell]

Nit: If we're prioritizing the below lookup's name, we can skip this if that one found a name.

[liuxingbaoyu]

But the original position of the parentheses will be pointed to the identifier, what do you think of this?

[jridgewell]

I'm not suggesting we take the original line/column from this lookup, but that we skip this lookup entirely.

// This is the lookup for this mark
const originalMapping = originalPositionFor(this._inputMap, {
  line,
  column,
});

if (originalMapping.name) {
  // If the we found a name, nothing else needs to be done
  // Maybe we're marking a `(` and the input map already had a name attached there,
  // or we're marking a `(` and the sourcemap spanned a `foo(`,
  // or we're marking an identifier, etc.
  identifierName = originalMapping.name;
} else if (identifierNamePos) {
  // We're trying to mark a `(` (as that's the only thing that provides
  // an identifierNamePos currently), and we the AST had an identifier attached.
  // Lookup it's original name.
  const originalIdentifierMapping = originalPositionFor(this._inputMap, identifierNamePos);
  if (originalIdentifierMapping.name) {
    identifierName = originalIdentifierMapping.name;
  }
} else {
  // noop, just use the passed in identifierName
}

[liuxingbaoyu]

It's all really great! I also left these amazing comments in the code.

[jridgewell]

What's this block used for?

[liuxingbaoyu]

We'll call _mark on each line, so we can avoid having to look up the original identifier name multiple times, maybe always search here?

[jridgewell]

Oh, we shouldn't mark a name for the start of a line. We can just pass nulls for the while loop

[liuxingbaoyu]

Thank you so much for this information!

It looks like reality is a little bit bad.
microsoft/vscode-js-debug#1383
I had this problem initially, vscode-js-debug was using a regular expression to extract identifiers in the generated code.
Our enum couldn't be previewed in debug because . was not included in the regular expression. (flags.xxxxx).
So I modified the logic to extract the identifier based on a span (or mapping?) based on babel's source map.
Here I'm assuming a span (or mapping?) with an identifier name that can point the start and end to an identifier in the originalgenerated code.
It looks wrong now, which is kind of bad, there doesn't seem to be a better way to extract the identifier in the generated code without parsing the AST.

[jridgewell]

It looks wrong now, which is kind of bad, there doesn't seem to be a better way to extract the identifier in the generated code without parsing the AST.

In general, no, there's not a better way to do this. But in this very specific case, we could fix babel's output. It looks like we're generating a single marking at the of _ and another marking at the ) following, making the range span the entire _scopeflags.SCOPE_FUNCTION. But really, _scopeflags. is generated code and shouldn't be marked.

Actually, we try to mark the generated _scopeflags, ., SCOPE_FUNCTION, all pointing to the same source position. gen-mapping's maybeAddMapping will coalesce these into a single segment spanning all of them. So we could switch back to the regular addMapping, and keep all 3 markings (so the regex would identify the _scopeflags -> SCOPE_FUNCTION rename, and SCOPE_FUNCTION -> SCOPE_FUNCTION). Or, we could mark the _scopeflags, . as generated code (don't call t.inherits, so that they don't have a .loc).

[liuxingbaoyu]

Thanks for your advice!
Sorry, maybe this topic is not so closely related to babel, just related or something I came across while debugging babel.

I'm not sure what the real logic of vscode-js-debug is, as far as I can guess:
It will go through all mappings and extract the ones with mapping.name, and get the generated code, rename it.
For example SCOPE_FUNCTION->_scopeflags.SCOPE_FUNCTION.
It finds that the _scopeflags.SCOPE_FUNCTION mapping has an identifier name SCOPE_FUNCTION, and then renames SCOPE_FUNCTION with _scopeflags.SCOPE_FUNCTION with scope promotion.
Previously it would do a regular expression match against _scopeflags.SCOPE_FUNCTION, extracting an identifier from it. This should be done to filter output like esbuild (a mapping of generated code has other tokens or spaces after the identifier).
So it extracts _scopeflags, which leads to renaming SCOPE_FUNCTION in the source code to _scopeflags, which cannot be previewed normally.
(SCOPE_FUNCTION->_scopeflags)

I opened a PR microsoft/vscode-js-debug#1419 to use the start and end of the mapping of the entire generated code.
This works great for babel's output, but in this PR I found that the output for esbuild is wrong, accidentally including other tokens or spaces.
So I'm wondering if there is any way to fix this.
This doesn't seem to be specified in the specification of the source map, and matching by regex is a bit inaccurate, which is a little bad.

I'm currently thinking that maybe it's possible to determine if the source map, like babel, always treats the identifier as a single mapping without any other characters, by checking for tokens or spaces in all the generated code mappings.

[nicolo-ribaudo]

The failures in the E2E Jest tests are ok, it's diff about the generated source maps. I will merge this PR as the last one before releasing, to avoid having CI red while merging all the others.

[stefcameron]

After upgrading to @babel/core@7.21.0, I'm seeing error stacks like this when using devtool: eval-source-map in my Webpack config (which was fine in 7.20.12):

ERROR in ../packages/tour/DOCS.mdx
Module build failed (from ../node_modules/babel-loader/lib/index.js):
TypeError: /packages/tour/DOCS.mdx: Cannot read properties of undefined (reading '0')
    at new SourceMap (/node_modules/@babel/generator/lib/source-map.js:29:99)
    at new Generator (/node_modules/@babel/generator/lib/index.js:13:35)
    at generate (/node_modules/@babel/generator/lib/index.js:88:15)
    at generateCode (/node_modules/@babel/core/lib/transformation/file/generate.js:47:39)
    at run (/node_modules/@babel/core/lib/transformation/index.js:39:33)
    at run.next (<anonymous>)
    at transform (/node_modules/@babel/core/lib/transform.js:22:41)
    at transform.next (<anonymous>)
    at step (/node_modules/gensync/index.js:261:32)
    at /node_modules/gensync/index.js:273:13
    at async.call.result.err.err (/node_modules/gensync/index.js:223:11)
 @ ./src/app/pages/pages.js 103:36-119
 @ ./src/app/components/App.js 25:0-46 311:35-47 313:27-39
 @ ./src/index.js 13:0-43 67:23-26

That seems to map to this bit of code in source-map.js (from node_modules):

      if (resolvedSources.length) {
        for (let i = 0; i < resolvedSources.length; i++) {
          (0, _genMapping.setSourceContent)(map, resolvedSources[i], this._inputMap.sourcesContent[i]);  // <- this line
        }
      }

If I remove the devtool configuration, the exception goes away (I presume, since the build is no longer generating source maps). My build system is quite complex; I'll try to provide more details if necessary.

[liuxingbaoyu]

@stefcameron Thank you for your report! Can you show opts.inputSourceMap?

[stefcameron]

@liuxingbaoyu You're welcome! Here is one sample for opts.inputSourceMap:

{
  "version": 3,
  "sources": [
    "/packages/core/DOCS.mdx"
  ],
  "names": [
    "props",
    "components",
    "corePkgPropDefs",
    "propBoxModes",
    "EXPORT",
    "coreDomExportDefs",
    "coreLayerExportDefs",
    "coreStyleExportDefs",
    "coreUtilExportDefs",
    "coreExportDefs"
  ],
  "mappings": ";;OACO,YAAY;OACZ,eAAe;QAEb,SAAS,mBAAoB;OAA6B,qBAAqB;OACX,oBAAoB;OACtB,uBAAuB;OACjB,yBAAyB;OACrB,yBAAyB;OACzB,wBAAwB;;;;;;;;;;;;kBAGzFA,MAAMC;;;gBAEvB;;;gBAEC;;gBAEJ;;gBAEA;;;;;;;;oBAYgB;cACNC;;;gBAGN;;gBAEJ;;;oBAIgB;YACRC,aAAaC;cACXC;;;gBAGN;;gBAEJ;;;oBAIgB;YACRF,aAAaC;cACXE;;;gBAGN;;gBAEJ;;;oBAIgB;YACRH,aAAaC;cACXG;;;gBAGN;;iBAEJ;;kBAA6B;UAA4B;;gBAEzD;;;oBAIgB;YACRJ,aAAaC;cACXI;;;gBAGN;;gBAEJ;;;oBAIgB;YACRL,aAAaC;cACXK;;;gBAGP;;kBAEoBT,MAAMC",
  "file": "/packages/core/DOCS.mdx"
}

[EvHaus]

I'm seeing the same issue as @stefcameron with the rehype-video dependency:

TypeError: /myproject/node_modules/rehype-video/lib/index.js: Cannot read properties of undefined (reading '0')

      10 | import rehypeRaw from 'rehype-raw';
      11 | import rehypeSanitize, { defaultSchema } from 'rehype-sanitize';
    > 12 | import rehypeVideo from 'rehype-video';
         | ^
      13 | import remarkBreaks from 'remark-breaks';
      14 | import remarkGemoji from 'remark-gemoji';
      15 | import remarkGfm from 'remark-gfm';

      at new SourceMap (../../node_modules/@babel/core/node_modules/@babel/generator/src/source-map.ts:61:42)
      at new Generator (../../node_modules/@babel/core/node_modules/@babel/generator/src/index.ts:24:35)
      at generate (../../node_modules/@babel/core/node_modules/@babel/generator/src/index.ts:271:15)
      at generateCode (../../node_modules/@babel/core/src/transformation/file/generate.ts:35:22)
      at run (../../node_modules/@babel/core/src/transformation/index.ts:59:48)
          at run.next (<anonymous>)
      at transform (../../node_modules/@babel/core/src/transform.ts:29:20)
          at transform.next (<anonymous>)
      at evaluateSync (../../node_modules/gensync/index.js:251:28)
      at sync (../../node_modules/gensync/index.js:89:14)
      at fn (../../node_modules/@babel/core/src/errors/rewrite-stack-trace.ts:97:14)
      at transformSync (../../node_modules/@babel/core/src/transform.ts:66:52)
      at ScriptTransformer.transformSource (../../node_modules/@jest/transform/build/ScriptTransformer.js:519:31)
      at ScriptTransformer._transformAndBuildScript (../../node_modules/@jest/transform/build/ScriptTransformer.js:648:40)
      at ScriptTransformer.transform (../../node_modules/@jest/transform/build/ScriptTransformer.js:700:19)
      at Object.require (src/Markdown.tsx:12:1)
      at Object.require (src/index.ts:1:1)
      at Object.require (__tests__/comments.test.tsx:3:1)

Given that this is an npm dependency, I think it should be easy to reproduce.

Looking at the code for rehype-video I'm not sure what the problem is.

[liuxingbaoyu]

@EvHaus This is fixed, just need to update babel!
#15444 (comment)!

[stefcameron]

@EvHaus #15444 was opened, and then appears to be fixed with #15445, will be published in Babel v7.21.1

[SimenB]

@nicolo-ribaudo incoming via jestjs/jest#14073, FWIW

(feel free to ping me whenever these change - happy to be quick to update in Jest as needed 🙂 )