Skip to content

Refine parameter adaptation logic for arrays #23591

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bracevac
Copy link
Contributor

@bracevac bracevac commented Jul 23, 2025

Fixes #23179

Since JDK 9, argument checks for LambdaMetaFactory have become stricter. Which is to say that its JDK 8 version was too lax.

We apply adaptation now in case the samType is an array, but the implType is not.

@bracevac bracevac requested a review from lrytz July 23, 2025 18:01
@sjrd
Copy link
Member

sjrd commented Jul 23, 2025

This seems misguided to me. Why AnyRef but not any of its subtypes? Also, this is not supported by spec text, whether from the Scala 2 or Scala 3 spec.

Changes to erasure are extremely risky, because they basically break tasty and binary compatibility every time. They need a very strong justification. Typically, only fixing unsoundness is a good enough reason to do this.

@bracevac
Copy link
Contributor Author

bracevac commented Jul 23, 2025

Scala 2 certainly erases this array type to Object[]. What should be the general rule?

Edit: Hmm it does not, at least judging by -Xprint:erasure. However in the resulting bytecode, the two relevant arguments to altMetaFactory are consistently array types, whereas in Scala 3 the second one ends up being Object.

BootstrapMethods:
  0: #38 REF_invokeStatic java/lang/invoke/LambdaMetafactory.altMetafactory:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite;
    Method arguments:
      #26 ([Ljava/lang/Object;)Ljava/lang/Object;
      #30 REF_invokeStatic Test$.$anonfun$new$1$adapted:([Ljava/lang/Object;)Ljava/lang/Object;
      #26 ([Ljava/lang/Object;)Ljava/lang/Object;
      #31 0

@bracevac
Copy link
Contributor Author

If we do not want to mess with erasure, could we just uniformly use the erased SAM methodtype for the implementation method type as well in the emitted bytecode? Would this always be safe for a type-correct source program?

@sjrd
Copy link
Member

sjrd commented Jul 24, 2025

Probably, but we would need to do that at the backend level. Basically consider it a bridge method, or an $adapted something something. The extracted private def likely cannot be altered to take a different parameter type.

Fixes scala#23179

Since JDK 9, argument checks for LambdaMetaFactory have become stricter.
Which is to say that its JDK 8 version was too lax.

We apply adaptation now in case the samType is an array,
but the implType is not.
@bracevac bracevac changed the title Ensure Array[? >: AnyRef] erases to Object[] Refine parameter adaptation logic for arrays Jul 24, 2025
Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new approach seems reasonable.

It might need similar changes in ExpandSAMs for Scala SAM types that don't translate to target SAM types.

!sameClass(implType, samType) && !autoAdaptedParam(implType)
|| (samType, implType).match {
case (defn.ArrayOf(_), defn.ArrayOf(_)) => false
case (defn.ArrayOf(_), _) => true // see #23179
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't there be the reverse order as well ((_, defn.ArrayOf(_)))? Like if the SAM takes an Array[T] where T <: AnyRef but the lambda is specialized to T = String, for example?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unable to construct such a reversed example that will be accepted by the compiler.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need something similar for the result type (resultAdaptationNeeded)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, I also fail here to construct an example that would cause a run time crash (which is of course weak evidence).

@bracevac
Copy link
Contributor Author

The new approach seems reasonable.

It might need similar changes in ExpandSAMs for Scala SAM types that don't translate to target SAM types.

I tried constructing each of the five cases in the comment at the top of ExpandSAMs, but those were either rejected or did not crash at run time.

@lrytz
Copy link
Member

lrytz commented Jul 25, 2025

It might be worth including the other combinations in the test case as regression tests.

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

Successfully merging this pull request may close these issues.

LambdaConversionException: Type mismatch for instantiated parameter
3 participants