Skip to content

Inline and beta reduction can generate incorrect Inlined trees #16374

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
nicolasstucki opened this issue Nov 18, 2022 · 2 comments · Fixed by #16377
Closed

Inline and beta reduction can generate incorrect Inlined trees #16374

nicolasstucki opened this issue Nov 18, 2022 · 2 comments · Fixed by #16377
Assignees
Milestone

Comments

@nicolasstucki
Copy link
Contributor

Compiler version

3.2.1

Minimized code

Compiling with -Ycheck:all

def method(using String): String = ???

inline def inlineMethod(inline op: String => Unit)(using String): Unit =
  println(op(method))

def test(using String) =
  inlineMethod(c => ())

Output

*** error while checking test.scala after phase inlining ***
java.util.NoSuchElementException: head of empty list while running Ycheck on tests.scala
exception occurred while compiling tests.scala
java.util.NoSuchElementException: head of empty list while compiling tests.scala
Exception in thread "main" java.util.NoSuchElementException: head of empty list
        at scala.collection.immutable.Nil$.head(List.scala:662)
        at scala.collection.immutable.Nil$.head(List.scala:661)
        at dotty.tools.dotc.transform.YCheckPositions$$anon$1.traverse(YCheckPositions.scala:45)
        ...

The tree is

def test(using x$1: String): Unit =
      {{ /* inlined from 
        inlineMethod(
          {
            def $anonfun(c: String): Unit = ()
            closure($anonfun)
          }
        )(x$1)
       */
        println(
          {{ /* inlined from outside */
            {
              val c$proxy1: String = method({{ /* inlined from outside */x$1}})
              ()
            }
          }}
        ):Unit
      }}

It seems we are adding an Inlined with no call when beta-reducing the lambda.

Expectation

It should work. Beta reduction should not add this additional Inlined node, or it should balance it. Maybe the proxy should be in the Inlined bindings.

@nicolasstucki
Copy link
Contributor Author

The issue is that we beta reduce

    def test(using x$1: String): Unit =
      {{/* inlined from 
        inlineMethod(
          {
            def $anonfun(c: String): Unit = ()
            closure($anonfun)
          }
        )(x$1)
       */
        println(
          {{/* inlined from outside */
            {
              def $anonfun(c: String): Unit = ()
              closure($anonfun)
            }
          }}.apply(method({{/* inlined from outside */x$1}}))
        ):Unit
      }}

to

 def test(using x$1: String): Unit =
      {{/* inlined from 
        inlineMethod(
          {
            def $anonfun(c: String): Unit = ()
            closure($anonfun)
          }
        )(x$1)
       */
        println(
          {{/* inlined from outside */
            {
              val c$proxy1: String = method({{/* inlined from outside */x$1}})
              ()
            }
          }}
        ):Unit
      }}

It looks like what we need is to generate the binding outside the Inlined tree

 def test(using x$1: String): Unit =
      {{/* inlined from 
        inlineMethod(
          {
            def $anonfun(c: String): Unit = ()
            closure($anonfun)
          }
        )(x$1)
       */
        println({
          val c$proxy1: String = method({{/* inlined from outside */x$1}})
          {{/* inlined from outside */ () }}
        }
        ):Unit
      }}

@nicolasstucki
Copy link
Contributor Author

A similar case that fails in a different place

def method(using String): String = ???

inline def identity[T](inline x: T): T = x

inline def inlineMethod(inline op: String => Unit)(using String): Unit =
  println(identity(op)(method))

def test(using String) =
  inlineMethod(c => print(c))

nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Nov 18, 2022
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Nov 21, 2022
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Nov 21, 2022
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Nov 21, 2022
Handle all inline beta-reduction in the InlineReducer. All these
applications will contain `Inlined` nodes that need to be handled
without changing the nestedness of expressions in inlining scopes.

Fixes scala#16374
nicolasstucki added a commit that referenced this issue Nov 21, 2022
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Nov 22, 2022
Handle all inline beta-reduction in the InlineReducer. All these
applications will contain `Inlined` nodes that need to be handled
without changing the nestedness of expressions in inlining scopes.

Fixes scala#16374
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Nov 22, 2022
Handle all inline beta-reduction in the InlineReducer. All these
applications will contain `Inlined` nodes that need to be handled
without changing the nestedness of expressions in inlining scopes.

Fixes scala#16374
smarter added a commit that referenced this issue Nov 23, 2022
Handle all inline beta-reduction in the InlineReducer. All these
applications will contain `Inlined` nodes that need to be handled
without changing the nestedness of expressions in inlining scopes.

Fixes #16374
little-inferno pushed a commit to little-inferno/dotty that referenced this issue Jan 25, 2023
Handle all inline beta-reduction in the InlineReducer. All these
applications will contain `Inlined` nodes that need to be handled
without changing the nestedness of expressions in inlining scopes.

Fixes scala#16374
@Kordyjan Kordyjan added this to the 3.3.0 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants