Skip to content

Conversation

@som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Jan 2, 2024

Scala 2 sometimes makes the companion object of a case class extend Function; Scala 3 does not, but includes other adaptations which are backported under -Xsource:3-cross and allow the following idioms with reduced ceremony:

case class B(i: Int)
case class C(i: Int, j: Int)
def mapped(xs: List[Int]): List[B] = xs.map(B)
def cross(xs: List[(Int, Int)]): List[C] = xs.map(C)
def f(xs: List[(Int, Int)]): List[C] = xs.map(C.tupled)
def g(xs: List[Int]): List[C] = xs.map(C.curried).map(_(42))

Under -Xsource:3, using the case class companion as a function will warn.

Backports Dotty migration support of case class companions by inserting apply. Dotty does not automatically add Function as a parent of the companion; for Scala 2 under -Xsource:3-cross, follow Dotty but also adapt where the companion is not already a Function.

Since Scala 2 lacks parameter untupling, also support explicit C.tupled and for completeness C.curried.

Fixes scala/bug#3664

@scala-jenkins scala-jenkins added this to the 2.13.14 milestone Jan 2, 2024
@som-snytt som-snytt modified the milestones: 2.13.14, 2.13.13 Jan 2, 2024
@joroKr21
Copy link
Member

joroKr21 commented Jan 2, 2024

Oh wait, why add a feature that is deprecated right away? 🤔

@som-snytt
Copy link
Contributor Author

Not sure about knobs for enablement yet, but Scala 3 warns about apply insertion. It's strictly a migration mitigation.

@som-snytt
Copy link
Contributor Author

blocked by #10573

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.

LGTM 👍 thanks!

@joroKr21
Copy link
Member

joroKr21 commented Jan 3, 2024

Not sure about knobs for enablement yet, but Scala 3 warns about apply insertion. It's strictly a migration mitigation.

C.apply works both in Scala 2 and Scala 3 right?

@som-snytt
Copy link
Contributor Author

@joroKr21 iirc the tupled case is not handled because scala 2 doesn't do parameter untupling adaptation. I'll try to nail it down while polishing.

The other missing parts are compose & andThen, so the adaptation here is incomplete in that sense. So really, we're just trying to make it easier for those slick users. I used slick once, do I have to try it again to test out the use case? 😿

@som-snytt
Copy link
Contributor Author

som-snytt commented Jan 4, 2024

@lrytz about to contribute a bit of doc for the incompatibility matrix as you pointed out.

I haven't tried it, but maybe it makes sense to support case class D(i: Int, j: Int); List(42->27).map(D) directly, by injection of tupled, where the syntax is the same as Scala 3 but the mechanism is slightly different.

It was interesting that the recommended idiom is what is implemented here. (Foo.apply _).curried(1)(true)

@lrytz
Copy link
Member

lrytz commented Jan 4, 2024

maybe it makes sense to support case class D(i: Int, j: Int); List(42->27).map(D) directly

Since this doesn't currently work on Scala 2, and only works in Scala 3 with a warning and is therefore discouraged, I don't think this is needed.

@som-snytt
Copy link
Contributor Author

som-snytt commented Jan 4, 2024

My case for tupled injection is cross-compilation. For the tupled case, it requires different sources because D.tupled is not supported by Scala 3. I haven't tried slick use case yet, but it's the tupled case they care about. I assume slick supports Scala 3.

Since this doesn't currently work on Scala 2, and only works in Scala 3 with a warning and is therefore discouraged, I don't think this is needed.

Also, as @joroKr21 pointed out, this is true of everything in this PR. It has limited shelf life, as was true for the Scala 3 feature when it was introduced recently.

@lrytz
Copy link
Member

lrytz commented Jan 5, 2024

OK, good point

@som-snytt som-snytt marked this pull request as ready for review January 6, 2024 17:19
@som-snytt
Copy link
Contributor Author

Not sure I'll get around to tupled.

@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Jan 18, 2024
@SethTisue SethTisue self-assigned this Jan 18, 2024
@SethTisue SethTisue modified the milestones: 2.13.13, 2.13.14 Jan 22, 2024
@SethTisue SethTisue removed their assignment Jan 22, 2024
@SethTisue

