Skip to content

Should Generate warnings when synchronized on AnyVal #17284

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.

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
He-Pin opened this issue Apr 15, 2023 · 8 comments · Fixed by #18021
Closed

Should Generate warnings when synchronized on AnyVal #17284

He-Pin opened this issue Apr 15, 2023 · 8 comments · Fixed by #18021
Labels
area:reporting Error reporting including formatting, implicit suggestions, etc itype:enhancement Spree Suitable for a future Spree
Milestone

Comments

@He-Pin
Copy link
Contributor

He-Pin commented Apr 15, 2023

Compiler version

3.2.2

Minimized code

1.synchronized{}

Output

image

Expectation

generate some warning as https://openjdk.org/jeps/390

@He-Pin He-Pin added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Apr 15, 2023
@He-Pin He-Pin changed the title Generate warnings when synchronized on AnyVal Should Generate warnings when synchronized on AnyVal Apr 16, 2023
@liang3zy22
Copy link
Contributor

This issue also exists with scala 2.13.10.
Screenshot 2023-04-17 at 15 43 11

@anatoliykmetyuk anatoliykmetyuk added itype:enhancement and removed itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Apr 17, 2023
@mbovel mbovel added the area:reporting Error reporting including formatting, implicit suggestions, etc label May 4, 2023
@som-snytt
Copy link
Contributor

som-snytt commented May 14, 2023

That is due to boxing conversion.

g does not compile in Scala 2, but in Scala 3 is accepted, as though written A.synchronized.

class A(val s: String) extends AnyVal {
  //def f = eq("hello, world")  // missing argument list for method eq in object Predef

  def g = synchronized { println("hello, world") }
}

The error on f is charming. I was expecting it to use (Predef: AnyRef).eq and was hoping for a warning.

Since this ticket is an enhancement, I created a separate issue for the missing error.

@mbovel mbovel added the Spree Suitable for a future Spree label Jun 13, 2023
@scala-center-bot
Copy link

This issue was picked for the Issue Spree No. 32 of 20 June 2023 which takes place in 7 days. @mbovel, @diogocanut, @jmesyou, @michaelpigg will be working on it. If you have any insight into the issue or guidance on how to fix it, please leave it here.

@mbovel
Copy link
Member

mbovel commented Jun 20, 2023

@mbovel
Copy link
Member

mbovel commented Jun 20, 2023

I don't know good ressources about autoboxing in Scala, but the one for Java might be worth reading: https://docs.oracle.com/javase/tutorial/java/data/autoboxing.html.

@michaelpigg
Copy link

It's not clear to me what phase of the compiler is relevant to this issue. I was thinking initially the parser, but maybe it's at erasure after the tree shows it converted to a BoxedUnit?

@mbovel
Copy link
Member

mbovel commented Jun 20, 2023

@michaelpigg, sorry we missed you for this spree! The phase where we added the check is RefChecks: https://github.com/lampepfl/dotty/blob/main/compiler/src/dotty/tools/dotc/typer/RefChecks.scala. This phase runs after the parser and the typer, but before erasure. It's where we have most checks that are independent of parsing and typing.

To investigate this issue, we first created a test file at tests/neg/17284.scala with the following content:

def test = 451.synchronized {}

And then printed the AST after the typer using scalac -Xprint:typer tests/neg/17284.scala:

[[syntax trees at end of                     typer]] // tests/neg/17284.scala
package <empty> {
  final lazy module val 17284$package: 17284$package = new 17284$package()
  final module class 17284$package() extends Object() {
    this: 17284$package.type =>
    def test: Unit =
      int2Integer(451).synchronized[Unit](
        {
          ()
        }
      )
  }
}

We also used the -Yplain-printer flag to print the actual tree structure, not the pretty printed version scalac -Xprint:typer -Yplain-printer tests/neg/17284.scala:

[[syntax trees at end of                     typer]] // tests/neg/17284.scala
PackageDef(Ident(<empty>), List(
  ValDef(17284$package, Ident(17284$package$),
    Apply(Select(New(Ident(17284$package$)), <init>), List())),
  TypeDef(17284$package$,
    Template(DefDef(<init>, List(List()), TypeTree(), Thicket(List())),
      List(Apply(Select(New(TypeTree()), <init>), List())),
      ValDef(_, SingletonTypeTree(Ident(17284$package)), Thicket(List())), List(
      DefDef(test, List(), TypeTree(),
        Apply(
          TypeApply(
            Select(Apply(Ident(int2Integer), List(Literal(451))), synchronized)
              ,
          List(TypeTree())),
        List(Block(List(), Literal(()))))
      )
    ))
  )
))

And noted that the part we are interested in is Select(Apply(Ident(int2Integer), List(Literal(451))), synchronized).

More specifically, we're looking for pattern of the form Select(qualifier, name) where qualifier's type is a boxed class, and name is synchronized.

You can find the result of our work in #18021.

Don't hesitate to ask if you have questions! I hope to see you at the next Spree.

@michaelpigg
Copy link

Thanks for the explanation, @mbovel !

@Kordyjan Kordyjan added this to the 3.4.0 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:reporting Error reporting including formatting, implicit suggestions, etc itype:enhancement Spree Suitable for a future Spree
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants