`given` initializer with side-effects is not invoked by `summon`

[armanbilge]

Compiler version

3.4.0-RC1-bin-20231223-938d405-NIGHTLY

Minimized code

//> using scala 3.4.0-RC1-bin-20231223-938d405-NIGHTLY

class Foo {
  inline def bar(x: String) = x + "bar"
}
object Foo {
  given Foo = {
    println("gimme")
    new Foo
  }
  // implicit def foo: Foo = {
  //   println("foops")
  //   new Foo
  // }
}

class Bar {
  def baz() = {
    val foobar = summon[Foo].bar("foo")
    println(foobar)
  }
}

Output

/*
 * Decompiled with CFR 0.151.
 */
import scala.Predef$;

public class Bar {
    public void baz() {
        String foobar = "foobar";
        Predef$.MODULE$.println(foobar);
    }
}

Expectation

If we replace given with implicit def foo:

/*
 * Decompiled with CFR 0.151.
 */
import scala.Predef$;

public class Bar {
    public void baz() {
        Foo x$proxy1 = Foo$.MODULE$.foo();
        String foobar = "foobar";
        Predef$.MODULE$.println(foobar);
    }
}

[erikerlandson]

In the particular case where a typeclass has inline methods, and no internal state or side-effects, I do like the current behavior of eliding any reference to it in the compiled output.

[hamzaremmal]

Minimisation :

object Foo:
  println("side-effect")
  inline def foo(x : String) = x + "bar"

@main def run = 
  val foobar = Foo.foo("foo")
  println(foobar)

if we change Foo from an object to a class, then we have the side effect.

[jducoeur]

That still seems intuitively correct to me -- since foo() is inlined, and doesn't reference its parent object, there isn't any reason for that parent to be initialized. The containing object is essentially just a logical module, not meaningfully a runtime object.

By contrast, when you're replacing it with a class, you are presumably explicitly instantiating (and thus initializing) the class before calling foo(), right?

[erikerlandson]

The original reproducer from @armanbilge seems slightly different - the implicit function that creates the Foo object has a side effect. The side effect is not attached to the class, or object.

[som-snytt]

It's always worked like this.

scala> object X { ???; final val x = 42 }
object X

scala> def f = X.x
def f: Int

scala> f
val res0: Int = 42

scala> X
scala.NotImplementedError: an implementation is missing
  at scala.Predef$.$qmark$qmark$qmark(Predef.scala:344)
  ... 33 elided

also compare

scala> trait T { ???; final val x = 42 }
trait T

scala> null.asInstanceOf[T].x
java.lang.NullPointerException: Cannot invoke "T.x()" because "null" is null
  ... 30 elided

scala> (null: T).x
val res3: Int = 42

[erikerlandson]

Are constructs like given Foo = ... always compiled as objects, when they are summoned, even if they have some kind of type parameter? I think of something like given Foo[X] = ... as behaving like a function whenever it is summoned, but is that true?

And how does that compare with the older implicit def getFoo[X]: Foo[X] = ..., in terms of how that is compiled when it is summoned? I think of implicit def and given as behaving the same at the point of summoning, but are they?

[odersky]

Parameterless givens produce lazy vals, parameterized ones produce defs.

So if you need the side effect you should add a dummy type parameter [X]. But I that's really a code smell. Generally, givens with side effects are a bad abuse of the language.

[armanbilge]

So if you need the side effect you should add a dummy type parameter [X]. But I that's really a code smell. Generally, givens with side effects are a bad abuse of the language.

I definitely agree! In that case ...

Parameterless givens produce lazy vals, parameterized ones produce defs.

Could parameterless givens produce @threadUnsafe lazy vals? If they are not supposed to be side-effectual anyway, then paying a synchronization cost for every access seems unnecessary.

[odersky]

Could parameterless givens produce @threadUnsafe lazy vals? If they are not supposed to be side-effectual anyway, then paying a synchronization cost for every access seems unnecessary.

I proposed that at some point, but was voted down, since people thought it's not that bad, and better to be safe. But I agree, if we declare side effects to be undefined behavior in givens then we might as well make the lazy val thread-unsafe.

However, there a workaround.

