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

Show filename when parse fail #9509

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Show filename when parse fail #9509

wants to merge 3 commits into from

Conversation

clinyong
Copy link

@clinyong clinyong commented Feb 14, 2019

Q                       A
Fixed Issues? No
Patch: Bug Fix? No
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR Link No
Any Dependency Changes? No
License MIT

@babel-bot
Copy link
Collaborator

babel-bot commented Feb 14, 2019

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

@nicolo-ribaudo
Copy link
Member

Can you show a before/after comparison?

@nicolo-ribaudo nicolo-ribaudo added PR: Polish 💅 A type of pull request used for our changelog categories pkg: parser labels Feb 14, 2019
@xtuc
Copy link
Member

xtuc commented Feb 14, 2019

@nicolo-ribaudo I think that means we have no tests for it.
@clinyong would you mind adding tests for it?

@clinyong
Copy link
Author

@nicolo-ribaudo An small reproducible example

const babelParser = require('@babel/parser')

babelParser.parse('const t: any = null')
// with filename
// babelParser.parse('const t: any = null', {sourceFilename: 'path/to/input-file.js'})

before

SyntaxError: Unexpected token (1:7)
    at Parser.raise (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:3833:17)
    at Parser.unexpected (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:5145:16)
    at Parser.parseVar (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:7888:18)
    at Parser.parseVarStatement (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:7713:10)
    at Parser.parseStatementContent (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:7309:21)
    at Parser.parseStatement (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:7250:17)
    at Parser.parseBlockOrModuleBlockBody (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:7814:25)
    at Parser.parseBlockBody (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:7801:10)
    at Parser.parseTopLevel (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:7183:10)
    at Parser.parse (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:8662:17)

after (same with before)

SyntaxError: Unexpected token (1:7)
    at Parser.raise (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:3833:17)
    at Parser.unexpected (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:5145:16)
    at Parser.parseVar (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:7888:18)
    at Parser.parseVarStatement (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:7713:10)
    at Parser.parseStatementContent (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:7309:21)
    at Parser.parseStatement (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:7250:17)
    at Parser.parseBlockOrModuleBlockBody (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:7814:25)
    at Parser.parseBlockBody (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:7801:10)
    at Parser.parseTopLevel (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:7183:10)
    at Parser.parse (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:8662:17)

after with filename

- SyntaxError: Unexpected token (1:7)
+ SyntaxError: Unexpected token (path/to/input-file.js:1:7)
    at Parser.raise (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:3833:17)
    at Parser.unexpected (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:5145:16)
    at Parser.parseVar (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:7888:18)
    at Parser.parseVarStatement (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:7713:10)
    at Parser.parseStatementContent (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:7309:21)
    at Parser.parseStatement (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:7250:17)
    at Parser.parseBlockOrModuleBlockBody (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:7814:25)
    at Parser.parseBlockBody (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:7801:10)
    at Parser.parseTopLevel (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:7183:10)
    at Parser.parse (/Users/Leo/workfile/ts-demo/node_modules/@babel/parser/lib/index.js:8662:17)

@xtuc test case is added.

@loganfsmyth
Copy link
Member

Babel itself already does this in

err.message = `${filename}: ${err.message}\n\n` + codeFrame;
so I'd be hesitant to land this because it will double up the name.

@clinyong
Copy link
Author

@loganfsmyth Can we remove the filename from babel-core? By now it is hard to find out which file parse fail when using babel parser.

@loganfsmyth
Copy link
Member

The issue is that users on older versions of @babel/core might still pull in newer versions of the parser, so even if we change core too, users of the older versions would see doubled-up names.

If we make this change, it'd probably need to be toggled with an off-by-default option.

@clinyong
Copy link
Author

@loganfsmyth You are right. And should we make this change? I means is it better to show fail filename here. If it is, I can add the option to @babel/parser as you said.

@nicolo-ribaudo
Copy link
Member

This seems nice to have, so that I get proper <filename>:<line>:<column> in errors in my terminal and my editor supports clicking on that filepath and brings me to the correct line in the correct file.

@JLHwung @liuxingbaoyu Given #9509 (comment), wdyt about doing this for Babel 8?

@nicolo-ribaudo nicolo-ribaudo changed the base branch from master to main August 23, 2023 16:17
@nicolo-ribaudo nicolo-ribaudo added PR: Polish (next major) 💅 A type of pull request used for our changelog categories for next major release and removed PR: Polish 💅 A type of pull request used for our changelog categories labels Aug 23, 2023
@nicolo-ribaudo nicolo-ribaudo marked this pull request as draft August 23, 2023 16:17
@nicolo-ribaudo
Copy link
Member

CI failure should be fixed by #15884.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: parser PR: Polish (next major) 💅 A type of pull request used for our changelog categories for next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants