Warn when calling synchronized on Predef or on synthetic file objects

Fixes #17266

Co-Authored-By: Dale Wijnand <[email protected]>
Co-Authored-By: itielsa <[email protected]>

Author: 3 people

compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala

@@ -194,6 +194,8 @@ enum ErrorMessageID(val isActive: Boolean = true) extends java.lang.Enum[ErrorMe
   case MissingArgumentListID // errorNumber: 178
   case MatchTypeScrutineeCannotBeHigherKindedID // errorNumber: 179
   case AmbiguousExtensionMethodID // errorNumber 180
+  case CallToPredefSynchronizedID // errorNumber: 181
+  case CallToPackageObjectSynchronizedID // errorNumber: 182
 
   def errorNumber = ordinal - 1
 

compiler/src/dotty/tools/dotc/reporting/messages.scala

@@ -2284,6 +2284,26 @@ class PureExpressionInStatementPosition(stat: untpd.Tree, val exprOwner: Symbol)
         |It can be removed without changing the semantics of the program. This may indicate an error."""
 }
 
+class CallToPredefSynchronized(stat: untpd.Tree)(using Context)
+  extends Message(CallToPredefSynchronizedID) {
+  def kind = MessageKind.PotentialIssue
+  def msg(using Context) = i"Suspicious call to ${hl("Predef.synchronized")}"
+  def explain(using Context) =
+    i"""Top-level unqualifed calls to ${hl("synchronized")} are resolved to ${hl("Predef.synchronized")}.
+        |Therefore, the call above will globally lock using the ${hl("Predef")} object as a monitor.
+        |This might not be what you intented ."""
+}
+
+class CallToPackageObjectSynchronized(stat: untpd.Tree)(using Context)
+  extends Message(CallToPackageObjectSynchronizedID) {
+  def kind = MessageKind.PotentialIssue
+  def msg(using Context) = i"Suspicious top-level call to ${hl("this.synchronized")}"
+  def explain(using Context) =
+    i"""Top-level calls to ${hl("this.synchronized")} are resolved to ${hl("<package object>.synchronized")}.
+        |Therefore, the call above will lock using the generated package object as a monitor.
+        |This might not be what you intented ."""
+}
+
 class TraitCompanionWithMutableStatic()(using Context)
   extends SyntaxMsg(TraitCompanionWithMutableStaticID) {
   def msg(using Context) = i"Companion of traits cannot define mutable @static fields"

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

@@ -1090,6 +1090,17 @@ object RefChecks {
 
   end checkImplicitNotFoundAnnotation
 
+  def checkSynchronizedCall(tree: Tree, symbol: Symbol)(using Context) =
+    if symbol == defn.Object_synchronized then
+      tree.tpe match
+        case tp: NamedType =>
+          val prefixSymbol = tp.prefix.typeSymbol
+          if prefixSymbol == defn.ScalaPredefModuleClass then
+            report.warning(CallToPredefSynchronized(tree), tree)
+          if prefixSymbol.isPackageObject && !tp.symbol.isConstructor then
+            report.warning(CallToPackageObjectSynchronized(tree), tree)
+        case _ => ()
+
 }
 import RefChecks._
 
@@ -1168,6 +1179,15 @@ class RefChecks extends MiniPhase { thisPhase =>
       report.error(ex, tree.srcPos)
       tree
   }
+
+  override def transformSelect(tree: Select)(using Context): Tree =
+    checkSynchronizedCall(tree, tree.denot.symbol)
+    tree
+
+  override def transformIdent(tree: Ident)(using Context): Tree =
+    checkSynchronizedCall(tree, tree.symbol)
+    tree
+
 }
 
 /* todo: rewrite and re-enable

compiler/test/dotty/tools/dotc/CompilationTests.scala

@@ -145,7 +145,7 @@ class CompilationTests {
       compileFilesInDir("tests/neg-custom-args/feature", defaultOptions.and("-Xfatal-warnings", "-feature")),
       compileFilesInDir("tests/neg-custom-args/no-experimental", defaultOptions.and("-Yno-experimental")),
       compileFilesInDir("tests/neg-custom-args/captures", defaultOptions.and("-language:experimental.captureChecking")),
-      compileFilesInDir("tests/neg-custom-args/explain", defaultOptions.and("-explain")),
+      compileFilesInDir("tests/neg-custom-args/explain", defaultOptions.and("-Xfatal-warnings", "-explain")),
       compileFile("tests/neg-custom-args/avoid-warn-deprecation.scala", defaultOptions.and("-Xfatal-warnings", "-feature")),
       compileFile("tests/neg-custom-args/i3246.scala", scala2CompatMode),
       compileFile("tests/neg-custom-args/overrideClass.scala", scala2CompatMode),

tests/neg-custom-args/explain/i17266.check

@@ -0,0 +1,22 @@
+-- [E181] Potential Issue Error: tests/neg-custom-args/explain/i17266.scala:2:2 ----------------------------------------
+2 |  synchronized { // error
+  |  ^^^^^^^^^^^^
+  |  Suspicious call to Predef.synchronized
+  |---------------------------------------------------------------------------------------------------------------------
+  | Explanation (enabled by `-explain`)
+  |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+  | Top-level unqualifed calls to synchronized are resolved to Predef.synchronized.
+  | Therefore, the call above will globally lock using the Predef object as a monitor.
+  | This might not be what you intented .
+   ---------------------------------------------------------------------------------------------------------------------
+-- [E182] Potential Issue Error: tests/neg-custom-args/explain/i17266.scala:7:7 ----------------------------------------
+7 |  this.synchronized { // error
+  |  ^^^^^^^^^^^^^^^^^
+  |  Suspicious top-level call to this.synchronized
+  |---------------------------------------------------------------------------------------------------------------------
+  | Explanation (enabled by `-explain`)
+  |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+  | Top-level calls to this.synchronized are resolved to <package object>.synchronized.
+  | Therefore, the call above will lock using the generated package object as a monitor.
+  | This might not be what you intented .
+   ---------------------------------------------------------------------------------------------------------------------

tests/neg-custom-args/explain/i17266.scala

@@ -0,0 +1,16 @@
+def test1 =
+  synchronized { // error
+    println("hello")
+  }
+
+def test2 =
+  this.synchronized { // error
+    println("hello")
+  }
+
+object MyLib
+def test3 =
+  import MyLib.*
+  synchronized { // not an error yet, but might be in the future
+    println("hello")
+  }