Fix: AST_Accessor missing start / end tokens #1493

OndrejSpanel · 2017-02-16T10:32:06Z

Fix for #1492

alexlamsl · 2017-02-16T10:44:27Z

Any test to show what issue it caused and whether this fix is working as intended?

OndrejSpanel · 2017-02-16T10:46:41Z

I will prepare the test. What location should I use for the test - a new file in test/mocha?

alexlamsl · 2017-02-16T10:54:13Z

test/mocha sounds good - there are a couple of other files with tests related to UglifyJS.parse() already.

alexlamsl · 2017-02-16T11:25:43Z

test/mocha/accessorTokens-1492.js

+
+        // test there are no nodes without tokens
+        var checkWalker = new UglifyJS.TreeWalker(function(node, descend) {
+            assert(node.start !== undefined);


Hmm, can we be more precise about the value of start and end here?

Preferably with the source code under test spans multiple lines so we can reasonably sure the fields have been populated with sensible values?

alexlamsl · 2017-02-16T11:35:09Z

lib/parse.js

@@ -1320,11 +1320,17 @@ function parse($TEXT, options) {
            var type = start.type;
            var name = as_property_name();
            if (type == "name" && !is("punc", ":")) {
+                var createAccessor = function() {


This looks somewhat similar to embed_tokens() which is use extensively throughout this file. Would you mind considering whether we can reuse that in this case as well?

I do not mind, however this is beyond my current JS abilities. What I have tried is:

a.push(new AST_ObjectGetter({ start : start, key : as_atom_node(), value : embed_tokens(function(){function_(AST_Accessor)}), end : prev() }));

This resulted in a parse error. If you can advise me how do to his properly, I will be glad to implement it.

I'm typing from mobile, but this might work:

var createAccessor = embed_tokens(function() { return function_(AST_Accessor); });

If not, I'll be back in a couple of hours and will take a look then.

Tried that (and thought: "Silly me. off course I have to return in JS, this is not Scala "), but I still get the parsing error:

'SyntaxError: Unexpected token punc «(», expected punc «,»'

My code was:

var createAccessor = function() {embed_tokens(function() { return function_(AST_Accessor); })};

I'm confused - did you try my suggestion or yours? Yours has an extra function() {...} wrapped around it.

I tried both, first what you wrote, then this - I was afraid your solution executes outside of the get / set scopes, now I see it does not - it returns a function, and executes nothing on its own.

Thanks for clarifying. I'll have a closer look when I get home.

I have tried your solution once again and I see its error is different. It outputs no parsing error, only the test is not passed, with assertion:

AssertionError: 22 == 12

What is the correct result - In the test code var obj = { get latest() { return undefined; } }, where should the accessor node start? Pos. 22 is right behind latest, pos. 12 is the beginning of get.

Checking http://lisperator.net/uglifyjs/ast, I think 22 makes more sense as a result, AST_Accessor is a child of AST_Lambda, and has no name. I am committing your implementation and adjusting the test,

@OndrejSpanel thanks for the detailed investigation 👍

Adjusted test to match expected result.

kzc · 2017-02-16T14:19:25Z

test/mocha/accessorTokens-1492.js

+        var checkWalker = new UglifyJS.TreeWalker(function(node, descend) {
+            if (node instanceof UglifyJS.AST_Accessor) {
+                assert.equal(node.start.pos, 22);
+                assert.equal(node.end.endpos, 46);


Doesn't the AST_Accessor start at 12 - the beginning of the get keyword?

get latest() { return undefined; }

I think is is a matter of opinion. I will try to explain why I think with it should start at 22:

The AST hierarchy is:
AST_ObjectProperty contains

key of type AST_KeySymbolRef

value of type AST_Accessor

As key is what contains the property name, I think it makes a sense to let the AST_Accessor to start after it. Perhaps its name is a bit misleading and it should be called something like AST_AccessorBody, but I doubt you will want to break AST compatibility because of this.

I see. For what it's worth, acorn reports the FunctionExpression start at 22 as well.

What is the start/end for the AST_ObjectProperty for get latest() { return undefined; }?

The start/end for the AST_ObjectProperty is 12 / 46.

Can you please check the AST_ObjectProperty start/end, as well as the AST_KeySymbolRef start/end in the test as well?

OndrejSpanel · 2017-02-16T14:38:47Z

The build has failed because of timeout in completely unrelated code. I doubt it is related to my fix, but I do not know.

alexlamsl · 2017-02-16T15:28:57Z

@OndrejSpanel the build failed due to a spurious time-out, the fix of which is available but pending to be merged.

You can restart the CI test by re-opening this PR.

OndrejSpanel · 2017-02-16T16:28:24Z

test/mocha/accessorTokens-1492.js

                assert.equal(node.end.endpos, 46);
+
+                assert(node.key instanceof UglifyJS.AST_SymbolRef);


I could not use the same way as before, as Walker is not visiting the key node, therefore it needs to be accessed from its parent. I like it more this way anyway.

Technically you don't need a tree walker at all. You could access each node from the toplevel ast node directly.

OndrejSpanel · 2017-02-20T08:13:25Z

I think I have implemented all changes you required. Is there anything else?

alexlamsl · 2017-02-20T08:25:42Z

@OndrejSpanel IMHO two nits would be the naming (create_accessor) and moving it out of scope so it doesn't get recreated for every iteration of while within every invocation of object_

Otherwise it looks functionally correct. If another reviewer approves, I'll lump this on top of #1485.

Edit: also I think this project prefers the commits to be squashed into one. But I can do that if it proves to be too much hassle.

fixes mishoo#1492 closes mishoo#1493

alexlamsl · 2017-02-20T09:17:41Z

@OndrejSpanel pushed to #1485 as 89e94b2 - thanks!

Fix: AST_Accessor missing start / end tokens - #1492

e587f3b

Test case for the fix.

84adda7

alexlamsl reviewed Feb 16, 2017

View reviewed changes

OndrejSpanel added 2 commits February 16, 2017 12:51

Improved the test case - test tokens against expected values.

4394fe7

Implement createAccessor using embed_tokens.

37ea9df

Adjusted test to match expected result.

kzc reviewed Feb 16, 2017

View reviewed changes

Travis, please try again.

97e55c3

OndrejSpanel closed this Feb 16, 2017

OndrejSpanel reopened this Feb 16, 2017

Testing AST_SymbolRef and AST_ObjectProperty as well.

452e2d3

OndrejSpanel commented Feb 16, 2017

View reviewed changes

Refactor as required (scope, naming).

8aff233

alexlamsl pushed a commit to alexlamsl/UglifyJS that referenced this pull request Feb 20, 2017

Fix: AST_Accessor missing start / end tokens

89e94b2

fixes mishoo#1492 closes mishoo#1493

alexlamsl closed this in d48a308 Feb 23, 2017

alexlamsl mentioned this pull request Feb 27, 2017

Harmony 2.8.0 #1509

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: AST_Accessor missing start / end tokens #1493

Fix: AST_Accessor missing start / end tokens #1493

OndrejSpanel commented Feb 16, 2017

alexlamsl commented Feb 16, 2017

OndrejSpanel commented Feb 16, 2017

alexlamsl commented Feb 16, 2017

alexlamsl Feb 16, 2017

alexlamsl Feb 16, 2017

OndrejSpanel Feb 16, 2017 •

edited

Loading

alexlamsl Feb 16, 2017

OndrejSpanel Feb 16, 2017 •

edited

Loading

alexlamsl Feb 16, 2017

OndrejSpanel Feb 16, 2017 •

edited

Loading

alexlamsl Feb 16, 2017

OndrejSpanel Feb 16, 2017 •

edited

Loading

OndrejSpanel Feb 16, 2017

alexlamsl Feb 16, 2017

kzc Feb 16, 2017

OndrejSpanel Feb 16, 2017 •

edited

Loading

kzc Feb 16, 2017

OndrejSpanel Feb 16, 2017

kzc Feb 16, 2017

OndrejSpanel commented Feb 16, 2017

alexlamsl commented Feb 16, 2017

OndrejSpanel Feb 16, 2017

kzc Feb 16, 2017 •

edited

Loading

OndrejSpanel commented Feb 20, 2017

alexlamsl commented Feb 20, 2017 •

edited

Loading

alexlamsl commented Feb 20, 2017

		assert.equal(node.end.endpos, 46);

		assert(node.key instanceof UglifyJS.AST_SymbolRef);

Fix: AST_Accessor missing start / end tokens #1493

Fix: AST_Accessor missing start / end tokens #1493

Conversation

OndrejSpanel commented Feb 16, 2017

alexlamsl commented Feb 16, 2017

OndrejSpanel commented Feb 16, 2017

alexlamsl commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OndrejSpanel Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OndrejSpanel Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OndrejSpanel Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OndrejSpanel Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OndrejSpanel Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OndrejSpanel commented Feb 16, 2017

alexlamsl commented Feb 16, 2017

Choose a reason for hiding this comment

kzc Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

OndrejSpanel commented Feb 20, 2017

alexlamsl commented Feb 20, 2017 • edited Loading

alexlamsl commented Feb 20, 2017

OndrejSpanel Feb 16, 2017 •

edited

Loading

OndrejSpanel Feb 16, 2017 •

edited

Loading

OndrejSpanel Feb 16, 2017 •

edited

Loading

OndrejSpanel Feb 16, 2017 •

edited

Loading

OndrejSpanel Feb 16, 2017 •

edited

Loading

kzc Feb 16, 2017 •

edited

Loading

alexlamsl commented Feb 20, 2017 •

edited

Loading