Fix withDottyCompat in Scala Native #1738
Conversation
lolgab
force-pushed
the
fix-withDottyCompat-scala-native
branch
from
6e9f879
to
1f6bfd5
Compare
| @@ -224,6 +226,33 @@ trait ScalaNativeModule extends ScalaModule { outer => | |||
| ) | |||
| )) | |||
| } | |||
|
|
|||
| override def transitiveIvyDeps: T[Agg[Dep]] = T { | |||
| // TODO when in bin-compat breaking window: Change list to `super.transitiveIvyDeps()` | |||
There was a problem hiding this comment.
Is there any particular reason, why you can't use super.transitiveIvyDeps() in ScalaNativeModule?
Could you post the output of MiMa here? Maybe, it's only something macro related, which won't really affect any users? Just from looking at the code, it should not change anything, as we already override a def.
There was a problem hiding this comment.
Sure.
Here you find the output:
Scanning binary compatibility in /Users/lorenzo/scala/mill/out/scalanativelib/compile.dest/classes ...
[#0] Found 1 issue when checking against com.lihaoyi:mill-scalanativelib_2.13:0.10.0
[#0] * abstract synthetic method mill$scalanativelib$ScalaNativeModule$$super$transitiveIvyDeps()mill.define.Target in interface mill.scalanativelib.ScalaNativeModule is present only in current version
filter with: ProblemFilter.exclude[ReversedMissingMethodProblem]("mill.scalanativelib.ScalaNativeModule.mill$scalanativelib$ScalaNativeModule$$super$transitiveIvyDeps")
1 targets failed
scalanativelib.mimaReportBinaryIssues Failed binary compatibility check! Found 1 potential problems
There was a problem hiding this comment.
Ok, I really don't like the fact that we need to work around bin compat that way. On the other side, bin compat issues are real. So let us think a bit harder if there is no better solution here. Or alternatively, how we can ensure, we fix this code duplication once we create the next major version.
There was a problem hiding this comment.
We can have a standard comment to grep and fix before release, or a milestone with issues for these kind of problems. The problem with the latter is that we might have non-issues like this (they are not real issues but maintenance reminders) in the Issue tracker.
There was a problem hiding this comment.
We could also create PRs, label them with bin-compat-breaker and assign them to next major milestone. Only downside would be, they clutter the PR view.
There was a problem hiding this comment.
It could be an indicator for the next release then, if too many of them laying around. 😀
There was a problem hiding this comment.
I still wonder in which situations this issue will be a problem. I'm not an expert, but my suspicion is, that we won't see any issues at all. At least non, we won't get anyways. If a derived class/trait overrides this def, it should be no problem no super calls are necessary. And if it calls super, it has to search the hierarchy anyways. If the issue should be, that the previous super target (which is in javaModule) is fixed in the bytecode, won't it miss our new overridden def in any case? If anybody has some good example, I'm more than glad to learn something new.
There was a problem hiding this comment.
There is some explanation here: https://github.com/lightbend/mima/blob/b20f56d9af7543ecbf7c78fe997ca0cf4e3b2a69/core/src/main/scala/com/typesafe/tools/mima/lib/analyze/method/MethodChecker.scala#L124
But I can't understand it.
From my side, since the maintenance cost is not so high, I would leave the duplication, and have an open PR with a special label as you suggested.
lolgab
force-pushed
the
fix-withDottyCompat-scala-native
branch
from
1f6bfd5
to
6d1c4b8
Compare
|
Thanks @lolgab ! |
Port of the workaround Scala Native is doing for Sbt in this PR: scala-native/scala-native#2553