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.

Sign up for GitHub

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: AST_Accessor missing start / end tokens #1493

Closed
wants to merge 7 commits into from
Closed

Fix: AST_Accessor missing start / end tokens #1493

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e587f3b
Fix: AST_Accessor missing start / end tokens - #1492
OndrejSpanel Feb 16, 2017
84adda7
Test case for the fix.
OndrejSpanel Feb 16, 2017
4394fe7
Improved the test case - test tokens against expected values.
OndrejSpanel Feb 16, 2017
37ea9df
Implement createAccessor using embed_tokens.
OndrejSpanel Feb 16, 2017
97e55c3
Travis, please try again.
OndrejSpanel Feb 16, 2017
452e2d3
Testing AST_SymbolRef and AST_ObjectProperty as well.
OndrejSpanel Feb 16, 2017
8aff233
Refactor as required (scope, naming).
OndrejSpanel Feb 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions lib/parse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@OndrejSpanel OndrejSpanel Feb 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@OndrejSpanel OndrejSpanel Feb 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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);
            })};

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@OndrejSpanel OndrejSpanel Feb 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@OndrejSpanel OndrejSpanel Feb 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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,

Copy link
Collaborator

Choose a reason for hiding this comment

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

@OndrejSpanel thanks for the detailed investigation 👍

var func = function_(AST_Accessor);
func.start = start;
func.end = prev();
return func;
};
if (name == "get") {
a.push(new AST_ObjectGetter({
start : start,
key : as_atom_node(),
value : function_(AST_Accessor),
value : createAccessor(),
end : prev()
}));
continue;
Expand All @@ -1333,7 +1339,7 @@ function parse($TEXT, options) {
a.push(new AST_ObjectSetter({
start : start,
key : as_atom_node(),
value : function_(AST_Accessor),
value : createAccessor(),
end : prev()
}));
continue;
Expand Down
22 changes: 22 additions & 0 deletions test/mocha/accessorTokens-1492.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
var UglifyJS = require('../../');
var assert = require("assert");

describe("Accessor tokens", function() {
it("Should fill the token information for accessors (issue #1492)", function() {
var ast = UglifyJS.parse("var obj = { get latest() { return undefined; } }");

/* a possible way to test, but walking through the whole tree seems more robust against possible AST changes
var accessor = ast.body["0"].definitions["0"].value.properties["0"].value;
assert(accessor instanceof UglifyJS.AST_Accessor);
assert(accessor.start !== undefined);
assert(accessor.end !== undefined);
*/

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

Choose a reason for hiding this comment

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

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?

assert(node.end !== undefined);
});
ast.walk(checkWalker);
});
});