Skip to content

[WIP] fix usage of transform() vs optimize()#1593

Closed
alexlamsl wants to merge 4 commits intomishoo:masterfrom
alexlamsl:transform-optimize
Closed

[WIP] fix usage of transform() vs optimize()#1593
alexlamsl wants to merge 4 commits intomishoo:masterfrom
alexlamsl:transform-optimize

Conversation

@alexlamsl
Copy link
Copy Markdown
Collaborator

@alexlamsl alexlamsl commented Mar 10, 2017

Given the current OPT() node:

  • optimize(compressor) works on the same level, and will only descend() if returned instance is different
  • transform(compressor) works on child nodes, and will always descend()

fixes #1592

kzc
kzc reviewed Mar 10, 2017
View reviewed changes
test/compress/evaluate.js Outdated
evaluate: true,
}
input: {
console.log(typeof void 0 != "undefined");
Copy link
Copy Markdown
Contributor

@kzc kzc Mar 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add:

console.log(1 == 1, 1 === 1);
console.log(1 != 1, 1 !== 1);

Copy link
Copy Markdown
Collaborator Author

@alexlamsl alexlamsl Mar 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@alexlamsl alexlamsl changed the title fix usage of transform() vs optimize() [WIP] fix usage of transform() vs optimize() Mar 10, 2017
@alexlamsl alexlamsl force-pushed the transform-optimize branch from 07790c7 to 429bbcb Compare March 10, 2017 08:25
@alexlamsl
Copy link
Copy Markdown
Collaborator Author

alexlamsl commented Mar 10, 2017

node test/benchmark.js timings (SHA1 sums are identical):

Code master #1593
jquery-3.1.1.js 1.687 1.625
angular.js 2.734 2.578
math.js 17.844 17.672
bootstrap.js 0.515 0.499
react.js 2.578 2.609
ember.prod.js 5.546 5.437
lodash.js 1.609 1.546
d3.js 3.671 3.484

So with the exception of react.js, everything seems to be a bit faster as well.

@kzc
Copy link
Copy Markdown
Contributor

kzc commented Mar 10, 2017

Prior to this PR, could you explain why the use of transform() rather than optimize() for boolean equalities/inequalities led to producing 1 instead of true and 0 instead of false?

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

alexlamsl commented Mar 10, 2017

It's basically down to this line

Consider console.log(1 === 1), when we call <1 === 1>.evaluate(compressor) we ended up with the following compressor.stack prior to this PR:

console.log(1 === 1)
1 === 1               // returns by compressor.parent()
true                  // pushed by .transform(compressor)

whereas calling .optimize(compressor) gives:

console.log(1 === 1)  // returns by compressor.parent()
1 === 1

which was hardcoded prior to #1477. However, that causes other places to have the wrong stack - and while I was working on that PR the bug manifested itself as compressor.in_boolean_context() malfunctioning.

@kzc
Copy link
Copy Markdown
Contributor

kzc commented Mar 10, 2017

That's quite some extensive analysis.

In light of all the other scenarios you say had the wrong stack, and the size of this PR, is there sufficient test coverage?

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

alexlamsl commented Mar 10, 2017

In light of all the other scenarios you say had the wrong stack, and the size of this PR, is there sufficient test coverage?

Nope. I've done convincing myself the code changes are satisfactory just now, and am currently scratching my head about the exact point you've just mentioned. If done right, these tests should also prevent future misuse of .transform() vs. .optimize(), as I reckon this PR is fixing issues introduced on multiple occasions from the past.

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

alexlamsl commented Mar 10, 2017

On the bright side, if this PR is done right, then the codebase would be more stable for future additions.

I suppoise at the end of the day I'll just have to go through and create a test case for every line changed in lib/compress.js. Between that and dinner though... 😏

@kzc
Copy link
Copy Markdown
Contributor

kzc commented Mar 10, 2017 •
edited
Loading

Because this is a rather serious bug, could the one line fix proposed in #1592 (comment) be used as a temporary stop-gap measure until this PR is more thoroughly tested?

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

alexlamsl commented Mar 10, 2017

@kzc sounds reasonable - at least I can think of more cases that one-line solution fixes than it could potentially break. I'll put it directly onto master and then publish v2.8.12 alongside #1596.

@kzc
Copy link
Copy Markdown
Contributor

kzc commented Mar 10, 2017

at least I can think of more cases that one-line solution fixes than it could potentially break

Can you show an example of something that would work with 2.8.11 but break with #1592 (comment) ?

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

alexlamsl commented Mar 10, 2017 •
edited
Loading

Can you show an example of something that would work with 2.8.11 but break with #1592 (comment) ?

The stack is all messed up, though I went through all the places where OPT(AST_Boolean) can be called, and they are either:

  • statements, i.e. you can't put it within a == stat ❌
  • discards AST_Node.evaluate(compressor)[0] and only use the constant value part
  • has to be compressor.in_boolean_context(), which excludes a == b

So AFAICT they all conveniently evaded https://github.com/mishoo/UglifyJS2/blob/b633706ce42576b7e2aa85a96c5691bde87e71ac/lib/compress.js#L3676-L3678

I guess I'll create a new PR with the one-liner just to close #1592, then leave this as the clean-up.

@alexlamsl
fix usage of transform() vs optimize()
7550c53 
Given the current `OPT()` node:
- `optimize(compressor)` works on the same level, and will only `descend()` if returned instance is different
- `transform(compressor)` works on child nodes, and will always `descend()`

Other fixes
- `loopcontrol_target()` should compare against `compressor.self()` since the current node may have already been displaced
- remove obsolete optimisation in `AST_Binary` after mishoo#1477

fixes mishoo#1592
@alexlamsl alexlamsl force-pushed the transform-optimize branch from c25a92e to 7550c53 Compare March 10, 2017 21:08
@alexlamsl
Copy link
Copy Markdown
Collaborator Author

alexlamsl commented Mar 12, 2017

Please don't try to review this PR for now - I'm taking things apart to see what is going on. 😅

This was referenced Mar 14, 2017
Merged
Merged
Merged
@kzc
Copy link
Copy Markdown
Contributor

kzc commented Mar 15, 2017

Is this PR obsoleted by recent commits?

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

alexlamsl commented Mar 15, 2017

@kzc it is - I'm just keeping it around to track the remaining changes for future PRs.

I can close it now if this is too much of an eye sore 😅

@alexlamsl alexlamsl closed this Mar 15, 2017
@alexlamsl alexlamsl deleted the transform-optimize branch March 25, 2017 21:19
@alexlamsl alexlamsl mentioned this pull request Apr 5, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@kzc kzc kzc left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unexpected dead code removal of string compare result

2 participants

@alexlamsl @kzc