From 8a98c21ace7dce1967cb525a8c3ff62d202fe9d8 Mon Sep 17 00:00:00 2001 From: Tobias Schlatter Date: Sun, 12 Sep 2021 14:28:20 +0200 Subject: [PATCH] Fix #4129: Warn if the default global execution context is used. The behavior in the presence of imports is unfortunately not ideal here, especially when the implicit ExecutionContext is imported. Ideally, we'd like a warning on the import already and the ability to silence the warning once for an entire file. However, imports get fully resolved in the typer phase, so this would require processing imports in the pre-typer phase, where the entire suppression infrastructure is not available (it requires typing of the @nowarn annotations). Therefore, we live with the sub-optimal situation, especially given that there is a compiler flag to disable the warnings overall. --- .../org/scalajs/nscplugin/JSDefinitions.scala | 5 + .../org/scalajs/nscplugin/PrepJSInterop.scala | 26 ++++ .../scalajs/nscplugin/ScalaJSOptions.scala | 5 + .../org/scalajs/nscplugin/ScalaJSPlugin.scala | 3 + .../GlobalExecutionContextNoWarnTest.scala | 47 +++++++ .../test/GlobalExecutionContextWarnTest.scala | 118 ++++++++++++++++++ .../org/scalajs/junit/async/package.scala | 8 +- .../scala/org/scalajs/junit/JUnitTask.scala | 9 +- project/Build.scala | 4 + .../scalajs/testing/bridge/HTMLRunner.scala | 8 +- .../org/scalajs/testing/bridge/JSRPC.scala | 9 +- 11 files changed, 238 insertions(+), 4 deletions(-) create mode 100644 compiler/src/test/scala/org/scalajs/nscplugin/test/GlobalExecutionContextNoWarnTest.scala create mode 100644 compiler/src/test/scala/org/scalajs/nscplugin/test/GlobalExecutionContextWarnTest.scala diff --git a/compiler/src/main/scala/org/scalajs/nscplugin/JSDefinitions.scala b/compiler/src/main/scala/org/scalajs/nscplugin/JSDefinitions.scala index c3bfdb2edf..f5c95e0aae 100644 --- a/compiler/src/main/scala/org/scalajs/nscplugin/JSDefinitions.scala +++ b/compiler/src/main/scala/org/scalajs/nscplugin/JSDefinitions.scala @@ -135,6 +135,11 @@ trait JSDefinitions { lazy val EnableReflectiveInstantiationAnnotation = getRequiredClass("scala.scalajs.reflect.annotation.EnableReflectiveInstantiation") + lazy val ExecutionContextModule = getRequiredModule("scala.concurrent.ExecutionContext") + lazy val ExecutionContext_global = getMemberMethod(ExecutionContextModule, newTermName("global")) + + lazy val ExecutionContextImplicitsModule = getRequiredModule("scala.concurrent.ExecutionContext.Implicits") + lazy val ExecutionContextImplicits_global = getMemberMethod(ExecutionContextImplicitsModule, newTermName("global")) } // scalastyle:on line.size.limit diff --git a/compiler/src/main/scala/org/scalajs/nscplugin/PrepJSInterop.scala b/compiler/src/main/scala/org/scalajs/nscplugin/PrepJSInterop.scala index 1889e43185..4d0faa7448 100644 --- a/compiler/src/main/scala/org/scalajs/nscplugin/PrepJSInterop.scala +++ b/compiler/src/main/scala/org/scalajs/nscplugin/PrepJSInterop.scala @@ -377,6 +377,32 @@ abstract class PrepJSInterop[G <: Global with Singleton](val global: G) |program is unlikely to function properly.""".stripMargin) super.transform(tree) + case tree if tree.symbol == ExecutionContext_global || + tree.symbol == ExecutionContextImplicits_global => + if (scalaJSOpts.warnGlobalExecutionContext) { + global.runReporting.warning(tree.pos, + """The global execution context in Scala.js is based on JS Promises (microtasks). + |Using it may prevent macrotasks (I/O, timers, UI rendering) from running reliably. + | + |Unfortunately, there is no way with ECMAScript only to implement a performant + |macrotask execution context (and hence Scala.js core does not contain one). + | + |We recommend you use: https://github.com/scala-js/scala-js-macrotask-executor + |Please refer to the README.md of that project for more details regarding + |microtask vs. macrotask execution contexts. + | + |If you do not care about macrotask fairness, you can silence this warning by: + |- Adding @nowarn("cat=other") (Scala >= 2.13.x only) + |- Setting the -P:scalajs:nowarnGlobalExecutionContext compiler option + |- Using scala.scalajs.concurrent.JSExecutionContext.queue + | (the implementation of ExecutionContext.global in Scala.js) directly. + |- Using scala.scalajs.concurrent.QueueExecutionContext.timeouts(): + | A setTimeout based ExecutionContext: Correct, but slow (due to clamping). + """.stripMargin, + WarningCategory.Other, tree.symbol) + } + super.transform(tree) + // Validate js.constructorOf[T] case TypeApply(ctorOfTree, List(tpeArg)) if ctorOfTree.symbol == JSPackage_constructorOf => diff --git a/compiler/src/main/scala/org/scalajs/nscplugin/ScalaJSOptions.scala b/compiler/src/main/scala/org/scalajs/nscplugin/ScalaJSOptions.scala index 0edf02fe11..50cc0bf1c8 100644 --- a/compiler/src/main/scala/org/scalajs/nscplugin/ScalaJSOptions.scala +++ b/compiler/src/main/scala/org/scalajs/nscplugin/ScalaJSOptions.scala @@ -38,6 +38,11 @@ trait ScalaJSOptions { * should they be mapped to)? */ def sourceURIMaps: List[URIMap] + /** Whether to warn if the global execution context is used. + * + * See the warning itself or #4129 for context. + */ + def warnGlobalExecutionContext: Boolean } object ScalaJSOptions { diff --git a/compiler/src/main/scala/org/scalajs/nscplugin/ScalaJSPlugin.scala b/compiler/src/main/scala/org/scalajs/nscplugin/ScalaJSPlugin.scala index 82d9de61a0..5e7ca6faba 100644 --- a/compiler/src/main/scala/org/scalajs/nscplugin/ScalaJSPlugin.scala +++ b/compiler/src/main/scala/org/scalajs/nscplugin/ScalaJSPlugin.scala @@ -62,6 +62,7 @@ class ScalaJSPlugin(val global: Global) extends NscPlugin { else relSourceMap.toList.map(URIMap(_, absSourceMap)) } + var warnGlobalExecutionContext: Boolean = true var _sourceURIMaps: List[URIMap] = Nil var relSourceMap: Option[URI] = None var absSourceMap: Option[URI] = None @@ -121,6 +122,8 @@ class ScalaJSPlugin(val global: Global) extends NscPlugin { fixClassOf = true } else if (option == "genStaticForwardersForNonTopLevelObjects") { genStaticForwardersForNonTopLevelObjects = true + } else if (option == "nowarnGlobalExecutionContext") { + warnGlobalExecutionContext = false } else if (option.startsWith("mapSourceURI:")) { val uris = option.stripPrefix("mapSourceURI:").split("->") diff --git a/compiler/src/test/scala/org/scalajs/nscplugin/test/GlobalExecutionContextNoWarnTest.scala b/compiler/src/test/scala/org/scalajs/nscplugin/test/GlobalExecutionContextNoWarnTest.scala new file mode 100644 index 0000000000..94decfd65a --- /dev/null +++ b/compiler/src/test/scala/org/scalajs/nscplugin/test/GlobalExecutionContextNoWarnTest.scala @@ -0,0 +1,47 @@ +/* + * Scala.js (https://www.scala-js.org/) + * + * Copyright EPFL. + * + * Licensed under Apache License 2.0 + * (https://www.apache.org/licenses/LICENSE-2.0). + * + * See the NOTICE file distributed with this work for + * additional information regarding copyright ownership. + */ + +package org.scalajs.nscplugin.test + +import org.scalajs.nscplugin.test.util._ +import org.scalajs.nscplugin.test.util.VersionDependentUtils.scalaSupportsNoWarn + +import org.junit.Assume._ +import org.junit.Test + +class GlobalExecutionContextNoWarnTest extends DirectTest with TestHelpers { + + override def extraArgs: List[String] = + super.extraArgs ::: List("-P:scalajs:nowarnGlobalExecutionContext") + + @Test + def noWarnOnUsage: Unit = { + """ + import scala.concurrent.ExecutionContext.global + + object Enclosing { + global + } + """.hasNoWarns() + } + + @Test + def noWarnOnImplicitUsage: Unit = { + """ + import scala.concurrent.ExecutionContext.Implicits.global + + object Enclosing { + scala.concurrent.Future { } + } + """.hasNoWarns() + } +} diff --git a/compiler/src/test/scala/org/scalajs/nscplugin/test/GlobalExecutionContextWarnTest.scala b/compiler/src/test/scala/org/scalajs/nscplugin/test/GlobalExecutionContextWarnTest.scala new file mode 100644 index 0000000000..d2e78305d4 --- /dev/null +++ b/compiler/src/test/scala/org/scalajs/nscplugin/test/GlobalExecutionContextWarnTest.scala @@ -0,0 +1,118 @@ +/* + * Scala.js (https://www.scala-js.org/) + * + * Copyright EPFL. + * + * Licensed under Apache License 2.0 + * (https://www.apache.org/licenses/LICENSE-2.0). + * + * See the NOTICE file distributed with this work for + * additional information regarding copyright ownership. + */ + +package org.scalajs.nscplugin.test + +import org.scalajs.nscplugin.test.util._ +import org.scalajs.nscplugin.test.util.VersionDependentUtils.scalaSupportsNoWarn + +import org.junit.Assume._ +import org.junit.Test + +class GlobalExecutionContextWarnTest extends DirectTest with TestHelpers { + + @Test + def warnOnUsage: Unit = { + """ + import scala.concurrent.ExecutionContext.global + + object Enclosing { + global + } + """ hasWarns + """ + |newSource1.scala:5: warning: The global execution context in Scala.js is based on JS Promises (microtasks). + |Using it may prevent macrotasks (I/O, timers, UI rendering) from running reliably. + | + |Unfortunately, there is no way with ECMAScript only to implement a performant + |macrotask execution context (and hence Scala.js core does not contain one). + | + |We recommend you use: https://github.com/scala-js/scala-js-macrotask-executor + |Please refer to the README.md of that project for more details regarding + |microtask vs. macrotask execution contexts. + | + |If you do not care about macrotask fairness, you can silence this warning by: + |- Adding @nowarn("cat=other") (Scala >= 2.13.x only) + |- Setting the -P:scalajs:nowarnGlobalExecutionContext compiler option + |- Using scala.scalajs.concurrent.JSExecutionContext.queue + | (the implementation of ExecutionContext.global in Scala.js) directly. + |- Using scala.scalajs.concurrent.QueueExecutionContext.timeouts(): + | A setTimeout based ExecutionContext: Correct, but slow (due to clamping). + | + | global + | ^ + """ + } + + @Test + def warnOnImplicitUsage: Unit = { + """ + import scala.concurrent.ExecutionContext.Implicits.global + + object Enclosing { + scala.concurrent.Future { } + } + """ hasWarns + """ + |newSource1.scala:5: warning: The global execution context in Scala.js is based on JS Promises (microtasks). + |Using it may prevent macrotasks (I/O, timers, UI rendering) from running reliably. + | + |Unfortunately, there is no way with ECMAScript only to implement a performant + |macrotask execution context (and hence Scala.js core does not contain one). + | + |We recommend you use: https://github.com/scala-js/scala-js-macrotask-executor + |Please refer to the README.md of that project for more details regarding + |microtask vs. macrotask execution contexts. + | + |If you do not care about macrotask fairness, you can silence this warning by: + |- Adding @nowarn("cat=other") (Scala >= 2.13.x only) + |- Setting the -P:scalajs:nowarnGlobalExecutionContext compiler option + |- Using scala.scalajs.concurrent.JSExecutionContext.queue + | (the implementation of ExecutionContext.global in Scala.js) directly. + |- Using scala.scalajs.concurrent.QueueExecutionContext.timeouts(): + | A setTimeout based ExecutionContext: Correct, but slow (due to clamping). + | + | scala.concurrent.Future { } + | ^ + """ + } + + @Test + def noWarnIfSelectivelyDisabled: Unit = { + assumeTrue(scalaSupportsNoWarn) + + """ + import scala.annotation.nowarn + import scala.concurrent.ExecutionContext.global + + object Enclosing { + global: @nowarn("cat=other") + } + """.hasNoWarns() + } + + @Test + def noWarnQueue: Unit = { + /* Test that JSExecutionContext.queue does not warn for good measure. + * We explicitly say it doesn't so we want to notice if it does. + */ + + """ + import scala.scalajs.concurrent.JSExecutionContext.Implicits.queue + + object Enclosing { + scala.concurrent.Future { } + } + """.hasNoWarns() + } + +} diff --git a/junit-async/js/src/main/scala/org/scalajs/junit/async/package.scala b/junit-async/js/src/main/scala/org/scalajs/junit/async/package.scala index 58eb712f36..6fe756b96a 100644 --- a/junit-async/js/src/main/scala/org/scalajs/junit/async/package.scala +++ b/junit-async/js/src/main/scala/org/scalajs/junit/async/package.scala @@ -13,7 +13,13 @@ package org.scalajs.junit import scala.concurrent.Future -import scala.concurrent.ExecutionContext.Implicits.global + +/* Use the queue execution context (based on JS promises) explicitly: + * We do not have anything better at our disposal and it is accceptable in + * terms of fariness: All we use it for is to map over a completed Future once. + */ +import scala.scalajs.concurrent.JSExecutionContext.Implicits.queue + import scala.util.{Try, Success} package object async { diff --git a/junit-runtime/src/main/scala/org/scalajs/junit/JUnitTask.scala b/junit-runtime/src/main/scala/org/scalajs/junit/JUnitTask.scala index 6d15d71858..a2ebe02212 100644 --- a/junit-runtime/src/main/scala/org/scalajs/junit/JUnitTask.scala +++ b/junit-runtime/src/main/scala/org/scalajs/junit/JUnitTask.scala @@ -13,7 +13,14 @@ package org.scalajs.junit import scala.concurrent.Future -import scala.concurrent.ExecutionContext.Implicits.global + +/* Use the queue execution context (based on JS promises) explicitly: + * We do not have anything better at our disposal and it is accceptable in + * terms of fariness: We only use it for test dispatching and orchestation. + * The real async work is done in Bootstrapper#invokeTest which does not take + * an (implicit) ExecutionContext parameter. + */ +import scala.scalajs.concurrent.JSExecutionContext.Implicits.queue import scala.util.{Try, Success, Failure} diff --git a/project/Build.scala b/project/Build.scala index b4b6584b5b..8913d7ad2c 100644 --- a/project/Build.scala +++ b/project/Build.scala @@ -1007,6 +1007,8 @@ object Build { ).zippedSettings("library")( commonLinkerSettings _ ).settings( + Test / scalacOptions ++= scalaJSCompilerOption("nowarnGlobalExecutionContext"), + if (isGeneratingForIDE) { unmanagedSourceDirectories in Compile += baseDirectory.value.getParentFile.getParentFile / "js/src/main/scala-ide-stubs" @@ -1576,6 +1578,7 @@ object Build { fatalWarningsSettings, name := "Scala.js test bridge", delambdafySetting, + Test / scalacOptions ++= scalaJSCompilerOption("nowarnGlobalExecutionContext"), /* By design, the test-bridge has a completely private API (it is * only loaded through a privately-known top-level export), so it * does not have `previousArtifactSetting` nor @@ -1973,6 +1976,7 @@ object Build { }, Test / scalacOptions ++= scalaJSCompilerOption("genStaticForwardersForNonTopLevelObjects"), + Test / scalacOptions ++= scalaJSCompilerOption("nowarnGlobalExecutionContext"), scalaJSLinkerConfig ~= { _.withSemantics(TestSuiteLinkerOptions.semantics _) }, scalaJSModuleInitializers in Test ++= TestSuiteLinkerOptions.moduleInitializers, diff --git a/test-bridge/src/main/scala/org/scalajs/testing/bridge/HTMLRunner.scala b/test-bridge/src/main/scala/org/scalajs/testing/bridge/HTMLRunner.scala index a997ba44a9..5213295a15 100644 --- a/test-bridge/src/main/scala/org/scalajs/testing/bridge/HTMLRunner.scala +++ b/test-bridge/src/main/scala/org/scalajs/testing/bridge/HTMLRunner.scala @@ -13,6 +13,13 @@ package org.scalajs.testing.bridge import scala.scalajs.js + +/* Use the queue (Promise) execution context by default to avoid slowdown by clamping + * + * To avoid blocking the UI thread, we use QueueExecutionContext.timeout in + * specific spots in the code. See #4129 for context. + */ +import scala.scalajs.concurrent.JSExecutionContext.Implicits.queue import scala.scalajs.concurrent.QueueExecutionContext import scala.scalajs.js.annotation._ import js.URIUtils.{decodeURIComponent, encodeURIComponent} @@ -20,7 +27,6 @@ import js.URIUtils.{decodeURIComponent, encodeURIComponent} import scala.collection.mutable import scala.concurrent.{Future, Promise} -import scala.concurrent.ExecutionContext.Implicits.global import scala.util.Try diff --git a/test-bridge/src/main/scala/org/scalajs/testing/bridge/JSRPC.scala b/test-bridge/src/main/scala/org/scalajs/testing/bridge/JSRPC.scala index f73473fd70..6dd8619f6e 100644 --- a/test-bridge/src/main/scala/org/scalajs/testing/bridge/JSRPC.scala +++ b/test-bridge/src/main/scala/org/scalajs/testing/bridge/JSRPC.scala @@ -15,8 +15,15 @@ package org.scalajs.testing.bridge import scala.scalajs.js import scala.scalajs.js.annotation._ +/* Use the queue execution context (based on JS promises) explicitly: + * We do not have anything better at our disposal and it is accceptable in + * terms of fariness: JSRPC only handles in-between test communcation, so any + * future chain will "yield" to I/O (waiting for a message) or an RPC handler in + * a finite number of steps. + */ +import scala.scalajs.concurrent.JSExecutionContext.Implicits.queue + import scala.concurrent.duration._ -import scala.concurrent.ExecutionContext.Implicits.global import org.scalajs.testing.common.RPCCore