Treat Scala.js pseudo-unions as real unions #11671

smarter · 2021-03-09T15:41:31Z

When unpickling a Scala 2 symbol, we now treat scala.scalajs.js.|[A, B]
as if it were a real A | B, this requires a special-case in erasure
to emit the correct signatures in SJSIR.

~~Unresolved issues:~~

The companion object of js.| defines implicit conversions like
undefOr2ops, those are no longer automatically in scope for values
with a union type (which breaks many JUnitTests). Should we add a
special-case in typer to handle this or can this be fixed on the
scalajs-library side?
When compiling Scala.js code from source, A | B is interpreted
as js.|[A, B] if js.| is in scope (e.g. via an import),
should we just ignore that import in typer?

sjrd · 2021-03-09T15:58:53Z

Thank you for this!

The companion object of js.| defines implicit conversions like
undefOr2ops, those are no longer automatically in scope for values
with a union type (which breaks many JUnitTests). Should we add a
special-case in typer to handle this or can this be fixed on the
scalajs-library side?

Can we fix it by automatically introducing the following import under -scalajs?

import scala.scalajs.js.|.undefOr2ops

The other two implicit conversions (UnionOps and undefOr2jsAny) can be ignored; they're niche, and it's OK if they break in Scala 3.

When compiling Scala.js code from source, A | B is interpreted
as js.|[A, B] if js.| is in scope (e.g. via an import),
should we just ignore that import in typer?

AFAICT, ignoring those imports would be fine, yes.

sjrd

The implementation looks good to me so far.

smarter · 2021-03-09T16:13:46Z

Can we fix it by automatically introducing the following import under -scalajs?

Sure, can you check if this import is enough to fix the sjsJUnitTests?

sjrd · 2021-03-09T16:50:36Z

It does fix all the errors related to methods of UndefOrOps, so that's good.

However, there are still a few errors left. One seems to be worse type inference for js.defined(x => x + 1) with expected type js.UndefOr[js.Function1[Int, Int]]:

[error] -- [E081] Type Error: C:\Users\sjrdo\Documents\Projets\dotty\tests\sjs-junit\fetched-scala-js-sources\scala-js-src-1.4.0-hack\test-suite\js\src\test\require-2.12\org\scalajs\testsuite\jsinterop\JSOptionalTest212.scala:206:34
[error] 206 |      override val f = js.defined(x => x + 1)
[error]     |                                  ^
[error]     |                        Missing parameter type
[error]     |
[error]     |                        I could not infer the type of the parameter x.

I guess we could live with that.

The others suggest that the unpickling is not always doing the right thing, especially if a parameter is a js.UndefOr[X] (which is a type alias to X | Unit):

[error] -- [E007] Type Mismatch Error: C:\Users\sjrdo\Documents\Projets\dotty\tests\sjs-junit\fetched-scala-js-sources\scala-js-src-1.4.0-hack\test-suite\js\src\test\scala\org\scalajs\testsuite\jsinterop\AsyncTest.scala:156:18
[error] 156 |          resolve(42)
[error]     |                  ^^
[error]     |                  Found:    (42 : Int)
[error]     |                  Required: Int | scalajs.js.Thenable[Int]
[error] -- [E007] Type Mismatch Error: C:\Users\sjrdo\Documents\Projets\dotty\tests\sjs-junit\fetched-scala-js-sources\scala-js-src-1.4.0-hack\test-suite\js\src\test\scala\org\scalajs\testsuite\jsinterop\AsyncTest.scala:188:8
[error] 188 |        (): Unit | js.Thenable[Unit]
[error]     |        ^^
[error]     |        Found:    Unit
[error]     |        Required: Unit | scalajs.js.Thenable[Unit]
[error] -- [E007] Type Mismatch Error: C:\Users\sjrdo\Documents\Projets\dotty\tests\sjs-junit\fetched-scala-js-sources\scala-js-src-1.4.0-hack\test-suite\js\src\test\scala\org\scalajs\testsuite\jsinterop\AsyncTest.scala:203:18
[error] 203 |          resolve(42)
[error]     |                  ^^
[error]     |                  Found:    (42 : Int)
[error]     |                  Required: Int | scalajs.js.Thenable[Int]
[error] -- [E007] Type Mismatch Error: C:\Users\sjrdo\Documents\Projets\dotty\tests\sjs-junit\fetched-scala-js-sources\scala-js-src-1.4.0-hack\test-suite\js\src\test\scala\org\scalajs\testsuite\jsinterop\AsyncTest.scala:215:8
[error] 215 |        (): Unit | js.Thenable[Unit]
[error]     |        ^^
[error]     |        Found:    Unit
[error]     |        Required: Unit | scalajs.js.Thenable[Unit]
[error] -- [E007] Type Mismatch Error: C:\Users\sjrdo\Documents\Projets\dotty\tests\sjs-junit\fetched-scala-js-sources\scala-js-src-1.4.0-hack\test-suite\js\src\test\scala\org\scalajs\testsuite\jsinterop\JSOptionalTest.scala:254:35
[error] 254 |      def foo(x: js.UndefOr[Int] = 6): Int =
[error]     |                                   ^
[error]     |  Found:    (6 : Int)
[error]     |  Required: scala.scalajs.js.UndefOr[Nothing]
[error]     |
[error]     |  The following import might make progress towards fixing the problem:
[error]     |
[error]     |    import scalajs.js.|.from
[error]     |
[error] -- [E007] Type Mismatch Error: C:\Users\sjrdo\Documents\Projets\dotty\tests\sjs-junit\fetched-scala-js-sources\scala-js-src-1.4.0-hack\test-suite\js\src\test\scala\org\scalajs\testsuite\jsinterop\PromiseMock.scala:251:25
[error] 251 |      `then`((x: A) => ((x: B): B | Thenable[B]), onRejected)
[error]     |                         ^^^^
[error]     |                 Found:    B
[error]     |                 Required: B | scalajs.js.Thenable[B]
[error]     |
[error]     |                 where:    B is a type in method then with bounds >: A

All of them are clearly wrong, since they all amount to things like "Found: X but required: X | Something".

smarter · 2021-03-09T17:39:50Z

All of them are clearly wrong, since they all amount to things like "Found: X but required: X | Something".

I think those are due to js.| being imported.

sjrd · 2021-03-09T17:48:42Z

Ah, some of them, indeed. By unimporting that, and rewriting two (x: AnyRef) into x.asInstanceOf[AnyRef] (which I could do upstream), I am down to three errors:

[error] -- [E081] Type Error: C:\Users\sjrdo\Documents\Projets\dotty\tests\sjs-junit\fetched-scala-js-sources\scala-js-src-1.4.0-hack\test-suite\js\src\test\require-2.12\org\scalajs\testsuite\jsinterop\JSOptionalTest212.scala:206:34
[error] 206 |      override val f = js.defined(x => x + 1)
[error]     |                                  ^
[error]     |                        Missing parameter type
[error]     |
[error]     |                        I could not infer the type of the parameter x.
[error] -- [E007] Type Mismatch Error: C:\Users\sjrdo\Documents\Projets\dotty\tests\sjs-junit\fetched-scala-js-sources\scala-js-src-1.4.0-hack\test-suite\js\src\test\scala\org\scalajs\testsuite\jsinterop\JSOptionalTest.scala:254:35
[error] 254 |      def foo(x: js.UndefOr[Int] = 6): Int =
[error]     |                                   ^
[error]     |  Found:    (6 : Int)
[error]     |  Required: scala.scalajs.js.UndefOr[Nothing]
[error]     |
[error]     |  The following import might make progress towards fixing the problem:
[error]     |
[error]     |    import scalajs.js.|.from
[error]     |
[error] -- [E007] Type Mismatch Error: C:\Users\sjrdo\Documents\Projets\dotty\tests\sjs-junit\fetched-scala-js-sources\scala-js-src-1.4.0-hack\test-suite\js\src\test\scala\org\scalajs\testsuite\jsinterop\PromiseMock.scala:165:31
[error] 165 |        enqueue(() => reaction(argument))
[error]     |                               ^^^^^^^^
[error]     |  Found:    (argument : A)
[error]     |  Required: Int
[error]     |
[error]     |  The following import might make progress towards fixing the problem:
[error]     |
[error]     |    import scalajs.js.|.from
[error]     |

