Skip to content
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

Introduce -Xsource-features, for customizing the behavior of -Xsource:3 and -Xsource:3-cross #10709

Merged
merged 5 commits into from
Mar 14, 2024

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Mar 9, 2024

This feature is only recommended for projects that cross-build between Scala 2.13 and Scala 3. For migrating to Scala 3, use plain -Xsource:3. See also https://docs.scala-lang.org/scala3/guides/migration/tooling-scala2-xsource3.html.

This PR makes Scala 3 language features opt-in under an option "buffet" or "à la carte". To view the menu:

$> scalac -Xsource-features:help
Enable Scala 3 features under -Xsource:3.

Instead of `-Xsource-features:_`, it is recommended to enable specific features, for
example `-Xsource-features:v2.13.14,-case-companion-function` (-x to exclude x).
This way, new semantic changes in future Scala versions are not silently adopted;
new features can be enabled after auditing the corresponding migration warnings.

`-Xsource:3-cross` is a shorthand for `-Xsource:3 -Xsource-features:_`.

Features marked with [bin] affect the binary encoding. Enabling them in a project
with existing releases for Scala 2.13 can break binary compatibility.

Available features:

  case-apply-copy-access    Constructor modifiers are used for apply / copy methods of case classes. [bin]
  case-companion-function   Synthetic case companion objects no longer extend FunctionN. [bin]
  infer-override            Inferred type of member uses type of overridden member. [bin]
  any2stringadd             Implicit `any2stringadd` is never inferred.
  unicode-escapes-raw       Don't process unicode escapes in triple quoted strings and raw interpolations.
  string-context-scope      String interpolations always desugar to scala.StringContext.
  leading-infix             Leading infix operators continue the previous line.
  package-prefix-implicits  The package prefix p is no longer part of the implicit search scope for type p.A.
  implicit-resolution       Use Scala-3-style downwards comparisons for implicit search and overloading resolution (see github.com/scala/scala/pull/6037).
  v2.13.13                  case-apply-copy-access,case-companion-function,infer-override,any2stringadd,unicode-escapes-raw,string-context-scope,leading-infix,package-prefix-implicits.
  v2.13.14                  v2.13.13 plus implicit-resolution

-Xsource:3 -Xsource-features:_ (or -Xsource:3-cross for convenience to start a REPL) offers all "courses" as "prix fixe". It's not recommended in a build script, use v2.13.14 instead to get migration warnings as new features are adopted.

-Xmigration is no longer "overloaded" to mean -Wconf:cat=scala3-migration:w. That incantation must be written in full.

Fixes scala/bug#12961

@scala-jenkins scala-jenkins added this to the 2.13.14 milestone Mar 9, 2024
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Mar 9, 2024
@som-snytt som-snytt force-pushed the issue/12961-source-compat branch 2 times, most recently from 3d44731 to c7a5c05 Compare March 10, 2024 19:29
@som-snytt som-snytt marked this pull request as ready for review March 10, 2024 19:30
@som-snytt som-snytt changed the title WIP: Introduce -Xsource:3-compat and buffet options [ci: last-only] Introduce -Xsource:3-compat and buffet options [ci: last-only] Mar 10, 2024
@som-snytt som-snytt requested a review from lrytz March 10, 2024 19:30
@som-snytt
Copy link
Contributor Author

To be squashed after review. Nominally (whatever that weaselly word means), I checked usages of isScala3... for reasonableness, but it's surprisingly difficult to verify. A clearer API would be more like hardMigration: either migrationWarning if isScala3, or discriminating progressiveWarning(at2 = old_behavior, at3 = warn, at3X = cross) where cross warns if verbose.

The -Ysource-options:case-access warning for apply is an unduly heavyweight mechanism for a niche feature or warning.

@SethTisue
Copy link
Member

associated doc change PR scala/docs.scala-lang#2994

@lrytz
Copy link
Member

lrytz commented Mar 12, 2024

Thank you for getting it rolling, I was going to pick it up this week.

I'm forming somewhat strong opinions about providing multiple ways to do the same thing. It takes time for people to learn / look up what all those variants mean.

  • -Xsource:3 is what it is in 2.13.13, that's good
  • -Ysource-options could work with -Xsource:3, we don't need -Xsource:3-compat for that
  • -Xsource:3-cross is the same as -Ysource-options:_
  • -Xsource:3-migration is the same as -Xsource:3 -Wconf:cat=scala3-migration:w
  • I agree to stop the -Xmigration overload of course

Compiler flags are not code, they are only touched once in a while and don't need to be as succinct as possible.

