Handle ARROW_FUNCTION in visitFunctionNode

[izgzhen]

For example:

var https=require('https');

https.get({}, (r)=>{}); // AST: v21 = dispatch v20:#get@18 v10,v22,v17:#null

But for:

var https=require('https');

// AST: v21 = construct v24@16 v23:#example.js@42 exception:v2example.js [28->58] (line 2)
https.get({}, function (r) {}); // AST: v16 = dispatch v15:#get@17 v5,v17,v21

I think this predicate should consider FunctionNode.ARROW_FUNCTION as well.

[msridhar]

Thanks for the contribution @izgzhen! I think we need a bit more, though. Arrow functions differ semantically from function expressions in important ways. See this article:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions#Description

At the least, I think we need to make sure the binding of this for calls to arrow functions works correctly. Are you interested in making further changes to make this work properly? If so, I can try to guide you to the relevant code.

[izgzhen]

At the least, I think we need to make sure the binding of this for calls to arrow functions works correctly. Are you interested in making further changes to make this work properly? If so, I can try to guide you to the relevant code.

Ah, makes sense! I am happy to give it a try. Thanks!

[msridhar]

Cool! This is kind of a long comment explaining how this might be done. I understand it could be kind of intimidating, but I think it's doable with some effort to understand the existing code and debug as you go.

We can learn something about what is needed for this feature based on how Babel translates away arrow functions. There's an example here. Given the following input (from the MDN page):

var obj = {
    count : 10,
    doSomethingLater : function(){    
        setTimeout( () => {
            this.count++;
            console.log(this.count);
        }, 300);
    }
}
obj.doSomethingLater();

Babel translates it to:

var obj = {
  count: 10,
  doSomethingLater: function doSomethingLater() {
    var _this = this;

    setTimeout(function () {
      _this.count++;
      console.log(_this.count);
    }, 300);
  }
};
obj.doSomethingLater();

Basically, Babel introduces a fresh variable _this in the enclosing function for the arrow that holds the value of this, and then rewrites all references to this inside the arrow function to reference _this instead.

I think a similar strategy would work for WALA, and we could do it inside RhinoToAstTranslator. To create the new variable in the function enclosing the arrow, you could call addNameDecl() on the context parameter around the same place you were modifying the code. Here is an example (but we could skip the noteSourcePosition stuff here):

https://github.com/wala/WALA/blob/60af67444dafe4071cd571b538cc2aff78b9d0ca/com.ibm.wala.cast.js.rhino/src/main/java/com/ibm/wala/cast/js/translator/RhinoToAstTranslator.java#L1736

For handling references to this inside the arrow function, we'd have to keep track of the fact we are in an arrow function while visiting the body, and what name to use instead of this. The obvious place to do that is within the WalkContext. We create the context used to visit a function here:

https://github.com/wala/WALA/blob/60af67444dafe4071cd571b538cc2aff78b9d0ca/com.ibm.wala.cast.js.rhino/src/main/java/com/ibm/wala/cast/js/translator/RhinoToAstTranslator.java#L1228

If the information on whether we are in an arrow function and the replacement name for this were stored there, we could look up the information here and adjust the variable reference appropriately:

https://github.com/wala/WALA/blob/60af67444dafe4071cd571b538cc2aff78b9d0ca/com.ibm.wala.cast.js.rhino/src/main/java/com/ibm/wala/cast/js/translator/RhinoToAstTranslator.java#L608

We'd need to be careful to handle multiple levels of nesting involving both arrow functions and regular function expressions; e.g. see this test case.

Let me know if you have questions!

[izgzhen]

(nothing important -- just that I was a bit busy recently -- hope to look into this issue around this weekend...)

[izgzhen]

@msridhar I made some attempts. It is still a bit hacky, but want to hear your opinions first. Also, I was modifying an old test in order to trigger the code path, but I think I should add a new test for it. Is there a more complete guide on how to add a new unit test? I tried duplicating the class com.ibm.wala.cast.js.test.TestForInLoopHack, but my IntelliJ IDE suggests "No tests available" when I run to launch it.

[msridhar]

@msridhar I made some attempts. It is still a bit hacky, but want to hear your opinions first.

It's a good start! I added a couple initial comments.

Also, I was modifying an old test in order to trigger the code path, but I think I should add a new test for it. Is there a more complete guide on how to add a new unit test? I tried duplicating the class com.ibm.wala.cast.js.test.TestForInLoopHack, but my IntelliJ IDE suggests "No tests available" when I run to launch it.

I wouldn't write a new test class for testing this. I would instead add new test methods to TestSimpleCallGraphShapeRhino, following the style of TestSimpleCallGraphShape. You can add the test input .js files to this folder. I would write the tests in such a way that the call graph will only be correct if arrow functions are handled correctly.

[ghost]

DeepCode's analysis on #9eb791 found:

• :information_source: 1 minor issue. :point_down:

Top issues

Description	Example fixes
The local variable name 'CG' doesn't match '[a-z][a-zA-Z0-9]*'. Occurrences: TestSimpleCallGraphShapeRhino.java:63 TestSimpleCallGraphShapeRhino.java:100	:wrench: Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

[izgzhen]

@msridhar I made some attempts. It is still a bit hacky, but want to hear your opinions first.

It's a good start! I added a couple initial comments.

Also, I was modifying an old test in order to trigger the code path, but I think I should add a new test for it. Is there a more complete guide on how to add a new unit test? I tried duplicating the class com.ibm.wala.cast.js.test.TestForInLoopHack, but my IntelliJ IDE suggests "No tests available" when I run to launch it.

I wouldn't write a new test class for testing this. I would instead add new test methods to TestSimpleCallGraphShapeRhino, following the style of TestSimpleCallGraphShape. You can add the test input .js files to this folder. I would write the tests in such a way that the call graph will only be correct if arrow functions are handled correctly.

Added a new test and made various fixes. Sorry for the procrastination ;)