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

parseFileSync span bug #1366

Open
JiangWeixian opened this issue Jan 28, 2021 · 42 comments
Open

parseFileSync span bug #1366

JiangWeixian opened this issue Jan 28, 2021 · 42 comments
Labels

Comments

@JiangWeixian
Copy link

Describe the bug

same file parse twice, different output

Input code

//test.jsx
const a = 1

import { parseFileSync } from '@swc/core';

const ast = parseFileSync('./test.jsx', { jsx: true, syntax: 'ecmascript', comments: true, dynamicImport: true, decorators: true, decoratorsBeforeExport: true, script: true })
const ast1 = parseFileSync('./test.jsx', { jsx: true, syntax: 'ecmascript', comments: true, dynamicImport: true, decorators: true, decoratorsBeforeExport: true, script: true })

console.log(JSON.stringify(ast))
console.log('====')
console.log(JSON.stringify(ast1))

{"type":"Module","span":{"start":0,"end":11,"ctxt":0},"body":[{"type":"VariableDeclaration","span":{"start":0,"end":11,"ctxt":0},"kind":"const","declare":false,"declarations":[{"type":"VariableDeclarator","span":{"start":6,"end":11,"ctxt":0},"id":{"type":"Identifier","span":{"start":6,"end":7,"ctxt":0},"value":"a","typeAnnotation":null,"optional":false},"init":{"type":"NumericLiteral","span":{"start":10,"end":11,"ctxt":0},"value":1},"definite":false}]}],"interpreter":null}
====
{"type":"Module","span":{"start":12,"end":23,"ctxt":0},"body":[{"type":"VariableDeclaration","span":{"start":12,"end":23,"ctxt":0},"kind":"const","declare":false,"declarations":[{"type":"VariableDeclarator","span":{"start":18,"end":23,"ctxt":0},"id":{"type":"Identifier","span":{"start":18,"end":19,"ctxt":0},"value":"a","typeAnnotation":null,"optional":false},"init":{"type":"NumericLiteral","span":{"start":22,"end":23,"ctxt":0},"value":1},"definite":false}]}],"interpreter":null}

first ast span start at 0, second start at 12

Config

{
    // Please copy and paste your .swcrc file here
}

Expected behavior
A clear and concise description of what you expected to happen.

Version
The version of @swc/core:

"@swc/core": "^1.2.46",

Additional context
Add any other context about the problem here.

@Brooooooklyn
Copy link
Member

By design,just change the file name passed into the parseFileSync you'll get the same result.

@JiangWeixian
Copy link
Author

By design,just change the file name passed into the parseFileSync you'll get the same result.

I think it's a bug

@kdy1
Copy link
Member

kdy1 commented Jan 29, 2021

It's not a bug.
If you want span to start with 0, you can create new instance of Compiler, which have same apis.

@kdy1 kdy1 closed this as completed Jan 29, 2021
@JiangWeixian
Copy link
Author

JiangWeixian commented Jan 29, 2021

Compiler

@kdy1

const compiler = new Compiler()
const ast = compiler.parseFileSync('test.jsx', { syntax: 'ecmascript' })
const compiler1 = new Compiler()
const ast1 = compiler1.parseFileSync('test.js', { syntax: 'ecmascript' })

// test.js
const b = 1
// test.jsx
const a = 1
{"type":"Module","span":{"start":0,"end":11,"ctxt":0},"body":[{"type":"VariableDeclaration","span":{"start":0,"end":11,"ctxt":0},"kind":"const","declare":false,"declarations":[{"type":"VariableDeclarator","span":{"start":6,"end":11,"ctxt":0},"id":{"type":"Identifier","span":{"start":6,"end":7,"ctxt":0},"value":"a","typeAnnotation":null,"optional":false},"init":{"type":"NumericLiteral","span":{"start":10,"end":11,"ctxt":0},"value":1},"definite":false}]}],"interpreter":null}
====
{"type":"Module","span":{"start":12,"end":23,"ctxt":0},"body":[{"type":"VariableDeclaration","span":{"start":12,"end":23,"ctxt":0},"kind":"const","declare":false,"declarations":[{"type":"VariableDeclarator","span":{"start":18,"end":23,"ctxt":0},"id":{"type":"Identifier","span":{"start":18,"end":19,"ctxt":0},"value":"b","typeAnnotation":null,"optional":false},"init":{"type":"NumericLiteral","span":{"start":22,"end":23,"ctxt":0},"value":1},"definite":false}]}],"interpreter":null}

second file still start with 12, not working

@kdy1 kdy1 reopened this Jan 29, 2021
@kdy1 kdy1 modified the milestones: v1.2.47, v1.2.48 Jan 29, 2021
@kdy1 kdy1 modified the milestones: v1.2.48, v1.2.49, v1.2.50 Feb 19, 2021
@kdy1 kdy1 removed this from the v1.2.50 milestone Mar 1, 2021
@mikob
Copy link

mikob commented Apr 15, 2021

