Fix name duplication in code generation for new definition typing

[chengluyu]

We might notice a strange behavior of code generation: if we declare a symbol x and then define it, the generated JavaScript function name will have a numeric postfix x1, which indicates that some other x was shadowed. But this is the very first time where x is defined in this file.

diff --git a/shared/src/test/diff/nu/CtorSubtraction.mls b/shared/src/test/diff/nu/CtorSubtraction.mls
--- a/shared/src/test/diff/nu/CtorSubtraction.mls
+++ b/shared/src/test/diff/nu/CtorSubtraction.mls
@@ -9,11 +9,11 @@
 fun x: ('a & Object) -> ('a \ Cls)
 fun x = case
   Cls then error
   y then y
 //│ fun x: forall 'b. (Cls | Object & 'b & ~#Cls) -> 'b
 //│ fun x: forall 'a. (Object & 'a) -> ('a & ~Cls)
 
 x : Int -> Int
 //│ Int -> Int
 //│ res
-//│     = [Function: x1] // <---- Oops?
+//│     = [Function: x]

Well, this is caused by an imperfect workaround for translating classes in new definition typing. Consider the following code.

let f x = x + 1
class Foo(value: Int) {
  let y = f of value
}

Code generation lifts class definitions (e.g., Foo) and translates them into a typing unit class before translating top-level let bindings (e.g., f). So, the Scope should contains some sort of stub symbols of top-level let bindings before translating classes. The current implementation does not handle this well, as it just declares ValueSymbol in the first place.

mlscript/shared/src/main/scala/mlscript/JSBackend.scala

Lines 1404 to 1412 in 231d9ec

	otherStmts.foreach {
	case fd @ NuFunDef(isLetRec, Var(nme), symNme, _, L(body)) =>
	val isByname = isLetRec.isEmpty
	val isByvalueRecIn = if (isByname) None else Some(true)
	val bodyIsLam = body match { case _: Lam => true case _ => false }
	val symb = symNme.map(_.name)
	scope.declareValue(nme, isByvalueRecIn, bodyIsLam, symb)
	case _ => ()
	}

Then, after the translation of the typing unit class, when translating the top-level let bindings, the symbols will be declared twice and the runtime will be renewed, thus the runtime name comes with a numeric postfix. This sometimes cause runtime errors of undefined references. For example,

fun foo: Int -> Int
fun foo = x => x + 1
class Bar {
  fun calc(x) = foo(x)
}
//│ fun foo: Int -> Int
//│ class Bar {
//│   constructor()
//│   fun calc: Int -> Int
//│ }
//│ fun foo: Int -> Int

foo
//│ Int -> Int
//│ res
//│     = [Function: foo1] <--- Note that `foo` has runtime name `foo1`

:re
(new Bar()).calc(0)
//│ Int
//│ res
//│ Runtime error:
//│   ReferenceError: foo is not defined <--- But the class thinks `foo` is `foo`.

This PR fixes this problem by adding a Boolean field forNewDefsDryRun to ValueSymbol, which indicates if this symbol is declare in the situation as described above. If new symbols are shadowing this symbol, we will reuse the runtime name rather than allocating new names.

[NeilKleistGao]

Related PRs: #167, #186

[NeilKleistGao]

LGTM. Thanks for the better solution. :)

[LPTK]

Ok, this looks like a very hacky solution on top of an extremely hacky foundation.

Thankfully, we'll replace this hacky stuff soon once the new symbols are implemented. So let's just merged the hack.

BTW, does it work in situations like the following?

fun foo: Int -> Int
fun foo = x => x + 1
class Bar { fun calc(x) = foo(x) }

class Bar { fun calc(x) = foo }
fun foo: Int
fun foo = 123

[chengluyu]

I agree. I think the original approach (declare top-level let bindings, and unregister them if there's any error) is imperfect and hacky. So, this is actually a hacky fix to address the imperfectness of previous approach and will be soon removed by the help of PreTyper.

I will try your example and add it to tests.

[chengluyu]

The test cases you suggested work.