Quote the error-causing text in parse errors #793
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kripken
added a commit
to emscripten-core/emscripten
that referenced
this pull request
Feb 8, 2019
…#7973) Fixes #6000 The key change here is to rewrite the JS optimizer passes that run in a normal `-O3` etc. build from the Uglify1 AST to ESTree. With ESTree we can use modern parsers etc. so that we support ES6+ inputs to js libraries, pre-jses, EM_ASM, etc. Aside from that rewrite, the other changes are less critical and can be altered later. Specifically, this uses acorn for parsing and terser for outputting, but we could switch to anything using ESTree very easily. Acorn is nice for parsing since it's small and standalone. For outputting I experimented with astring, which is small and nice, and escodegen, which looks very robust, but neither could output compact-enough JS to not regress our JS code sizes. This is not truly critical since for minimal code size people should use closure anyhow, however, it's nice for default builds to be small (and we don't run closure by default), and I didn't want to regress anything. Using the terser outputter achieves that. (Since it uses the Uglify2 AST internally, this means using their tool to convert ESTree to Uglify2.) They may be some minor code size changes with this PR, just because we use a different outputter now, but nothing major in either direction. Most changes seem positive actually. Sizes after closure are unchanged. This uses almost unmodified versions of acorn and terser, but they are stripped down to what we need, and I had to make two modifications, see these PRs: [acornjs/acorn#793](acornjs/acorn#793) (quote the error on parse exceptions) and [mishoo/UglifyJS#3323](mishoo/UglifyJS#3323) (preserve quoted properties). This may very slightly regress compile times when using those passes, as Uglify1 was just very fast. However, the change should be very small. This does _not_ rewrite every single JS optimizer pass. In particular the asm.js passes don't need to support ES6, and so don't need to be rewritten. There are also optional passes that do not run by default, that we can convert later depending on priority.
|
This seems like something that could be fairly easily done on top of Acorn? I imagine many people might have different preferences regarding error format... |
|
I think I agree—this is quite invasive (see the failing tests, for example, but also expectations that existing client code may have), and can be implemented relatively easily in code that actually reports the error. |
|
Fair enough, I thought this might be nice as a default error, but I understand. Closing. |
yeputons
added a commit
to yeputons/emscripten
that referenced
this pull request
Jul 26, 2019
…s on Windows This fixes a patch applied to acorn.js for quoting error-causing text in parse errors (acornjs/acorn#793). Previously it splitted binary data on \n to get lines, which was inconsistent with the rest of the parser and caused excess \r in the error message on Windows. Now it uses the same regex as the rest of acorn.js (e.g. getLineInfo used by error reporting).
pull bot
pushed a commit
to Pandinosaurus/emscripten
that referenced
this pull request
Jul 26, 2019
* Fix emscripten-core#9068: get rid of malformed newlines in emcc errors on Windows This fixes a patch applied to acorn.js for quoting error-causing text in parse errors (acornjs/acorn#793). Previously it splitted binary data on \n to get lines, which was inconsistent with the rest of the parser and caused excess \r in the error message on Windows. Now it uses the same regex as the rest of acorn.js (e.g. getLineInfo used by error reporting). * Fix emscripten-core#9057 by fixing by emscripten-core#9068
ghost
mentioned this pull request
belraquib
pushed a commit
to belraquib/emscripten
that referenced
this pull request
Dec 23, 2020
* Fix emscripten-core#9068: get rid of malformed newlines in emcc errors on Windows This fixes a patch applied to acorn.js for quoting error-causing text in parse errors (acornjs/acorn#793). Previously it splitted binary data on \n to get lines, which was inconsistent with the rest of the parser and caused excess \r in the error message on Windows. Now it uses the same regex as the rest of acorn.js (e.g. getLineInfo used by error reporting). * Fix emscripten-core#9057 by fixing by emscripten-core#9068
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes parse errors look like this:
That is, it adds a line that quotes the text with the error, and a line under it with
^to point directly at the error.