Skip to content

Add warning for synchronized on value classes #20301

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Add warning for synchronized on value classes #20301

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d89fbaa
Add warning for synchronized on value classes
nox213 Apr 30, 2024
f8b2b2a
Fix check files
nox213 Apr 30, 2024
68c30d1
Move the test file to tests/warn directory
nox213 May 1, 2024
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ enum ErrorMessageID(val isActive: Boolean = true) extends java.lang.Enum[ErrorMe
case MatchTypeNoCasesID // errorNumber: 184
case UnimportedAndImportedID // errorNumber: 185
case ImplausiblePatternWarningID // errorNumber: 186
case SynchronizedCallOnBoxedClassID // errorNumber: 187
case SynchronizedCallOnValueID // errorNumber: 187
case VarArgsParamCannotBeGivenID // errorNumber: 188
case ExtractorNotFoundID // errorNumber: 189
case PureUnitExpressionID // errorNumber: 190
Expand Down
10 changes: 5 additions & 5 deletions compiler/src/dotty/tools/dotc/reporting/messages.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2443,13 +2443,13 @@ class UnqualifiedCallToAnyRefMethod(stat: untpd.Tree, method: Symbol)(using Cont
|you intended.$getClassExtraHint"""
}

class SynchronizedCallOnBoxedClass(stat: tpd.Tree)(using Context)
extends Message(SynchronizedCallOnBoxedClassID) {
class SynchronizedCallOnValue(stat: tpd.Tree)(using Context)
extends Message(SynchronizedCallOnValueID) {
def kind = MessageKind.PotentialIssue
def msg(using Context) = i"Suspicious ${hl("synchronized")} call on boxed class"
def msg(using Context) = i"Suspicious ${hl("synchronized")} call on value"
def explain(using Context) =
i"""|You called the ${hl("synchronized")} method on a boxed primitive. This might not be what
|you intended."""
i"""|You called the ${hl("synchronized")} method on a boxed primitive or on a class extending ${hl("AnyVal")}.
|This might not be what you intended."""
}

class ExtensionNullifiedByMember(method: Symbol, target: Symbol)(using Context)
Expand Down
8 changes: 7 additions & 1 deletion compiler/src/dotty/tools/dotc/typer/RefChecks.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1155,6 +1155,11 @@ object RefChecks {
then report.warning(ExtensionNullifiedByMember(sym, target.typeSymbol), sym.srcPos)
end checkExtensionMethods

def checkSynchronizedCallOnValue(tree: Tree)(using Context): Unit =
if tree.symbol == defn.Object_synchronized
&& ctx.owner.enclosingClass.isValueClass then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't do the right thing for an imported synchronized.

You shouldn't look at the context owner, but at the prefix of tree.tpe.
report.warning(SynchronizedCallOnValue(tree), tree)

/** Verify that references in the user-defined `@implicitNotFound` message are valid.
* (i.e. they refer to a type variable that really occurs in the signature of the annotated symbol.)
*/
Expand Down Expand Up @@ -1326,11 +1331,12 @@ class RefChecks extends MiniPhase { thisPhase =>

override def transformIdent(tree: Ident)(using Context): Tree =
checkAnyRefMethodCall(tree)
checkSynchronizedCallOnValue(tree)
tree

override def transformSelect(tree: tpd.Select)(using Context): tpd.Tree =
if defn.ScalaBoxedClasses().contains(tree.qualifier.tpe.typeSymbol) && tree.name == nme.synchronized_ then
report.warning(SynchronizedCallOnBoxedClass(tree), tree.srcPos)
report.warning(SynchronizedCallOnValue(tree), tree.srcPos)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably reuse checkSynchronizedCallOnValue. Otherwise explicit selections on AnyVal won't do the right thing, AFAICT.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We tried the following during the Spree:

//> using options -explain class A(val s: String) extends AnyVal def Test = val a = A("hello") a.synchronized { println("hello, world") } // warn

And this is already throws an error:

 ~/scala-snippets-5 scalac foo.scala -- [E008] Not Found Error: foo.scala:5:4 --------------------------------------- 5 | a.synchronized { println("hello, world") } // warn | ^^^^^^^^^^^^^^ |value synchronized is not a member of A, but could be made available as an extension method. | |The following import might fix the problem: | | import scala.reflect.Selectable.reflectiveSelectable | 1 error found

Is this the case you have in mind?

Anyway, I guess it might be nice to use the same logic both for Select and Apply.
tree
}

Expand Down
11 changes: 11 additions & 0 deletions tests/neg/i17493.check
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
-- [E187] Potential Issue Warning: tests/neg/i17493.scala:3:10 ---------------------------------------------------------
3 | def g = synchronized { println("hello, world") } // warn
| ^^^^^^^^^^^^
| Suspicious synchronized call on value
|---------------------------------------------------------------------------------------------------------------------
| Explanation (enabled by `-explain`)
|- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
| You called the synchronized method on a boxed primitive or on a class extending AnyVal.
| This might not be what you intended.
---------------------------------------------------------------------------------------------------------------------
No warnings can be incurred under -Werror (or -Xfatal-warnings)
5 changes: 5 additions & 0 deletions tests/neg/i17493.scala
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should probably move this file to the tests/warn directory and remove the -Xfatal-warnings from the compiler's options
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

68c30d1
fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
//> using options -explain -Xfatal-warnings
class A(val s: String) extends AnyVal {
def g = synchronized { println("hello, world") } // warn
}
// nopos-error: No warnings can be incurred under -Werror (or -Xfatal-warnings)
18 changes: 9 additions & 9 deletions tests/warn/17284.check
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,30 @@
-- [E187] Potential Issue Warning: tests/warn/17284.scala:4:6 ----------------------------------------------------------
4 | 451.synchronized {} // warn
| ^^^^^^^^^^^^^^^^
| Suspicious synchronized call on boxed class
| Suspicious synchronized call on value
|---------------------------------------------------------------------------------------------------------------------
| Explanation (enabled by `-explain`)
|- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
| You called the synchronized method on a boxed primitive. This might not be what
| you intended.
| You called the synchronized method on a boxed primitive or on a class extending AnyVal.
| This might not be what you intended.
---------------------------------------------------------------------------------------------------------------------
-- [E187] Potential Issue Warning: tests/warn/17284.scala:8:4 ----------------------------------------------------------
8 | x.synchronized {} // warn
| ^^^^^^^^^^^^^^
| Suspicious synchronized call on boxed class
| Suspicious synchronized call on value
|---------------------------------------------------------------------------------------------------------------------
| Explanation (enabled by `-explain`)
|- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
| You called the synchronized method on a boxed primitive. This might not be what
| you intended.
| You called the synchronized method on a boxed primitive or on a class extending AnyVal.
| This might not be what you intended.
---------------------------------------------------------------------------------------------------------------------
-- [E187] Potential Issue Warning: tests/warn/17284.scala:11:7 ---------------------------------------------------------
11 | true.synchronized {} // warn
| ^^^^^^^^^^^^^^^^^
| Suspicious synchronized call on boxed class
| Suspicious synchronized call on value
|--------------------------------------------------------------------------------------------------------------------
| Explanation (enabled by `-explain`)
|- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
| You called the synchronized method on a boxed primitive. This might not be what
| you intended.
| You called the synchronized method on a boxed primitive or on a class extending AnyVal.
| This might not be what you intended.
--------------------------------------------------------------------------------------------------------------------
6 changes: 3 additions & 3 deletions tests/warn/i17266.check
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,12 @@
-- [E187] Potential Issue Warning: tests/warn/i17266.scala:22:4 --------------------------------------------------------
22 | 1.synchronized { // warn
| ^^^^^^^^^^^^^^
| Suspicious synchronized call on boxed class
| Suspicious synchronized call on value
|--------------------------------------------------------------------------------------------------------------------
| Explanation (enabled by `-explain`)
|- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
| You called the synchronized method on a boxed primitive. This might not be what
| you intended.
| You called the synchronized method on a boxed primitive or on a class extending AnyVal.
| This might not be what you intended.
--------------------------------------------------------------------------------------------------------------------
-- [E181] Potential Issue Warning: tests/warn/i17266.scala:108:2 -------------------------------------------------------
108 | wait() // warn
Expand Down