Add warning for synchronized on value classes

[nox213]

fix #17493
Thanks to @mbovel for his collaboration during the Spree.

[hamzaremmal]

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

[nox213]

68c30d1
fixed

[hamzaremmal]

Can you also squash everything in a single commit ?

Let's wait for @sjrd's review before merging it

[sjrd]

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.

[sjrd]

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

[mbovel]

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.

[mbovel]

Can you also squash everything in a single commit ?

What is the benefit of squashing before merging? I would usually just use the "Squash and merge" feature from Github.

[mbovel]

Thanks to @sjrd's comments and re-reading discussions on #17449, I think what we did here is not correct.

The call to synchronized is resolved to a call to Predef.synchronized (as @som-snytt already noted on the issue's description):

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 synchronized, so we should probably just adapt it for when the call is inside a value class definition. I will open a separate PR with a new fix suggestion in a minute.

[nox213]

I see. The issue wasn't that simple than I expected. I will close this PR since a new suggestion is already ready.

[nox213]

Better approach #20312