Fix this being captured by immediately invoked functions

[chengluyu]

This is mentioned in #65 .

:js
class A: { x: int }
  method MA = let _ = 1 in this.x
//│ Defined class A
//│ Defined A.MA: A -> int
//│ // Prelude
//│ class A {
//│   constructor(fields) {
//│     this.x = fields.x;
//│   }
//│   get MA() {
//│     return ((function (_) {
//│       return this.x;
//│     })(1));
//│   }
//│ }
//│ // End of generated code

(A { x = 1 }).MA
//│ res: int
//│ Runtime error:
//│   TypeError: Cannot read properties of undefined (reading 'x')

[LPTK]

Cool!

But does this really solve the problem in general? What if there is a captured this in a generated local function?

[chengluyu]

Sadly this PR only works in this particular case.

The root of the problem is we implement let and pattern matches with IIFEs. IIFEs introduce extra function scopes in generated code, which shadow this. Although we can keep track of runtime scopes and add alias variable declarations but that would be cumbersome.

A better solution is to avoid IIFEs and generate a series of JavaScript statements.

[LPTK]

Why not just add a const self = this at the beginning of methods and then refer to this self instead? It should be a trivial change that would avoid mistranslating the semantics of MLscript.

[LPTK]

Of course we need to make sure that self itself is not shadowed. I think your Scope infrastructure should be able to deal with that.

Also, if the self symbol is never accessed in the method body, we can omit the binding in the generated code.

[chengluyu]

It's done.

[LPTK]

Please ad some tests making sure that this self cannot be shadowed.

[chengluyu]

I added some test cases but found that shadowing this caused problems. The reason is that we didn't allocate runtime names for parameters (as well as parameter patterns) in translatePattern. The latest commit fixes this. I also added test cases for illegal parameters.

[LPTK]

Ok, now that #117 is merged you can adapt your changes here and we can merge it.

[chengluyu]

I rebased the branch. This PR is ready.

[LPTK]

Ok, LGTM.

But it actually occurred to me that we could also define self in the class scope, instead of in each method, which would produce leaner code. As in:

class C { self = this; ... }

Would that be easy to change now?

[chengluyu]

Ok, LGTM.

But it actually occurred to me that we could also define self in the class scope, instead of in each method, which would produce leaner code. As in:

class C { self = this; ... }

Would that be easy to change now?

There had been some trouble implementing this. Now I think self = this is a temporary and ugly fix. It will be unnecessary after we remove IIFEs. So let’s not do it right now.

[LPTK]

Now I think self = this is a temporary and ugly fix. It will be unnecessary after we remove IIFEs.

I disagree. JS semantics is just stupid and we should NOT follow it, so the self-fix will be needed in general.

The following MLscript (using the future syntax):

class A {
  fun m() =
    fun f() = this.x
    f
}

should generate this JS:

class A {
  m() {
    let self = this
    function f() { return self.x }
    return f
  }
}

or alternatively:

class A {
  #self = this
  m() {
    function f() { return #self.x }
    return f
  }
}

But it's okay if we don't do the latter for now if it's too annoying to implement.

[chengluyu]

Now I think self = this is a temporary and ugly fix. It will be unnecessary after we remove IIFEs.

I disagree. JS semantics is just stupid and we should NOT follow it, so the self-fix will be needed in general.

The following MLscript (using the future syntax):

class A {
  fun m() =
    fun f() = this.x
    f
}

should generate this JS:

class A {
  m() {
    let self = this
    function f() { return self.x }
    return f
  }
}

or alternatively:

class A {
  #self = this
  m() {
    function f() { return #self.x }
    return f
  }
}

But it's okay if we don't do the latter for now if it's too annoying to implement.

This is another case. It’s not IIFE. In your case, we should create alias because of the nested function f. It should only be created when the body of f refers to this.

But now we add the alias anyway. That's why I say it’s ugly because the generated code is not easy to understand. In the future, we will deprecate IIFEs for pattern matching and let expressions. Then we can remove the fix in this PR. By the time when the nested function syntax arrives, we should add anther mechanism which prevents inner functions shadowing this.

[LPTK]

By the time when the nested function syntax arrives, we should add anther mechanism which prevents inner functions shadowing this.

But why add another mechanism? It's the same problem. The fix could be improved later, but there is no reason to just remove it.