Partial fix for lampepfl#7113: Fix Scala.js codegen for Null/Nothing types

[Lacaranian]

• Fix an issue where non-null parameters with default arguments of null would generate default parameter functions returning a type of null, which is unknown to the ScalaJS backend. Use the type of the non-null parameter as the return type, instead of null
• Enable the URITest for Scala.js, which was blocked by the previous issue

[dottybot]

Hello, and thank you for opening this PR! 🎉

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

Have an awesome day! ☀️

[sjrd]

As I said on Gitter, I think this is the wrong fix. It may be a good change to do anyway, because it improves binary compatibility of default accessors (for the JVM as much as for JS), but as a fix to the Scala.js issue at hand, it is a workaround, not an actual fix, because it will fail to address regular methods whose result type is Null.

The proper fix would be to fix JSEncoding to correctly encode Null and Nothing.

[joroKr21]

Hmm does this handle generic methods? In Scala 2 we can write def foo[A](bar: A = 42) and it works - foo() infers Int.

[Lacaranian]

Good points - I'll work on getting a better fix in later today

[Lacaranian]

To partially answer your question @joroKr21 , it appears that the change to Desugar changes the output IR from

def expectURI$default$4__Ljava_net_URI__Z__Z__T(uri: Ljava_net_URI, isAbsolute: boolean, isOpaque: boolean): null = {
    null
  }

to

def expectURI$default$4__Ljava_net_URI__Z__Z__T(uri: Ljava_net_URI, isAbsolute: boolean, isOpaque: boolean): T = {
    null
  }

A quick test of def foo[A](y: A = 42) = x does result in

• failing to compile with my change to Desugar
• successfully compiling without it

so in the interest of Scala 2 compatibility, it may be worth leaving out of the final version of this PR. Investigating improvements to JSEncoding instead.

[sjrd]

Sorry for the long delay on this PR. It had fallen off my radar.

[sjrd]

That's the spirit of the fix, but it is a bit in the wrong place, because it applies to too many things. In particular, this would apply to classOf[Null] or Array[Null], where we want to keep scala.runtime.Null$.

The only place where we want to replace scala.runtime.Null$ by ir.Definitions.NullClass is in the signature of methods (and even there, only at the "top level", not in array types). Hence the correct place to do that would be in internalName, after getting the result of toTypeRef and before passing it to encodeTypeRef. That is the correct place to intercept ClassRef("sr_Null$") and turn it into ClassRef(ir.Definitions.NullClass).

(of course, ditto for Nothing).

[Lacaranian]

The tests also succeed with this set of changes in the right spot, though I'm not as sure about the typeRef matching I'm doing here; are there other sets of symbols that are less stringly typed for those?

[sjrd]

Hum ... normally you should exclusively test for sr_Null$ and sr_Nothing$ at this point, similarly to what's done for the scalac side at
https://github.com/scala-js/scala-js/blob/68bae6f971f2f8176d1d36937d1e5f07c4fadeaf/compiler/src/main/scala/org/scalajs/nscplugin/JSEncoding.scala#L285-L288

I'm surprised the test even passes if you're matching sr_Null (without $) and s_Null. Neither should even happen at this point :s

[Lacaranian]

Investigation so far: Scala 2's version uses a needsModuleClassSuffix to add the $ in encodeClassFullName, where Scala 3's version does not do so directly (so far as i can tell, fullyMangledString/mangledName encodes characters in the NameTransformer, but does not add the $ suffix, which may be more the domain of .fullName here).

It appears that in Scala 3 the ModuleClassName SuffixNameKind is responsible for adding the $ suffix, though I'm still working through how that works/where that happens.

[sjrd]

OK, thanks for thé investigation.

I'm not sure sure yet why this happens the way it happens, but given the investigation and that the test passes, I propose we book this progress as is.

Could you just check which one of s_Null or sr_Null is actually required to make the test pass, and drop the other alternative?

[Lacaranian]

Done - surprisingly, it is the s_Null that is coming through, not the sr_Null, which hints that whatever usually maps the language version to the runtime version isn't getting used here. Something to dig into later.

[Lacaranian]

Hmm, I'll see if I can track down why we're seeing the non-runtime and non-$'ed Null (and Nothing) at this point.