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

fix chained evaluation #1610

Merged
merged 1 commit into from
Mar 16, 2017
Merged

fix chained evaluation #1610

merged 1 commit into from
Mar 16, 2017

Conversation

alexlamsl
Copy link
Collaborator

reduce_vars enables substitution of variables but did not clone the value's AST_Node.

This confuses collapse_vars and result in invalid AST and subsequent crash.

fixes #1609

`reduce_vars` enables substitution of variables but did not clone the value's `AST_Node`.

This confuses `collapse_vars` and result in invalid AST and subsequent crash.

fixes mishoo#1609
var a = "long piece of string";
(function() {
var c;
c = f(a);
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks odd. collapse_vars would not make this substitution of f(b) -> f(a) because function f is undeclared and would be a side effect. There might be still another clone issue in reduce_vars in var b of value a.

v2.7.5 by comparison produces:

$ uglifyjs chained_evaluation_2.js -c collapse_vars,reduce_vars -b
WARN: Replacing constant a [chained_evaluation_2.js:4,24]
!function() {
    !function() {
        var c, b = "long piece of string";
        c = f(b), c.bar = b;
    }();
}();

Copy link
Contributor

Choose a reason for hiding this comment

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

@alexlamsl Did reduce_vars eliminate var b altogether and replace all its references with a? If so, then it makes sense.

Copy link
Contributor

@kzc kzc Mar 16, 2017

Choose a reason for hiding this comment

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

Indeed - with this PR or 2.8.12:

$ bin/uglifyjs chained_evaluation_2.js -c collapse_vars=false,reduce_vars=true -b
WARN: Dropping unused variable b [chained_evaluation_2.js:4,20]
!function() {
    var a = "long piece of string";
    !function() {
        var c;
        c = f(a), c.bar = a;
    }();
}();

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, you need both reduce_vars and collapse_vars to trigger this issue.

It's reduce_vars fault, because it didn't clone the AST_Node when performing the substitution, then collapse_vars operated on the resulting invalid AST and gave us AST_Binary.right === null.

@kzc
Copy link
Contributor

kzc commented Mar 16, 2017

Travis CI is erring out on installing node 4:

https://travis-ci.org/mishoo/UglifyJS2/jobs/211675141

@alexlamsl
Copy link
Collaborator Author

Travis CI is erring out on installing node 4:

See #1604 (comment)

(Sorry I was away playing badminton.)

@alexlamsl alexlamsl merged commit ac40301 into mishoo:master Mar 16, 2017
@alexlamsl alexlamsl deleted the issue-1609 branch March 16, 2017 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeError: Cannot read property '_walk' of null
2 participants