The current proposal also doesn't really solve the auditing problem. If I'm using 3-compat and upgrade to a new Scala that adds some semantic change, how do I find out? I can go back to 3 but then the new warnings are mixed up with all the older warnings which I already addressed. So instead of 3-compat, I think it's better to add all semantic changes to -Ysource-options, not just the ones affecting binary encoding.

I'll give it a go today and push to this PR.

@som-snytt
Copy link
Contributor Author

Happy to have you progress this experiment to your liking.

Here, you find out "what's new" by -Ysource-options:verbose (if you triggered it) or help (IIRC there was help text).

I think just having -Xnew-stuff:case-access etc is fine, in the xlint mode; I just wanted to see if we could avoid it, because it results in "fine-tuning". The experiment here was 3-compat allows for opt-in to dangerous options. So there was the conservative flag, the all-in flag, and the goldilocks flag.

Here, -Xsource:3-migration is just an alias for -Xsource:3, for clarity.

@som-snytt som-snytt changed the title Introduce -Xsource:3-compat and buffet options [ci: last-only] Introduce [your options here] [ci: last-only] Mar 12, 2024
@@ -1128,7 +1128,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
def adaptApplyInsertion(): Tree = doAdaptApplyInsertion(retry = false)

def doAdaptApplyInsertion(retry: Boolean): Tree =
if (currentRun.isScala3Cross && !isPastTyper && tree.symbol != null && tree.symbol.isModule && tree.symbol.companion.isCase && isFunctionType(pt))
if (currentRun.isScala3X && !isPastTyper && tree.symbol != null && tree.symbol.isModule && tree.symbol.companion.isCase && isFunctionType(pt))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize that apply insertion should be enabled by default, not even under -Xsource:3.

For a library built with -Xsoruce:3-cross, case class companions don't extend FunctionN. This should not break List(1).map(A) for users of the library that use the default compiler options.

@som-snytt wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The library would use 3-compat (or equivalent) to avoid incompatible changes.

The librarian, for the same reason, would not add object A companion that has the same effect; or would be sorry if they did.

But you may be right that this should be considered a "benign" feature change that everyone could benefit from. Fork all the language features! It is of general utility. The downside is your 2.12 build breaks; but who does that? I guess only middle-size fish who consume smaller fish and are consumed in turn.

@lrytz
Copy link
Member

lrytz commented Mar 12, 2024

We might rename -Ysource-options

Also I could probably live with an alias Xsource:3-cross for -Ysource-options:_, it would be handy for testing stuff (not just for us, also for actual users of Ysource-options when they want to fire up a REPL).

@lrytz lrytz force-pushed the issue/12961-source-compat branch 2 times, most recently from 111b007 to 4d5fed9 Compare March 13, 2024 15:56
@lrytz
Copy link
Member

lrytz commented Mar 13, 2024

I squashed almost everything into one commit, only a few semantic changes as separate commits as good as possible.

|`-Xsource:3-cross` is a shorthand for `-Xsource:3 -Xsource-features:_`.
|
|Available features:
|""".stripMargin)
Copy link
Contributor Author

@som-snytt som-snytt Mar 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra strip (it would be interesting if that failed to compile)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 IntelliJ adds it when typing sm"""<return>...

