scala / bug

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

Scala 2.13.12 fatal warning with -Xsource:3 and private constructor for case class #12883

Closed vladimirkl closed 7 months ago

vladimirkl commented 9 months ago

Reproduction steps

Scala version: 2.13.12

scala> case class Data private (s: String)
                  ^
       error: constructor modifiers are assumed by synthetic `copy` method
       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=Data

Problem

Same warning is for synthetic apply method. This cannot be fixed without redefining copy and apply explicitly and doesn't help with any Scala 3 migration issue. In all cases I have to suppress it with @nowarn

guizmaii commented 9 months ago

FYI, also reported here: https://discord.com/channels/632150470000902164/632150470000902166/1150748646296518707

lrytz commented 9 months ago

I have to suppress it with @nowarn

You can also use a compiler flag -Wconf:msg=constructor modifiers are assumed:s

vladimirkl commented 9 months ago

I have to suppress it with @nowarn

You can also use a compiler flag -Wconf:msg=constructor modifiers are assumed:s

Sure, this is better than polluting code with @nowarn. But anyway warning looks like a compiler bug.

lrytz commented 9 months ago

But anyway warning looks like a compiler bug.

I don't think it's a bug; the main use case of -Xsource:3 is to help migration to Scala 3, the warning exists because the code is handled differently between Scala 2 and 3.

But I understand that many projects keep -Xsource:3 enabled for a long time, for example libraries that are cross-published; -Xsoruce:3 helps here for example because of the syntax backports. For the warning in question there's nothing one can do, so it needs to be silenced. Maybe that's too inconvenient?

vladimirkl commented 9 months ago

Looks like warning is confusing because it is not clear how this code is handled differently between Scala 2 and 3. Is it possible to add some documentation?

lrytz commented 9 months ago

You're right, "constructor modifiers are assumed by synthetic copy method" is really hard to understand. The difference is this:

scala> case class A private (x: Int); object A { def f = A(1) }
class A
object A

scala> A.f.copy(x = 2)
val res0: A = A(2) // allowed in Scala 2, disallowed in Scala 3 because `copy` is private

I also notice that Scala 2 changes semantics for this example with -Xsource:3 (it makes the copy method private), I'm not sure it should do that; I thought the idea in general was to warn about semantic changes in Scala 3, but not replicate them.

Documenting -Xsource:3 is indeed something we need to do. https://github.com/scala/scala-dev/issues/470#issuecomment-1715740755.

Maybe we can also find ways have better defaults. For example focus -Xsource:3on migration (as it is now) and provide -Xsource:3cross for cross-building.

guizmaii commented 9 months ago

I also notice that Scala 2 changes semantics for this example with -Xsource:3 (it makes the copy method private), I'm not sure it should do that; I thought the idea in general was to warn about semantic changes in Scala 3, but not replicate them.

That's one of the main reasons I enable -Xsource:3 in all my projects. I want this behaviour in my Scala2 code. Not sure we need to emit a warning if the Scala3 behaviour is already applied in Scala2 code using -Xsource:3. The code will not compile anyway.

som-snytt commented 9 months ago

This is also on my radar since seeing it in std lib https://github.com/scala/scala/pull/10551

-Xsource:3 was always intended to minimize migration, and not just warn: for example, the change in implicit lookup (-Yscala3-implicit-resolution) was retracted only because it was buggy, but it was a behavior change that let you edit code now and minimize change later. It's true that -Xsource:3 is mostly syntax and warnings.

The synthetic apply warning is significant for libraries that must plan around compatibility. I agree there is no great solution, which attests to the severity of the breaking change between the compilers.

I'll try to propose something; maybe another knob.

I noticed the warning about apply is incorrect when user has supplied one, because of an implementation detail. That is a separate bug.

Edit: the change in inference of type of overriding method is an obvious example of behavior change that I think is a win, but also requires a code change if you prefer the old inferred type; and is annoying because it requires -Wconf to silence the warning as an acknowledgment, "Yes, I saw the warning and I like the change." The alternative would be -quickfix to apply the source change.

Oh, maybe -quickfix should be able to emit an edit that either retains old behavior or encodes the new.

som-snytt commented 9 months ago

I will try Lukas's idea of the regular option to warn and the x-tended option to apply new behaviors. Maybe it will make it into 2.13.13 on Friday the 13th. I'll try to document the situation, but I will likely underdeliver, as I am no sjrd. I'll sample the canine cuisine on the stdlib PR.

Edit: current help for -Xsource, because it took over for -future:

"Enable features that will be available in a future version of Scala, for purposes of early migration and alpha testing."

Obviously, the accommodation is to only warn under ~-Xsource:2.14~. I misspoke, I ought to have said -Xsource:2.13, which seems to be a legal setting.

som-snytt commented 9 months ago

Sample improved message

Deadline.scala:32:12: access modifiers for `apply` method are copied from the case class constructor