Skip to content
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

[2.13.9] productIterator change to case classes breaks bincompat if the class is also a value class #12650

Closed
adamw opened this issue Sep 23, 2022 · 15 comments · Fixed by scala/scala#10155

Comments

@adamw
Copy link

adamw commented Sep 23, 2022

MiMA reports the following error when trying to update to 2.13.9, for value classes:

[error] core: Failed binary compatibility check against com.softwaremill.sttp.model:core_2.13:1.5.2! Found 2 potential problems
[error]  * extension static method productIterator$extension(java.lang.String)scala.collection.Iterator in class sttp.model.Method does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("sttp.model.Method.productIterator$extension")
[error]  * extension method productIterator$extension(java.lang.String)scala.collection.Iterator in object sttp.model.Method does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("sttp.model.Method.productIterator$extension")

this is for a value case class:

case class Method(method: String) extends AnyVal

is this something that can be safely ignored?

@som-snytt
Copy link

I think it is not safe.

Reported at lightbend-labs/mima#723

@adamw
Copy link
Author

adamw commented Sep 23, 2022

Oh thanks! I'll close this one as it is clearly a duplicate :)

@adamw adamw closed this as completed Sep 23, 2022
@som-snytt
Copy link

I'll probably need a ticket over here...

@adamw
Copy link
Author

adamw commented Sep 23, 2022

Ah sure, sorry, reopening then ;)

@adamw adamw reopened this Sep 23, 2022
@som-snytt
Copy link

➜ scalac -d /tmp value.scala
➜ scalac -d /tmp -cp /tmp valuation.scala
➜ scala -cp /tmp Test
hello, world
➜ skalac -d /tmp value.scala  # recompile the case class
➜ scala -cp /tmp Test
Exception in thread "main" java.lang.NoSuchMethodError: 'scala.collection.Iterator Value$.productIterator$extension(java.lang.String)'
        at C.f(valuation.scala:4)

where scalac is 2.13.8 and skalac is 2.13.9 and

➜ cat value.scala

case class Value(s: String) extends AnyVal
➜ cat valuation.scala

class C {
  def f() =
    Value("hello, world").productIterator.foreach(println)
}

object Test extends App {
  new C().f()
}

@SethTisue SethTisue changed the title [2.13.9] mima reports a missing productIterator$extension(java.lang.String)scala.collection.Iterator [2.13.9] productIterator change to value classes breaks bincompat Sep 23, 2022
@SethTisue SethTisue added this to the 2.13.10 milestone Sep 23, 2022
@SethTisue
Copy link
Member

SethTisue commented Sep 23, 2022

The PR in question is scala/scala#10002 .

In addition figuring out how to address the issue itself, we should also give a little thought to whether our overall testing regimen could have or should have caught this.

(As Som noted on the MiMa ticket, apparently the Scala standard library doesn't have any case classes which are also value classes — if it did, this issue would have shown up.)

@SethTisue SethTisue changed the title [2.13.9] productIterator change to value classes breaks bincompat [2.13.9] productIterator change to value classes breaks bincompat Sep 23, 2022
@som-snytt
Copy link

som-snytt commented Sep 23, 2022

For convenience, copying my comment

A case class extending AnyVal had product methods:

  public static java.lang.String productElementName$extension(java.lang.String, int);
  public static scala.collection.Iterator<java.lang.Object> productIterator$extension(java.lang.String);
  public static java.lang.Object productElement$extension(java.lang.String, int);
  public static int productArity$extension(java.lang.String);
  public static java.lang.String productPrefix$extension(java.lang.String);
  public scala.collection.Iterator<java.lang.String> productElementNames();
  public java.lang.String productPrefix();
  public int productArity();
  public java.lang.Object productElement(int);
  public scala.collection.Iterator<java.lang.Object> productIterator();
  public java.lang.String productElementName(int);

where productElementNames lacks an extension method and would therefore always box.

Now productIterator also lacks an $extension method for some reason. (Presumably, the reason is that methods inherited from a trait don't get the value class treatment.)

Product extends Any is a universal trait.

@SethTisue SethTisue changed the title [2.13.9] productIterator change to value classes breaks bincompat [2.13.9] productIterator change to case classes breaks bincompat if the class is also a value class Sep 23, 2022
@som-snytt
Copy link

Worth noting that we had considered binary compatibility in the PR for case classes, but value classes did not appear on the radar.

Sometimes a bytecode diff is performed to witness any changes in output; it would be a bit much to attempt that on CB scala; but one could imagine a modest set of classes in test to validate diffs in bytecode (not only for compatibility).

@som-snytt
Copy link

To confirm, the SIP specifically excludes inherited members. https://docs.scala-lang.org/sips/value-classes.html

It is cold comfort that Scala 3 does not generate productIterator for case classes.

@mkurz
Copy link

mkurz commented Sep 26, 2022

If this is getting fixed, how long until Scala 2.13.10?

@lrytz
Copy link
Member

lrytz commented Sep 26, 2022

I think we should do 2.13.10 very soon to avoid (too many) releases built with 2.13.9 to make their way into the wild.

Should we keep this ticket open to make it easier to find?

@lrytz lrytz reopened this Sep 26, 2022
@armanbilge
Copy link

I think we should do 2.13.10 very soon to avoid (too many) releases built with 2.13.9 to make their way into the wild.

Can we trust MiMa to catch all instances of this? Or should we avoid releasing projects on 2.13.9 altogether?

@lrytz
Copy link
Member

lrytz commented Sep 26, 2022

If you're using MiMa you should be safe - modulo unknowns, of course...

@SethTisue
Copy link
Member

I think we should do 2.13.10 very soon

There's now a thread to discuss release contents and timing: https://contributors.scala-lang.org/t/scala-2-13-10-release-planning/5943

@SethTisue
Copy link
Member

SethTisue commented Oct 8, 2022

Should we keep this ticket open to make it easier to find?

Closing now since we'll be publishing 2.13.10 shortly, and since we've done a bunch of publicity about the regression in various venues (updated GitHub release notes, Discourse, Twitter, etc)

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