If the RHS of a parameterless given is a simple immutable reference, the given is implemented as a def. So you can make your value a threadunsafe lazy val (or even strict val), and then forward to it with a given:

import annotation.threadUnsafe
class C
object Test:
  def foo(): C = C()
  @threadUnsafe lazy val _C = foo()
  given C = _C

If you print the code at erasure phase this is what you get:

[[syntax trees at end of                   erasure]] // test.scala
package <empty> {
  @SourceFile("test.scala") class C() extends Object() {}
  final lazy module val Test: Test = new Test()
  @SourceFile("test.scala") final module class Test() extends Object() {
    private def writeReplace(): Object =
      new scala.runtime.ModuleSerializationProxy(classOf[Test])
    def foo(): C = new C()
    @threadUnsafe lazy def _C(): C = Test.foo()
    final given def given_C(): C = Test._C()
  }
}

[armanbilge]

Thanks!

since people thought it's not that bad

Has anyone benchmarked this? @djspiewak and I were discussing this recently.

you know, thinking about it, that synchronization penalty is actually quite insidious. it probably prevents the PIC from properly inlining dynamic dispatch on typeclasses in the common case. has anyone done any serious benchmarking on Cats-style use cases with given?

It's great that there are workarounds, and in fact we'll probably go straight for @static given thanks to https://github.com/lampepfl/dotty/issues/19304. But most users won't have the bandwidth to implement these elaborate workarounds.

[odersky]

As far as I know, no benchmarks for this exist far. But it would be great if they did!

[sjrd]

The synchronization of a thread-safe lazy val is not meant to protect the rhs. It protects the reading/writing of the cache (and bitmap) so that we stay within the JMM. Therefore, even side-effect-free rhs'es need synchronization to be correct.

[armanbilge]

If I understand you correctly, are you saying that @threadUnsafe is currently broken? Because if not, then I got your point, and I'll say it 😅

import scala.annotation.threadUnsafe

class Foo {

  @threadUnsafe lazy val bar = ???

}

decompiles to

/*
 * Decompiled with CFR 0.151.
 * 
 * Could not load the following classes:
 *  scala.Predef$
 *  scala.runtime.Nothing$
 */
import scala.Predef$;
import scala.runtime.Nothing$;

public class Foo {
    private Nothing$ bar$lzy1;
    private boolean barbitmap$1;

    public Nothing$ bar() {
        if (!this.barbitmap$1) {
            this.bar$lzy1 = Predef$.MODULE$.$qmark$qmark$qmark();
            this.barbitmap$1 = true;
        }
        return this.bar$lzy1;
    }
}

The problem is that a thread may see that barbitmap$1 is true before the value of bar$lzy1 is published.

[armanbilge]

are you saying that @threadUnsafe is currently broken? Because if not, then I got your point, and I'll say it 😅

Aha, I'm sorry, I take it back. It's not broken, it is "thread unsafe" which is exactly what is promised.

[armanbilge]

To expand on my confusion, there are at least three semantics here, from weakest to strongest:

1. the lazy val is never safe to use in a multi-threaded setting (aka "thread unsafe")
2. the lazy val may be used in a multi-threaded setting, but its value may be initialized multiple times
3. the lazy val may be used in a multi-threaded setting and its value will be initialized at most once

Until now I was quite wrongly under the impression that @threadUnsafe was meant to implement (2). In fact, it's implementing (1). In practice, what we'd really like is semantic (2), so maybe this needs to be a feature request.

[sjrd]

If you want (2) for a specific purpose, you can implement it in user space. I did that to make tasty-query thread-safe: https://github.com/scalacenter/tasty-query/pull/433/commits/057d3ae7c3d126bbe9d760d9bf76ecfd16989cc7#diff-e6bebd9578265450914d8f7472dada3c2bb505da0a8642dbd9aa402848af7750L19

[armanbilge]

Thanks, yes, I've done that before on many occasions. It really comes back to this:

most users won't have the bandwidth to implement these elaborate workarounds.

Which is unfortunate, because it seems that given makes no promise about evaluation of side-effects yet our ecosystem will proliferate with otherwise unnecessary synchronization overhead and possible impacts to the JIT.