Improvements to the Scala 3 crossbuild

[som-snytt]

Exercise in -Xsource:3-cross.

Not sure if it needs to stay on 3.3.3 LTS.

Scala 2 needs uninitialized. Scala 3 needs inline tap.

Scala 2 could support import chaining.given directly, though it doesn't matter for Scala 2 implicits. scala/bug#12999

[SethTisue]

Yes, we should stay on 3.3.x. That is the default policy for libraries.

[SethTisue]

(I can be a reviewer of last resort here if needed, but I'm hoping one or more collections volunteers might step in)

[OndrejSpanel]

If no one else will step in sooner, I will take a look tomorrow. At first sight it looks like a stuff I should be familiar with.

[som-snytt]

The change that is not syntactic is protected[this] to protected. I assume protected[this] is the most pernicious form of qualified access.

[OndrejSpanel]

The minimal PR porting to 2.13.14 while staying on 3.3.3 could just replace the library version in build.sbt (and perhaps update sbt version at the same time). This PR does a lot of work which makes the code future proof and prepared for 3.4.x / 3.5.x.

Much the PR are syntactic changes so that 3.4.x accepts the code without warning, which, while not necessary at this point, does no harm and it will have to be done sooner or later.

To me the most debatable seems the use of -Wnonunit-statement. It resulted in the warning being silenced in many places by using val _ = on result of updateWith or I did not find a single place where the warning would detect a real issue.

Than there is a special case of tap being used to silence the warning in takeUntilException. There is some setup done for Scala 2 to make sure tap is inlined. There is no such solution in place for Scala 3. I do not think using @inline has any effect on Scala 3, and I dot think tap will be inlined there (conf. https://contributors.scala-lang.org/t/scala-3-specific-stdlib-improvements/6661/5).

I suggest not to use -Wnonunit-statement, at it brings nothing useful, and not to rewrite the code using val _ and tap. At the minimum, do not use tap, as inlining it is complicated on Scala 2 and not possible on Scala 3, and silence the warning by some other way.

Regarding protected[this], removing it should have no effect. It does not make the functions accessible to outside world and we already know functions from scala.collection access those members only via this. Unlike private[this], protected[this] does not trigger any special features in Scala 2. The only concern could be binary compatibility, which I do not understand much, but I think MiMa check should handle this.

[som-snytt]

-Wnonunit-statement did its job, because cruft is generated and unused. The better solution is

elems.getOrElseUpdate(k, Set.empty[V]).addOne(v)

on the add side. Not sure about the subtract side, where you want the remove.

It indicates that updateWith is not great for internal operations.

Actually, it warns on the addOne because even though it's the this.type of the set, you don't know which set that is. It demands a stabilized value:

    val vs = elems.getOrElseUpdate(k, Set.empty[V])
    vs.addOne(v)

I don't see this as "unnecessary ceremony".

Use of tap is also a test of cross-compilation. Scala 3 ought already to intercept that API to make it inline, so this is a way to raise awareness.

[OndrejSpanel]

Scala 3 ought already to intercept that API to make it inline, so this is a way to raise awareness.

Is that documented somewhere? https://www.scala-lang.org/api/current/scala/util/ChainingOps.html does not say a word about this. When I discussed in https://contributors.scala-lang.org/t/scala-3-specific-stdlib-improvements/6661/5, nobody mentioned that there.

[OndrejSpanel]

Regarding -Wnonunit-statement, I guess it is your call. While some of the changes are not necessary, all changes look harmless to me.

[som-snytt]

On the warning, I think it's like saying, "Well, that unit test didn't uncover any issues, so just delete the test."

[som-snytt]

I forgot to retract the experiment with tap, though I think it's a worthwhile experiment.

For the updateWith, we're not really updating, we're getting and mutating. In the case where the mutated set or count is exhausted, we remove the key.

[SethTisue]

why closed?

[OndrejSpanel]

Reopening. To close this intentionaly I think it is at least worth a comment why (or a link to issue or PR supeseding this).

[SethTisue]

Looks like Som has lost interest; would a volunteer from @scala/collections like to finish it up in a replacement PR?

[OndrejSpanel]

I can prepare a PR as described in:

The minimal PR porting to 2.13.14 while staying on 3.3.3 could just replace the library version in build.sbt (and perhaps update sbt version at the same time)

That should be just a few SBT lines.

[OndrejSpanel]

o 2.13.14 while staying on 3.3.3 could just replace the library version in build.sbt (and perhaps update sbt version at the same time)

This is both already merged in master as PRs made by scala-steward (#245, #246)

Scala 3.3.3 is also already used since #234, also by scala-steward.

I can create a PR based on commits which avoid updateWith, as that makes sense to me and should make future maintenance easier.

One other change which might be beneficial is to use 2.3.14 stdlib even with Scala 3.3.3 (cf. https://users.scala-lang.org/t/what-scala-library-version-is-used-by-which-scala-3-version/9999/7)

@SethTisue @som-snytt any opinion on this?

[SethTisue]

One other change which might be beneficial is to use 2.3.14 stdlib even with Scala 3.3.3 (cf. https://users.scala-lang.org/t/what-scala-library-version-is-used-by-which-scala-3-version/9999/7)

@SethTisue @som-snytt any opinion on this?

Hmm... is there a specific reason to add the override? If not I'd just leave it alone and wait for whatever 3.3.4 gives us.

[OndrejSpanel]

Hmm... is there a specific reason to add the override?

Not really. Just a way to sneakily force the 2.13.14 fixes on unsuspecting users.

[SethTisue]

@OndrejSpanel the 2.13.14 bump has been merged for inclusion in Scala 3.3.4 (and the Steward should give us that upgrade here once it's available, so I think we can just wait)