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

[Bug]: Nodes without loc included in sourcemap #16052

Open
1 task done
DylanPiercey opened this issue Oct 18, 2023 · 10 comments
Open
1 task done

[Bug]: Nodes without loc included in sourcemap #16052

DylanPiercey opened this issue Oct 18, 2023 · 10 comments

Comments

@DylanPiercey
Copy link

💻

  • Would you like to work on a fix?

How are you using Babel?

Programmatic API (babel.transform, babel.parse)

Input code

Babel seems to be assigning source locations to nodes which do not have a source location at all.
The below example code adds some ast without a sourcemap around an existing node.

In this case the source map for the output || 1 points to the b identifier in the source code.
This is especially problematic for code coverage which will cause this output code to count as a potentially uncovered branch.

import * as t from "@babel/types";
import * as babel from "@babel/core";
const result = await babel.transformAsync(`a = b`, {
  babelrc: false,
  configFile: false,
  sourceMaps: "inline",
  plugins: [
    {
      visitor: {
        AssignmentExpression(assignmentExpression) {
          assignmentExpression
            .get("right")
            .replaceWith(
              t.logicalExpression(
                "||",
                assignmentExpression.get("right").node,
                t.numericLiteral(1)
              )
            );
        },
      },
    },
  ],
});

Outputs the following code:

a = b || 1;
//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJuYW1lcyI6WyJhIiwiYiJdLCJzb3VyY2VzIjpbInVua25vd24iXSwic291cmNlc0NvbnRlbnQiOlsiYSA9IGIiXSwibWFwcGluZ3MiOiJBQUFBQSxDQUFDLEdBQUdDLENBQUMifQ==

You can view that the sourcemap is incorrect here https://evanw.github.io/source-map-visualization/#MjMyAGEgPSBiIHx8IDE7Ci8vIyBzb3VyY2VNYXBwaW5nVVJMPWRhdGE6YXBwbGljYXRpb24vanNvbjtjaGFyc2V0PXV0Zi04O2Jhc2U2NCxleUoyWlhKemFXOXVJam96TENKdVlXMWxjeUk2V3lKaElpd2lZaUpkTENKemIzVnlZMlZ6SWpwYkluVnVhMjV2ZDI0aVhTd2ljMjkxY21ObGMwTnZiblJsYm5RaU9sc2lZU0E5SUdJaVhTd2liV0Z3Y0dsdVozTWlPaUpCUVVGQlFTeERRVUZETEVkQlFVZERMRU5CUVVNaWZRPT0xMTUAeyJ2ZXJzaW9uIjozLCJuYW1lcyI6WyJhIiwiYiJdLCJzb3VyY2VzIjpbInVua25vd24iXSwic291cmNlc0NvbnRlbnQiOlsiYSA9IGIiXSwibWFwcGluZ3MiOiJBQUFBQSxDQUFDLEdBQUdDLENBQUMifQ==

Configuration file name

No response

Configuration

No response

Current and expected behavior

Currently the output sourcemap contains unnecessary and invalid additional mappings.

I would expect that only mappings related to nodes with a loc get output in the sourcemap.

Environment

System:
OS: macOS 13.5
Binaries:
Node: 18.18.0 - ~/.nvm/versions/node/v18.18.0/bin/node
Yarn: 1.22.19 - /opt/homebrew/bin/yarn
npm: 9.8.1 - ~/.nvm/versions/node/v18.18.0/bin/npm
pnpm: 8.8.0 - ~/pnpm/pnpm
npmPackages:
@babel/core: ^7.23.2 => 7.23.2

Possible solution

The issue appears to be related to this code which is not cleared and applies it's position to future nodes.

Additional context

No response

@babel-bot
Copy link
Collaborator

Hey @DylanPiercey! We really appreciate you taking the time to report an issue. The collaborators on this project attempt to help as many people as possible, but we're a limited number of volunteers, so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack community that typically always has someone willing to help. You can sign-up here for an invite.

@DylanPiercey DylanPiercey changed the title [Bug]: [Bug]: Nodes without loc included in sourcemap Oct 18, 2023
@SiddiqAM1
Copy link

SiddiqAM1 commented Oct 20, 2023

@DylanPiercey How Do you Know the Posibble solution. just curious . new to open source. What are the prerequisite knowledge required to fin solutions to these problems

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Oct 20, 2023

@DylanPiercey
Copy link
Author

@nicolo-ribaudo the problem is that final mapping of the || 1. That code is not in the source and should not be mapped back anywhere.

My main issue is I've got a Babel transform that outputs something like this, but the || 1 is being marked as an uncovered branch in the test coverage. This ends up being very confusing because this code is not in the source at all. Generally my understanding is that nodes added (not in the source) should not be added to the source map.

@DylanPiercey
Copy link
Author

@SiddiqAM1 I scanned through @babel/generator which is what creates the source maps. While debugging through that I could see the reason for the additional mappings was because of a long lived _sourcePosition which I believe should be cleared when we enter a new unmapped node.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Oct 20, 2023

@DylanPiercey Source maps do not map "source ranges", but they map "source points". In this case, we are mapping these points of the output (adding _ between characters just for clarity):

_a_ _=_ _b_ _||_ _1_;_
↓ ↓     ↓ ↓
_a_ _=_ _b_;_

We are not mapping || 1 explicitly to anything.

@DylanPiercey
Copy link
Author

@nicolo-ribaudo am I wrong in thinking that the map should be reset or something like that (pardon my lack of understanding) after the b though? It just confuses me that both sourcemap visualizers show the || 1 as mapped (I see that it's a "point" rather than a range as you say) and that this mapping is seemingly also picked up via sourcemap consumers.

Appreciate your time discussing this!

@nicolo-ribaudo
Copy link
Member

Source map don't really support "resetting". One possible way is to generate a source map referring to two source files: one is your original file and one is just an empty virtual file. Then, all the generated code could point to the empty file. However, this feels very hacky 😬

We are working on a proper source maps spec, and there is some relevant discussion going on at tc39/source-map#32.

@DylanPiercey
Copy link
Author

@nicolo-ribaudo really appreciate the link. In that issue they bring up terser/terser#1106 (comment) as a similar issue. Although from what I understand they fixed that on the terser side.

It looks like they did this here: https://github.com/terser/terser/pull/1106/files#diff-1378f0e2a198059d9d7cf6a4718b869f277986aef3198af9d92af29e1ce2f906R93-R98

Is that something babel could do as well?

@jridgewell
Copy link
Member

Source map don't really support "resetting".

Source maps have a "sourceless" segment ([genColumn] segment instead of a "source" segment [genColumn, sourcesIndex, sourceLine, sourceColumn] or "named" segment [genColumn, sourcesIndex, sourceLine, sourceColumn, namesIndex]), which is for exactly this purpose. But, the problem here isn't the || 1 that we're inserting, Babel isn't marking that with source index. It's the AssignmentExpression itself and the parent ExpressionStatement, we're marking the end locations of those two nodes.

Compare:

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

No branches or pull requests

5 participants