Ignore private constructors #778
Conversation
This enables the doMethodOverloads in MimaUnpicker for constructors as well which was commented out with a todo.
…rivate constructors.
…on field on the case class. This also fixed binary compatibility of mima-core.
|
The build failed due to binary compatibility problems caused by an additional field in DefDef in the TastyUnpickler. I don't really see a way to avoid this. I've added the problems to the mima filters for now. |
…DefDef to the mima filters.
|
I'm sorry we haven't found a reviewer for this yet. I know it's discouraging to work on something and then get weeks of silence. Sadly, MiMa is without a primary owner at present. It has had a series of them in the past, but they've moved on to other things. If I can't find someone more knowledgeable than myself to review it before too much longer, I'll take a look and presumably optimistically-merge it before too much longer. (But not this week, busy with Scala Days.) |
There was a problem hiding this comment.
Thank you @jeroentervoorde for the pull request!
I am not familiar with the codebase but the approach looks good to me, although I am not sure about the changes in TastyUnpickler.scala. It would be good to get feedback from someone more experienced with the project.
I agree that adding a test with a case class and -Xsource:3 would be amazing!
core/src/main/scala/com/typesafe/tools/mima/core/TastyUnpickler.scala
Outdated
Show resolved
Hide resolved
| def copyClassPrivate(tree: Tree, pkgInfo: PackageInfo): Unit = new ClassTraverser(pkgInfo) { | ||
| override def forEachClass(clsDef: ClsDef, cls: ClassInfo): Unit = { | ||
| } | ||
|
|
||
| override def traverseTemplate(tmpl: Template): Unit = { | ||
| super.traverseTemplate(tmpl) | ||
| doMethods(tmpl) | ||
| } | ||
|
|
||
| def doMethods(tmpl: Template) = { | ||
| val clazz = currentClass | ||
| tmpl.meths.iterator | ||
| .toSeq.groupBy(_.name).foreach { case (name, pickleMethods) => | ||
| doMethodOverloads(clazz, name, pickleMethods) | ||
| } | ||
| } | ||
|
|
||
| def doMethodOverloads(clazz: ClassInfo, name: Name, pickleMethods: Seq[DefDef]) = { | ||
| val bytecodeMethods = clazz.methods.get(name.source).filter(!_.isBridge).toList | ||
| if (pickleMethods.size == bytecodeMethods.size && pickleMethods.exists(_.classPrivate)) { | ||
| bytecodeMethods.zip(pickleMethods).foreach { case (bytecodeMeth, pickleMeth) => | ||
| bytecodeMeth.classPrivate = pickleMeth.classPrivate | ||
| } | ||
| } | ||
| } | ||
| }.traverse(tree) |
There was a problem hiding this comment.
Could you please explain why it is not possible to do the job in copyPrivateWithin? I am a bit lost.
There was a problem hiding this comment.
I looked into it a bit more and actually was possible. The classPrivate flags are now also copied for ValDefs but I guess that's ok.
To make this work:
- Added option to skip tests (using skip-${binaryVersion}.txt)
- Added support for scala-options-${binaryVersion}.txt) which can be used to set -Xsource:3 (or other scalac options)
|
I did add a test for the private constructor change. I had to add some new functionality to the test runner though:
I also noticed that i did not get a warning for the unapply method in scala 3 as the signature doesn't actually change anymore (at least in my test). Is this always the case with scala 3? |
Yes. I think weird things could happen at run-time, but the type signature is the same. |
|
@dwijnand would you be interested in reviewing this PR? |
The comment is outdated as the issue was fixed in MiMa: lightbend-labs/mima#778
I've added a classPrivate flag in the same fashion as the scopedPrivate flag and got it value from the pickled flags.
This works for normal classes with private constructors as demonstrated by the test but I could use some input if there are other constructs that would require additional unit tests.
Case classes work as well but they need the -Xsource:3 option and private unapply (as documented here) to pass.
I could add a test for that, either by making it a nok test or by adding some way to enable the -Xsource:3 option for that specific test.
I also enabled the doMethodOverloads in MimaUnpicker for constructors as well which was commented out with a todo.
A side effect is that private[...] constructors are now also ignored (using the existing isScopedPrivate flag) but I'm not actually sure this is the wanted behaviour. If not I propose to add a doConstructorOverloads that only checks the classPrivate flag and either keep doMethodOverloads as is or also include the check for classPrivate is done in the PR.
References #738