Fix #19951: Align TASTy with the Java annotation model.

[sjrd]

Scala annotations are classes, with a real constructor, which has a real signature where order is relevant but names are irrelevant.

On the contrary, Java annotations are interfaces, without any real constructors. The names of "fields" are relevant, whereas their order is irrelevant.

As illustrated by #19951, trying to shoehorn Java annotations into the Scala annotation model is not sustainable, and breaks in real ways. Therefore, in this commit we align how Java annotations are stored in TASTy with the Java annotation model.

During pickling:

• Selection of the constructor is pickled without a signature.
• Default arguments are dropped.
• (Due to the parent commit, all arguments are NamedArgs at this point.)

During unpickling:

• Selection of the constructor resolves to the unique constructor (instead of complaining because a signature-less SELECT should not resolve to a member with a signature).
• Arguments to the constructor are reordered and extended with defaults to match the target constructor; we can do this because all the arguments are NamedArgs.

For backward compatibility, during unpickling:

• If we read a SELECTin for a Java annotation constructor, we disregard its signature and pretend it was a SELECT.
• We adapt arguments in best-effort way if not all of them are NamedArgs.

---

Still needs new tests. CI only for now.

[smarter]

The change to signatures looks like the only part that couldn't be backported to 3.3 (because it would break the existing unpickler logic). If we agree that we'd like to backport the rest to 3.3, maybe this change should be done in a separate PR after we've validated that the other changes work together?

[sjrd]

If we leave this out, we also need to leave out the other change in this file. Otherwise we'll pickle constructor calls where the actual arguments do not match the signature, so an older compiler will also choke on it.

That said, as the latest push shows, even explicitly expanding all NamedArgs (the first commit) will break the current version of WartRemover. So I don't think we can backport this to 3.3.x at all.

[smarter]

I guess this will need to be discussed more, but I think breaking WartRemover could be OK if we allow enough time for upstream to release a fixed version before we release a 3.3.x with the backport in. It seems like this would lead to less issues in practice in the long term.

[sjrd]

/cc @xuwei-k for the changes to the test at
https://github.com/scala/scala3/pull/20539/files#diff-3514058c43797515edabf8b5ca77c277e9d338f82d0ad7f9b022db83e27fd674
which shows that WartRemover's current implementation will be broken by this change.

[sjrd]

@Gedochao If we merge this now, we must backport it to 3.5.0-RC2. The alternative is to wait for 3.6.0.

[Gedochao]

@Gedochao If we merge this now, we must backport it to 3.5.0-RC2. The alternative is to wait for 3.6.0.

Sounds like a job for Core 🦸
Let's discuss and decide later this week.

[smarter]

Could you add a test in sbt-test/scala3-compat/ to check that we can read annotations produced by an older Scala 3 as best as we can?

[smarter]

Java classes get a dummy extra constructor when parsing from sources:

scala3/compiler/src/dotty/tools/dotc/parsing/JavaParsers.scala

Lines 124 to 131 in 04ada79

	// A dummy first constructor is needed for Java classes so that the real constructors see the
	// import of the companion object. The constructor has parameter of type Unit so no Java code
	// can call it.
	// This also avoids clashes between the constructor parameter names and member names.
	if (needsDummyConstr) {
	if (constr1 == EmptyTree) constr1 = makeConstructor(List(), Nil, Parsers.unimplementedExpr)
	stats1 = constr1 :: stats1
	constr1 = makeConstructor(List(UnitTpt()), tparams, EmptyTree, fakeFlags)

So I wonder if this check is enough in the case where we're unpickling from tasty a reference to a java annotation that is also being passed to the compiler as source in the current run (this would be good to test anyway). The dummy constructor has the flag PrivateLocal set so we could filter that out explicitly.

[sjrd]

Hum. I'm happy to filter out PrivateLocal. That said I'm not sure how we can construct such a scenario in practice. Since the Scala class uses the annotation, it must be compiled together with or later than the annotation. But then how would we have the Scala class from TASTy but the Java class from source? Perhaps in some complicated incremental compilation scenario with mixed compilation? 🤔

[sjrd]

I believe I addressed all the comments so far.

[Gedochao]

This has been decided to be included in the (TBA) 3.6.0 release.

[sjrd]

Rebased to be in the 3.6.0 cycle.