Skip to content

Resolve how to handle errors of closures taking by-name arguments with wildcard types #16789

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
WojciechMazur opened this issue Jan 30, 2023 · 7 comments · Fixed by #16799
Closed
Assignees
Milestone

Comments

@WojciechMazur
Copy link
Contributor

Compiler version

3.3.1-RC1-bin-20230129-b8ad7b1-NIGHTLY

Related to closures with by-name arguments change b439deb

Durated the meeting it was mentioned, that maybe we should treat Java types differently

Minimized code

Based on the results of OpenCB build (links to build logs below)

Case 1

Based on compilation issue in zio/zio-http

def shutdownGracefully(): java.util.concurrent.Future[_] = ???

def executedWildcard(jFuture: => java.util.concurrent.Future[_]): Unit = ???
def executedGeneric[A](jFuture: => java.util.concurrent.Future[A]): Unit = ???

def works = executedWildcard(shutdownGracefully())
def fails = executedGeneric(shutdownGracefully())

Output

-- Error: /workspace/bisect/test.scala:7:27 ------------------------------------
7 |def fail = executedGeneric(shutdownGracefully())
  |                           ^^^^^^^^^^^^^^^^^^^^
  |argument for by-name parameter contains capture conversion skolem types:
  |java.util.concurrent.Future[?1.CAP]
  |
  |where:    ?1 is an unknown value of type scala.runtime.TypeBox[Nothing, Object]

Case 2

When used with Specs2 4.x in tests of reactivemongo/reactivemongo-bson and playframework/play-ws

//> using lib  "org.specs2::specs2-matcher-extra:4.19.0"

class Foo

final class RegressionTest extends org.specs2.mutable.Specification {
  val cls = classOf[Foo]
  val instance = new Foo()
  val works = cls must_== cls
  val fails = instance.getClass must_== cls
  val failsCase2 = instance.getClass() must not(throwA[ClassNotFoundException])
}

Output

[error] ./test.scala:10:20: value must is not a member of Class[?1.CAP].
[error] An extension method was tried, but could not be fully constructed:
[error] 
[error]     this.theValue[T](this.instance.getClass[Foo]().$asInstanceOf[Class[?1.CAP]])
[error] 
[error] where:    ?1 is an unknown value of type scala.runtime.TypeBox[Nothing, Foo]
[error]   val failsCase2 = instance.getClass() must not(throwA[ClassNotFoundException])
[error]   

Expectation

@odersky
Copy link
Contributor

odersky commented Jan 31, 2023

Yes, I think excluding Java types is worth trying. Capture conversion is a core element of Java type checking, so we might want to trade off soundness there. Not sure whether capture conversion in Java itself is sound, the examples seem to indicate that it probably isn't.

@SethTisue SethTisue self-assigned this Jan 31, 2023
@dwijnand
Copy link
Member

Indeed, Java isn't sound here either:

interface Magic<S> {
  S init();
  String step(S s);
}
class IntMagic implements Magic<Integer> {
  public Integer init() { return 0; }
  public String step(Integer s) { return "" + (s - 1); }
}
class StrMagic implements Magic<String> {
  public String init() { return "hi"; }
  public String step(String s) { return new StringBuilder(s).reverse().toString(); }
}
import java.util.*;
import java.util.function.*;

class Main {
  static <T> String onestep(Supplier<Magic<T>> m) {
    return m.get().step(m.get().init());
  }

  public static void main(String... args) {
    Iterator<Magic<?>> iter = new Iterator<Magic<?>>() {
      private Boolean toggle = false;
      public boolean hasNext() { return true; }
      public Magic<?> next() {
        Magic<?> next = toggle ? new IntMagic() : new StrMagic();
        toggle = toggle ? false : true;
        return next;
      }
    };
    onestep(() -> iter.next());
  }
}
$ javac *.java && java Main
Exception in thread "main" java.lang.ClassCastException: class java.lang.Integer cannot be cast to class java.lang.String (java.lang.Integer and java.lang.String are in module java.base of loader 'bootstrap')
	at StrMagic.step(StrMagic.java:1)
	at Main.onestep(Main.java:6)
	at Main.main(Main.java:19)

@odersky
Copy link
Contributor

odersky commented Jan 31, 2023

@dwijnand That's good to know! I wonder whether something on that has been published yet.

@SethTisue
Copy link
Member

fyi @S11001001, whose bug report (scala/bug#9419) and blog post (https://typelevel.org/blog/2015/07/30/values-never-change-types.html) are where we got this from

@SethTisue
Copy link
Member

SethTisue commented Jan 31, 2023

Case 1 (zio-http) and Case 2 (specs2) are pretty different from each other — or at least, they seem so to us at the moment.

Dale thinks Case 2 might be fixable by reordering the sequence of events in typer. If we can reverse the order so that wildcard capture isn't tried until after extension methods are resolved, then the problem might go away in this case. And it seems like an appealing change, since we do think of wildcard capture as a questionable last resort, and in Case 2 it isn't actually needed at all.

As for Case 1, we think we could advise the library author that they ought to change the signature of their method to use [? <: A] instead of [A] and that this would be a reasonable request for us to make of them, since their code is all about wrapping a Java type and it's normal for library authors to have to take some extra care when doing that due to the various impedance mismatches that can arise — in this case, the particular impedance mismatch is that Java doesn't have declaration-site variance, so although juc.Future is morally covariant, the compiler doesn't know that.

We've been discussing this back and forth for 2+ hours now, so I haven't been able to write down all of our thinking, but I wanted to at least get this summary down.

@SethTisue
Copy link
Member

Just opening the barn door for Java-defined types remains an option, but I don't find it very satisfactory, which is why we've been trying so hard to find alternatives. I feel like it simultaneously opens the barn door too wide (we don't need to exempt all Java types from this checking) and not wide enough (these two use cases happen to involve a Java-defined type, but I'm not convinced that it's a necessary condition).

@SethTisue
Copy link
Member

Dale thinks Case 2 might be fixable by reordering the sequence of events in typer

his PR takes a different approach — see #16799

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants