Don't Use Return in Scala?

While doing some research on the other query that I recently opened (“The Return of the Blob”), I came across this:

https://tpolecat.github.io/2014/05/09/return.html

Don’t Use Return in Scala

The return keyword is not “optional” or “inferred”; it changes the meaning of your program, and you should never use it.

But after reading the article, I am not convinced of the claim above. It seems to me that it shows the pitfalls of so-called non-local returns but not return in general. Am I missing something?

The article also points out that early returns are never needed. That is true, but in my experience they can simplify the code in many cases, which is why I use them. They allows me to dispense with exceptional cases early without adding an indentation level to the entire method. For example, consider the following method for adding a base leg to an arrival route of a flight if the turn to final approach exceeds 90 degrees:

protected def addBaseLegIfNeeded: Route = {

if (isNotAnArrival) return this
if (waypts.length < 3) return this
if (abs(turnAngles.last) < 91 * deg) return this // base leg not needed

// normal case processed here

Yes, I could avoid the early returns, but I don’t think the resulting code would be as clean and clear. Am I the only one who thinks that?

3 Likes

I strongly disagree with tpolecat’s advice for code of any significant complexity unless you are using the full set of FP tools (where you have made considerable investments in getting everything to be referentially transparent, and early return can too easily break that).

You do have to be aware how early returns work (including throwing and catching stackless exceptions to get out of closures). But you absolutely don’t have to avoid them.

Rust, for instance, uses early return ubiquitously, especially when error-handling. It works very well.

That said, your example works just fine without early return:

if (isNotAnArrival) this
else if (waypts.length < 3) this
else if (abs(turnAngles.last) < 91 * deg) this
else {
  // normal case processed here
}

But in cases where the evaluation logic is more tricky, and you’re building up intermediate results, it really helps keep the logic clear, I find. It is idiomatic to avoid return when all else is equal, but sometimes it’s not all equal:

if (isNotAnArrival) return this
val waypts = arrival.generatePath()
if (waypts.length < 3) return this
val turnAngles = waypts.sliding(2).map(x => x(0) angle x(1))
if (abs(turnAngles.last) < 91*deg)) return this

// normal case processed here

If you were nesting these, you’d be three levels deep already for no particular benefit.

1 Like

As with so many things related to code readability, how readable the code is depends heavily on what you are used to. I feel that if you are used to approaching things in a more functional manner, you just get used to the last expression in a function being the result. Anything else winds up being a surprise, and surprises hurt readability.

For the simple set of if statements that you have here, the difference isn’t that big. However, you don’t have to add many lines before a return can get hidden in the logic. I find returns embedded in loops, whether for or while, to be worse. In both cases, I make assumptions about when the loop will terminate. I’m expecting a certain post-condition to be reached before it stops. A return statement embedded in either of those breaks that expectation and makes it more likely that I will miss something when going through the code.

1 Like

It is absolutely possible to train yourself to no longer notice early returns. Once you do this, you’d better avoid them, because you won’t understand the flow of the code.

In my case, any time I have a return value of Option or Either or the like, and the method isn’t tiny, I’m on the lookout for early returns because of how much it simplifies the code (i.e. handle the error right where it happens, and return already–just like you would with an exception, but your caller won’t forget to catch it).

My goal is to encourage people slow down and think about whether return is really warranted, given the tradeoffs. I think a lot of newcomers are surprised to see that most of the time it’s unnecessary. But in any case I think we both have thought about it and have come down on different sides based on the kind of code we enjoy working with, which is fine. Even if you’re wrong :wink:

3 Likes

There is a plug for -Xlint:nonlocal-return, at least.

I used to be more tolerant of many happy returns in Java, but now even guards at the top of a method irk me.

In Scala, I don’t mind a one-liner guard followed by the actual body in an else block. I wouldn’t mind syntax, similar to require, where a guard block evaluates some conditions and defaults to an innocuous value, without throwing and without using the r-word.

1 Like

“I wouldn’t mind syntax, similar to require , where a guard block evaluates some conditions and defaults to an innocuous value, without throwing and without using the r-word.”

That’s an interesting idea, but how would it be fundamentally any different than a conditional early return?

It’s declarative?
As opposed to imperative?

Just a thought :smiley:

1 Like

You are certainly entitled to your opinion about early return statements, but I think your article is misleading because it implies that they can cause surprise results in cases where they can’t. In particular, you wrote that

“The return keyword is not “optional” or “inferred”; it changes the meaning of your program, and you should never use it.”

That statement is misleading. Unless it is a “non-local” return, a return statement does NOT change the meaning of the program, nor does it significantly affect efficiency.

