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

Allow either -Xsource:3 (for preparing to switch to 3) or -Xsource:3-cross (for crossbuilding on 2 and 3) #10573

Merged
merged 2 commits into from Jan 30, 2024

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Oct 18, 2023

Documentation

A new section of the Scala 3 Migration Guide documents -Xsource:3 and -Xsource:3-cross: https://docs.scala-lang.org/scala3/guides/migration/tooling-scala2-xsource3.html

Description

-Xsource:3 remains intended for users preparing to upgrade a codebase from 2 to 3. It enables migration warnings but doesn't change semantics. By default, the warnings are configured to emit errors; to report them only as warnings, use something like the following: -Wconf:cat=scala3-migration:w.

The new -Xsource:3-cross is intended for users crossbuilding the same code for 2 and 3. It adopts Scala 3 semantics for certain constructs and skips warnings that aren't actionable in crossbuilt code. But it still warns for things that should be addressed because they are deprecated / unsupported in Scala 3.

Important: When upgrading to Scala 2.13.13 from an earlier version, consider switching from -Xsource:3 to -Xsource:3-cross, if you are crossbuilding between Scala 2 and 3, or if you were relying on particular behaviors of -Xsource:3. Some behaviors that were formerly enabled by -Xsource:3 are now only enabled by -Xsource:3-cross, for example around return type inference for inherited methods.

Documentation of what specifically is enabled by these flags is in the new -Xsource:help, as well as the new doc page.

Also fixes scala/bug#12886 by un-deprecating package object inheritance.

@scala-jenkins scala-jenkins added this to the 2.13.13 milestone Oct 18, 2023
@som-snytt som-snytt force-pushed the issue/12883-source-3-X branch 2 times, most recently from 6ef2b2e to 49942b7 Compare October 18, 2023 17:09
@som-snytt som-snytt marked this pull request as ready for review October 18, 2023 18:45
@som-snytt
Copy link
Contributor Author

@lrytz @SethTisue I'm not sure distinguishing "warning 3" from "behavior 3" is the right direction. That is because of Lukas's recent remark, and also the ticket about case class apply access. I saw an early comment from Seth about "the behavior change is only under -Xsource:3", so I think "behind a flag" meant "anything goes".

In the new regime, there is regular 2.13 which has some deprecations, -Xsource:3 which makes them cat=Scala3Migration (error by default) and also progresses more warnings, then -Xsource:3-X (or any version > 3.0.0) which adds behavior changes such as previously witnessed only in moody teenagers.

Changes are more-or-less listed under -Xsource:help (yay).

@lrytz
Copy link
Member

lrytz commented Oct 19, 2023

To me this looks good. Thank you for all the attention to detail, and for the help text! @SethTisue wdyt?

@SethTisue SethTisue added release-notes worth highlighting in next release notes prio:hi high priority (used only by core team, only near release time) labels Nov 1, 2023
@SethTisue SethTisue self-assigned this Nov 1, 2023
@som-snytt
Copy link
Contributor Author

I had forgotten that this proceeded. I see the point here was to split migration help over the two settings.

There's a different branch issue/12883-xsource-ergo to improve ergonomics but maybe that stalled when switching version setting to a Choice enumeration. That was September. trial/library-xsource3 was June.

@som-snytt
Copy link
Contributor Author

som-snytt commented Nov 8, 2023

I see that even if a deprecation is issued at 2.13, at 3 a (possibly more menacing) warning is issued as Scala3Migration, and 3-X may implement the new behavior.

The possible exception to this rule is "benign syntax", such as accepting import p.*, which is not benign if you're trying to import that identifier. So the lines to be drawn here might be fine (i.e. debatable).

Edit: ...and the warning issued at 3 is configured at error, so -Xmigration is the quickie way to detune to warning only. IIRC.

Somewhat optimistically, I'll try to audit that the 4 config are reflected in tests: 2.13., 3+migration, 3, 3-X. The code switches between 2, 3, 3-X.

It's not reasonable to warn about how parser constructs interpolations, unless it emits a warning that is only summarized: at the end, it could say you used string interpolation, which has new semantics etc; assuming it's too much work to check for shadowing StringContext; or refcheck that interpolations only use scala.StringContext, including extension methods.

