Partial fix for lampepfl#7113: Fix Scala.js codegen for Explicit Returns

[Lacaranian]

In order to return from a tail recursive tree in Scala.js IR, a label
must be placed outside the loop to serve as a jump/break point. While
a fresh return label had been generated for such methods (in the
LocalNameGenerator), it had not been placed at the top level of its
method, and was otherwise unused.

After generating a method's body, if a enclosing return label had been
generated, use it to wrap the method body (just as Scala 2's JS codegen
does) for such breaks.

Enable the tests of ReadersTest.scala of the Scala.js test suite, of
which InputStreamReaderTest used a tail-recursive method that depended
on this fix.

[dottybot]

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

[sjrd]

The fix looks correct, but I believe it also applies to user-written (local) return statements. Could you check that those work as well, then adapt the commit message to be more general than tail rec and instead talk about all return statements?

[allanrenucci]

It seems body doesn't need to be by-name

[sjrd]

It is needed. returnLabelName will be mutated from None to Some(...) somewhere inside the execution of body (or not if there is no Return() node inside the body).

[allanrenucci]

I may be missing something. The value of returnLabelName is not saved (or used) before the evaluation of body. So what difference does it make to evaluate it before the function is called?

[sjrd]

Oh, right. I didn't think about it like that. Yeah I guess it can be made a by-value parameter, then, but I'd advocate for a different name in that case, for example wrapInReturnLabeledIfRequired or something like that. The name withXyz suggests that something by-name is happening, at least to me.

[Lacaranian]

Good catch - the Scala 2 version of this used ScopedVars in a way that required by-name, but as a mutable handle to the same localNames, the Dotty version has no such need. Made this by-value and updated the method's name accordingly.

[Lacaranian]

This does indeed affect all explicit local return statements, whether in or out of a loop. A simple example with an early return

trait MyTest {
  def foo(): Boolean = {
    var i = 1
    if(i <= 5) {
      return false
    }
    true
  }
}

yielded the following as the resulting IR:

interface Lhello_MyTest {
  def $$init$__V() {
    /*<skip>*/
  }
  def foo__Z(): boolean = {
    _return[boolean]: {
      var i: int = 1;
      if ((i <=[int] 5)) {
        return@_return false
      };
      true
    }
  }
}

While this isn't strictly necessary for this example (an equivalent transformation could simply assign the value to a temporary variable and return its value later), it does avoid returning the wrong value, and becomes vitally import for early returns inside of otherwise inescapable loops.

I also fixed up the commit message, and changed the unnecessary by-name parameter to by-value (along with changing the method name, and giving it an accompanying comment to make what is happening there more clear).

[sjrd]

One tiny detail, see below.

[sjrd]

This val is not needed anymore, now that body is by-value. You can directly use body inside the cases of the match.

[sjrd]

Can someone from the dotty team (@anatoliykmetyuk ?) say whether it makes sense for the commit message to include Issue lampepfl#7113:, given that it's (obviously) not a complete fix for that issue?

[smarter]

I would put Partial fix for #7113 or something like that in the commit message.

[anatoliykmetyuk]

I think a good idea would be to make a #7113 a milestone composed of multiple issues, or else define what needs to be done precisely and concisely and place this info in the first message of that issue. The "Partial fix" wording implies a bug-grade issue. From the current info in #7113, it is hard to judge at a glance the scope of work intended.

If someone looks at these commits a year from now and attempts to trace the reasoning behind them to the original issue, they will lose time reading all the info at best, be lost at worst. Another option is to mention the issue as "See #7113 for the discussion" in the subsequent commits.