Skip to content

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

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

Closed
vladimirkl opened this issue Sep 26, 2023 · 10 comments · Fixed by scala/scala#10562
Closed
Assignees
Milestone

Comments

@vladimirkl
Copy link

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
Copy link

@lrytz
Copy link
Member

lrytz commented Sep 26, 2023

I have to suppress it with @nowarn

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

@vladimirkl
Copy link
Author

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
Copy link
Member

lrytz commented Sep 26, 2023

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
Copy link
Author

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
Copy link
Member

lrytz commented Sep 26, 2023

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. scala/scala-dev#470 (comment).

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
Copy link

guizmaii commented Sep 26, 2023

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
Copy link

som-snytt commented Sep 26, 2023

This is also on my radar since seeing it in std lib scala/scala#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
Copy link

som-snytt commented Sep 27, 2023

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
Copy link

Sample improved message

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants