improve AST tests & tools #4873
Conversation
.github/workflows/build.yml
Outdated
| @@ -8,7 +8,11 @@ jobs: | |||
| strategy: | |||
| fail-fast: false | |||
| matrix: | |||
| options: [ '-mb braces', '--ie8 -c', '-mc', '--toplevel -mc passes=3,pure_getters,unsafe' ] | |||
| options: | |||
| - '-p acorn -mb braces -o spidermonkey' | |||
There was a problem hiding this comment.
@kzc so what I did is to augment --in-situ to handle -o spidermonkey by doing a round-trip, i.e. AST_Node.from_mozilla_ast(uglified.to_mozilla_ast()), which should cover this beyond .ts files which you've done in #4870 (comment)
There was a problem hiding this comment.
You weren't alone there 😉
There was a problem hiding this comment.
So does --in-situ unconditionally round-trip uglify -> mozilla -> uglify AST now?
There was a problem hiding this comment.
Not with --in-situ alone − you need to specify --output spidermonkey
| FROM_MOZ_STACK.pop(); | ||
| return ret; | ||
| return node; |
There was a problem hiding this comment.
@kzc this is #4870 (comment)
lib/mozilla-ast.js
Outdated
| if (moz) { | ||
| if (!HOP(MOZ_TO_ME, moz.type)) { | ||
| var s = my_start_token(moz); | ||
| js_error("Unsupported type: " + moz.type, s.file, s.line, s.col, s.pos); |
There was a problem hiding this comment.
I tried that too - the s.file, s.line, s.col, s.pos part is useless for mozilla nodes. Only moz.start and moz.end positions are available. The other use of js_error in this source file may also be wrong.
There was a problem hiding this comment.
Yeah, probably should just stick to throw new Error() like most other places in this file...
| @@ -15,7 +15,7 @@ minify_in_situ() { | |||
| for i in `find $DIRS -type f -name '*.ts' | grep -v '\.d\.ts'` | |||
| do | |||
| echo "$i" | |||
| node_modules/.bin/esbuild --loader=ts --target=node14 < "$i" \ | |||
| node_modules/.bin/esbuild --loader=ts --target=es2019 < "$i" \ | |||
There was a problem hiding this comment.
--target=es2020 is needed to produce ChainExpressions, otherwise they are down-leveled to ES2019.
There was a problem hiding this comment.
?. aren't supported by uglify-js yet.
There was a problem hiding this comment.
Also we'll probably need es2021 when we eventually clear that since otherwise private class prorperties won't get through (no idea if esbuild does that, but that's how acorn works...)
There was a problem hiding this comment.
esbuild handles everything in the ES standard, Typescript, and likely to be approved stage 3 proposals already implemented by Chrome, including private class properties. esbuild would have a different set of generated helper functions than what Babel or tsc produce for down-leveling to earlier versions of ES.
There was a problem hiding this comment.
$ esbuild --version
0.11.14
$ echo 'class C { #p; static #s; }' | esbuild --target=es2020
var _p, _s;
class C {
constructor() {
_p.set(this, void 0);
}
}
_p = new WeakMap();
_s = new WeakMap();
_s.set(C, void 0);
$ echo 'class C { #p; static #s; }' | esbuild --target=esnext
class C {
#p;
static #s;
}
|
Using #4873: |
|
Consider round tripping all compress tests through mozilla API as terser does to get better coverage: https://github.com/terser/terser/blob/7230d7eb65/test/run-tests.js#L117-L126 |
|
TBH I'm not that bothered by this functionality − I have absolutely zero use for it, not to mention I never use any features from ECMAScript in the first place. So if build testing turns out to give me too much of a headache I'll just back those extra tests out, let along adding more. |
|
No big deal. I don't use ESTree either. This is just a crossword puzzle like distraction for me. Tools like the Parcel bundler use ESTree. They might find this useful. Terser does not have full ES6+ ESTree support. |
|
To be fair, this exercise did uncover two parser bugs in |
|
Oh great − those pesky |
Yeah, that had crossed my mind as well. Once it passes through acorn that information is lost. Acorn does have a comments API but it is not associated with AST nodes, rather it is solely based on position. Nevermind the parentheses issue. |
|
For uglify-only purposes the CallExpression and NewExpression ESTree nodes could be augmented with a |
|
Guess I'll be backing those build test changes back out then − will fix up whatever it found and verifies them locally. |
|
Can you put the test changes behind a flag? Which files are we talking about specifically? |
Oh I'm only changing
I don't have a comprehensive list yet since I'm still bisecting through the failed jobs, but this one issue is a showstopper for use on GitHub Actions anyway. |
|
Sorry, I wasn't clear... I meant which uglify-js source code files or test script files need to be backed out? |
Only |
|
OT: part of the motivation for working on |
|
This is how rollup uses acorn to assign comments to ESTree nodes. With a little effort pure annotated calls/news could be found... https://github.com/acornjs/acorn/tree/master/acorn#interface onComment: If a function is passed for this option, whenever a comment is encountered the function will be called with the following parameters: |
|
@kzc would you mind running |
|
Another reason we won't be testing |
|
TIL: acorn |
These commands ran successfully to completion on macos with #4873: |
It's an ECMAScript thing... NodeJS |
|
I've tried both Node.js v14.16.0 & v14.16.1 − firstly, Anyway, with |
Did it ever work on your mac? What version of macos are you running?
Clearly the uglify-js alias is not working in your version of bash - try using a bash $variable in the script instead. This is my local version:
I'm running mac OSX 10.9.5 with a macports legacy dylib installed in order to run esbuild and the go runtime. But this error is something I've never encountered. Wait...
This might be related to trying to replace a file while it is being read... Try something like:
I'm less troubled by these watch timeouts. These tests are flaky at the best of times. Also, if npm wasn't able to build or install the native version of |
|
Regarding the In the event of failure, the resultant source file will not exist and will not build. |
|
I forgot that we're using esbuild-wasm instead of native esbuild, but the pipe/mv thing above still applies. What does this command produce on your mac? |
So I've never used $ sh --version
GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin15)$ bash --version
GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin15)
Copyright (C) 2007 Free Software Foundation, Inc.$ uname -a
Darwin MacBook-Pro.local 15.6.0 Darwin Kernel Version 15.6.0: Thu Jun 21 20:07:40 PDT 2018; root:xnu-3248.73.11~1/RELEASE_X86_64 x86_64 |
I am rather concerned, as this isn't intermittent failures − they are readily reproducible. Yet they must have worked before, as I verified and committed the script from this exact machine in the first place❗🤨 |
$ ./node_modules/.bin/esbuild --version && echo 'console.log(1 ? 2 : 3)' | ./node_modules/.bin/esbuild --minify
0.8.56
console.log(2); |
|
FWIW |
|
|
Just a few patch versions ahead of mine. Anything different here?
Interesting. Even with no minify options specified for rollup-ts.sh?
It's quite puzzling indeed. Your system is more recent than mine, but not substantially different. Did replacing the uglify-js alias in the script with an env var help? Maybe try upgrading esbuild-wasm? |
I missed this part - so |
|
What's your default shell? https://www.howtogeek.com/444596/how-to-change-the-default-shell-to-bash-in-macos-catalina/ |
Looks like fewer $ ulimit -a | grep -v unlimited
core file size (blocks, -c) 0
open files (-n) 256
pipe size (512 bytes, -p) 1
stack size (kbytes, -s) 8192
max user processes (-u) 709
$ echo $SHELL
/bin/bash
No parameters at all for both
I think given #4873 (comment), $ ./test/release/rollup-ts.shbehaves differently than |
Using |
|
Yeah $ alias uglify-js=$PWD/bin/uglifyjs
$ uglify-js -v
uglify-js 3.13.4$ cat test.sh
#!/bin/sh
alias uglify-js=$PWD/bin/uglifyjs
uglify-js -v$ chmod a+x test.sh
$ ./test.sh
uglify-js 3.13.4$ bash test.sh
test.sh: line 4: uglify-js: command not found |
|
So where does your machine stand? Can it successfully run |
|
Nothing's changed − still getting those time-outs as shown in 2nd part of #4873 (comment) |
Granted, the alias problem is bash specific. So it works with sh or using a regular env var, I assume. But does running Did the If you can isolate the esbuild-wasm problem then a bug can be filed with esbuild. |
|
Without the
None of those made any difference when |
Great, use
That stands to reason. |
|
Here's a sub-optimal but working proof of concept for parsing pure annotations with acorn. It's a peculiar algorithm where non-call non-new nodes consume all (invalid) pure comments before them, leaving the valid pure annotations solely to be consumed by the outermost --- a/bin/uglifyjs
+++ b/bin/uglifyjs
@@ -314,16 +314,36 @@ function run() {
try {
if (options.parse) {
if (options.parse.acorn) {
+ var pureAnnotations = [];
files = convert_ast(function(toplevel, name) {
return require("acorn").parse(files[name], {
allowHashBang: true,
ecmaVersion: "latest",
locations: true,
+ onComment: function(block, text, spos, epos) {
+ if (/[@#]__PURE__/.test(text)) {
+ pureAnnotations.push({ text: text, epos: epos, });
+ }
+ },
program: toplevel,
sourceFile: name,
sourceType: "module",
});
});
+ files.walk(new UglifyJS.TreeWalker(function(node) {
+ if (node instanceof UglifyJS.AST_SimpleStatement) return;
+ if (node instanceof UglifyJS.AST_Sequence) return;
+ var before = pureAnnotations.filter(function(c) {
+ return c.epos <= node.start.pos;
+ });
+ if (before.length && node instanceof UglifyJS.AST_Call) {
+ node.pure = before[before.length - 1].text;
+ }
+ before.forEach(function(c) {
+ var pos = pureAnnotations.indexOf(c);
+ if (pos >= 0) pureAnnotations.splice(pos, 1);
+ });
+ }));
} else if (options.parse.spidermonkey) {
files = convert_ast(function(toplevel, name) {
var obj = JSON.parse(files[name]);
diff --git a/lib/mozilla-ast.js b/lib/mozilla-ast.js
index 028441b..d044ab4 100644
--- a/lib/mozilla-ast.js
+++ b/lib/mozilla-ast.js
@@ -570,8 +570,8 @@
map("AssignmentExpression", AST_Assign, "operator=operator, left>left, right>right");
map("AssignmentPattern", AST_DefaultValue, "left>name, right>value");
map("ConditionalExpression", AST_Conditional, "test>condition, consequent>consequent, alternate>alternative");
- map("NewExpression", AST_New, "callee>expression, arguments@args");
- map("CallExpression", AST_Call, "callee>expression, arguments@args");
+ map("NewExpression", AST_New, "callee>expression, arguments@args, pure=pure");
+ map("CallExpression", AST_Call, "callee>expression, arguments@args, pure=pure");
map("SequenceExpression", AST_Sequence, "expressions@expressions");
map("SpreadElement", AST_Spread, "argument>expression");
map("ObjectExpression", AST_Object, "properties@properties");A test case: The patch also extends the mozilla translation layer by adding a Demonstration using the acorn parser, output to estree, input from estree, output with annotations: same as above, but with compress: Demonstration using the uglify parser, output to estree, input from estree, output with annotations: same as above, but with compress: This ESTree This ESTree NOTE: The patch doesn't work with multiple inputs and I did not bother to investigate it further: expected: actual: |
|
This is a much more efficient implementation of the acorn pure annotation parser. Only the diff --git a/bin/uglifyjs b/bin/uglifyjs
index 69a98a4..33d5d72 100755
--- a/bin/uglifyjs
+++ b/bin/uglifyjs
@@ -314,16 +314,35 @@ function run() {
try {
if (options.parse) {
if (options.parse.acorn) {
+ var pureAnnotations = [], pureProcessed = 0;
files = convert_ast(function(toplevel, name) {
return require("acorn").parse(files[name], {
allowHashBang: true,
ecmaVersion: "latest",
locations: true,
+ onComment: function(block, text, spos, epos) {
+ if (/[@#]__PURE__/.test(text)) {
+ pureAnnotations.push({ text: text, epos: epos, });
+ }
+ },
program: toplevel,
sourceFile: name,
sourceType: "module",
});
});
+ files.walk(new UglifyJS.TreeWalker(function(node) {
+ if (node instanceof UglifyJS.AST_SimpleStatement) return;
+ if (node instanceof UglifyJS.AST_Sequence) return;
+ while (pureProcessed < pureAnnotations.length) {
+ var comment = pureAnnotations[pureProcessed];
+ if (comment.epos > node.start.pos) break;
+ var pure = comment;
+ ++pureProcessed;
+ }
+ if (pure && node instanceof UglifyJS.AST_Call) {
+ node.pure = pure.text;
+ }
+ }));
} else if (options.parse.spidermonkey) {
files = convert_ast(function(toplevel, name) {
var obj = JSON.parse(files[name]);
diff --git a/lib/mozilla-ast.js b/lib/mozilla-ast.js
index 028441b..d044ab4 100644
--- a/lib/mozilla-ast.js
+++ b/lib/mozilla-ast.js
@@ -570,8 +570,8 @@
map("AssignmentExpression", AST_Assign, "operator=operator, left>left, right>right");
map("AssignmentPattern", AST_DefaultValue, "left>name, right>value");
map("ConditionalExpression", AST_Conditional, "test>condition, consequent>consequent, alternate>alternative");
- map("NewExpression", AST_New, "callee>expression, arguments@args");
- map("CallExpression", AST_Call, "callee>expression, arguments@args");
+ map("NewExpression", AST_New, "callee>expression, arguments@args, pure=pure");
+ map("CallExpression", AST_Call, "callee>expression, arguments@args, pure=pure");
map("SequenceExpression", AST_Sequence, "expressions@expressions");
map("SpreadElement", AST_Spread, "argument>expression");
map("ObjectExpression", AST_Object, "properties@properties");However, both this patch and the previous one have a bug... Expected result using the uglify parser: Actual incorrect result using the acorn parse option: And sure enough, Rollup has the same bug due to its similar algorithm: One possible solution would be to scan the input source code string backwards from the node start to the end of the previous pure comment to verify that there's only whitespace - or possibly open parentheses There's probably a better solution. Have to give it some thought. |
No description provided.