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
Fix sneaky binary incompatibility in Discover.scala #2752
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a pure usability perspective this seems to fix the issues I was seeing locally. From a code perspective 😅 I'll just refer to your expertise here, but it still seems that Mima is unhappy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please ads @deprecate
annotations denoting these additions as bincompat shims. It helps with cleaning later.
Co-authored-by: Chris Kipp <ckipp@pm.me>
Co-authored-by: Chris Kipp <ckipp@pm.me>
Co-authored-by: Tobias Roeser <le.petit.fou@web.de>
Co-authored-by: Tobias Roeser <le.petit.fou@web.de>
I applied more tweaks due to binary compatibility requirements against |
fixes #2749
The basic problem is that the signature of
Discover.apply()
changed, but it only changed in the argument's generic type parameters, so it was nominally the same type after erasure, and accepted theMap[Class[_], Seq]
even though it was expecting aMap[Class[_], (Seq, Seq)]
, but later on in the code it would blow up with aClassCastException
This causes problems with Mill plugins containing external modules, which have their own
Discover[T]
macro pre-expanded and would not get re-compiled with the new 0.11.3 version of Mill.This PR works around the problem by adding a forwarder
Discover.apply(value: Map[Class, Seq])
that does the right thing and expands theSeq
into a(Seq, Seq)
, while adding a dummy paramter tocase class Discover
to avoid conflicts. A similar forwarderDiscover.<init>(value: Map[Class, Seq])
is needed to preserve binary compatibility (I think?)Tested manually via
./mill -i dev.run example/basic/1-simple-scala --import ivy:io.chris-kipp::mill-github-dependency-graph::0.2.6 io.kipp.mill.github.dependency.graph.Graph/generate
, which fails before this PR and passes after