scala / scala3

The Scala 3 compiler, also known as Dotty.
https://dotty.epfl.ch
Apache License 2.0
5.87k stars 1.06k forks source link

Regression in `playframework/playframework` for access to private static class public members #21599

Closed WojciechMazur closed 1 month ago

WojciechMazur commented 1 month ago

Based on Open CB failure in playframework/playframework - builds logs

Compiler version

3.6.0-RC1-bin-20240915-ad8c21a-NIGHTLY Bisect points to e7d479f80a29c6ae21d0755047e563963b87ac82 / #21362

Minimized code

import java.util.List;

public class RoutingDsl {

  final List<Route> routes = null;

  static class Method {}
  private static class Route {
    final Method actionMethod = null;
  }
}
@main def Test =
  val x: RoutingDsl = ???
  x.routes.forEach: route =>
    println(route.actionMethod) // error

Output

Compiling project (Scala 3.6.0-RC1-bin-20240915-ad8c21a-NIGHTLY, JVM (17))
[error] ./test.scala:4:13
[error] Unable to emit reference to value actionMethod in class Route, class Route is not accessible in the top-level definitions in package <empty>
[error]     println(route.actionMethod)
[error]             ^^^^^

Expectation

Should compile. This code does compile in Scala 3.5.0 and lower. It also compiles in Scala 2.12/2.13

bracevac commented 1 month ago

IMHO, this is the correct behavior for private static access in Java. E.g., we'll get a similar error message with an equivalent Java client program:

public class Main {
    public static void main(String[] args) {
        RoutingDsl x = null;
        for(RoutingDsl.Route route : x.routes) {
                System.out.println(route.actionMethod);
        }
    }
}

which results in

Main.java:4: error: Route has private access in RoutingDsl
        for(RoutingDsl.Route route : x.routes) {
                      ^
1 error

So it looks like we got it wrong in Scala 3 until https://github.com/scala/scala3/pull/21362 which is to say, that playframework would have to fix that on their end.

Edit: The following is closer to the Scala version, but my point remains the same:

public class Main {
    public static void main(String[] args) {
        RoutingDsl x = null;
        x.routes.forEach(route -> System.out.println(route.actionMethod));
    }
}
lrytz commented 1 month ago

@bracevac I think one could argue both ways.

If the Route class is explicitly referenced in the Scala source, all Scala versions issue a "cannot be accessed" error.

However, in the example source code there's no reference to Route. The class is public in bytecode and the generated GETFIELD RoutingDsl$Route.actionMethod bytecode is valid. So the error message "Unable to emit reference to value actionMethod" is not really accurate.

I haven't looked in detail how Scala 2 makes the difference between the two cases, it could even be accidental / a bug.

To make an argument for your side, replacing RoutingDsl.Route route by var route in the equivalent Java program still causes an error in javac.

dwijnand commented 1 month ago

The class is public in bytecode and the generated GETFIELD RoutingDsl$Route.actionMethod bytecode is valid.

I wonder why javac emits the class as public. So it can be instantiated by RoutingDsl?

lrytz commented 1 month ago

class is public in bytecode

I was wrong here, the nested class has default access (package protected). Scala 2 is still correct, GETFIELD RoutingDsl$Route.actionMethod is OK within the same package. Moving the RoutingDsl into a different package, Scala 2 gives errors on compilation ("cannot emit reference").

Dale and me tried a simpler example:

// package p
public class A {
  public static B b() { return new B(); }
  private static class B {
    public String bar() { return "l"; }
  }
}
object T {
  def main(args: Array[String]): Unit = {
    println(/* p. */ A.b.bar)
  }
}

Again, no reference to the private B in the Scala file, but this time Scala 2 doesn't compile it ("Unable to emit reference"). So Scala 2 is inconsistent.

Scala 3.5 compiles the example even when moving A.java to a different package, which is clearly a bug, things then fail at runtime (IllegalAccessError).

Overall, I htink enforcing access as current main does is fine.

WojciechMazur commented 1 month ago

When creating a fix for playframework I've seen one interesting issue in their Netty integration

public abstract class AbstractChannel {
    protected AbstractChannel() {}
    protected abstract AbstractUnsafe newUnsafe();
    protected abstract class AbstractUnsafe {
        public abstract void connect();
    }
}
class Channel extends AbstractChannel() {
  override def newUnsafe(): AbstractChannel#AbstractUnsafe = new AbstractUnsafe {
    override def connect(): Unit = ???
  }
}

Fails with

[error] ./test.scala:2:66
[error] Unable to emit reference to constructor AbstractUnsafe in class AbstractUnsafe, class AbstractUnsafe is not accessible in anonymous class AbstractChannel.this.AbstractUnsafe {...}
[error]   override def newUnsafe(): AbstractChannel#AbstractUnsafe = new AbstractUnsafe {
[error]                                                                  ^^^^^^^^^^^^^^
Error compiling project (Scala 3.6.0-RC1-bin-20240922-22ed2fb-NIGHTLY, JVM (17))

What's interesting it fails only when using compilation server. When we compile it with scala compile AbstractChannel.java test.scala -S 3.nightly --server=false it compiles

edit

It compiles with --server=false only because it's not actually being compiled - it's not emitting bytecode for Java class

Gedochao commented 1 month ago

@WojciechMazur regarding the Netty integration issue, let's raise that as a separate ticket, not to confuse requirements. Regarding this ticket, the behaviour seems to be as intended, the fix should be done on playframework's side. Closing this.