Having the same issue. Any known workarounds?

Here's a slightly simpler reproduction:

    const compiler1 = new Compiler();
    console.log(compiler1.parseSync("const a = 1", { syntax: "ecmascript" }));
    const compiler2 = new Compiler();
    console.log(compiler2.parseSync("const b = 1", { syntax: "ecmascript" }));

@kdy1
Copy link
Member

kdy1 commented Apr 15, 2021

Currently, there's no workaround.

I think api to convert a span from swc to line-col span is required, though.

@mikob
Copy link

mikob commented Jun 13, 2021

@kdy1 why is line-col needed? Can't \n just be consistently be used for line endings if it's a windows/unix EOL problem?

@kdy1
Copy link
Member

kdy1 commented Jun 14, 2021

@mikob What do you mean? line-col is about span, not line endings.

@mikob
Copy link

mikob commented Jun 14, 2021

Right, I just don't see the immediate benefit to using line-col. I thought maybe because you wanted to be explicit about line endings. I use spans to get offsets and extract pieces of the code using .substr and line-col would only make that harder.

@jdalton
Copy link

jdalton commented Aug 17, 2021

Ran into this issue too trying to make an adapter for estree AST.

@kdy1
Copy link
Member

kdy1 commented Aug 18, 2021

@jdalton I want to remove parse and print with v2 of swc.
I'm going to provide a rust-based plugin API instead.

@jdalton
Copy link

jdalton commented Aug 19, 2021

@kdy1

I want to remove parse and print with v2 of swc.
I'm going to provide a rust-based plugin API instead.

Ah cool. Will there still be a way to expose the AST? I currently have been able workaround this issue using an adapter to track the offset of the start of the root AST and smooth over the other differences with estree.

@kdy1
Copy link
Member

kdy1 commented Aug 20, 2021

You mean, you want to get actual offset from Span of node side?
I think I can provide an API to get line-col span from lo-hi span.

This API, if added, will be removed along with parse/print on v2, but it will work for now.

@jdalton
Copy link

jdalton commented Aug 20, 2021

@kdy1 I've noticed some confusion around this from another user. Folks use the AST from parsers, in formats like babylon and estree, to perform source transformations. The AST usually has properties such as .start, .end or maybe a .range array of [start, end] values. In the case of swc it has a .span object with .start and .end properties. These coordinates allow for source to be modified in substrings at the start and end of various nodes. So having AST is required for this but also the AST should have coordinates of the nodes (start and end string positions).

@kdy1
Copy link
Member

kdy1 commented Aug 20, 2021

Most ast nodes already have span. What swc is lacking is an api to convert span to line-col coordinates.
Do you mean there should be line-col coordinates?

@jdalton
Copy link

jdalton commented Aug 20, 2021

