Removing method while making trait package-private is NOK #699
Conversation
|
Fixed my test, but broke two existing ones:
|
|
IIUC IMHO it should also be raising an error. According to scalacenter/sbt-version-policy#71 forward compatibility is used as a proxy for checking source-compatibility. Making a class public is not a source-compatible change. |
|
First I'll just address the point about source compat. It's not really MiMa’s business, as I understand it, to test anything except binary compatibility proper. I'm aware that sbt-version-policy chooses to use forward-and-backward bincompat as a "proxy" for source compatibility — an approximation, a stand-in, however you want to to describe it. But the fact that it’s doing so isn’t really, in my view, something you can bring back to this repo as the basis for a decision. The readme here states:
arguably that’s too unequivocal, and some such wording as “provides only incomplete insight” would be better, but regardless, my point stands. |
I see your point, but I think it's a little blurry. If we were testing only binary-compatibility and nothing more, then MiMa shouldn't automatically filter any binary-incompatible changes, even those to package-private stuff, because package-private is a source-level concept. Currently MiMa does have some understanding of whether changes are source-breaking or not and uses that to determine what to report. For example, MiMa will not report a change to a package-private method, because even though it is binary-breaking, it is not source-breaking (because it's not public). |
|
As you were writing the above, I was writing the following: That said, this business about package-private does seem to rather blur the line, doesn't it? I'm not sure what the best way to think or talk about it this is. Perhaps you noticed that I never came back to scalacenter/sbt-version-policy#71 with any resolution of the ambivalence I expressed there. It isn't because I forgot, it's because even after all this time, I still don't know what I think. I don't mean to rule out any invocation of any concept of source compatibility here in this repo, I'm only specifically casting shade on leaning on the "source compat (approximately) equals forward-and-backward-bincompat" equation/analogy/whatever-it-is. In practice, MiMa users have a real and justifiable need to let pass some API changes which are technically not binary compatible at the bytecode level. This package-private business is an example of that. I need to give the specific test cases you mention some further thought. |
|
One thing I'd like to ask @dwijnand here is: how do we currently think about forward compatibility in this repo? Historically, MiMa hasn't had a separate concept of forward compatibility. The exact same code was used to check both directions, and the forward-compat check consisted of simply swapping the order of the inputs. Is that still the case, or did MiMa acquire a subtler notion of direction since I last looked? Was this considered at the time the package-private exemption was added? |
|
That question bears on whether it's valid to make arguments like:
That seems hand-wavy and questionable to me. If I understand correctly, that test case was written and intended as a test of backward compatibility. Because the change is, in fact, backward compatible, yes? MiMa is fundamentally a backward-compatibility checker and I think we need to tread very carefully as soon as we start talking about either “forward compatibility”, or “source compatibility". wdyt? |
|
Maybe @sjrd would like to bat these issues around along with us :-) |
I don't know. I'm just going by the fact that the test is making an assertion in the forwards direction, not the backwards :) |
Yeah. It looks like Dale added the The readme is silent on the subject. |
You're right and that test is wrong. I think I just wrote both directions and didn't think properly about it: the old API should be the reference point (thinking in terms of backwards binary compatibility being the main target use case for MiMa). I can look to making the suite pass as a whole.
The idea was not coming at it from a "source-breaking" perspective but as a qualifier for what the public API is. For instance we always filtered out bytecode private methods, even if they changed, because no client code would suffer that change. We took that concept and extended it to the reality of package private access and annotated APIs, even if they're not enforced by the runtime, as a pragmatic convenience. Not because of any source compatibility concerns or desires.
Still the same: swap the order, compare the other way.
That's would be to get the test to pass, with the various invariants the setup makes them hold (e.g. if you say there's a backwards problem, then running an app compiled against v2 running against v1 should blow up - fail the test if that's not true). Using a "fowards.txt" there is misleading, I agree - and there's probably already a way to make it work in a different way, with the various "pending"/etc escape hatches that are set up. |
Reclassify package-private-class-becomes as breaking, my bad. Fix package-private-class-unbecomes, and the fix also correct the confusion: it's a backwards binary compatibility test.
Write the test in Scala seeing though we can, and it's hard to write it in Java such that it cross-compiles in Scala 2.11-3
On mobile (and on vacation this week), so I'll limit my comment to saying that I fully agree with that quoted comment. ;) |
ah okay good well that's simple then :-) |
Even though the trait is package-private in the current version, bincompat should still be enforced against the public version of the trait in the prior version.
This PR adds a failing test to demonstrate the issue. Not sure if I have the chops to fix it, but I'll give it a shot.