@som-snytt som-snytt force-pushed the issue/12883-source-3-X branch 2 times, most recently from c00a987 to 32d36c4 Compare November 8, 2023 18:04
@lrytz lrytz force-pushed the issue/12883-source-3-X branch 3 times, most recently from bec5686 to 026cc00 Compare November 20, 2023 15:33
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

@som-snytt I rebased it (without changing your commits) and added one commit. I added 5 tests in the various categories (source3neg, source3Xneg, deprecationsForSource3, source3run, source3Xrun), it just made it easier for me to align things. They mostly duplicate existsing tests which I didn't remove.

For all cases where semantics change under source:3-X, I made sure there's a warning under source:3 and no warning under 3-X.

Let me know if that looks fine.

@som-snytt
Copy link
Contributor Author

som-snytt commented Nov 20, 2023

@lrytz thanks for not asking me to revisit this PR. Your clever use of attachments cleans up nicely.

Thanks for the improved mnemonic for -Xsource:3-X, "cross-building" instead of merely "extended".

I see you quashed a couple of messages under X, thanks. I would need to run under a debugger to see the change for any2stringadd. Does that defer the deprecation?

"cross building" is when you get mad at sbt.

@lrytz
Copy link
Member

lrytz commented Nov 20, 2023

... the change for any2stringadd. Does that defer the deprecation?

any2stringadd is annotated @deprecated, so you get an ordinary deprecation with source:2.13 and now also source:3. The compiler hack is now under source:3-X, it hides the symbol from implicit search. It's debatable whether that's actually useful, as it doesn't change any behavior (it only disables it). Cross-building projects anyway use the dotty compiler to figure out what doesn't work in Scala 3.

@SethTisue
Copy link
Member

maybe -Xsource:cross as the name?

@som-snytt
Copy link
Contributor Author

@SethTisue it's actually any version string that compares greater than "3.0.0", which includes "3-X" where the X is anything like FINAL or SNAPSHOT, etc. I liked that it was not a magic string.