Are we talking past each other? Maybe an example would help. In ast-explorer using acorn with an example of export * as ns from 'a'; the ExportAllDeclaration node has a start of 0 and an end of 24. swc provides these as a span property (almost identical to the values of acorn's range/state/end) but because of #1366 its coordinates are off on subsequent parsings.

@kdy1
Copy link
Member

kdy1 commented Aug 20, 2021

Now I understand it. But I'm not sure about the change.
Plugin api / parse / print is misdesigned and going to be removed in v2.
In other words, AST will not be exposed to js world.

@rizrmd
Copy link

rizrmd commented Nov 20, 2023

This bug is kind of deal breaker for switching to swc, In our product we need to invoke parser multiple times in the same process. Increasing span.start with each invocation will make swc stopped working at some point – especially when parsing huge file.

twada added a commit to twada/power-assert-monorepo that referenced this issue Dec 30, 2023
Yash-Singh1 added a commit to Yash-Singh1/stylex-vscode that referenced this issue Jan 10, 2024
- add move away from swc to roadmap because of accumalating offsets (swc-project/swc#1366)
- strict requirements for doc color in keyvalue properties
@Yash-Singh1
Copy link

For my case, I need this to convert spans to line-column values in an LSP that I am implementing. Currently, this breaks in swc when I have a file with a comment at the top, because that offsets all of the positions of my spans.

@knightedcodemonkey
Copy link

knightedcodemonkey commented Jun 1, 2024

There is a workaround if you run each parse call in it's own child_process.

import { spawnSync } from 'node:child_process'
import { join } from 'node:path'

import type { Module } from '@swc/core'

const spawnChild = (...args: string[]) => {
  const { stdout } = spawnSync('node', [join(import.meta.dirname, 'child.js'), ...args])
  const ast = JSON.parse(stdout.toString()) as Module

  return ast
}
export const reparse = (source: string): Module => {
  return spawnChild(source)
}

const ast1 = reparse('const foo = "bar"')
const ast2 = reparse('const foo = "bar"')

console.log(ast1.span.start === ast2.span.start) // true

child.js

import { argv, stdout } from 'node:process'
import { parseSync } from '@swc/core'

stdout.write(JSON.stringify(parseSync(argv[2])))

I've published this to npm as @knighted/reparse. It also supports passing a filename.

@denolfe
Copy link

denolfe commented Jul 26, 2024

I ran into this when unit testing a function that performed an swc.parse for AST parsing purposes. Only was an issue if performing multiple tests at a time.

The workaround was to always check the span.end when in NODE_ENV, then use that as the offset. Here is a simplified example.

let parseOffset = 0

if (process.env.NODE_ENV === 'test') {
  parseOffset = (await swc.parse('')).span.end
}

// Pass the offset as needed. Example function below

function insertBeforeAndAfterSWC(
  content: string,
  span: ModuleItem['span'],
  parseOffset: number,
): string {
  const { end: preOffsetEnd, start: preOffsetStart } = span

  const start = preOffsetStart - parseOffset
  const end = preOffsetEnd - parseOffset

  const insert = (pos: number, text: string): string => {
    return content.slice(0, pos) + text + content.slice(pos)
  }

  // Perform manipulation of code
  content = insert(end - 1, ')')
  content = insert(start - 1, 'withPayload(')

  return content
}

denolfe added a commit to payloadcms/payload that referenced this issue Jul 26, 2024
Support new `next.config.ts` config file.

Had to do some weird gymnastics around `swc` in order to use it within
unit tests. Had to pass through the `parsed.span.end` value of any
previous iteration and account for it.

Looks to be an open issue here:
swc-project/swc#1366

Fixes #7318
d3jawu added a commit to d3jawu/ectype that referenced this issue Sep 3, 2024
This introduces a framework for analyzer errors that allow analysis to continue (instead of exiting immediately after the first error). It also introduces pretty-printing of errors, with an excerpt of the file where the error was encountered.

Some things are still missing:

* Tests that expect errors do not check the error type, just that an error happened.
* A lot of errors still throw instead of continuing.
* Error codes are inconsistent and overloaded.
* Error messages are inconsistent in style.
* Because of [an SWC bug](swc-project/swc#1366), file excerpts in imported files will be wrong.

However, this PR still adds more functionality than previously existed without removing any, so I'm putting it in despite its incompleteness.

See merge request dejawu/ectype!5
d3jawu added a commit to d3jawu/ectype that referenced this issue Sep 3, 2024
This PR migrates the static analyzer to the [acorn](https://github.com/acornjs/acorn) library. There's a few things that motivate this:

- SWC has [a bug](swc-project/swc#1366) where span positions are not reset when scanning multiple files in the same session.
- SWC's documentation is all for the Rust side, which doesn't match up 1:1 with the JS side.
- SWC does not recognize comments, which I want to use later to annotate tests (e.g. for cases where an error is expected).

I chose acorn because it's by far the most popular JS parsing library, and it supports all the features I need. When I found out Marijn Haverbeke was behind it, that sealed the deal for me.

PR: dejawu/ectype!6
@mmis1000
Copy link

mmis1000 commented Sep 15, 2024

I think this get even worse now, it seems span of Module now exclude comments before first statement and after last statement, so use span.end of previous parse as offset will yield incorrect results now.

@JSerFeng
Copy link
Contributor

JSerFeng commented Nov 8, 2024

Same issue, can we solve this by make a new Compiler here ?

@kdy1
Copy link
Member

kdy1 commented Nov 8, 2024

@JSerFeng Why are you using parseFileSync? parseFileSyncshould not be used in general.

@smokeelow
Copy link

@kdy1 hi, what should be used instead?

@kdy1
Copy link
Member

kdy1 commented Nov 8, 2024

@smokeelow You should use rust APIs instead

@JSerFeng
Copy link
Contributor

@JSerFeng Why are you using parseFileSync? parseFileSyncshould not be used in general.

I'm using new Compiler().parseFile from JS api, but it has the same issue

@kdy1
Copy link
Member

kdy1 commented Nov 11, 2024

@JSerFeng Compiler#parse* or parse* should not be used

@JSerFeng
Copy link
Contributor

JSerFeng commented Nov 11, 2024

Are all parse related JS API is going to be deprecated ?

@kdy1
Copy link
Member

kdy1 commented Nov 11, 2024

Yeap.

@JSerFeng
Copy link
Contributor

JSerFeng commented Nov 11, 2024

astexplore and swc-playground are very helpful, if js api is deprecated, are those 2 tools stay available after then ?

@kdy1
Copy link
Member

kdy1 commented Nov 11, 2024

That's why parse APIs are still there, but I'll find a way to preserve AST explorer/playground AST viewer while dropping parse* apis. I may create a separate Wasm package for it.

@fz6m
Copy link

fz6m commented Nov 12, 2024

Since 2022, I have noticed many issues with the parse/AST related JS API and stopped using them.
I think they should be removed immediately, as continuing to use them is very dangerous.

@kdy1
Copy link
Member

kdy1 commented Nov 12, 2024

Removing an API is a breaking change. That's the only important blocker.

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

No branches or pull requests