Experiment: Allow indented regions after operators

[odersky]

Allow indented regions after operators that appear on their own line. With this change, the following compiles:

import language.experimental.fewerBraces

def test(y: Int) =
    val firstValue = y
        * y
    val secondValue =
        firstValue
        +
            if firstValue < 0 then 1 else 0
        +
            if y < 0 then y else -y

    val result =
        firstValue < secondValue
        ||
            val thirdValue = firstValue * secondValue
            thirdValue > 100
        ||
            def avg(x: Double, y: Double) = (x + y)/2
            avg(firstValue, secondValue) > 0.0
        ||
            firstValue
            * secondValue
            *
                val firstSquare = firstValue * firstValue
                firstSquare + firstSquare
            <=
                firstValue `max` secondValue

This PR eliminates one of two areas where braces were previously (and annoyingly) still required. It will need a SIP before it is merged.

I hope that the example shows that requiring braces here is unnecessary and that the code is clearer without them.

[som-snytt]

this is like watching the next episode of The Chosen where you wonder what is Jesus up to now. He likes to say, "I bet you weren't expecting that."

[tgodzik]

I must be honest this doesn't look like a great idea. It seems to encourage bad code practices, I would never accept code like that in a review. I would recommend this code to look like:

import language.experimental.fewerBraces


def test(y: Int) =
    val firstValue = y * y
    val extractedValue1 =  if firstValue < 0 then 1 else 0
    val extractedValue2 =   if y < 0 then y else -y
    val secondValue =  firstValue + extractedValue1 + extractedValue2
    def thirdValue = firstValue * secondValue
    def avg(x: Double, y: Double) = (x + y)/2
    def cond = 
       val firstSquare = firstValue * firstValue
       firstValue * secondValue * (firstSquare + firstSquare)
    val result = firstValue < secondValue || 
      thirdValue > 100 || 
      avg(firstValue, secondValue) > 0.0 ||
      cond    <=  firstValue `max` secondValue
           

The values would be properly named to make sure that everything we write is legible.

I don't believe we should promote writing things in an utterly unreadable manner. I would not do it with or without braces.

[odersky]

It was a synthetic example, of course. I agree that often it is better to name things (in fact, I stressed that point in my Scala - The Simple Parts talk), but sometimes we just want to have a long sequence of || or && outlining all the possible conditions in sequence. Sort of like a decision diagram. Pulling out sub-definitions makes things less clear IMO. It's similar to the way we generally find long chains of .method(...) operations clear and don't require them to be broken down into intermediate results.

Here's some actual code from the compiler that is not bad, but has some annoying parentheses and curlies.

    /** Is this denotation defined in the same scope and compilation unit as that symbol? */
    final def isCoDefinedWith(other: Symbol)(using Context): Boolean =
      this.effectiveOwner == other.effectiveOwner
      &&
      (  !this.effectiveOwner.is(PackageClass)
        || this.isAbsent(canForce = false) 
        || other.isAbsent(canForce = false)
        || { // check if they are defined in the same file(or a jar)
           val thisFile = this.symbol.associatedFile
           val thatFile = other.associatedFile
           thisFile == null
           || thatFile == null
           || thisFile.path == thatFile.path // Cheap possibly wrong check, then expensive normalization
           || thisFile.canonicalPath == thatFile.canonicalPath
         }
      )

Note the baroque combination of closing parens and braces at the end. Of course you can do all braces, but my point is the current lack of indentation gives us awkward choices. Here's what it would be under the new indentation rules:

    /** Is this denotation defined in the same scope and compilation unit as that symbol? */
    final def isCoDefinedWith(other: Symbol)(using Context): Boolean =
      this.effectiveOwner == other.effectiveOwner
      &&
        !this.effectiveOwner.is(PackageClass)
        || this.isAbsent(canForce = false) 
        || other.isAbsent(canForce = false)
        || // check if they are defined in the same file(or a jar)
           val thisFile = this.symbol.associatedFile
           val thatFile = other.associatedFile
           thisFile == null
           || thatFile == null
           || thisFile.path == thatFile.path // Cheap possibly wrong check, then expensive normalization
           || thisFile.canonicalPath == thatFile.canonicalPath

Note that it would be really hard to refactor that method to flat named definitions and keep the same performance profile. You have embedded vals, which you can't pull out as vals since you want to evaluate them only when the actual case applies. You can't pull them out as defs either because they are referenced several times. And you can't pull them out as lazy vals because that would imply boxing, which is too expensive.

And, btw, let's write all operators in front. The previous style of writing them in the back is much less readable.

I guess my point is: we want to solve the problem that braces are sometimes still required. When we introduced fewer braces that was a big argument of the people who were against, and my counter-argument was that we'll get to that. Here we have a solution that works, is clear, and 100% compatible with the whole community build. And arguably there are situations where this style is appropriate.

[odersky]

Here's another example, this time from scaladoc

  def checkParaEnded(): Boolean = {
    (char == endOfText) ||
    ((char == endOfLine) && {
      val poff = offset
      nextChar() // read EOL
      val ok = {
        checkSkipInitWhitespace(endOfLine) ||
        checkSkipInitWhitespace('=') ||
        checkSkipInitWhitespace("{{{") ||
        checkList ||
        check(TableCellStart) ||
        checkSkipInitWhitespace('\u003D')
      }
      offset = poff
      ok
    })
  }

Note again the weird parens at the end. The logic is quite hard to follow. It becomes much easier if we "draw a decision diagram in code", like this:

  def checkParaEnded(): Boolean =
    char == endOfText
    || char == endOfLine
       &&
          val poff = offset
          try 
            nextChar() // read EOL
            checkSkipInitWhitespace(endOfLine) 
            || checkSkipInitWhitespace('=') 
            || checkSkipInitWhitespace("{{{") 
            || checkList 
            || check(TableCellStart) 
            || checkSkipInitWhitespace('\u003D')
          finally
            offset = poff

[odersky]

Another data point: If you grep the standard library, you find 91 occurrences of && { or || [. So these would all be places where we have to keep the braces unless we argue that they are better refactored to named. But I think that will be a hard argument to make.

[tgodzik]

The examples you show later are much less terrifying I must admit. The first example scared me a bit.

I am not convinced we need the change, but it's less concerning for sure.

[olhotak]

I find some of the large Boolean expressions in the compiler difficult to follow.

A problem with naming subexpressions is that the naming def or val needs to come before the overall expression. A Haskell-style where syntax could help here: you could describe the overall shape of the expression first, then give the detailed subexpressions later. It doesn't have to be where specifically, but a way to put the naming def/val after the overall expression instead of before.

[odersky]

I find some of the large Boolean expressions in the compiler difficult to follow.

I agree. But maybe it would help if we could lay them out as and/or decision diagrams? Certainly operators in front are much better than the previous trailing operator style. If not for backwards compatibility I'd propose to outlaw trailing operators althogether.

[odersky]

Closing to relieve the PR queue. We can come back to this later.

[odersky]

Closing to relieve the PR queue. We can come back to this later.