Skip to content

Tailrec jammed by safer exceptions #17147

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

Closed
abgruszecki opened this issue Mar 24, 2023 · 8 comments · Fixed by #17313
Closed

Tailrec jammed by safer exceptions #17147

abgruszecki opened this issue Mar 24, 2023 · 8 comments · Fixed by #17313
Assignees
Labels
area:saferExceptions scala.language.experimental.saferExceptions itype:bug Spree Suitable for a future Spree
Milestone

Comments

@abgruszecki
Copy link
Contributor

Compiler version

$head -1 build.sbt 
ThisBuild / scalaVersion := "3.3.1-RC1-bin-20230322-1f574e8-NIGHTLY"

Minimized code

object TestyFactorial:
  // minimal example
  import scala.annotation.tailrec
  import language.experimental.saferExceptions

  @tailrec
  final def fact(n: Int, acc: Int): Int throws Exception =
    if n < 0 then
      throw new Exception
    else if n == 0 then
      acc
    else
      fact(n - 1, acc * n)

Output

$sbt clean compile
  ...
[error]  |    fact(n - 1, acc * n)
[error]  |    ^^^^^^^^^^^^^^^^^^^^
[error]  |    Cannot rewrite recursive call: it is not in tail position
@abgruszecki abgruszecki added itype:bug area:saferExceptions scala.language.experimental.saferExceptions labels Mar 24, 2023
@abgruszecki
Copy link
Contributor Author

/cc @odersky

@abgruszecki
Copy link
Contributor Author

This seems like a reasonably simple issue. Maybe it's good for the issue spree?

@dwijnand
Copy link
Member

Seems plausible to me too. Let's label it and if anyone disagrees they can remove it.

@dwijnand dwijnand added the Spree Suitable for a future Spree label Mar 24, 2023
@scala-center-bot
Copy link

This issue was picked for the Issue Spree No. 29 of 18 April 2023 which takes place in a week from now. @bishabosha, @Linyxus, @colin4124, @nmcb will be working on it. If you have any insight into the issue or guidance on how to fix it, please leave it here.

@bishabosha
Copy link
Member

it broke between 3.3.1-RC1-bin-20230302-8020c77-NIGHTLY and 3.3.1-RC1-bin-20230303-6ad22aa-NIGHTLY

@bishabosha
Copy link
Member

most likely it seems that #16507 cause the issue

@bishabosha
Copy link
Member

bishabosha commented Apr 18, 2023

minimisation found by @Linyxus

object TestyFactorial:
  import scala.annotation.tailrec
  import language.experimental.erasedDefinitions

  erased class Foo1
  class Foo2

  @tailrec
  final def test1(n: Int, acc: Int): Foo1 ?=> Int =
    if n <= 0 then acc
    else test1(n - 1, acc * n)

  @tailrec
  final def test2(n: Int, acc: Int): Foo2 ?=> Int =
    if n <= 0 then acc
    else test2(n - 1, acc * n)

@bishabosha
Copy link
Member

bishabosha commented Apr 18, 2023

It seems the problem is with erasure, as I noted in #16507 (comment), before the change, the erasure of test1 is @ContextResultCount(1) @tailrec def test1(n: Int, acc: Int): Int, but now it is @tailrec final def test1(n: Int, acc: Int): Function0

This seems to involve two factors transform.ContextFunctionResults.annotateContextResults, which deliberately does not "integrate" erased context function parameters, and the second part which is whatever handles elimination of the closure for context parameters.

natsukagami added a commit to natsukagami/dotty that referenced this issue Apr 19, 2023
natsukagami added a commit to natsukagami/dotty that referenced this issue Apr 20, 2023
nicolasstucki added a commit that referenced this issue Apr 21, 2023
…17313)


## Fix #17147

- Allow contextual functions with erased parameters to be integrated

<!-- TODO description of the change -->


<!-- Ideally should have a called "Fix #XYZ: A SHORT FIX DESCRIPTION"
-->
@Kordyjan Kordyjan added this to the 3.3.1 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:saferExceptions scala.language.experimental.saferExceptions itype:bug Spree Suitable for a future Spree
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants