Make Scala-3-style implicit resolution explicitly opt-in rather than bundled in `-Xsource:3` #10012

povder · 2022-04-21T22:41:40Z

Motivation

Using -Xsource:3 to enable some Scala 3 syntax and behavior breaks implicit resolution in certain cases, as described in bug report scala/bug#12437. Most notably it makes Scala.js unions not work. This PR disables Scala-3-style implicit resolution when -Xsource:3 is used (because it is currently broken) while making it possible to opt in to use it with a newly introduced flag -Yscala3-implicit-resolution.

Usage

If you use -Xsource:3 and are impacted by scala/bug#12437 then do not add -Yscala3-implicit-resolution to the Scala compiler options. It will disable Scala-3-style implicit resolution while still preserving other Scala 3 behavior in your Scala 2 code.

If you use -Xsource:3 currently but are not impacted by scala/bug#12437, add -Yscala3-implicit-resolution to the Scala compiler options to preserve the Scala 2.13.8 behavior of -Xsource:3 flag. Note that even if you are not currently impacted by the bug you may be in the future so the recommendation is not to use -Yscala3-implicit-resolution unless your code doesn't compile without it (because Scala-3-style implicit resolution is partially broken).

If you don't use -Xsource:3 currently you probably don't want to enable Scala-3-style implicit resolution without using -Xsource:3 so there's no action required for you.

smarter · 2022-04-21T23:04:20Z

The flag is -Yscala3-implicit-resolution and is false by default, but gets set to true by -Xsource:3.

I would be in favor of having it be false under -Xsource:3 by default too, better to not enable something we know is broken.

SethTisue · 2022-04-21T23:24:23Z

I would be in favor of having it be false under -Xsource:3 by default too, better to not enable something we know is broken.

I agree.

A little history on this: at the time these changes were added to -Xsource:3, all the way back in 2017, -Xsource:3 was rather new (it might even have been called something else at the time? I don't remember) and it's future wasn't clear to us yet. At the time, we were thinking of it more as a way for Scala contributors and language mavens to experimentally enable some behaviors on a preview basis.

Over time, this has shifted and -Xsource:3 is now something we want to be usable in real projects that want to cross-build between 2 and 3. It's less experimental/exploratory in flavor and has become more of a real aid to for actual migration.

(And besides, it took time for scala/bug#12437 to come to light; but now we know.)

I'd like to know what @lrytz thinks, though.

povder · 2022-04-22T01:56:54Z

Thx for taking a look. I made a change to not enable -Yscala3-implicit-resolution with -Xsource:3. My rationale behind enabling it together with -Xsource:3 was to keep the current behaviour for -Xsource:3 builds, even if it's partially broken.

I'll squash the commits when the final decision about that is made.

lrytz · 2022-04-22T07:41:22Z

SerializationStabilityTest

Maybe that test depends on the Java version? Not sure...

I'd like to know what @lrytz thinks, though.

SGTM 👍

It's probably worth keeping the ticket open, or creating a new one, as the issue is not fixed.

povder · 2022-04-22T07:52:04Z

SGTM +1

It's probably worth keeping the ticket open, or creating a new one, as the issue is not fixed.

I'll remove "Fixes" from the commit message so that the issue doesn't get auto-closed.

povder · 2022-04-25T21:05:37Z

@lrytz @SethTisue @smarter good to merge? :]

smarter · 2022-04-25T21:30:26Z

src/compiler/scala/tools/nsc/settings/ScalaSettings.scala

@@ -283,6 +283,7 @@ trait ScalaSettings extends StandardScalaSettings with Warnings { _: MutableSett
  val YpickleWrite = StringSetting("-Ypickle-write", "directory|jar", "destination for generated .sig files containing type signatures.", "", None).internalOnly()
  val YpickleWriteApiOnly = BooleanSetting("-Ypickle-write-api-only", "Exclude private members (other than those material to subclass compilation, such as private trait vals) from generated .sig files containing type signatures.").internalOnly()
  val YtrackDependencies = BooleanSetting("-Ytrack-dependencies", "Record references to in unit.depends. Deprecated feature that supports SBT 0.13 with incOptions.withNameHashing(false) only.", default = true)
+  val Yscala3ImplicitResolution = BooleanSetting("-Yscala3-implicit-resolution", "Use Scala-3-style downwards comparisons for implicit search and overloading resolution", default = false)


Maybe add a link to scala/bug#12437 in the description

Will do 👍

I've made a change like this: 8bb6e43#diff-87059578fba24c0fb0b0fcca877242dff060ac85c89dbf2dfd04548d477f8e6bR278

smarter

2.13.9 is close so I hope we can get this merged before the release, I just had one comment (https://github.com/scala/scala/pull/10012/files#r858043339) but it shouldn't block us from merging this.

SethTisue · 2022-06-29T16:37:36Z

@povder Could you attempt to revise the PR description to be what we call "user-facing", that is, to be maximally digestible even by an end user who has little/no understanding of the history here? It's most important to be clear about 1) the motivation for the change, 2) what users should do (and which users need to do anything at all).

SethTisue · 2022-06-30T16:10:25Z

@milessabin would you like to review this?

povder · 2022-07-01T18:45:55Z

@povder Could you attempt to revise the PR description to be what we call "user-facing", that is, to be maximally digestible even by an end user who has little/no understanding of the history here? It's most important to be clear about 1) the motivation for the change, 2) what users should do (and which users need to do anything at all).

Will do today 👍

povder · 2022-07-01T20:48:55Z

@SethTisue I've changed the description of this PR in an attempt to address your comment.

SethTisue · 2022-07-02T01:00:32Z

Perfect!

som-snytt · 2022-07-20T04:26:39Z

src/compiler/scala/tools/nsc/Global.scala

@@ -1168,6 +1168,7 @@ class Global(var currentSettings: Settings, reporter0: Reporter)

    // We hit these checks regularly. They shouldn't change inside the same run, so cache the comparisons here.
    val isScala3 = settings.isScala3
+    val isScala3ImplicitResolution = settings.Yscala3ImplicitResolution


This is a BooleanSetting, not a Boolean.

som-snytt · 2022-07-20T04:28:10Z

src/compiler/scala/tools/nsc/typechecker/Infer.scala

-          case _ if !currentRun.isScala3 => existentialAbstraction(undef1, tpe1) <:< existentialAbstraction(undef2, tpe2)
-          case _                         =>
+          case PolyType(tparams2, rtpe2)                   => isAsSpecificValueType(tpe1, rtpe2, undef1, undef2 ::: tparams2)
+          case _ if !currentRun.isScala3ImplicitResolution => existentialAbstraction(undef1, tpe1) <:< existentialAbstraction(undef2, tpe2)


This incurs the implicit conversion. I wonder why I don't see a warning currently. Edit: I catch this in that PR.

I saw your comment and I was going to quickly make a change to avoid the conversion but upon checking out the most recent code I realized the conversion doesn't happen anymore with your change 0d252f6#diff-7b7600384a160cb93e5eceff814f6aed9f5c7b72b752a18c3f61d895482e75fcR907

Thanks for checking my check. (That's what I meant by I caught it in my PR, which was about the same time as yours.) I might change them to vars, except there were a lot of isScala3 calls, so removing the .value is not appealing. But that is why sed is the "scala editor".

scala-jenkins added this to the 2.13.9 milestone Apr 21, 2022

SethTisue added the release-notes worth highlighting in next release notes label Apr 21, 2022

povder force-pushed the bug-12437 branch 2 times, most recently from 24c91fa to 3570c0f Compare April 22, 2022 06:46

povder changed the title ~~Fix scala/bug#12437: introduce a flag to disable Scala-3-style implicit resolution~~ Introduce a flag to disable Scala-3-style implicit resolution Apr 22, 2022

povder changed the title ~~Introduce a flag to disable Scala-3-style implicit resolution~~ Introduce a flag to enable Scala-3-style implicit resolution Apr 22, 2022

povder force-pushed the bug-12437 branch from 3570c0f to 42da707 Compare April 22, 2022 07:56

smarter reviewed Apr 25, 2022

View reviewed changes

smarter approved these changes Jun 29, 2022

View reviewed changes

SethTisue changed the title ~~Introduce a flag to enable Scala-3-style implicit resolution~~ Make Scala-3-style implicit resolution explicitly opt-in rather than bundled in -Xsource:3 Jun 30, 2022

Introduce a flag to enable Scala-3-style implicit resolution

8bb6e43

povder force-pushed the bug-12437 branch from 42da707 to 8bb6e43 Compare July 1, 2022 20:18

lrytz approved these changes Jul 7, 2022

View reviewed changes

lrytz merged commit d5cfa6f into scala:2.13.x Jul 7, 2022

povder deleted the bug-12437 branch July 7, 2022 19:54

som-snytt reviewed Jul 20, 2022

View reviewed changes

hmemcpy mentioned this pull request Dec 20, 2022

Prefer type of overridden member when inferring (under -Xsource:3) #9891

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Scala-3-style implicit resolution explicitly opt-in rather than bundled in `-Xsource:3` #10012

Make Scala-3-style implicit resolution explicitly opt-in rather than bundled in `-Xsource:3` #10012

povder commented Apr 21, 2022 •

edited

smarter commented Apr 21, 2022

SethTisue commented Apr 21, 2022 •

edited

povder commented Apr 22, 2022

lrytz commented Apr 22, 2022

povder commented Apr 22, 2022

povder commented Apr 25, 2022

smarter Apr 25, 2022

povder Jul 1, 2022

povder Jul 1, 2022 •

edited

smarter left a comment

SethTisue commented Jun 29, 2022

SethTisue commented Jun 30, 2022

povder commented Jul 1, 2022

povder commented Jul 1, 2022

SethTisue commented Jul 2, 2022

som-snytt Jul 20, 2022

som-snytt Jul 20, 2022 •

edited

povder Jul 20, 2022 •

edited

som-snytt Jul 20, 2022

Make Scala-3-style implicit resolution explicitly opt-in rather than bundled in -Xsource:3 #10012

Make Scala-3-style implicit resolution explicitly opt-in rather than bundled in -Xsource:3 #10012

Conversation

povder commented Apr 21, 2022 • edited

Motivation

Usage

smarter commented Apr 21, 2022

SethTisue commented Apr 21, 2022 • edited

povder commented Apr 22, 2022

lrytz commented Apr 22, 2022

povder commented Apr 22, 2022

povder commented Apr 25, 2022

smarter Apr 25, 2022

Choose a reason for hiding this comment

povder Jul 1, 2022

Choose a reason for hiding this comment

povder Jul 1, 2022 • edited

Choose a reason for hiding this comment

smarter left a comment

Choose a reason for hiding this comment

SethTisue commented Jun 29, 2022

SethTisue commented Jun 30, 2022

povder commented Jul 1, 2022

povder commented Jul 1, 2022

SethTisue commented Jul 2, 2022

som-snytt Jul 20, 2022

Choose a reason for hiding this comment

som-snytt Jul 20, 2022 • edited

Choose a reason for hiding this comment

povder Jul 20, 2022 • edited

Choose a reason for hiding this comment

som-snytt Jul 20, 2022

Choose a reason for hiding this comment

Make Scala-3-style implicit resolution explicitly opt-in rather than bundled in `-Xsource:3` #10012

Make Scala-3-style implicit resolution explicitly opt-in rather than bundled in `-Xsource:3` #10012

povder commented Apr 21, 2022 •

edited

SethTisue commented Apr 21, 2022 •

edited

povder Jul 1, 2022 •

edited

som-snytt Jul 20, 2022 •

edited

povder Jul 20, 2022 •

edited