Skip to content

throw inside async block causes dead code warning #105

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
jrray opened this issue Jun 17, 2015 · 10 comments
Closed

throw inside async block causes dead code warning #105

jrray opened this issue Jun 17, 2015 · 10 comments
Assignees
Milestone

Comments

@jrray
Copy link

jrray commented Jun 17, 2015

scala> async { throw new Exception }
<console>:13: warning: dead code following this construct

This block does return a failed Future as expected, but the warning is a cosmetic problem. Is it possible to remove? Or is there a better way to fail an async block?

@retronym
Copy link
Member

retronym commented Jul 6, 2015

This warning is issued before the macro is expanded. To avoid this, we'd probably need to change the type signature of await to use a by-name parameter.

I'd recommend using Future.failed(new Exception) for this use case.

@retronym retronym self-assigned this Jul 6, 2015
@dwhjames
Copy link

I’ve also encountered dead code warnings with the -Ywarn-dead-code compiler options.

Please excuse the strangeness of the example that follows, this has been whittled down from a much larger code snippet.

$ sbt -Dscala.async.debug=true console

scala> :paste
// Entering paste mode (ctrl-D to finish)

import scala.async.Async._
import scala.concurrent.duration.Duration
import scala.concurrent.{Await, Future}
import scala.concurrent.ExecutionContext.Implicits.global

def foo() = async {
  val opt = await(async(Option.empty[String => Future[Unit]]))
  opt match {
    case None =>
      throw new RuntimeException("case a")
    case Some(f) =>
      await(f("case b"))
  }
}

// Exiting paste mode, now interpreting.

The warning is that there is dead code after the throw.

The relevant snippet of the state machine is copied below:

[async] async state machine transform expands to:
…
            case 3 => {
              {
                ();
                stateMachine$macro$29.this.f$bind$macro$34$macro$37 = null
              };
              {
                ();
                stateMachine$macro$29.this.await$macro$31$macro$35 = null
              };
              throw new scala.`package`.RuntimeException("case a");
              stateMachine$macro$29.this.state = 5;
              ()
            }
…
            case 5 => {
              {
                stateMachine$macro$29.this.result.complete(Success.apply[Unit](()));
                ()
              };
              return ()
            }
…

The dead code is pretty obvious now.

I can eliminate the dead code warning if I refactor to

scala> :paste
// Entering paste mode (ctrl-D to finish)
def foo() = async {
  val opt = await(async(Option.empty[String => Future[Unit]]))
  await { opt match {
    case None =>
      throw new RuntimeException("case a")
    case Some(f) =>
      f("case b")
  }}
}
// Exiting paste mode, now interpreting.

The relevant case in the state machine is now:

[async] async state machine transform expands to:
…
            case 2 => {
              val opt: Option[String => scala.concurrent.Future[Unit]] = stateMachine$macro$40.this.await$macro$42$macro$47;
              <synthetic> var matchres$macro$43: scala.concurrent.Future[Unit] @scala.reflect.internal.annotations.uncheckedBounds = null;
              opt match {
                case scala.None => matchres$macro$43 = (throw new scala.`package`.RuntimeException("case a").asInstanceOf[scala.concurrent.Future[Unit] @scala.reflect.internal.annotations.uncheckedBounds]: scala.concurrent.Future[Unit] @scala.reflect.internal.annotations.uncheckedBounds)
                case (x: String => scala.concurrent.Future[Unit])Some[String => scala.concurrent.Future[Unit]]((f @ _)) => matchres$macro$43 = (f.apply("case b").asInstanceOf[scala.concurrent.Future[Unit] @scala.reflect.internal.annotations.uncheckedBounds]: scala.concurrent.Future[Unit] @scala.reflect.internal.annotations.uncheckedBounds)
              };
              <synthetic> val awaitable$macro$44: scala.concurrent.Future[Unit] @scala.reflect.internal.annotations.uncheckedBounds = matchres$macro$43;
              stateMachine$macro$40.this.state = 3;
              if (awaitable$macro$44.isCompleted)
                {
                  if (awaitable$macro$44.value.get.isFailure)
                    {
                      {
                        stateMachine$macro$40.this.result.complete(awaitable$macro$44.value.get.asInstanceOf[scala.util.Try[Unit]]);
                        ()
                      };
                      return ()
                    }
                  else
                    {
                      stateMachine$macro$40.this.await$macro$45$macro$46 = awaitable$macro$44.value.get.get.asInstanceOf[Unit];
                      stateMachine$macro$40.this.state = 4
                    };
                  ()
                }
              else
                {
                  awaitable$macro$44.onComplete[Unit](this)(stateMachine$macro$40.this.execContext);
                  return ()
                };
              ()
            }
…

Out of interest I changed to the throw … to a Future.failed(…)

scala> :paste
// Entering paste mode (ctrl-D to finish)