Your statement startled me because it made me wonder for a few minutes if I had major bugs in a software code base that I have been developing for years. I was relieved to find out that it was a false alarm.

If the difference is between using

def doThis(sth: Seq[String]) = {
    if (sth.exists(???)) return Seq.empty
    ... method logic...

versus

def doThis(sth: Seq[String]) = 
    if (sth.exists(???)) Seq.empty
    else {
        ... method logic...
    }

Just a personal opinion, but then the first looks cleaner and avoids unnecessary indentation

@ def a: Int = (1 to 3).foldRight(0)((a, b) => a + b) 
defined function a

@ a 
res3: Int = 6

@ def a: Int = (1 to 3).foldRight(0)((a, b) => return a + b) 
defined function a

@ a 
res5: Int = 3

All I did was add return to a function body and it changed the meaning of my program. This is easy to do when you’re refactoring and inlining code. I’m not sure how you can argue with this point. It’s fine if you don’t care but you can’t say it’s meaningless.

4 Likes

This is definitely where the style that you are used to impacts your opinion on readability. I used to agree with you. Back when Java and C++ were my primary languages, I would have preferred the option with the embedded return. I don’t feel that way anymore. Since adopting Scala as my primary language a number of years ago, my style has been steadily shifting to a more functional one. At this point, I really dislike functions with embedded returns and I find the second version to be cleaner, largely because I prefer how the logic is structured.

A lot of readability is the ability to understand a chunk of code quickly when you see it. The second version makes that more clear to me. The if is being used as an expression, and I know that the value of the function will be the result of that expression. In the first option, I could easily miss the return keyword and misinterpret this code. In this example that might seem silly, but imagine the next programmer who comes and touches this code adds more logic inside that if before the return. Now the original version has, through the evolution of maintenance, turned into the following.

def doThis(sth: Seq[String]) = {
    if (sth.exists(???)) {
        ... additional logic ...
        return Seq.empty
    }
    ... method logic...

Now it is a lot easier to miss the return. That type of thing is bound to happen in various places over time, and the original version holds up better to that type of modification in terms of purity of logic and readability, IMO.

2 Likes

Here again is what you wrote in your article:

“The return keyword is not “optional” or “inferred”; it changes the meaning of your program, and you should never use it.”

If you had said, “it can change the meaning of your program,” that would have been accurate. But to assert unconditionally that it changes the meaning of the program is misleading in my opinion. In the vast majority of cases it doesn’t.

By the way, the example you gave is important to understand, but I doubt that anyone who writes that kind of code would use return in a case like that.

I could imagine a beginner thinking that this is a good idea, and not realizing that it does something different.

seq.foldLeft(start){ (a, b) =>
  if (shouldIgnore(b)) return a
  
  // main logic here
}
1 Like

Guys - is this really a problem, or unexpected? return means "return from the def", and its meaning isn’t magically overridden if used in a lambda.

None of this is really a problem for people who know every little detail of how Scala works, and are very careful when they’re refactoring their code…
Still, the real reason I avoid return like the plague is just that I find it a lot more elegant to write a method as an expression instead of as a sequence of statements.
I could only imagine using it for some low level optimized code.

The funny thing is, Mark, I went from using early return a lot, to much less as I picked up Scala and wrote more functionally, and then back again in nontrivial cases, because in practice it actually was harder.

(@Jasper-M – maybe you can imagine this?)

Let me give an example. Suppose you are writing a game server, and you get a message that says player Tobnoddy wants to send 100 river gold to Slayerrrz, encoded in JSON. You might write something like

def handleMessage(msg: String): Either[Err, Unit] =
  msg.parseAsJson.flatMap{ j: Json =>
    j("type").to[String].flatMap{
      case "transfer" => j.parseAsTransfer().flatMap(_.handleTransfer)
      case ... => /* other code */
    }
  }

Pretty reasonable, no nonlocal returns, and everything is cool. (A for-comprehension doesn’t help much because of the match statement smack in the middle.)

But if you had a magical method ? that took the error case and returned it right away (and I’m labeling it ? because Rust has exactly that), you might instead write it like this:

def handleMessage(msg: String): Either[Err, Response] = {
  val j = msg.parseAsJson.?
  val t = j("type").to[String].?
  t match {
    case "transfer" => j.parseAsTransfer.?.handleTransfer
    case ... => /* other code */
  }
}

Now, I don’t know about you, but I find this considerably clearer. All that visually cluttering nesting that is being used only to communicate that you’re repeatedly doing early returns of bad values is gone. Where the early returns are is clearly marked–with a smidgen of training you can pick them out–and are also easy to not pay attention to when you’re focusing on the logic of the correct path. It’s like a for-comprehension except more flexible.

This illustrates that early returns aren’t necessarily bad, but it doesn’t illustrate that they’re really good in more complex situations. Let’s try a fictitious handleTransfer with both styles, where errors with the “from” user are just plain errors, but errors with user2 are actually responses that are sent to user1. First, the early-return way, because I just tried and can’t keep the logic straight on the other one without something to work from:

def handleTransfer(req: TransferRequest): Either[Err, Response] = {
  val u1 = req.user1.lookup(db).?
  val u2 = req.user2.lookup(db) match {
    case Left(e) => return Response(s"Cannot find ${req.user2}: $e")
    case Right(u) => u
  }
  val depositor = u2.checkAccess(u1) match {
    case Left(e) => return Result(s"$u2 has not given you deposit access")
    case Right(dep) => dep
  }
  val withdrawal = u1.withdraw(req.amount).?
  val conf = depositor.accept(withdrawal).?
  Result(s"You transferred $withdrawal to $u2, confirmation number $conf")
}

Trying to do this without early returns is considerably more painful, at least for me:

def handleTransfer(req: TransferRequest): Either[Err, Response] =
  req.user1.lookup(db).flatMap{ u1 =>
    req.user2.lookup(db) match {
      case Left(e) => Response(s"Cannot find ${req.user2}: $e")
      case Right(u2) =>
        u2.checkAccess(u1) match {
          case Left(e) => Response(s"$u2 has not given you deposit access")
          case Right(depositor) =>
            u1.withdraw(req.amount).flatMap{ withdrawal =>
              depositor.accept(withdrawal).map(conf =>
                Result(s"You transferred $withdrawal to $u2, confirmation number $conf")
              )
            }
        }
    }
  }

The reason it’s painful isn’t that it’s much longer (it’s not); it’s that you aren’t allowed to forget that you are on some arm of some control flow, which continually raises the possibility that you forgot a branch; also, there’s noticeably more clutter in finding what variables are in scope (and you can accidentally shadow them). And unlike in the first case, you can’t just replace it all with a for-comprehension because the logic is more elaborate.

The second way isn’t awful. I have tons of code that looks like that. But I can read through the first code considerably faster, because whenever you don’t need to think about anything any more, it goes away.

So I am a big advocate for early returns in serious code. Having a utility method (needs to be a macro, actually, in Scala) like .? makes it considerably better for error-handling, but as the methods grow larger even handling that by hand is a win, I find.

Again, if you’re doing 100% FP all the time, you have other ways to solve this problem, and even when you don’t have any alternatives it’s worth not having to think about as much stuff. But for anyone else, I think early return is really worth considering. I don’t use it flippantly; if (p) x else y is clearer than if (p) return x; y. But in cases where I need to clear out the cruft so I can focus on the relevant logic, it’s brilliant.

4 Likes

I feel like it would be interesting to see all of those expanded out to not use the ? method with an early return.

I can see the ? method being safer than an early return because it has a specific usage of dealing with errors. Part of the problem with the early return as a general construct is that it is completely general and could appear anywhere for whatever reason the user decided to exit early.

Well, it’s pretty trivial to do it without ?. Here’s one:

def handleTransfer(req: TransferRequest): Either[Err, Response] = {
  val u1 = req.user1.lookup(db) match {
    case Left(e) => return Left(e)
    case Right(r) => r
  }
  val u2 = req.user2.lookup(db) match {
    case Left(e) => return Response(s"Cannot find ${req.user2}: $e")
    case Right(u) => u
  }
  val depositor = u2.checkAccess(u1) match {
    case Left(e) => return Result(s"$u2 has not given you deposit access")
    case Right(dep) => dep
  }
  val withdrawal = u1.withdraw(req.amount) match {
    case Left(e) => return Left(e)
    case Right(r) => r
  }
  val conf = depositor.accept(withdrawal) match { 
    case Left(e) => return Left(e)
    case Right(r) => r 
  }
  Result(s"You transferred $withdrawal to $u2, confirmation number $conf")
}

The boilerplate gets annoying after a while (even if you stuff it all on one line), but at least when you get past it what you have left to think about is simplified.

Note that in this example I’m not just returning errors early, I’m also returning the “success” branch early. This way, when you’re done with something you’re actually done, not still entangled with the scopes of where you were doing control flow, but which don’t matter any more.

@andreak the problem is that you can’t inline or factor out expressions that contain return because of the weird scoping rule. This is very surprising if you’re a functional programmer and you’re used to being able to factor things mechanically.