The first one is the same type inference issue js.defined as above. The other two seem nonsensical.

smarter · 2021-03-09T17:53:13Z

I am down to three errors:

Could you try to minimize those and push them as either standalone tests or as part of the erasure-scalajs scripted test I added?

def foo(x: js.UndefOr[Int] = 6): Int =

This has likely something to do with the special way we need to typecheck the rhs of default getters (https://github.com/lampepfl/dotty/blob/95fa9cb9f3a6989ae41bbe047fc977cc33716b87/compiler/src/dotty/tools/dotc/typer/Namer.scala#L1389-L1394)

smarter · 2021-03-09T18:46:14Z

Unfortunately just importing undefOr2ops breaks more stuff than it fixes, the problem is that it's applicable for any subtype of A | Unit, and therefore for anything, and because it's an import it takes precedence over implicits found in companions, for example Any.jsArrayOps. Any idea how to proceed?

smarter · 2021-03-09T18:54:32Z

My best idea would be to inject undefOr2ops in OfTypeImplicits#ref when tp is an OrType: https://github.com/lampepfl/dotty/blob/95fa9cb9f3a6989ae41bbe047fc977cc33716b87/compiler/src/dotty/tools/dotc/typer/Implicits.scala#L272-L275

smarter · 2021-03-09T19:42:05Z

My best idea would be to inject undefOr2ops in OfTypeImplicits#ref when tp is an OrType:

This seems to work OK, but it's a pretty weird special case, should these operations be deprecated eventually?

smarter · 2021-03-09T20:12:46Z

To limit the impact of this thing, we could restrict the injection of undefOr2ops to only happen when the alias UndefOr is explicitly used (...but that might be inconvenient if type inference strips it).

sjrd · 2021-03-10T08:55:34Z

Ideally, this extension would be in the companion object of Unit, actually. That way it would not have to be imported, and it would only apply when Unit is part of the type, which is exactly what we want.

Could we inject its definition in the companion object of Unit somehow? Or inject |.undefOr2ops in the implicit scope at the same time/place that we would inject the definitions in Unit?

My best idea would be to inject undefOr2ops in OfTypeImplicits#ref when tp is an OrType:

This seems to work OK, but it's a pretty weird special case, should these operations be deprecated eventually?

Eventually, if and when we have flow typing for Unit, then I think we'll be able to deprecate them. But as long as we don't have flow typing for Unit, deprecating those operations would be a huge regression for Scala.js codebases.

smarter · 2021-03-10T12:24:18Z

Could we inject its definition in the companion object of Unit somehow?

Injecting just that definition and not the rest of object Union would be tricky, so I'd keep the current solution over that.

sjrd · 2021-03-10T12:36:58Z

Would it help if we had another object that only declared that implicit conversion?

smarter · 2021-03-10T12:39:38Z

Yes, in that case we could easily add it when we collect the companions if tp is Unit: https://github.com/lampepfl/dotty/blob/c414e8ce0845cf9c6dd8384688f3d534f23cdf69/compiler/src/dotty/tools/dotc/typer/Implicits.scala#L664

sjrd · 2021-03-10T12:54:04Z

Then I guess we can put the following in a new file library-js/src/scala/scalajs/js/internal/UnitOps.scala:

package scala.scalajs.js.internal

import scala.scalajs.js

object UnitOps:
  implicit def unitOrOps[A](x: A | Unit): js.UndefOrOps[A] =
    new js.UndefOrOps(x)

and add the directory to the build by replacing
https://github.com/lampepfl/dotty/blob/c414e8ce0845cf9c6dd8384688f3d534f23cdf69/project/Build.scala#L706-L707
by

      unmanagedSourceDirectories in Compile := {
        val shared = (unmanagedSourceDirectories in (`scala3-library-bootstrapped`, Compile)).value
        val jsSpecified = baseDirectory.value / "src"
        shared :+ jsSpecific
      },

smarter · 2021-03-10T18:36:34Z

One seems to be worse type inference for js.defined(x => x + 1) with expected type js.UndefOr[js.Function1[Int, Int]]

I think the right thing to do here is get it working without js.defined at all, I've opened #11694 for that

smarter · 2021-03-10T19:00:33Z

I'm working on a fix for the default getter issue, locally the remaining issues I see cannot be fixed in the dotty repo itself:

[error] -- [E081] Type Error: /home/smarter/opt/dotty/tests/sjs-junit/../out/bootstrap/sjsJUnitTests/scala-js-src-1.4.0/test-suite/js/src/test/require-2.12/org/scalajs/testsuite/jsinterop/JSOptionalTest212.scala:204:34 --------------------------
[error] 204 |      override val f = js.defined(x => x + 1)
[error]     |                                  ^
[error]     |                                  Missing parameter type
[error]     |
[error]     |                                  I could not infer the type of the parameter x.
[error] -- Error: /home/smarter/opt/dotty/tests/sjs-junit/../out/bootstrap/sjsJUnitTests/scala-js-src-1.4.0/test-suite/js/src/test/scala/org/scalajs/testsuite/jsinterop/PromiseMock.scala:169:13 ---------------------------------------------------
[error] 169 |        if ((resolution: AnyRef) eq (this: AnyRef)) {
[error]     |             ^^^^^^^^^^
[error]     |             the result of an implicit conversion must be more specific than AnyRef

I don't think there's a good way to tweak type inference to keep js.defined working so until we get Infer the type of x in val f: (Int => Int) | Unit = x => x + 1 #11694, this will have to be (x: Int) => x + 1)
The second error is a bit weird but just means that the type of resolution (here A | Thenable[A]) is not a subtype of AnyRef, which is true, so this needs to be changed upstream probably into .asInstanceOf|AnyRef].

@sjrd can you update the test suite upstream to fix those?

sjrd · 2021-03-10T19:54:42Z

I can definitely change the second one to use asInstanceOf[AnyRef] upstream.

For the first one, the whole point of that test is to make sure that the parameter type can be inferred in that very situation, so I will instead isolate it in a separate file that we can blacklist in the dotty build. In the meantime, you can already blacklist JSOptionalTest212.scala as a whole, if it allows to make progress.

Blacklisting PromiseMock.scala is a bit more awkward, because some other files depend on it. I'm aware of at least AsyncTest.scala; there might be others.

smarter · 2021-03-10T20:19:44Z

Sounds good, though depending on when the next scala.js release is out we might have to disable these tests to get this PR into RC2.

sjrd · 2021-03-10T21:34:33Z

I think it's fine to disable them for now. Also doing that might uncover other issues down the line, which we should also address in a release of Scala.js, so it's better if we can discover and food everything as soon as possible.

sjrd

Beautiful! Thank you so much for doing this.

I only have a few minor suggestions.

compiler/src/dotty/tools/dotc/typer/Implicits.scala

compiler/src/dotty/tools/dotc/core/TypeErasure.scala

project/Build.scala

This will be used in the next commit to deal with Scala.js unions. Also change the erasure of value classes to preserve flags like `isSymbol` when erasing its underlying type.

When unpickling a Scala 2 symbol, we now treat `scala.scalajs.js.|[A, B]` as if it were a real `A | B`, this requires a special-case in erasure to emit the correct signatures in SJSIR. Remaining issues after this commit fixed in later commits of this PR: - The companion object of `js.|` defines implicit conversions like `undefOr2ops`, those are no longer automatically in scope for values with a union type (which breaks many JUnitTests). - When compiling Scala.js code from source, `A | B` is interpreted as `js.|[A, B]` if `js.|` is in scope (e.g. via an import). Additionally, a few tests had to be disabled until we can depend on their fixed upstream version (scala-js/scala-js#4451).

This fixes various compilation errors with sjsJUnitTests after the previous commit.

The companion of `js.|` contains the following widely used implicit conversion to a value class: /** Provides an [[Option]]-like API to [[js.UndefOr]]. */ implicit def undefOr2ops[A](value: js.UndefOr[A]): js.UndefOrOps[A] = new js.UndefOrOps(value) (where `UndefOr[A]` dealiases to `A | Unit`). Since we re-interpret Scala.js unions as real unions, this companion is not in the implicit scope of `A | Unit`, we work around this by injecting a new `UnitOps` with the implicit conversion we want in the implicit scope of `Unit` (we could have directly injected the object `js.|`, but that contains other implicits we do not need). This finally lets us compile and run the sjsJUnitTests after the previous two commits.

To improve our testing of unions until we get a release of Scala.js with the fixed PromiseMock. This also lets us re-enable AsyncTest which depends on PromiseMock.

Now that we unpickle Scala.js unions as actual unions, these implicits are suddenly applicable in many more situations and can end up polluting error messages with implicit suggestions since they should never be used explicitly, so blacklist them from the suggestions.

Disallow lambdas in statement position or if the expected type is Unit. Fixes scala#11671

smarter requested a review from sjrd March 9, 2021 15:41

smarter assigned sjrd Mar 9, 2021

smarter added this to the 3.0.0-RC2 milestone Mar 9, 2021

sjrd reviewed Mar 9, 2021

View reviewed changes

smarter assigned smarter and unassigned sjrd Mar 9, 2021

smarter force-pushed the sjs-union branch from 15a9450 to 83b94ff Compare March 9, 2021 18:41

smarter assigned sjrd and unassigned smarter Mar 9, 2021

smarter unassigned sjrd Mar 10, 2021

smarter requested a review from sjrd March 16, 2021 16:47

smarter assigned sjrd and unassigned smarter Mar 16, 2021

smarter marked this pull request as ready for review March 16, 2021 16:56

sjrd requested changes Mar 16, 2021

View reviewed changes

compiler/src/dotty/tools/dotc/typer/Implicits.scala Outdated Show resolved Hide resolved

compiler/src/dotty/tools/dotc/core/TypeErasure.scala Outdated Show resolved Hide resolved

project/Build.scala Outdated Show resolved Hide resolved

sjrd assigned smarter and unassigned sjrd Mar 16, 2021

smarter force-pushed the sjs-union branch from 140f82c to 62a8dfb Compare March 16, 2021 17:42

smarter requested a review from sjrd March 16, 2021 17:45

smarter changed the title ~~Treat Scala.js pseudo-unions in Scala 2 symbols as real unions~~ Treat Scala.js pseudo-unions as real unions Mar 16, 2021

smarter added the release-notes Should be mentioned in the release notes label Mar 16, 2021

sjrd approved these changes Mar 16, 2021

View reviewed changes

smarter enabled auto-merge March 16, 2021 18:50

smarter added 5 commits March 16, 2021 21:05

Add TypeErasure#isSymbol, true when erasing a symbol info

65df24d

This will be used in the next commit to deal with Scala.js unions. Also change the erasure of value classes to preserve flags like `isSymbol` when erasing its underlying type.

Ignore imports of js.|

de1b04e

This fixes various compilation errors with sjsJUnitTests after the previous commit.

sjsJUnitTest: copy-paste PromiseMock from scala.js upstream

f1b5f02

To improve our testing of unions until we get a release of Scala.js with the fixed PromiseMock. This also lets us re-enable AsyncTest which depends on PromiseMock.

smarter force-pushed the sjs-union branch from 62a8dfb to 03ed937 Compare March 16, 2021 20:05

smarter force-pushed the sjs-union branch from 03ed937 to 62965b9 Compare March 16, 2021 20:06

smarter merged commit a582d01 into scala:master Mar 16, 2021

smarter deleted the sjs-union branch March 16, 2021 21:18

odersky added a commit to dotty-staging/dotty that referenced this pull request Mar 17, 2021

Disallow lambdas in statement position

4dddb70

Disallow lambdas in statement position or if the expected type is Unit. Fixes scala#11671

michelou pushed a commit to michelou/scala3 that referenced this pull request Mar 22, 2021

Disallow lambdas in statement position

bc37acd

Disallow lambdas in statement position or if the expected type is Unit. Fixes scala#11671

smarter mentioned this pull request Mar 24, 2021

Compiler forgets that scala.scalajs.js.UndefOr is covariant #11865

Closed

griggt mentioned this pull request Mar 30, 2021

Dottydoc fails to read symbol in scalajs project on RC2 #11943

Closed

griggt mentioned this pull request May 12, 2022

Warn unused imports #14524

Closed

Kordyjan modified the milestones: 3.0.0-RC2, 3.0.0 Aug 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Treat Scala.js pseudo-unions as real unions #11671

Treat Scala.js pseudo-unions as real unions #11671

smarter commented Mar 9, 2021 •

edited

Loading

sjrd commented Mar 9, 2021 •

edited

Loading

sjrd left a comment

smarter commented Mar 9, 2021

sjrd commented Mar 9, 2021 •

edited

Loading

smarter commented Mar 9, 2021

sjrd commented Mar 9, 2021

smarter commented Mar 9, 2021 •

edited

Loading

smarter commented Mar 9, 2021

smarter commented Mar 9, 2021 •

edited

Loading

smarter commented Mar 9, 2021

smarter commented Mar 9, 2021

sjrd commented Mar 10, 2021

smarter commented Mar 10, 2021

sjrd commented Mar 10, 2021

smarter commented Mar 10, 2021 •

edited

Loading

sjrd commented Mar 10, 2021

smarter commented Mar 10, 2021

smarter commented Mar 10, 2021

sjrd commented Mar 10, 2021

smarter commented Mar 10, 2021 •

edited

Loading

sjrd commented Mar 10, 2021

sjrd left a comment

Treat Scala.js pseudo-unions as real unions #11671

Treat Scala.js pseudo-unions as real unions #11671

Conversation

smarter commented Mar 9, 2021 • edited Loading

sjrd commented Mar 9, 2021 • edited Loading

sjrd left a comment

Choose a reason for hiding this comment

smarter commented Mar 9, 2021

sjrd commented Mar 9, 2021 • edited Loading

smarter commented Mar 9, 2021

sjrd commented Mar 9, 2021

smarter commented Mar 9, 2021 • edited Loading

smarter commented Mar 9, 2021

smarter commented Mar 9, 2021 • edited Loading

smarter commented Mar 9, 2021

smarter commented Mar 9, 2021

sjrd commented Mar 10, 2021

smarter commented Mar 10, 2021

sjrd commented Mar 10, 2021

smarter commented Mar 10, 2021 • edited Loading

sjrd commented Mar 10, 2021

smarter commented Mar 10, 2021

smarter commented Mar 10, 2021

sjrd commented Mar 10, 2021

smarter commented Mar 10, 2021 • edited Loading

sjrd commented Mar 10, 2021

sjrd left a comment

Choose a reason for hiding this comment

smarter commented Mar 9, 2021 •

edited

Loading

sjrd commented Mar 9, 2021 •

edited

Loading

sjrd commented Mar 9, 2021 •

edited

Loading

smarter commented Mar 9, 2021 •

edited

Loading

smarter commented Mar 9, 2021 •

edited

Loading

smarter commented Mar 10, 2021 •

edited

Loading

smarter commented Mar 10, 2021 •

edited

Loading