@@ -26,6 +27,7 @@ object Test extends DirectTest {
"""
def show() = {
assert(!compile())
//assertEquals(expected.linesIterator.toList, Files.readAllLines(argfile).asScala.toList)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debug cruft. The expected needs .trim and assert could be

    import scala.tools.testkit.AssertUtil.assertSameElements
    assertSameElements(expected.linesIterator, Files.readAllLines(argfile).asScala)

The test is because -Vprint-args was broken; the -Xsource is not relevant.

I'll submit the tweak separately. :)

@som-snytt
Copy link
Contributor Author

The buffet option makes it easier to see in the source where the features take effect.

It also simplifies the warnings under the rule of "no warning if they asked for a feature". (I just remembered that the point of verbose mode was to enable "audit", so you could see "this override was inferred differently", for example. Reverting to migration might show the future warning, but it might also just show an error?) (Probably no one would ever use verbose audit anyway.)

It took me two tries to correctly test moving the "case function" check from ident to adapt. Very nice!

@som-snytt som-snytt changed the title Introduce [your options here] [ci: last-only] Introduce -Xsource-features [ci: last-only] Mar 13, 2024
@lrytz lrytz force-pushed the issue/12961-source-compat branch from 4d5fed9 to bf8b650 Compare March 13, 2024 18:41
@som-snytt
Copy link
Contributor Author

som-snytt commented Mar 13, 2024

LGTM. Lukas pulled that old github mind trick where he gets me to open the PR so that only he can approve it later. :)

I partially edited the OP comment, which is probably still inaccurate.

@lrytz lrytz force-pushed the issue/12961-source-compat branch from bf8b650 to 33559dd Compare March 13, 2024 19:52
@lrytz lrytz force-pushed the issue/12961-source-compat branch 2 times, most recently from fad7e0b to 9c96b42 Compare March 13, 2024 21:07
@som-snytt
Copy link
Contributor Author

Your old trick of emitting the "how to suppress it" message would be helpful, especially when the category differs if not -Xsource:3.

https://discord.com/channels/632150470000902164/632150470000902166/1217725189018615848

@som-snytt
Copy link
Contributor Author

travis claims

-t12919.scala:48: error: Implicit method myClassToSeq was found in a package prefix of the required type, which is not part of the implicit scope in Scala 3.

+t12919.scala:48: error: Implicit method myClassToSeq was found in a package prefix of the required type, which is not part of the implicit scope in Scala 3 (or with -Xsource-features:package-prefix-implicits).

lrytz and others added 5 commits March 14, 2024 08:50
Co-authored-by: A. P. Marki <som.snytt@gmail.com>
A dependency might be built with
-Xsource-features:case-companion-function.
Apply insertion should work for users that don't enable that flag.

Co-authored-by: A. P. Marki <som.snytt@gmail.com>
Move the warning to adapt. This makes it work also for selections,
not just idents.

Co-authored-by: A. P. Marki <som.snytt@gmail.com>
Workaround for scala/bug 12968.

Co-authored-by: A. P. Marki <som.snytt@gmail.com>
This allows enabling Scala 3 semantics for cross-building
individually.

Co-authored-by: A. P. Marki <som.snytt@gmail.com>
@lrytz lrytz force-pushed the issue/12961-source-compat branch from 9c96b42 to 68bab66 Compare March 14, 2024 07:51
@lrytz
Copy link
Member

lrytz commented Mar 14, 2024

semantic merge conflict with #10706, rebased.

Your old trick of emitting the "how to suppress it" message would be helpful

Maybe somebody will eventually figure out how to do interactive verbose (not "re-run with ...").

➜ sandbox s -Wconf:any:wv
> scala repl -S 2.13 -cp . -Wconf:any:wv
Welcome to Scala 2.13.13 (OpenJDK 64-Bit Server VM, Java 21.0.1).
Type in expressions for evaluation. Or try :help.

scala> implicit val x = 41
                    ^
       warning: Implicit definition should have explicit type (inferred Int) [quickfixable]
       Applicable -Wconf / @nowarn filters for this warning: msg=<part of the message>, cat=other-implicit-type, site=x
val x: Int = 41

@lrytz lrytz merged commit 6b1b9d7 into scala:2.13.x Mar 14, 2024
3 checks passed
@SethTisue SethTisue changed the title Introduce -Xsource-features [ci: last-only] Introduce -Xsource-features for customizing the behavior of -Xsource:3 and -Xsource:3-cross Mar 14, 2024
@SethTisue SethTisue changed the title Introduce -Xsource-features for customizing the behavior of -Xsource:3 and -Xsource:3-cross Introduce -Xsource-features, for customizing the behavior of -Xsource:3 and -Xsource:3-cross Mar 14, 2024
@som-snytt som-snytt deleted the issue/12961-source-compat branch March 14, 2024 15:34
@SethTisue
Copy link
Member

I'm noticing in the community build that despite this change:

-Xmigration is no longer "overloaded" to mean -Wconf:cat=scala3-migration:w. That incantation must be written in full.

I'm still seeing errors like:

[playframework] [error] Scala 3 migration messages are errors under -Xsource:3. Use -Wconf / @nowarn to filter them or add -Xmigration to demote them to warnings.

seems like an oversight that this advice wasn't updated?

@som-snytt
Copy link
Contributor Author

som-snytt commented Mar 16, 2024

I never read those messages all the way to the end.

Edit: until now. Tweak at #10718

@SethTisue
Copy link
Member

a bit of good news: over in the community build, I removed -Xmigration from about a two dozen repos and only had to replace it with -Wconf:cat=scala3-migration:w in five:

proj/blaze.conf:    "appendScalacOptions -Wconf:cat=scala3-migration:w"
proj/json4s.conf:    "appendScalacOptions -Wconf:cat=scala3-migration:w"
proj/mima.conf:    "appendScalacOptions -Wconf:cat=scala3-migration:w"
proj/playframework.conf:    "appendScalacOptions -Wconf:cat=scala3-migration:w"
proj/scalafx.conf:    "appendScalacOptions -Wconf:cat=scala3-migration:w"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes
Projects
None yet
4 participants