Announcement

Collapse
No announcement yet.

[CLOSED] [#999] [1.11.1] Lambda-to-named-function-lifting's scope analysis fails with nested lambdas

Collapse
X
  • Filter
  • Time
  • Show
Clear All
new posts

    [CLOSED] [#999] [1.11.1] Lambda-to-named-function-lifting's scope analysis fails with nested lambdas

    I've been really putting 1.11 through its paces and checking up on some of my old bugs and suggestions and I'm happy to say that nearly everything is working fine! (The changes to including generic method type parameters in the signature required a re-release of my NuGet packages, but I've done that and they are behaving properly again - better, in fact, due to this change and the support of [IgnoreGeneric] on methods!).

    However.. I've found a bug with the "[#921] Lift simple anonymous functions into named-functions for performance wins" change - which I'm annoyed with myself about because I tried to find ways to break it when you guys proposed the first implementation of it!

    A lambda may only be lifted into a named function if it only access its own arguments or class-scoped members (or statics), however the detection of what is in or out of the available scope seems to fail if there is one lambda nested within the other. To demonstrate:

    public class Class1
    {
        [Ready]
        public static void Go()
        {
            Console.WriteLine(string.Join(", ", (new Example(123)).GetValues()));
        }
    }
    
    public class Example
    {
        private readonly int _offset;
        public Example(int offset)
        {
            // Lambda should be lifted to anonymous function (works correctly)
            Console.WriteLine(string.join(", "), new[] { 1, 2, 3 }.Select(value => value + 1));
            
            // Lambda shouldn't be lifted (works correctly)
            Console.WriteLine(string.join(", "), new[] { 1, 2, 3 }.Select(value => value + offset));
            
            DoWork(() =>
            {
                // Lambda shouldn't be lifted into an anonymous function, but it is - I presume because it
                // sees that "offset" is declared in a parent scope and presumes that that is the class member
                // scope and that "offset" will then be available in the named function..
                Console.WriteLine(string.join(", "), new[] { 1, 2, 3 }.Select(value => value + offset)); // Shouldn't lift (but DOES and fails)
            });
        }
        
        private void DoWork(Action work)
        {
            work();
        }
    }
    (Also available at http://live.bridge.net/#92d65a37a1d1dddeb1a0).

    (function (globals) {
        "use strict";
    
        Bridge.define('Demo.Example', {
            _offset: 0,
            constructor: function (offset) {
                // Lambda should be lifted to anonymous function (works correctly)
                console.log(Bridge.String.format(String.join(", "), Bridge.Linq.Enumerable.from([1, 2, 3]).select($_.Demo.Example.f1)));
        
                // Lambda shouldn't be lifted (works correctly)
                console.log(Bridge.String.format(String.join(", "), Bridge.Linq.Enumerable.from([1, 2, 3]).select(function (value) {
                    return value + offset;
                })));
        
                this.doWork($_.Demo.Example.f2);
            },
            doWork: function (work) {
                work();
            }
        });
        
        var $_ = {};
        
        Bridge.ns("Demo.Example", $_)
        
        Bridge.apply($_.Demo.Example, {
            f1: function (value) {
                return value + 1;
            },
            f2: function () {
                // Lambda shouldn't be lifted into an anonymous function, but it is - I presume because it
                // sees that "offset" is declared in a parent scope and presumes that that is the class member
                // scope and that "offset" will then be available in the named function..
                console.log(Bridge.String.format(String.join(", "), Bridge.Linq.Enumerable.from([1, 2, 3]).select(function (value) {
                    return value + offset;
                }))); // Shouldn't lift (but DOES and fails)
            }
        });
        
        Bridge.define('Demo.Class1', {
            statics: {
                config: {
                    init: function () {
                        Bridge.ready(this.go);
                    }
                },
                go: function () {
                    console.log((new Demo.Example(123)).getValues().join(", "));
                }
            }
        });
        
        Bridge.init();
    })(this);
    Within the lambda passed to "DoWork", the constructor argument "offset" is accessed. This should prevent the nested lambda from being lifted (since "offset" is not a class member). This works in the non-nested lambda that accesses "offset" but not within the nested lambda. I'm sort of guessing that the scope analysis looks to see if the accessed variables are all in a parent scope and presumes that they're class-scoped members if so, but unfortunately that's not the case here (on the other hand, it could be caused by something completely different!).
    Last edited by geoffrey.mcgill; 2016-03-15 @ 05:50 PM.

    #2
    This reminds me of the movie Inception. I'm scared a portal will unlock if we solve this.

    We will investigate right away and try to fix. There shouldn't be a problem releasing a patch (1.11.1). Ah, that's a palindromic release number. Who know what might happen.

    Comment


      #3
      Two questions about your .cs sample:

      1. A call to GetValues() is made on Line #6. What namespace or package is that member located?
      2. On Line #12, _offset is defined, but never used. Is _offset required for the demo?

      I'm not sure either of these questions actually affect the issue being demonstrated here. I'm just putting together a reproducible case.

      Comment


        #4
        There's something funky going on with Live Bridge as well. I'm getting a different output when using 1.11.0 from NuGet.

        Comment


          #5
          Yes, we're going to need some clarification on the use of _offset. I don't think GetValues() has any impact on the scenario, but _offset appears that it might.

          Comment


            #6
            Arghhhh, sorry, copy-paste fail! Class1 should not be in the repro case, only the class Example. The class member _offset is leftover from my real code that I tried to whittle down and should also be ignored.

            Comment


              #7
              Excellent. Thanks for the clarification.

              Is the following the expected result? I've removed the comments in DoWork just for this demo:

              (function (globals) {
                  "use strict";
              
                  Bridge.define('ClassLibrary9.Example', {
                      constructor: function (offset) {
                          // Lambda should be lifted to anonymous function (works correctly)
                          console.log(Bridge.toArray(Bridge.Linq.Enumerable.from([1, 2, 3]).select($_.ClassLibrary9.Example.f1)).join(", "));
                  
                          // Lambda shouldn't be lifted (works correctly)
                          console.log(Bridge.toArray(Bridge.Linq.Enumerable.from([1, 2, 3]).select(function (value) {
                              return value + offset;
                          })).join(", "));
                  
                          this.doWork(function () {
                              console.log(Bridge.toArray(Bridge.Linq.Enumerable.from([1, 2, 3]).select(function (value) {
                                  return value + offset;
                              })).join(", "));
                          });
                      },
                      doWork: function (work) {
                          work();
                      }
                  });
                  
                  var $_ = {};
                  
                  Bridge.ns("ClassLibrary9.Example", $_)
                  
                  Bridge.apply($_.ClassLibrary9.Example, {
                      f1: function (value) {
                          return value + 1;
                      }
                  });
                  
                  Bridge.init();
              })(this);

              Comment


                #8
                I've created #999 to track this issue.

                https://github.com/bridgedotnet/Bridge/issues/999

                Another palindromic number. Strange.

                Comment


                  #9
                  Yes, that looks exactly right.

                  Comment


                    #10
                    A more simple example :
                    var one = new List<int>() {1};
                            int sum = 0;
                            one.ForEach(el =>
                            {
                                var list = new List<int>() {1,2};
                                list.ForEach(el2 =>
                                {
                                   sum = sum +el2;
                                });
                            });
                            Global.Alert(sum.ToString());
                    http://live.bridge.net/#4d8ce25ae0df365ad263

                    The strange, if i move var list = .. at the top, the code changes and works.
                    This is a very blocking bug, it renders jQuery unusuable (nesting with using the previous result => never works).

                    Comment


                      #11
                      Hi ProductiveRage and Cestbienmoi !

                      We have fixed the issue. The fix will be included into the upcoming 1.11.1 release.

                      Please let me know if you need the fix before the release - we can send you the latest Bridge nuget package.

                      Comment


                        #12
                        I'm seeing what appears to be a related issue. A generic parameter ("T") seems to be experiencing the same problem. Does the fix address this scenario? I've created a reproduction below. There are 2 variations: one with a nested lambda and one without. In the emitted JavaScript, both TestA and TestB's lambdas are pulled out and in both, a reference to 'T' is made even though it isn't in scope.

                        Thanks,
                        Steve

                         namespace ClassLibrary5
                          {
                              using System;
                              using System.Collections.Generic;
                          
                              public class Test<T> where T : new()
                              {
                                  private Test2 test2;
                          
                                  public Test()
                                  {
                                      this.test2 = new Test2();
                                  }
                          
                                  public void TestA()
                                  {
                                      this.Execute(
                                          () =>
                                          {
                                              this.test2.Save<T>();
                                          });
                                  }
                          
                                  public void TestB()
                                  {
                                      this.Execute(
                                          () =>
                                          {
                                              this.test2.SaveAsync<T>(
                                                  () =>
                                                  {
                                                  });
                                          });
                                  }
                          
                                  private void Execute(Action callback)
                                  {
                                  }
                              }
                          
                              public class Test2
                              {
                                  public void Save<T>()
                                  {
                                  }
                          
                                  public void SaveAsync<T>(Action callback)
                                  {
                                  }
                              }
                          }
                        Last edited by strygo; 2016-02-28 @ 06:51 PM.

                        Comment


                          #13
                          Hi Steve,

                          The issue might be related to the following:

                          http://forums.bridge.net/forum/bridg...mbda-functions

                          https://github.com/bridgedotnet/Bridge/issues/1003

                          If you post a sample demonstrating how to reproduce, we can test, or you can confirm by running your code against the latest in master. The issue from #1003 has been fixed and the fix will be included in Bridge 1.11.1 which will be released on Monday.

                          Comment


                            #14
                            I've just built the latest version from GitHub and it has solved my problem.

                            While I had the code, I tried reproducing strygo's issue and I think that it is indeed the same problem and that 1.11.1 fixes it. The C# code provide results in the following on live.bridge:

                            Bridge.define('Demo.Test$1', function (T) { return {
                                    test2: null,
                                    constructor: function () {
                                        this.test2 = new Demo.Test2();
                                    },
                                    testA: function () {
                                        this.execute(Bridge.fn.bind(this, $_.Demo.Test$1.f1));
                                    },
                                    testB: function () {
                                        this.execute(Bridge.fn.bind(this, $_.Demo.Test$1.f3));
                                    },
                                    execute: function (callback) {
                                    }
                                }; });
                                
                                var $_ = {};
                                
                                Bridge.ns("Demo.Test$1", $_)
                                
                                Bridge.apply($_.Demo.Test$1, {
                                    f1: function () {
                                        this.test2.save(T);
                                    },
                                    f2: function () {
                                    },
                                    f3: function () {
                                        this.test2.saveAsync(T, $_.Demo.Test$1.f2);
                                    }
                                });
                                
                                Bridge.define('Demo.Test2', {
                                    save: function (T) {
                                    },
                                    saveAsync: function (T, callback) {
                                    }
                                });
                            Note that "f1" tries to reference "T", which it does not have access to ("T" is in the scope of testA, which calls f1 but does not forward the "T" reference).

                            In the 1.11.1 build, the following JavaScript is generated -

                            Bridge.define('Demo.Test$1', function (T) { return {
                                    test2: null,
                                    constructor: function () {
                                        this.test2 = new Demo.Test2();
                                    },
                                    testA: function () {
                                        this.execute(Bridge.fn.bind(this, function () {
                                            this.test2.save(T);
                                        }));
                                    },
                                    testB: function () {
                                        this.execute(Bridge.fn.bind(this, function () {
                                            this.test2.saveAsync(T, $_.Demo.Test$1.f1);
                                        }));
                                    },
                                    execute: function (callback) {
                                    }
                                }; });
                                
                                Bridge.ns("Demo.Test$1", $_)
                                
                                Bridge.apply($_.Demo.Test$1, {
                                    f1: function () {
                                    }
                                });
                                
                                Bridge.define('Demo.Test2', {
                                    save: function (T) {
                                    },
                                    saveAsync: function (T, callback) {
                                    }
                                });
                            Now, there is no named function that tries to access a generic type parameter that it does not have access to.

                            There's something niggling at the back of my mind, though, that there might still be a way for this lifting-to-named-functions and generic type parameters to not play nicely together, I'm going to have a bit more of a play with it and will let you know whether I manage to break anything or not.
                            Last edited by geoffrey.mcgill; 2016-03-15 @ 05:51 PM.

                            Comment


                              #15
                              Thanks for the update ProductiveRage. Good to hear these latest issues have been fixed with 1.11.1.

                              Certainly let us know if you run into any other issues and we'll try to patch as quickly as we can. We plan to release 1.11.1 on Monday.

                              Comment

                              Working...
                              X