(Entrance to warren: I think 3-SNAPSHOT compares wrong, but didn't pursue the cottontail.)

@SethTisue
Copy link
Member

SethTisue commented Nov 20, 2023

I liked that it was not a magic string

The advantage of a magic string is that it gives something a fixed name so we — and not just we, but the whole community — can refer to it conversation, documentation, PR review, blog posts, etc. I think it should have one name. (And I think we can find a better one than 3-X, which I find cryptic.)

@som-snytt
Copy link
Contributor Author

-Xsource:3-X starts and ends in -X, it's a mnemonic!

Since I never get what I love, you or Lukas at your behest may invent something, or invent a version string that always compares last, namely, future for -Xsource:future.

Now that I said it, I also love that idea. Then we can also say the tag line, "-Xsource: Back to the future!"

@som-snytt
Copy link
Contributor Author

I see from a glance at the source that none sorts higher than any version, so -Xsource:none should already work. The mnemonic is that there is no future in Scala 2.

@lrytz lrytz changed the title Support -Xsource:3-X [ci: last-only] Distinguish between -Xsource:3migrate and -Xsource:3cross Nov 21, 2023
@lrytz
Copy link
Member

lrytz commented Nov 21, 2023

@SethTisue this is ready for review

@som-snytt
Copy link
Contributor Author

som-snytt commented Jan 18, 2024

I guess you can click on it, too. The first weird error:

[info] Running 152 / 152 (100.00%) scripted tests with RunFromSourceMain
[info] Running apiinfo/circular-structure
[info] Running source-dependencies/malformed-class-name
[info] Running apiinfo/java-basic
[info] Running source-dependencies/malformed-class-name-with-dollar
[info] Running apiinfo/main-discovery
[info] Running source-dependencies/mixed-java-invalidations
[info] Running apiinfo/source-path
[info] [info] welcome to sbt 1.9.7 (Oracle Corporation Java 1.8.0_332)
[info] [info] loading project definition from /tmp/sbt_499533b0/project
[info] [info] compiling 1 Scala source to /tmp/sbt_499533b0/project/target/scala-2.12/sbt-1.0/classes ...
[info] [info] done compiling
[info] [info] set current project to sbt_499533b0 (in build file:/tmp/sbt_499533b0/)
[info] [success] Total time: 0 s, completed Jan 18, 2024 10:18:01 AM
[info] [info] welcome to sbt 1.9.7 (Oracle Corporation Java 1.8.0_332)
[info] [info] loading project definition from /tmp/sbt_499533b0/project
[info] [info] loading settings for project sbt_499533b0 from common.sbt ...
[info] [info] set current project to sbt_499533b0 (in build file:/tmp/sbt_499533b0/)
[info] [info] compiling 2 Scala sources and 1 Java source to /tmp/sbt_499533b0/target/classes ...
[info] [error] ## Exception when compiling 3 sources to /tmp/sbt_499533b0/target/classes
[info] [error] java.util.ServiceConfigurationError: xsbti.compile.CompilerInterface2: jar:file:/home/jenkins/.ivy2/local/org.scala-lang/scala2-sbt-bridge/2.13.13-bin-SNAPSHOT/jars/scala2-sbt-bridge.jar!/META-INF/services/xsbti.compile.CompilerInterface2:1: Illegal provider-class name: 6scala.tools.xsbt.CompilerBridge
[info] [error] java.util.ServiceLoader.fail(ServiceLoader.java:239)

edit: scrolling right

Illegal provider-class name: 6scala.tools.xsbt.CompilerBridge

@som-snytt
Copy link
Contributor Author

If this were merged at Thanksgiving, it wouldn't have weird build errors after MLK, Jr Day. (U.S. reference points.)

lrytz just re-rebases on top, but I am way too lazy slash process-averse to do that.

@SethTisue
Copy link
Member

SethTisue commented Jan 26, 2024

My main concern here is the naming. (Which I did ponder repeatedly during my long silence... sorry about that...)

I do want:

  • fixed names so everyone can consistently refer to it in documentation, chat rooms, etc

I don't want:

  • multiple ways of the saying the same thing (e.g. -Xsource:3 and -Xsource:3migrate)
  • magic behavior around sort order of option names
    • (that's not there anymore, is it?)
  • new naming formats if we can avoid them (I can't think of anything we already have resembling the mashed-together "3migrate" and "3cross")

I suggest we keep -Xsource:3 and let it continue to have its current meaning, and I think we should not add the -Xsource:3migrate alias.

So then this PR would simply add -Xsource:cross with the new behavior. I don't think the "3" in the name is necessary.

@som-snytt @lrytz wdyt?

@SethTisue
Copy link
Member

-Xsource:help is awesome, thanks for that.

@som-snytt
Copy link
Contributor Author

Any label is OK by me.

Dotty has -source:3.0-migration which is "more forgiving", so if anything I would suggest -Xsource:3-migration for warning and -Xsource:3 for behavior ("actually setting the source version, whatever that entails").

However, 3 and cross would be just as distinctive and effective.

"Gee that's a dumb name for an option" does not impede the dotty team. Nor should it us.

@SethTisue
Copy link
Member

SethTisue commented Jan 26, 2024

Let's see what Lukas thinks.

The vibe I'm going for here is "There's -Xsource:3, you already know what that does, it still does that. We also added a new thing you might want instead." Instead of a "we changed everything around" vibe.

@som-snytt
Copy link
Contributor Author

som-snytt commented Jan 26, 2024

We did change everything around. There used to be a mix of warnings and behavior changes until Lukas imposed Swiss tidiness.

The user complaint was "don't warn about the behavior change I asked for", so -Xsource:3 used to mean behavior. My fault for not adopting the dotty model, but I think I assumed that option would always maximize whatever was "backported" from dotty. That broke down when "downwards" implicit search was split out.

But whatever option names are chosen now is OK, people will figure it out.

@SethTisue
Copy link
Member

SethTisue commented Jan 26, 2024

We did change everything around

Okay, I see what you mean. Yes, I acknowledge that I'm idealizing the coherence of the prior state a bit :-)

@lrytz
Copy link
Member

lrytz commented Jan 29, 2024

The most common use case for -Xsource is likely migration to a new version, so -Xsource:3 and -Xsource:3cross sounds good. I'm in favor of keeping the 3, it adds useful information for build file readers - is 3-cross better?

It seems Scala 2 is anyway different from dotty

  • If youre on 3.x, then 3.x-migration dials back some hard errors (and enables -rewrite)
  • future enables deprecation warnings, future-migration is for -rewrite of those

I don't know if -Xsource:3.x[-migration] are intended to be used if you're on 3.(x-n) or on 3.(x+n).

@SethTisue SethTisue changed the title Distinguish between -Xsource:3migrate and -Xsource:3cross Distinguish between -Xsource:3 (for preparing to move to 3) and -Xsource:cross (for crossbuilding on 2 and 3) Jan 29, 2024
@SethTisue
Copy link
Member

I'd be okay with -Xsource:3-cross. They are using hyphens in the Scala 3 names at https://docs.scala-lang.org/scala3/reference/language-versions/source-compatibility.html

@SethTisue SethTisue changed the title Distinguish between -Xsource:3 (for preparing to move to 3) and -Xsource:cross (for crossbuilding on 2 and 3) Distinguish between -Xsource:3 (for preparing to move to 3) and -Xsource:3-cross (for crossbuilding on 2 and 3) Jan 29, 2024
@SethTisue
Copy link
Member

It seems Scala 2 is anyway different from dotty

(I just suggested at https://users.scala-lang.org/t/scala-3-porting-experiences/9753/5 that perhaps Scala 3 ought to also offer a flag specifically for crossbuilt projects, since 3.4 will now warn by default about tons of stuff that isn't actionable in crossbuilt projects.)

@SethTisue SethTisue changed the title Distinguish between -Xsource:3 (for preparing to move to 3) and -Xsource:3-cross (for crossbuilding on 2 and 3) Distinguish between -Xsource:3 (for preparing to switch to 3) and -Xsource:3-cross (for crossbuilding on 2 and 3) Jan 29, 2024
som-snytt and others added 2 commits January 30, 2024 09:33
Add documentation to `-Xsource:help`.

-Xsource:3migrate is an alias for `-Xsource:3`, it enables fatal
warnings but doesn't change semantics.

-Xsource:3cross adpots Scala 3 semantics for certain constructs and
skips the warning, for example modifiers of case class apply / copy
methods. Warnings for things that should be addressed because they are
deprecated / unsupported in Scala 3 remain.
@lrytz lrytz merged commit 8e03f3b into scala:2.13.x Jan 30, 2024
4 checks passed
@SethTisue
Copy link
Member

🎉

@som-snytt som-snytt deleted the issue/12883-source-3-X branch January 30, 2024 14:57
@som-snytt
Copy link
Contributor Author

Anyone else notice that the original -Xsource:3-X is the same as -Xsource:3-cross?

@SethTisue
Copy link
Member

@som-snytt @lrytz I just did an edit pass over the PR description — y'all might want to take another look as well.

@SethTisue SethTisue changed the title Distinguish between -Xsource:3 (for preparing to switch to 3) and -Xsource:3-cross (for crossbuilding on 2 and 3) Allow either -Xsource:3 (for preparing to switch to 3) and -Xsource:3-cross (for crossbuilding on 2 and 3) Feb 10, 2024
@SethTisue SethTisue changed the title Allow either -Xsource:3 (for preparing to switch to 3) and -Xsource:3-cross (for crossbuilding on 2 and 3) Allow either -Xsource:3 (for preparing to switch to 3) or -Xsource:3-cross (for crossbuilding on 2 and 3) Feb 10, 2024
@som-snytt
Copy link
Contributor Author

som-snytt commented Feb 26, 2024

I see that -Xsource:3-migration doesn't mean anything. Only -Xsource:3-cross is a specially labeled version. (In the normal universe, 3-foo sorts higher than 3.0.0, so that was the reason or mnemonic for it to mean "drop migration semantics, we're way beyond that".)

The help text mentions scala3-migration is the "warning category" for -Xsource:3, but it's not in the comment here; also how to detune it so you can limp along with mere warnings.

Edit: added a few words above. Now, I would like to go grocery shopping, please, so that I can squeeze in some exercise after.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prio:hi high priority (used only by core team, only near release time) release-notes worth highlighting in next release notes
Projects
None yet
4 participants