This comment was marked as resolved.

@som-snytt som-snytt force-pushed the issue/3664-case-function branch 2 times, most recently from d1986de to 0996ac6 Compare January 30, 2024 18:16
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.

LGTM once the details are worked out

@som-snytt som-snytt force-pushed the issue/3664-case-function branch from e897b8a to 751a250 Compare January 31, 2024 19:51
@som-snytt

This comment was marked as resolved.

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.

This LGTM, let me know if you want to add anything or we can merge.

@SethTisue SethTisue modified the milestones: 2.13.14, 2.13.13 Feb 1, 2024
@som-snytt
Copy link
Contributor Author

som-snytt commented Feb 1, 2024

@lrytz added a commit for "insert apply with tupling adaptation". It's a bit tricky (subtle), so it is unsquashed.

I re-enabled the lrytz maneuver for your convenience. (Feel free to push here again.)

Perhaps there is better API usage for the simple questions in adaptApplyInsertion, such as "does it have an apply method and does its arity match the expected type or the tuple param", etc.

Slightly augmented tests. My takeaway vis-a-vis TDD is that for a code change of a certain complexity, discipline is required to add tests for intersecting cases. I don't evince that discipline here. A nice IDE tool would be a test generator that probes feature interactions, scalacheck + fuzzing + chatgpt. It's nice to have the community build, but we could use the corpus to generate targeted tests (because we know how people use case classes and tupled, for instance). Oh, I just thought of another test.

Edit: I checked that abstract case class (so there is no apply) shows the same error as arity mismatch, found C.type. In fact, without this commit it SOs. I checked out the parent commit and tried it standalone, it errors correctly.

@som-snytt som-snytt force-pushed the issue/3664-case-function branch from 393e77e to d8958d0 Compare February 2, 2024 17:32
@som-snytt
Copy link
Contributor Author

som-snytt commented Feb 2, 2024

Rebased and added support for case class E() (which is adapted by dotty).

The first typecheck of C.apply uses pt (so E works). On error, use wildcard (so tupled works).

Note: using wildcard first emits spurious warning about "empty application" of E(). Even though the result is discarded by us, the successful result makes silent emit the warnings. (Possibly you could typer.context.reporter.clearAll() before returning from the silent op, but it's clunky.)

Also added test for overloaded apply. Instead of erroring nicely, it just works because expected type is used first.

@som-snytt som-snytt force-pushed the issue/3664-case-function branch from d8958d0 to f57bef9 Compare February 2, 2024 17:54
@som-snytt

This comment was marked as resolved.

@som-snytt
Copy link
Contributor Author

/rebuild

@SethTisue
Copy link
Member

wasn't sure if /rebuild does older commits (the ones that the "combined" check is complaining about), but I see at https://scala-ci.typesafe.com/job/scala-2.13.x-validate-main/ that it does

@som-snytt

This comment was marked as resolved.

@som-snytt

This comment was marked as resolved.

@lrytz

This comment was marked as resolved.

lrytz

This comment was marked as resolved.

@lrytz lrytz merged commit 521cb82 into scala:2.13.x Feb 5, 2024
@som-snytt som-snytt deleted the issue/3664-case-function branch February 6, 2024 00:41
@SethTisue
Copy link
Member

SethTisue commented Feb 10, 2024

@som-snytt can you do a round of making the PR description user-facing? I think it would be valuable to include an example, and to explicitly say what the behavior is on -Xsource:3 vs -Xsource:3-cross vs neither.

@SethTisue SethTisue changed the title Accommodate case companion as function For migration to 3, accommodate case companion as function Feb 10, 2024
@som-snytt
Copy link
Contributor Author

I'll try to nail it down while polishing.

The pun was 💅 .

@lrytz
Copy link
Member

lrytz commented Oct 21, 2024

9 months and noone noticed...

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

Development

Successfully merging this pull request may close these issues.

explicit case class companion does not extend Function / override toString

5 participants