scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
232 stars 21 forks source link

Case companions no longer extend `FunctionN` under `-Xsource:3-cross`, breaking binary compatibility #12961

Closed lrytz closed 8 months ago

lrytz commented 9 months ago

2.13.13 aligned synthetic case companions with Scala 3 to no longer extend FunctionN under -Xsource:3-cross (https://github.com/scala/scala/pull/10648).

Projects that already have cross-published releases for Scala 2.13 and 3.x cannot adopt -Xsource:3-cross because this change is not binary compatible.

The workaround of manually writing out object C extends FunctionN is not a solution because that source change would also affect Scala 3, causing a binary incompatibility there. (It would also be very tedious.)

This situation can be a blocker for libraries to upgrade to Scala 2.13.13. Example:

A more general question is how do we add further language changes to -Xsource:3-cross in the future. Every semantic change comes with a warning under -Xsource:3. But projects that are already on 3-cross would silently get new semantics in a new Scala version.

lrytz commented 9 months ago

@SethTisue mentioned the possibility of having separate flags for language changes adopted from Scala 3. This would help here:

So -Xsource:3 to enable warnings (like we did in 2.13.13), something like -Xscala-3-semantics:caseCompanionParents,unicodeInterpolators,... to adopt changes and disable the corresponding warnings. -Xsource:3-cross would be removed.

SethTisue commented 9 months ago

-Xsource:3-cross would be removed

Oh, I wasn't thinking of going that far! I was thinking something like -Xsource3-cross:-caseCompanionParents, by analogy with e.g. -Xlint:-byname-implicit

som-snytt commented 9 months ago

3-cross is for projects that want to cross-compile, not for projects that can't cross-compile.

Sometimes you need versioned sources; that is the mechanism.

We did not have a discussion, but I avoided the fine-grained feature selection as in -Xlint, which is a hassle and finicky. (To say more, -Xlint has opt-out as a last resort; you're not supposed to need it in normal operation because these are recommended warnings. Similarly, -Xsource:3 means you're eagerly migrating and you want to make any adaptations early in the migration process.)

Arguably, dotty -source:foo-migration is a coarse-grained mechanism, but it's not obvious what to call such "virtual future versions" in 2.13.x. That would be rolling cumulative changes and not pick-and-choose.

I agree about silent semantics in 3-cross; I might have suggested a verbose mode for that so you can audit your build. Currently, the obvious thing is that it should emit scala3-migration warnings that are silenced by default, so you just have to enable them to do an audit.

The purpose of future source migration is to shake out the code base for migration to minimize pain as much as possible. (Pain could be maintaining two source versions or the pain of switching to dotty at some future point in time.) It's not a feature buffet.

SethTisue commented 9 months ago

3-cross is for projects that want to cross-compile, not for projects that can't cross-compile.

I don't think I understand your point here?

Sometimes you need versioned sources

In an ordinary project, though, it would incredibly cumbersome to have versioned sources for all of your case classes, so I don't think that's a satisfying answer for this particular issue (case class companion parentage).

SethTisue commented 9 months ago

oh btw big thanks to @mkurz for discovering and reporting this promptly (over at https://github.com/playframework/playframework/pull/12401)

lrytz commented 9 months ago

source:3 is about minimizing the pain of migrating, source:3-cross about reducing some of the pain of cross-building. Projects will probably be cross-building for a very long time, so it's certainly worth investing here and trying to get it right.

som-snytt commented 9 months ago

Sample investment would be https://github.com/scala/scala/pull/10648 "For migration to 3, accommodate case companion as function".

Sample ticket where accommodation may (arguably) be unreasonable (because they are interested in piecemeal language forking): https://github.com/scala/bug/issues/12960

If we wanted forking options, we would introduce -Y. (For that ticket, there is -Yno-predef already.)

If MyLib_2.13 is at v1.0, it's reasonable to say MyLib_2.13 crossbuilt as MyLib_3 is v2.0 for binary reasons (if nothing else). (Whatever the compatibility policy of MyLib happens to be; and for "end users" there may be no policy. They just want to leverage "early adoption".)

3-cross is for projects that want to cross-compile, not for projects that can't cross-compile.

I don't think I understand your point here?

"I don't want to lose case companion parentage" is incompatible with "I want one version of my case class that cross-compiles."

There's nothing wrong with, "We can't use -Xsource because there are too many differences. If we want to use Scala 3 later, we'll cross that bridge when we can see the river gorge from a distance."

SethTisue commented 9 months ago

"I don't want to lose case companion parentage" is incompatible with "I want one version of my case class that cross-compiles."

Ah, I see. Yes, good point. I strongly agree that we should keep in mind that the purpose of -Xsource:3-cross is to enable cross-compilation, not to give people a la carte access to bits and pieces of Scala 3.

However, in this particular situation I think offering a la carte access might be justifiable, because we would be adding it exactly to allow people who were already cross-building to continue to cross-build.

som-snytt commented 9 months ago

My other basic question is, why is "case companion breaks binary compat" different from "method result inference breaks binary compat". Both require tweaking source code (adding an object or an explicit type).

Maybe -Xsource:3-safe or 3-compat to turn off those "features which might break binary compat".

SethTisue commented 9 months ago

My other basic question is, why is "case companion breaks binary compat" different from "method result inference breaks binary compat". Both require tweaking source code (adding an object or an explicit type).

This case companion issue is different because there is no workaround (besides the unreasonably cumbersome version-specific sources option, I mean). You cannot work around it by tweaking cross-compiled source code. At least, it didn't seem to me and Lukas that a workaround exists — are we mistaken?

As Lukas wrote above,

The workaround of manually writing out object C extends FunctionN is not a solution because that source change would also affect Scala 3, causing a binary incompatibility there. (It would also be very tedious.)

som-snytt commented 9 months ago

I wrote "reasonable", but I may have meant "conceivable":

If MyLib_2.13 is at v1.0, it's reasonable to say MyLib_2.13 crossbuilt as MyLib_3 is v2.0 for binary reasons (if nothing else).

I guess that is just to say, Yes, if you want to cross-build, break binary compatibility. What do they call such a response in Intro to Rhetoric? In engineering, it is "cutting the Gordian knot".

lrytz commented 9 months ago

I see your point, but in reality we have to accomodate projects that are already cross-building and want to continue to do so on 2.13.13. Bumping the major version is a big step and it should not be required by a compiler upgrade.

lrytz commented 9 months ago

I finally remembered https://github.com/scala/scala-xml/pull/675: the "inferred return type of an overriding method" change is also not easy to work around. Spelling out the 2.13 return type is not enough because that's a binary incompatible change on Scala 3.

SethTisue commented 9 months ago

I finally remembered https://github.com/scala/scala-xml/pull/675: the "inferred return type of an overriding method" change is also not easy to work around. Spelling out the 2.13 return type is not enough because that's a binary incompatible change on Scala 3.

Oh good point, I should have thought of that. So if we end up doing the a-la-carte thing, I guess should make that one opt-out-able too.

som-snytt commented 9 months ago

How about -Xsource:3-compat to mean 2.13.12 -Xsource:3 + 2.13.13 -Xsource:3?

or -Xsource3 means -Xsource:3-compat and -Xsource:3-migration means warnings only. That way, leave -Xsource:3 alone when updating.

Edit: I will PR (as POC) -Xsource:3-migration (equivalent to -Xsource:3 -Xmigration) to warn, -Xsource:3 (includes previous behavior and warnings), -Xsource:3-cross -Xmigration (verbosely warn).

-Xmigration currently says configure warnings at w not e. In case someone forgot.

However, override inference was on -Xsource:3 for 2.13.9. Everything was simpler "back then", but the same questions were in play. That suggests -Xsource:3-compat to mean "without the behaviors that affect compatibility" (2 of them).

https://github.com/scala/scala/pull/9891

The nerd-sniping was on the ancient ticket: https://github.com/scala/bug/issues/7212

Previous to the 3-cross regime, the state of the art was "Emit migration warnings under -Xsource:3 as fatal warnings, not errors; -Xmigration turns off erroring" https://github.com/scala/scala/pull/10439

lrytz commented 9 months ago

I don't really follow, sorry. As far as I can tell, -Xsource:3 is fine the way it is.

We have 2 problems to solve for -Xsource:3-cross:

Maybe it looks overkill, but I'm currently thinking about a flag that replaces -Xsource:3-cross.

scalac -Xsource:3 -Yscala3semantics:since-2.13.13,-case-companion-function enables all semantic changes that are implemented in 2.13.13 except the "case companion FunctionN parent". If 2.13.14 adds new semantic changes they would see corresponding the warnings until they enable since-2.13.14.

eed3si9n commented 9 months ago

By default, I think compilers should be silent. I think we should avoid warnings if it did exactly what was told (i.e. make semantic changes similar to Scala 3) and there is nothing actionable, as opposed to quickfixes that are actionable.

As the code base gets larger in work settings, it should be assumed that the majority of the users probably will not read or notice compiler warnings because they're buried in existing warnings.

It might be more helpful if there were authoritative guide like https://docs.scala-lang.org/scala3/guides/migration/tooling-scala2-xsource3.html highlighting changes in 2.13.x, and those who venture into the -Xsource:3 flag or any variants would learn to read the page.

eed3si9n commented 9 months ago

Speaking of the semantic changes, I feel like Scala 2.13.13 shifts the meaning of -Xsource:3. -Xsource (originally -Xfuture) was all about enabling the language features that are not compatible with the 2.13.0 compatibility. See https://docs.scala-lang.org/overviews/compiler-options/:

-Xsource VERSION: Treat compiler input as Scala source for the specified version, see scala/bug#8126.

Here's what I suggest to restore -Xsource:3:

Feature No flag -Xmigration:3 -Xsource:3
(x: Any) + "" is deprecated Warning Warning does not compile, implicit any2stringadd is not inferred
Unicode escapes in triple-quoted strings and raw interpolations ( """\u0061""") Warning, escape is processed escape is not processed
An implicit for type p.A is found in the package prefix p Warning the package prefix p is no longer part of the implicit search scope
eed3si9n commented 9 months ago

@lrytz

If 2.13.14 adds new semantic changes they would see corresponding the warnings until they enable since-2.13.14.

Could we use -Xmigration:2.13.13 to mean that since I don't think anyone is using that flag much?

som-snytt commented 9 months ago

To summarize, -Xsource:3-cross is not binary compat, -Xsource:3-compat is binary compat, -Xsource:3-migration is warnings only, -Xsource:3 is an alias for -Xsource:3-migration in so far as it used to introduce any binary incompat.

Also, -Xmigration says turn on warnings for -Xsource:3-cross for auditing purposes.

I think just corralling the binary-compat issue is simpler than offering an option buffet.

som-snytt commented 9 months ago

@eed3si9n yes, -Xsource:3 was made the "weaker" option.

In a normal universe, all this would be in 2.14.

The notion that the entire -source:future is rolled into one 2.13 option is antithetical to planning or evolution, and is predicated on "if people are moving to dotty, then they will do so in a finite time frame." If they're still using -Xsource:3-cross and still updating 2.13 (if it has more updates), then it's the same bind as -Xlint, which is also "cumulative". Maybe that is the argument for an option buffet. Let them complain about having to tweak the build file when upgrading.

eed3si9n commented 9 months ago

@som-snytt I do recognize the need for gentler-Xsource:3, the one with superficial syntax support without semantic changes, but then the new one should be given the title of -Xsource:3-cross instead of changing the semantics of -Xsource:3.

Feature No flag -Xmigration:2.13.13 + -Xsource:3-cross -Xsource:3-cross / 3-compat -Xsource:3
import a.* Error
(x: Any) + "" Warning Warning Warning does not compile, implicit any2stringadd is not inferred
Unicode escapes in triple-quoted strings and raw interpolations ( """\u0061""") Warning, escape is processed escape is not processed
An implicit for type p.A is found in the package prefix p Warning the package prefix p is no longer part of the implicit search scope

Update: Updated -Xmigration:3 to -Xmigration:2.13.13 + -Xsource:3-cross.

som-snytt commented 9 months ago

@eed3si9n Yes. Until I finish my coffee, I won't remember whether "override inference" was already -Xsource:3; I guess it must have been when it was introduced two years ago. I'll attempt a nice grid at some point.

-Xmigration:2.12 means "show warnings for API that changed semantics since 2.12". For -Xsource:3-cross the value is arbitrary to say "show warnings with my behavior changes".

som-snytt commented 9 months ago

@lrytz actually, I don't wish to experiment. I'll try a simpler version of your overkill, which is to name the behaviors that can be opted-out under some separate flag. I would prefer to keep -Xsource:3-cross as the usual case.

(Then it doesn't matter if "override inference" was introduced under 3 or 3-cross, it can be opted-out just the same.)

As a reminder, -Xsource takes a version string, and 3-cross means 3.0.0 with build qualifier cross.

eed3si9n commented 9 months ago

-Xmigration:2.12 means "show warnings for API that changed semantics since 2.12".

Yea. One useful context is that circa -Xmigration:2.12 (or -Xmigration:2.7), when people said "Scala migration" it was understood that the destination of migration is the current Scala 2.x like 2.13, whereas in today, Scala 2.13.x is both the destination (from older 2.13.x) as well as the point of origin to emigrate to Scala 3.x, which on its own is a moving target.

So to either show warnings about newly introduced semantic changes etc, it seems logical to combine -Xmigration:2.13.13 with either -Xsource:3 or -Xsource:3-cross?

lrytz commented 9 months ago

compilers should be silent

I agree, once you opt in to a Scala 3 language semantic there are no more warnings about it.

Scala 2.13.13 shifts the meaning of -Xsource:3

That's right, we now draw the line between warnings (fatal by default) for migration and adopting language changes (benign new syntax is enabled in migration mode).

I still think that's the right distinction.

There are many options for which flags to use for implementing that distinction, but at this point I think we should aim at minimizing change in 2.13.14. So I'd leave -Xsource:3 as it is in 2.13.13, because that's what most peple will be using.

I'm not in favor overloading -Xmigration even more, in principle that flag has a defined functionality in conjunction with the @migration annotation.

The issue we need to solve only affects library authors that cross-build. This is a small group of experts, that's why I think it's fine (and valuable) to give them the "buffet" flag.

There's one adjustment we probably should do for 2.13.14 about any2stringadd: even under -Xsource:3, it's reported as a deprecation. All other migration warnings are cat=scala3-migration, which is fatal by default. We can turn the deprecation into a fatal migration warning.

Welcome to Scala 2.13.13 -Xsource:3.0.0

scala> def f(a: AnyRef) = a + ""
                          ^
       warning: method any2stringadd in object Predef is deprecated (since 2.13.0): Implicit injection of + is deprecated. Convert to String to call +
def f(a: AnyRef): String

scala> raw"\u0040"
           ^
       error: Unicode escapes in raw interpolations are ignored in Scala 3; use literal characters instead
       Scala 3 migration messages are errors under -Xsource:3. Use -Wconf / @nowarn to filter them or add -Xmigration to demote them to warnings.
       Applicable -Wconf / @nowarn filters for this fatal warning: msg=<part of the message>, cat=scala3-migration, site=res0
som-snytt commented 8 months ago

Eugene's original any2stringadd coping mechanism was noisy under -Vimplicit-conversions. That was 2018, or six years ago. Time really does fly when you're having fun.