Skip to content

Commit

Permalink
Fix scala-js#4129: Warn if the default global execution context is used.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
gzm0 committed Sep 26, 2021
1 parent c322cd9 commit 776962c
Show file tree
Hide file tree
Showing 13 changed files with 261 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
28 changes: 28 additions & 0 deletions compiler/src/main/scala/org/scalajs/nscplugin/PrepJSInterop.scala
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,34 @@ 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.
|
|If you do not care about performance, you can use
|scala.scalajs.concurrent.QueueExecutionContext.timeouts().
|It is based on setTimeout which makes it fair 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 =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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("->")

Expand Down
Original file line number Diff line number Diff line change
@@ -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()
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
/*
* 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.
|
|If you do not care about performance, you can use
|scala.scalajs.concurrent.QueueExecutionContext.timeouts().
|It is based on setTimeout which makes it fair 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.
|
|If you do not care about performance, you can use
|scala.scalajs.concurrent.QueueExecutionContext.timeouts().
|It is based on setTimeout which makes it fair 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()
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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 fairness: 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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 fairness: 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}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@

package scala.tools.partest.scalajs

import java.io.File

import scala.tools.partest.nest
import scala.tools.partest.nest.{AbstractRunner, DirectCompiler, TestInfo}

Expand All @@ -23,6 +25,12 @@ class ScalaJSRunner(testInfo: ScalaJSTestInfo, suiteRunner: AbstractRunner,
new DirectCompiler(this) with ScalaJSDirectCompiler
}

override def flagsForCompilation(sources: List[File]): List[String] = {
// Never warn, so we do not need to update tons of checkfiles.
"-P:scalajs:nowarnGlobalExecutionContext" ::
super.flagsForCompilation(sources)
}

override def extraJavaOptions = {
super.extraJavaOptions ++ Seq(
s"-Dscalajs.partest.optMode=${options.optMode.id}",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,13 @@ class ScalaJSRunner(testFile: File, suiteRunner: SuiteRunner,
}

override def newCompiler = new DirectCompiler(this) with ScalaJSDirectCompiler

override def flagsForCompilation(sources: List[File]): List[String] = {
// Never warn, so we do not need to update tons of checkfiles.
"-P:scalajs:nowarnGlobalExecutionContext" ::
super.flagsForCompilation(sources)
}

override def extraJavaOptions = {
super.extraJavaOptions ++ Seq(
s"-Dscalajs.partest.optMode=${options.optMode.id}",
Expand Down
6 changes: 6 additions & 0 deletions project/Build.scala
Original file line number Diff line number Diff line change
Expand Up @@ -846,6 +846,8 @@ object Build {
).settings(
commonLinkerInterfaceSettings,

Test / scalacOptions ++= scalaJSCompilerOption("nowarnGlobalExecutionContext"),

/* Add the sources of scalajs-logging to managed sources. This is outside
* of `target/` so that `clean` does not remove them, making IDE happier.
*/
Expand Down Expand Up @@ -1007,6 +1009,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"
Expand Down Expand Up @@ -1576,6 +1580,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
Expand Down Expand Up @@ -1973,6 +1978,7 @@ object Build {
},

Test / scalacOptions ++= scalaJSCompilerOption("genStaticForwardersForNonTopLevelObjects"),
Test / scalacOptions ++= scalaJSCompilerOption("nowarnGlobalExecutionContext"),

scalaJSLinkerConfig ~= { _.withSemantics(TestSuiteLinkerOptions.semantics _) },
scalaJSModuleInitializers in Test ++= TestSuiteLinkerOptions.moduleInitializers,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,20 @@
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}

import scala.collection.mutable

import scala.concurrent.{Future, Promise}
import scala.concurrent.ExecutionContext.Implicits.global

import scala.util.Try

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 fairness: 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

Expand Down

0 comments on commit 776962c

Please sign in to comment.