Add warning for synchronized on value classes #20301
Conversation
Co-authored-by: Matt Bovel <matthieu@bovel.net>
Co-authored-by: Matt Bovel <matthieu@bovel.net>
tests/neg/i17493.scala Outdated
There was a problem hiding this comment.
You should probably move this file to the tests/warn directory and remove the -Xfatal-warnings from the compiler's options
There was a problem hiding this comment.
Can you also squash everything in a single commit ?
Let's wait for @sjrd's review before merging it
| | ||
| def checkSynchronizedCallOnValue(tree: Tree)(using Context): Unit = | ||
| if tree.symbol == defn.Object_synchronized | ||
| && ctx.owner.enclosingClass.isValueClass then |
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
This should probably reuse checkSynchronizedCallOnValue. Otherwise explicit selections on AnyVal won't do the right thing, AFAICT.
There was a problem hiding this comment.
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") } // warnAnd 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.
What is the benefit of squashing before merging? I would usually just use the "Squash and merge" feature from Github. |
| Thanks to @sjrd's comments and re-reading discussions on #17449, I think what we did here is not correct. The call to sbt:scala3> scalac -Xprint:typer -Yprint-debug tests/warn/i17493.scala ... [[syntax trees at end of typer]] // tests/warn/i17493.scala package <root>.this.<empty> { final class A(s: scala.this.Predef.String) extends <root>.this.scala.AnyVal. <init>() { val s: scala.this.Predef.String def g: scala.this.Unit(inf) = scala.this.Predef.synchronized[scala.this.Unit^(inf)]( { scala.this.Predef.println("hello, world") } ) } ...So we should not warn about a call on a value. However, we have an existing machinery for warning about top-level calls to |
| I see. The issue wasn't that simple than I expected. I will close this PR since a new suggestion is already ready. |
| Better approach #20312 |
4 participants
fix #17493
Thanks to @mbovel for his collaboration during the Spree.