Skip to content

Warn when calling synchronized on Predef or on synthetic file objects #17266

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
mbovel opened this issue Apr 14, 2023 · 6 comments · Fixed by #17449
Closed

Warn when calling synchronized on Predef or on synthetic file objects #17266

mbovel opened this issue Apr 14, 2023 · 6 comments · Fixed by #17449
Labels
area:reporting Error reporting including formatting, implicit suggestions, etc itype:enhancement Spree Suitable for a future Spree
Milestone

Comments

@mbovel
Copy link
Member

mbovel commented Apr 14, 2023

Consider the following code in sync.scala:

package myPackage:
	def test1 = synchronized { println("1 starting"); Thread.sleep(2000); println("Ok t1 finished.") }
	def test2 = this.synchronized { println("2 starting"); Thread.sleep(2000); println("Ok t2 finished"); }

object MyObject:
	def test3 = synchronized { println("3 starting"); Thread.sleep(2000); println("Ok t3 finished.") }
	def test4 = this.synchronized { println("4 starting"); Thread.sleep(2000); println("Ok t4 finished"); }

scala-cli compile -Xprint:typer -Yprint-debug sync.scala ouputs:

package <root>.this.myPackage {
    final lazy module val sync2$package: myPackage.this.sync2$package$ = 
      new myPackage.this.sync2$package$.<init>()
    final module class sync2$package$() extends Object.<init>() { 
      this: myPackage.this.sync2$package.type =>
      def test1: scala.this.Unit(inf) = 
        scala.this.Predef.synchronized[scala.this.Unit^(inf)](
          {
            scala.this.Predef.println("1 starting")
            java.this.lang.Thread.sleep(2000L)
            scala.this.Predef.println("Ok t1 finished.")
          }
        )
      def test2: scala.this.Unit(inf) = 
        this.synchronized[scala.this.Unit^(inf)](
          {
            scala.this.Predef.println("2 starting")
            java.this.lang.Thread.sleep(2000L)
            scala.this.Predef.println("Ok t2 finished")
          }
        )
    }
  }
  final lazy module val MyObject: <empty>.this.MyObject$ = 
    new <empty>.this.MyObject$.<init>()
  final module class MyObject$() extends Object.<init>() { 
    this: <empty>.this.MyObject.type =>
    def test3: scala.this.Unit(inf) = 
      MyObject$.this.synchronized[scala.this.Unit^(inf)](
        {
          scala.this.Predef.println("3 starting")
          java.this.lang.Thread.sleep(2000L)
          scala.this.Predef.println("Ok t3 finished.")
        }
      )
    def test4: scala.this.Unit(inf) = 
      this.synchronized[scala.this.Unit^(inf)](
        {
          scala.this.Predef.println("4 starting")
          java.this.lang.Thread.sleep(2000L)
          scala.this.Predef.println("Ok t4 finished")
        }
      )
  }

C.f., the first synchronized is desugared to Predef.synchronized. This sounds rather confusing.

We should probably issue a warning in this case, and also when calling this.synchronized from top-level functions.

More context in #7713 (comment).

@mbovel mbovel added itype:enhancement area:reporting Error reporting including formatting, implicit suggestions, etc Spree Suitable for a future Spree labels Apr 14, 2023
@scala-center-bot
Copy link

This issue was picked for the Issue Spree No. 29 of 18 April 2023 which takes place in a week from now. @mbovel, @jan-pieter, @yawen-guan 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 Author

mbovel commented Apr 18, 2023

I had to rebalance the issue spree teams due to some absences. In the end, we will not tackle this issue today. @jan-pieter, I re-assigned you to #12460.

@mbovel
Copy link
Member Author

mbovel commented May 4, 2023

Related: #17284

@scala-center-bot
Copy link

This issue was picked for the Issue Spree No. 30 of 09 May 2023 which takes place in a week from now. @mbovel, @shivakg 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 Author

mbovel commented May 8, 2023

Background

AnyRef

AnyRef is the base class for all objects in Scala, similarly to Object in Java. See this diagram from the “Unified Types” documentation page:

image

synchronized

synchronized is a method of AnyRef, see its documentation:

image

Normally, an un-qualified call to synchronize is de-sugared to this.synchronized.

So what does this corresponds to when we are in a top-level definition? For example:

def test1 = synchronized { println("1 starting"); Thread.sleep(2000); println("Ok t1 finished.") }

Well, in this case, synchronized corresponds to Predef. synchronized. This is what we see in the description of this issue:

      def test1: scala.this.Unit(inf) = 
        scala.this.Predef.synchronized[scala.this.Unit^(inf)](
          {
            scala.this.Predef.println("1 starting")
            java.this.lang.Thread.sleep(2000L)
            scala.this.Predef.println("Ok t1 finished.")
          }
        )

So, what is Predef?

Predef

Predef is an object containing methods and types than can be accessed without qualification; that's all the “built-in” stuff we can access directly. See the documentation:

image

For example, Predef contains the println method and common types like List or Map.

AnyRef methods on Predef

Like any object, Predef inherits from AnyRef methods.

The problem is that calling AnyRef-methods like synchronized, wait or toString directly when we are not in an object doesn't really make sense. In the case of synchronized, it means that the lock of the singleton Predef object—a global lock—will be used, which is likely not what the user wants.

To solve this, we want to add a warning when calling AnyRef-methods on Predef.

mbovel added a commit to mbovel/dotty that referenced this issue May 9, 2023
@scala scala deleted a comment from scala-center-bot May 28, 2023
@scala scala deleted a comment from scala-center-bot May 28, 2023
@mbovel
Copy link
Member Author

mbovel commented May 28, 2023

Please ignore the 2 previous message from the Scala Center bot. Teams for the next spree can be found here: https://airtable.com/shrSni1OItSuErosU/tblLEWc2ISeGHPbsc. Sorry for the spam!

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.

3 participants