Handle HKT bounds in Java generic signatures#25744
Conversation
tanishiking
left a comment
There was a problem hiding this comment.
Overall approach looks good, but hasNested doesn't seem capture the case where expanded type's type arguments has different shape.
| case TypeParamRef(binder, paramNum) => | ||
| binder.paramInfos(paramNum).hi match | ||
| case hkt @ HKTypeLambda(_, _) => | ||
| val instantiated = hkt.instantiate(a.args).dealias |
There was a problem hiding this comment.
[note]
.dealias is required, otherwise type M[X,Y] appears in generic signature in the test below. 👍
| // However, since Java doesn't have a way to refer to HKTs in generic signatures, | ||
| // we must trade precision for termination by only resolving one level, | ||
| // otherwise we end up in infinite loops, e.g., in `X[A] <: Thing[X[A]]` we keep resolving `X -> Thing[X[A]]`. | ||
| val hasNested = instantiated.existsPart: |
There was a problem hiding this comment.
[note]
def f[A, X[T] <: Iterable[X[T]]]: X[A] = ???this resolves to
X[A] -> Iterable[X[A]] -> Iterable[Iterable[X[A]]] -> ...
We detect X[A] in the next level (Iterable[X[A]]) and if we found a same structure in 1-level resolved signature, stop resolving.
There was a problem hiding this comment.
[important]
Oh wait, but what if we have slightly different resolved type signature like:
trait RecursiveSig:
def example[A, X[t] <: Iterable[X[List[t]]]](s: String): X[A] = ???X[A] -> Iterable[X[List[A]] which doesn't captured by hasNested. It ends up infinite loop.
We should stop if we found X[...] with what ever type arguments.
There was a problem hiding this comment.
thanks, that's indeed more correct and also simpler
| @@ -0,0 +1 @@ | |||
| scala.collection.Iterable<scala.Tuple2<K, A>> | |||
There was a problem hiding this comment.
maybe we wanna move these tests into tests/generic-java-signatures instead of tests/run.
There was a problem hiding this comment.
what's the purpose of this generic-java-signatures folder exactly? we could move a lot of tests in there indeed
There was a problem hiding this comment.
files under tests/generic-java-signatures runs in separate JUnit suite
scala3/compiler/test/dotty/tools/dotc/CompilationTests.scala
Lines 181 to 184 in d23d5ef
On the other hand, files under tests/run could be belong to multiple JUnit suite, like run (with 2 different options), pickle, and recheck. That means we could run test multiple times.
So, if the test is specific for checking java generic signatures, they would be nice to go to tests/generic-java-signatures. (While I also somtimes forget about that...). And yeah, maybe we wanna move bunch of tests in there in another PR.
There was a problem hiding this comment.
Not testing multiple times is nice... also I just realized it means we don't need to explicitly add scalajs: --skip and that's already a good enough reason for me :D
I'll grep scalajs: --skip and move the ones that belong to genericJavaSignatures in another PR 👍
eb44b2d to
206a364
Compare
| @@ -0,0 +1 @@ | |||
| scala.collection.Iterable<A> | |||
There was a problem hiding this comment.
Is this right? If I expand X[A] once to its bound I get Iterable[X[List[A]], which doesn't look like Iterable[A]. It's not something changed / introduced by this PR, but worth a ticket?
There was a problem hiding this comment.
No, indeed, that's not right... what should we have instead? Iterable as a raw type?
There was a problem hiding this comment.
part of the problem is I don't know what the spec of these things is
is still true 16 years later, I guess (scala/bug#3249)...
A raw type is probably sound? Or a wildcard argument scala.collection.Iterable<*>?
There was a problem hiding this comment.
I tried understanding what these things are supposed to be, but in another recent PR I ran into a javac assert on what I believed to be a valid signature, so I stopped trying. Let's go with a raw type here as long as tests pass.
There was a problem hiding this comment.
I agree with @lrytz, from X[A] <: Iterable[X[List[A]]], the best approximation seems Iterable<?>. A raw Iterable is ok, but it loses more generic information. If javac / signature accepts it (I'm wondering when javac assert for wildcard 🤔) ideally we should have Iterable<?> here.
but I think it's good to go with raw type, and open another issue to track this?
tanishiking
left a comment
There was a problem hiding this comment.
LGTM, thanks!
I think we wanna move tests to tests/generic-java-signatures, but that can be done in another PR with other test files all together.
| @@ -0,0 +1 @@ | |||
| scala.collection.Iterable<scala.Tuple2<K, A>> | |||
There was a problem hiding this comment.
files under tests/generic-java-signatures runs in separate JUnit suite
scala3/compiler/test/dotty/tools/dotc/CompilationTests.scala
Lines 181 to 184 in d23d5ef
On the other hand, files under tests/run could be belong to multiple JUnit suite, like run (with 2 different options), pickle, and recheck. That means we could run test multiple times.
So, if the test is specific for checking java generic signatures, they would be nice to go to tests/generic-java-signatures. (While I also somtimes forget about that...). And yeah, maybe we wanna move bunch of tests in there in another PR.
| @@ -0,0 +1 @@ | |||
| scala.collection.Iterable<A> | |||
There was a problem hiding this comment.
I agree with @lrytz, from X[A] <: Iterable[X[List[A]]], the best approximation seems Iterable<?>. A raw Iterable is ok, but it loses more generic information. If javac / signature accepts it (I'm wondering when javac assert for wildcard 🤔) ideally we should have Iterable<?> here.
but I think it's good to go with raw type, and open another issue to track this?
Fixes scala#15903 ## How much have you relied on LLM-based tools in this contribution? zero much ## How was the solution tested? issue tests (+ 1 case inverting the arg order)
Fixes #15903
How much have you relied on LLM-based tools in this contribution?
zero much
How was the solution tested?
issue tests (+ 1 case inverting the arg order)