JS optimizer: Fix scoping issues with non-defined functions #23159
Conversation
|
@sbc100 I'm not sure what to do with the prettier error here. On CI the error is without explanation, so I tried to run it locally, |
test/js_optimizer/emitDCEGraph6.js
Outdated
| @@ -0,0 +1,35 @@ | |||
|
|
|||
There was a problem hiding this comment.
I wonder if we can give this (and the other tests 1-5) better names? Can be followup maybe?
There was a problem hiding this comment.
I'll rename the new one in this PR. I can look at the others later.
test/js_optimizer/emitDCEGraph6.js
Outdated
| } | ||
|
|
||
| wasmImports = {}; | ||
|
|
There was a problem hiding this comment.
Remove extra newline at start and end of this file.
If you run that on |
tools/acorn-optimizer.mjs
Outdated
| } | ||
| } | ||
| } | ||
| } else if (node.type === 'Property' && node.method) { |
There was a problem hiding this comment.
assert(specialScopes > 0) ?
There was a problem hiding this comment.
(Sorry just saw the one below.. probably don't need both)
There was a problem hiding this comment.
Why, I think both make equal sense?
|
Correct, it does nothing on the main branch, but does a 334-line diff on this one, though my changes are an order of magnitude smaller than that. I'm not familiar with prettier - is this a known issue? |
No idea at all.. that seems very odd. It should always format the whole file.. regardless of which part you touch. |
|
Ok, I pushed the prettier results (and the other feedback). The diff is now larger, but most of that is whitespace. I don't quite understand why it decided to change that whitespace, but it doesn't look bad... |
Its because it changed the indentation of argument to fullWalk on line 711. Viewing with ignore whitespace works. |
|
Do you know offhand if methods in js classes work with metadce? |
|
Does that use the same syntax as object methods? ( |
|
Landing to fix the user issue - if there is more to do here, we can look into that later. |
…en-core#23159) Other than simple defined functions (function foo() {}), JS also has arrow functions and object methods. We need to be aware of those in metadce. This fixes emscripten-core#22968 which was an issue with an object method that enclosed a function of the same name as another toplevel function. To fix this, this PR makes us aware of such scopes, and we do not optimize functions in them. That is, we only ever optimize defined functions at the toplevel scope, and anything enclosed is considered fixed.
3 participants
Other than simple defined functions
function foo() {}, JS also hasarrow functions and object methods. We need to be aware of those
in metadce.
This fixes #22968 which was an issue with an object method that
enclosed a function of the same name as another toplevel function.
To fix this, this PR makes us aware of such scopes, and we do not
optimize functions in them. That is, we only ever optimize defined
functions at the toplevel scope, and anything enclosed is considered
fixed.