def foo() = async {
  val opt = await(async(Option.empty[String => Future[Unit]]))
  await { opt match {
    case None =>
      Future.failed(new RuntimeException("case a"))
    case Some(f) =>
      f("case b")
  }}
}

// Exiting paste mode, now interpreting.

And the state machine is much the same:

case 2 => {
              val opt: Option[String => scala.concurrent.Future[Unit]] = stateMachine$macro$50.this.await$macro$52$macro$56;
              <synthetic> var matchres$macro$53: scala.concurrent.Future[Unit] @scala.reflect.internal.annotations.uncheckedBounds = null;
              opt match {
                case scala.None => matchres$macro$53 = (scala.concurrent.Future.failed[Nothing](new scala.`package`.RuntimeException("case a")).asInstanceOf[scala.concurrent.Future[Unit] @scala.reflect.internal.annotations.uncheckedBounds]: scala.concurrent.Future[Unit] @scala.reflect.internal.annotations.uncheckedBounds)
                case (x: String => scala.concurrent.Future[Unit])Some[String => scala.concurrent.Future[Unit]]((f @ _)) => matchres$macro$53 = (f.apply("case b").asInstanceOf[scala.concurrent.Future[Unit] @scala.reflect.internal.annotations.uncheckedBounds]: scala.concurrent.Future[Unit] @scala.reflect.internal.annotations.uncheckedBounds)
              };
              <synthetic> val awaitable$macro$54: scala.concurrent.Future[Unit] @scala.reflect.internal.annotations.uncheckedBounds = matchres$macro$53;
              stateMachine$macro$50.this.state = 3;
              if (awaitable$macro$54.isCompleted)
                {
                  if (awaitable$macro$54.value.get.isFailure)
                    {
                      {
                        stateMachine$macro$50.this.result.complete(awaitable$macro$54.value.get.asInstanceOf[scala.util.Try[Unit]]);
                        ()
                      };
                      return ()
                    }
                  else
                    {
                      stateMachine$macro$50.this.await$macro$55$macro$57 = awaitable$macro$54.value.get.get.asInstanceOf[Unit];
                      stateMachine$macro$50.this.state = 4
                    };
                  ()
                }
              else
                {
                  awaitable$macro$54.onComplete[Unit](this)(stateMachine$macro$50.this.execContext);
                  return ()
                };
              ()
            }
…

Despite the dead code in the original transformation… it doesn’t seem like anything is necessarily going wrong here… right?

@retronym
Copy link
Member

@dwhjames I can't reproduce the warning, although I can see the dead code. Could you please post a transcript that shows the warning in context?

Maybe the warning only shows up after untypechecking / retypechecking the expansion of the async macro?

@dwhjames
Copy link

I dropped my first snippet in an object in the project’s test scope, and got:

[warn] scala-async/src/test/scala/scala/async/foo.scala:11: dead code following this construct
[warn]         throw new RuntimeException("case a")
[warn]         ^

after adding the warning option in test scope:

scalacOptions in Test ++= Seq("-Yrangepos", "-Ywarn-dead-code")

In fact, this shows a lot of other dead code warnings too, in files such as AnfTransformSpec.scala

@retronym
Copy link
Member

Thanks, I can reproduce. Seems like range positions are required, oddly enough.

I think the easiest fix is to wrap any Nothing typed expressions in <expr>: Any when we construct the state machine.

The other approach I'm trying is to emit try { user code } finally { generated code } rather than { user code; generated code }. This might be important if when we get around to supporting try / catch in the async transform. Right now, we don't really care that we haven't advanced the state after an exception is thrown, as it always aborts the entire async block.

@retronym
Copy link
Member

@retronym retronym modified the milestone: 0.9.6 Jul 30, 2015
@retronym
Copy link
Member

@dwhjames Would you mind verifying this against the master branch? I've merged my fix.

@dwhjames
Copy link

@retronym I’ve tested the head of master and everything seems to compile and run fine with the project I’m working on.

However, I went back to 0.9.5 and 0.9.3 and couldn’t trigger any dead code warnings. Turns out my work around for #120 of including an explicit return type is related.

In any case, your fix in #132 doesn’t appear to have any regressions for my project. (I didn’t have any instances of the original example filed in this ticket.)

However, setting

scalacOptions in Test ++= Seq("-Yrangepos", "-Ywarn-dead-code")

for the scala-async build still shows lots of dead code warnings. Is this still to be expected?

Also, you commented about -Yrangepos. Is this something that one should typically have turned on (I couldn’t find much info about exactly what it does). And I noted that this is only turned on here for tests, which seems to stem from c780810 referencing SI-7649

@retronym
Copy link
Member

retronym commented Aug 4, 2015

Thanks for the feedback. I'm working on a few more instances of the problem that this has uncovered.

@abdolence
Copy link

I think this has returned back with 1.0.1 / Scala 2.13.6. Have to disable "-Ywarn-dead-code" because of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants