JSUtils modification to handle es6 constructs #13635

swmitra · 2017-08-23T19:02:30Z

This PR will replace the existing regex based function def extraction with Acorn and also adds the capability to identify classes and methods (es6). Our current implementations is having some limitations and issues. One issue which is noticed during my dev test is wrong location information for function definition. Consider the following snippet -

Handsome.prototype.some = function () {
}

From the function/type list when we select some, instead of highlighting the member "some" , "some" from Handsome gets highlighted. With the new implementation, this issue is resolved as we get location information from the AST generated by Acorn.

Couple of other observations which are actually open questions to all reviewers, there are some odd syntax which is considered valid in our current regex based implementation. Consider the following snippets -

something: function () {
}

function hello() {
     something: function () {
     }
}

It has been handled in the new implementation to let the existing tests pass, should we consider this to be valid?

Second observation is regarding a broken syntax for which we have a negative test case(this single test fails in the new implementation), but in the new implementation, Acorn parses it successfully. The snippet in consideration is -

function invalid name () {
}

How do we handle this case?

@zaggino @petetnt @ficristo @marcelgerber Can you please have a look at this PR?

petetnt

The functionality works great and I only have couple of nits.

petetnt · 2017-08-23T19:38:40Z

src/language/JSUtils.js

            match;
-
+            


Nit: whitespace

petetnt · 2017-08-23T19:38:59Z

src/language/JSUtils.js


        PerfUtils.addMeasurement(PerfUtils.JSUTILS_REGEXP);
-
+        


Nit: whitespace

petetnt · 2017-08-23T19:39:36Z

src/language/JSUtils.js

 /**
 * Set of utilities for simple parsing of JS text.
 */
 define(function (require, exports, module) {
    "use strict";

    var _ = require("thirdparty/lodash");
+    var Acorn_Loose = require("node_modules/acorn/dist/acorn_loose");


NIT: Should we be consistent and use AcornLoose and ASTWalker?

petetnt · 2017-08-23T19:41:25Z

FWIW I think all of those three broken ones should be invalid

saurabh95 · 2017-08-24T08:15:30Z

src/extensions/default/QuickOpenJavaScript/main.js

-            functionList.push(new FileLocation(null, funcEntry.lineStart, chFrom, chTo, funcEntry.name));
+            var chFrom = funcEntry.columnStart;
+            var chTo = funcEntry.columnEnd;
+            functionList.push(new FileLocation(null, funcEntry.nameLineStart, chFrom, chTo, funcEntry.label || funcEntry.name));


Instead, you can write directly
functionList.push(new FileLocation(null, funcEntry.nameLineStart, funcEntry.columnStart, funcEntry.columnEnd, funcEntry.label || funcEntry.name));

swmitra · 2017-08-24T08:29:13Z

@petetnt Addressed code review NITs . Added test cases for classes, inheritance, getter/setter and static methods.

ficristo · 2017-08-24T08:47:06Z

src/language/JSUtils.js

-    var _ = require("thirdparty/lodash");
+    var _          = require("thirdparty/lodash"),
+        AcornLoose = require("node_modules/acorn/dist/acorn_loose"),
+        ASTWalker  = require("node_modules/acorn/dist/walk");


I'm quite sure this will not work on dist.
You should add some logic here:

brackets/Gruntfile.js

Line 143 in abdf55d

}

Maybe going forward we should use more webpack, but in another PR.

ficristo · 2017-08-24T08:49:04Z

src/language/JSUtils.js

-            functionName = (match[2] || match[5]).trim();
-
+
+        var AST = AcornLoose.parse_dammit(text, {locations: true});


I think this work by checking indentation of the code.
So you need

use always first Acorn.parse and if it fails use AcornLoose.parse_dammit

add some tests of code not indented correctly (and / or without indentation at all)

ficristo · 2017-08-24T08:51:36Z

src/language/JSUtils.js

+                function <functionName> () {}
+            */
+            FunctionDeclaration: function (node) {
+                if (node.id.name !== '✖') { // A hack to reject anonymous declarations 


If Acorn use '✖' I don't think the commit is really good.
Maybe something saying that Acorn use '✖' to detect anonymous declarations and we want to skip them.

Consider this snippet -

key: function () { }

This is actually a broken syntax and acorn can't parse it, but acorn_loose does it. FYI the acorn_loose code which uses this as is -

LooseParser.prototype.dummyIdent = function dummyIdent () { var dummy = this.dummyNode("Identifier"); dummy.name = "✖"; return dummy };

The problem here is FunctionDeclaration walk option catches this and if I don't have a check, ends up putting "✖" as a function name. To actually put key as function name we have a LabeledStatement walk option. For your reference this is how the node looks like -

This is not exactly a normal case but rather a broken syntax handler where this gets initialized with this name. I am not sure of any better way to handle this.

Sorry @swmitra I meant comment not commit !
I simply want you to rephrase the commit to avoid the word hack, since is how acorn works.

I am sorry @ficristo , misinterpreted the comment 😞 . I have changed the comment in the code now and also added the ArrowFunctionExpression construct along with addressing other code review comments and test cases.

ficristo · 2017-08-24T09:01:35Z

Is JSUtils supposed to work on code not completed? If so you need to add some tests.

ficristo · 2017-08-24T09:04:16Z

Is it possible to detect arrow functions? i.e. var foo = () => {}
Does it work with async functions? i.e. var foo = { async bar() {} };

petetnt · 2017-08-24T09:19:11Z

Arrow functions (and functionProperties) worked when I tested, I assume that it works with async ones too

ficristo · 2017-08-24T09:23:45Z

worked when I tested

Great! Make sure to add some test (if there aren't)

swmitra · 2017-08-24T09:29:10Z

This never got handled though -

var foo = () => {}

Adding a walk option for this now 😃

…ions with new test case

swmitra · 2017-08-24T11:09:25Z

@ficristo Can you please have a look at the change list again? I have addressed all the comments .

ficristo · 2017-08-24T12:27:35Z

Are generator functions supposed to be handled? If so, please add a test.

I just want to note I actually didn't test this PR, but the code seems good.

* JSUtils modification to handle es6 constructs * Addressed review comments and added es6 test cases * Address Code review comments and add support for ArrowFunctionExpressions with new test case * Update comments * Refactor to remove extra variables

JSUtils modification to handle es6 constructs

193110f

swmitra requested review from zaggino, marcelgerber, petetnt, ficristo and ingorichter August 23, 2017 19:03

petetnt reviewed Aug 23, 2017

View reviewed changes

Addressed review comments and added es6 test cases

61a132c

saurabh95 approved these changes Aug 24, 2017

View reviewed changes

ficristo suggested changes Aug 24, 2017

View reviewed changes

swmitra added 2 commits August 24, 2017 16:30

Address Code review comments and add support for ArrowFunctionExpress…

a4c904b

…ions with new test case

Update comments

d27b75e

ficristo approved these changes Aug 24, 2017

View reviewed changes

Refactor to remove extra variables

042b539

saurabh95 merged commit 9281909 into master Aug 28, 2017

swmitra added this to the Release 1.11 milestone Sep 12, 2017

ficristo deleted the swmitra/ES6Support branch September 19, 2017 18:45

core-ai-bot mentioned this pull request Aug 30, 2021

[CLOSED] JSUtils modification to handle es6 constructs brackets-archive/bracketsIssues#11458

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSUtils modification to handle es6 constructs #13635

JSUtils modification to handle es6 constructs #13635

swmitra commented Aug 23, 2017 •

edited

Loading

petetnt left a comment

petetnt Aug 23, 2017

petetnt Aug 23, 2017

petetnt Aug 23, 2017

petetnt commented Aug 23, 2017

saurabh95 Aug 24, 2017

swmitra commented Aug 24, 2017 •

edited

Loading

ficristo Aug 24, 2017

ficristo Aug 24, 2017

ficristo Aug 24, 2017

swmitra Aug 24, 2017

ficristo Aug 24, 2017

swmitra Aug 24, 2017

ficristo commented Aug 24, 2017

ficristo commented Aug 24, 2017

petetnt commented Aug 24, 2017

ficristo commented Aug 24, 2017 •

edited

Loading

swmitra commented Aug 24, 2017

swmitra commented Aug 24, 2017

ficristo commented Aug 24, 2017

		functionName = (match[2] \|\| match[5]).trim();


		var AST = AcornLoose.parse_dammit(text, {locations: true});

JSUtils modification to handle es6 constructs #13635

JSUtils modification to handle es6 constructs #13635

Conversation

swmitra commented Aug 23, 2017 • edited Loading

petetnt left a comment

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

petetnt commented Aug 23, 2017

Choose a reason for hiding this comment

swmitra commented Aug 24, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ficristo commented Aug 24, 2017

ficristo commented Aug 24, 2017

petetnt commented Aug 24, 2017

ficristo commented Aug 24, 2017 • edited Loading

swmitra commented Aug 24, 2017

swmitra commented Aug 24, 2017

ficristo commented Aug 24, 2017

swmitra commented Aug 23, 2017 •

edited

Loading

swmitra commented Aug 24, 2017 •

edited

Loading

ficristo commented Aug 24, 2017 •

edited

Loading