Feedback sought: Optional Braces

Language Design
265

For me the, by far, most important decision is what comes after classes and objects as that is, by far, the most basic and common stuff c.f. givens and anonymous classes etc. I can live with what ever works best/good in practice for givens and anonymous classes.

Having said that, and to still give you some opinion:
I don’t find the colon after anonymous classes so disturbing and those who does can always use braces. It’s ok with with but colon seems nicer, but not a big deal.

My hunch is, though, that the new can be vital for a learner to realize that there is instantiation going on here of an anonymous class, and that a silent new might be too silent not helping interpretation by humans; but this is just guesswork as I haven’t tried it yet on neither myself in longer coding sessions nor on my students.

(Anonymous classes are included in my course but just as a small thing for completeness, and student don’t create their own implicits, except in some advanced extra exercises.)

266

Maybe we can come up with a small set of examples of code written by skilled Scala software engineers that do something nontrivial? Of course, a lot of code is trivial, so if you get a big win on the trivial side maybe that is enough to outweigh making the hard stuff even more confusing. (Go is a good example of making this kind of choice.)

Here are a few from me (either live code, or modified in a way that I wish I had time to make the live version):

//// 1 ////
  final class Absent[A <: Access[_, A], B <: Access[_, B]](
    val first: A, val second: B, val policies: Policies
  )
  extends Pairing[A, B, Unit] { 
    def info: Unit = ()
  }


//// 2 ////
  object Hold {
    sealed abstract class Checking[H, V, C](hidden: H)
    extends Hold[V] {
      protected var h: H = hidden
      ...
    }
    ...
  }

//// 3 ////
sealed trait Alerter extends AsJson {
  def alert(message: String): Ok[String, Unit]
}
object Alerter extends FromJson[Alerter] {
  object Screen extends Alerter {
    def alert(message: String): Ok[String, Unit] = {
      println(message)
      Ok.UnitYes
    }
    val label: String = "screen"
    def json: Json = Json(label)
  }
  ...
}

Here they are with colon:

//// 1 ////
  final class Absent[A <: Access[_, A], B <: Access[_, B]](
    val first: A, val second: B, val policies: Policies
  )
  extends Pairing[A, B, Unit]:
    def info: Unit = ()


//// 2 ////
  object Hold:
    sealed abstract class Checking[H, V, C](hidden: H)
    extends Hold[V]:
      protected var h: H = hidden
      ...
    ...

//// 3 ////
sealed trait Alerter extends AsJson:
  def alert(message: String): Ok[String, Unit]

object Alerter extends FromJson[Alerter]:
  object Screen extends Alerter:
    def alert(message: String): Ok[String, Unit] =
      println(message)
      Ok.UnitYes
    val label: String = "screen"
    def json: Json = Json(label)
  ...

And here with with:

//// 1 ////
  final class Absent[A <: Access[_, A], B <: Access[_, B]](
    val first: A, val second: B, val policies: Policies
  )
  extends Pairing[A, B, Unit] with
    def info: Unit = ()


//// 2 ////
  object Hold with
    sealed abstract class Checking[H, V, C](hidden: H)
    extends Hold[V] with
      protected var h: H = hidden
      ...
    ...

//// 3 ////
sealed trait Alerter extends AsJson with
  def alert(message: String): Ok[String, Unit]

object Alerter extends FromJson[Alerter] with
  object Screen extends Alerter with
    def alert(message: String): Ok[String, Unit] =
      println(message)
      Ok.UnitYes
    val label: String = "screen"
    def json: Json = Json(label)
  ...

To me, the biggest problem is actually parsing apart the different blocks, and that makes braces best, with second best, and : worst; at least to me it is hard to pick out especially from multiple lines. But others may differ, and their code may differ in structure from mine.

6 Likes
267

Just wanted to appreciate odersky’s feedback to the feedback.

If odersky had proposed a Swiss retreat, to last 3-5 years, to explore various dimensions of Scala 3 syntax, I would have signed up, on my own tab, especially if propensive showed up.

My blog would have said: propensive flew in today. Something about higher-kinded something. I’m joining the hike to the Gletscher.

4 Likes
268

I find : much (muuuuuuuch) easier to read.

Also your example that you crafted above that you said looks weird, it didn’t look weird to me; it looked fine to me actually. :slightly_smiling_face:

(And no, I don’t have Python experience.)

269

While it may not be possible to mandate it, if we want larger indents to happen at all we need to come to a consensus up front so we can begin to nudge the community in the right direction.

If the Scala 3 team sets the official standard at 4-space indents, I believe that the IntelliJ folks, METALS folks, Scalafmt folks, Scalastyle folks, etc. will fall in line quickly with their defaults, and the bulk of the community will follow suite sooner or later. Conversely, if there isn’t any guidance, it’s unlikely that any party will be able to bring the others over to 4-space indents on their own, and we’ll likely be on 2-space indents (and their issues w.r.t. indent-delimited syntax) for the foreseeable future, even if there’s broad agreement that 4-space indents are better.

This is a classic coordination problem that the core Scala 3 team is best positioned to resolve.

3 Likes
270

On balance, users of all types, novices to experts, can handle the 2 meanings of :
That : is used for type ascription isn’t optimal. But : is preferable to with.
The survey done by the Professor is relevant. I am not a Python user at all and prefer : to with.
: has the right intuitive semantics which is probably why it was chosen in Python as the indentation marker.
In places where : is confusing, braces can be an option, correct?
Please, no where. It is literally a road to nowhere.

The 4 spaces vs 2 spaces stylistic default is not unimportant.
Like @lihaoyi I vote for 4 spaces.

271

I would like to throw out one more data point: Python itself as of Python 3.6 (released Q3 2016) uses : both for type ascriptions as well as opening indent-delimited blocks:

primes: List[int] = []

captain: str  # Note: no initial value!

class Starship:
    stats: ClassVar[Dict[str, int]] = {}

def inproduct(v: Vector[T]) -> T:
    return sum(x*y for x, y in v)

def dilate(v: Vector[T], scale: T) -> Vector[T]:
    return ((x * scale, y * scale) for x, y in v)

While there have been complaints about Python type annotations, I have seen no specific unhappiness with the fact that : is overloaded to mean both type annotations and block-opening. It just doesn’t seem to be a problem in practice.

272

It is very hard to confuse them because type ascriptions appear within parentheses. For the return type, where : would be confusing, they use ->. So I don’t think that tells us anything about Scala, except that where : means “open block” in Python it means “type ascription” in Scala, and you use = instead of : to open the block in Scala there.

(Except in places where you can’t have a type ascription, you open the block with :.)

7 Likes
273

My main problem with that is that for now we recommend that programs drop braces gradually. There’s no need to do a global rewrite, you can drop braces as you incrementally change code. But in that situation a change of indentation width for some part of a program but not others would look weird. Also, I believe we have to gain more experience to have a strong opinion what’s the right width.

Except where they annotate the type of a val. From the mypy cheat sheet:

from typing import List, Set, Dict, Tuple, Optional

# For simple built-in types, just use the name of the type
x: int = 1
x: float = 1.0
x: bool = True
x: str = "test"
x: bytes = b"test"

# For collections, the name of the type is capitalized, and the
# name of the type inside the collection is in brackets
x: List[int] = [1]
x: Set[int] = {6, 7}
274

My completely subjective opinion is that this looks awful. It’s extremely clear that : for type ascriptions is an afterthought. I really don’t see how this should be a role model. No one will convince me that this is somehow desirable syntax:

$ python
Python 3.9.0 (default, Oct  7 2020, 23:09:01) 
[GCC 10.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from typing import Callable
>>> f: Callable[[int], int] = lambda x: x + 1
>>> def x() -> int: return 1
... 
>>> f(x())
2

I could spam this thread with Haskell snippets, but then people would say “oh, but this only works in Haskell, not in Scala”

We can look at other languages for inspiration, but we can’t use the fact that other languages do things one way as evidence that it’s optimal in Scala.

2 Likes
275

So forgive me for doing this anyway, but really:

This is a side by side comparison of Haskell and Scala both using where, feel free to substitute with any other keyword:

(Minimized to make people hate me less)
Haskell Scala 
module Demo where

  type List a = [a]

  class SemiGroup t where
    combine :: t -> t -> t

  class SemiGroup t => Monoid t where
    unit :: t

  instance SemiGroup String where
    combine x y = x ++ y

  instance Monoid String where
    unit = ""

  instance SemiGroup Int where
    combine x y = x + y

  instance Monoid Int where
    unit = 0

  combineAll :: Monoid t => List t -> t
  combineAll xs = foldl combine unit xs

  main = putStrLn lst where
    lst = combineAll ["Hello"," ","World"]


object Demo where

  trait SemiGroup[T] where
    extension (x: T) def combine (y: T): T

  trait Monoid[T] extends SemiGroup[T] where
    def unit: T

  given Monoid[String] where
    extension (x: String) where
      def combine (y: String): String = x.concat(y)
    def unit: String = ""

  given Monoid[Int] where
    extension (x: Int) where
      def combine (y: Int): Int = x + y
    def unit: Int = 0

  def combineAll[T: Monoid](xs: List[T]): T =
    xs.foldLeft(summon[Monoid[T]].unit)(_.combine(_))

  def main(args: Array[String]) =
    println(combineAll(List("Hello"," ","World")))

The Haskellers use where even more than it would be used in the Scala example.

Since the only problem with where is that it’s long, and this is not a problem in Haskell, I’ve now proven by natural deduction that where is superior to everything else. QED.

Or I’m wrong and examples of Python code “working” is not proof of : being a good idea.

EDIT: To clarify, I’ve dropped the whole where thing. The point I’m trying to make here is that this argument is a logical fallacy, and not proof of anything.

3 Likes
276

The critical difference to Python is that in Python, it is simple and uniform:

In Python, a block starts if and only if there is a colon at the end of the line.

In Scala’s OBS, however, we have:

  • Some blocks start with colon at the end of the line, some do not
  • Some colons at the end of the line mean block start, some do not
2 Likes
277

I’ve said before, I actually don’t think this is a bad compromise! : in class, trait, and object definitions I think : is less of a problem. It doesn’t seem stupid at all to me to have with work everywhere, and in class definitions etc, you can use : as a shorthand.

1 Like
278

Meta question: How did you do that fold-out? It could be very useful at times to be able to hide long explanations or code sequences, but I don’t know how to achieve that in markdown.

EDIT: Nevermind, I found the explanation here: How to add a collapsible section in markdown. · GitHub

1 Like
279

Student poll update by December 21 at noon
image
Sent via Discord to 134 students and 17 teaching assistants with currently 53 responses.

Result so far:
(24+26)/53 == 94% did not vote for with…

1 Like
280

I vote for 2 spaces!

Otherwise with double amount of white space the code gets too wide and its more difficult to fit several open files besides each other without lines folding. And its also difficult to fit code on slides when teaching and presenting. The editor can help with visual hints to highlight the nesting level.
(And, subjectively, I also think it looks ugly with heavy, Java-style indent.)

Better use end-markers when blocks of code are to long to make indent level obvious at a glance…

2 Likes
281

I think this is by far the best idea yet. It’s also in line with what I was trying to suggest earlier. i.e. use a scheme were people can either

  • just use ( ), or
  • use a library-defined DSL for passing multiline arguments.

I guess it should be possible to define <| or whatever else a DSL author chooses as a simple macro that optimizes the unnecessary function values away.

282

As I said: there is a valid concern that Scala will gradually turn into a language primarily used for teaching and research, and less for actual software engineering.

283

I experimented with tab sizes a bit. I took a file I had written that used exclusively indentation syntax and
rendered it with tab sizes of 2, 3, and 4. Please bear with me since the file is about 300 lines long, but that way I hope to get a more representative impression.

Nullables.scala, tab size = 2 
/** Operations for implementing a flow analysis for nullability */
object Nullables:
  import ast.tpd._

  /** A set of val or var references that are known to be not null, plus a set of
  *  variable references that are not known (anymore) to be not null
  */
  case class NotNullInfo(asserted: Set[TermRef], retracted: Set[TermRef]):
    assert((asserted & retracted).isEmpty)

    def isEmpty = this eq NotNullInfo.empty

    def retractedInfo = NotNullInfo(Set(), retracted)

    /** The sequential combination with another not-null info */
    def seq(that: NotNullInfo): NotNullInfo =
      if this.isEmpty then that
      else if that.isEmpty then this
      else NotNullInfo(
        this.asserted.union(that.asserted).diff(that.retracted),
        this.retracted.union(that.retracted).diff(that.asserted))

    /** The alternative path combination with another not-null info. Used to merge
    *  the nullability info of the two branches of an if.
    */
    def alt(that: NotNullInfo): NotNullInfo =
      NotNullInfo(this.asserted.intersect(that.asserted), this.retracted.union(that.retracted))

  object NotNullInfo:
    val empty = new NotNullInfo(Set(), Set())
    def apply(asserted: Set[TermRef], retracted: Set[TermRef]): NotNullInfo =
      if asserted.isEmpty && retracted.isEmpty then empty
      else new NotNullInfo(asserted, retracted)
  end NotNullInfo

  /** A pair of not-null sets, depending on whether a condition is `true` or `false` */
  case class NotNullConditional(ifTrue: Set[TermRef], ifFalse: Set[TermRef]):
    def isEmpty = this eq NotNullConditional.empty

  object NotNullConditional:
    val empty = new NotNullConditional(Set(), Set())
    def apply(ifTrue: Set[TermRef], ifFalse: Set[TermRef]): NotNullConditional =
      if ifTrue.isEmpty && ifFalse.isEmpty then empty
      else new NotNullConditional(ifTrue, ifFalse)
  end NotNullConditional

  /** An attachment that represents conditional flow facts established
  *  by this tree, which represents a condition.
  */
  private[typer] val NNConditional = Property.StickyKey[NotNullConditional]

  /** An attachment that represents unconditional flow facts established
    *  by this tree.
    */
  private[typer] val NNInfo = Property.StickyKey[NotNullInfo]

  /** An extractor for null comparisons */
  object CompareNull:

    /** Matches one of
    *
    *    tree == null, tree eq null, null == tree, null eq tree
    *    tree != null, tree ne null, null != tree, null ne tree
    *
    *  The second boolean result is true for equality tests, false for inequality tests
    */
    def unapply(tree: Tree)(using Context): Option[(Tree, Boolean)] = tree match
      case Apply(Select(l, _), Literal(Constant(null)) :: Nil) =>
        testSym(tree.symbol, l)
      case Apply(Select(Literal(Constant(null)), _), r :: Nil) =>
        testSym(tree.symbol, r)
      case _ =>
        None

    private def testSym(sym: Symbol, operand: Tree)(using Context) =
      if sym == defn.Any_== || sym == defn.Object_eq then Some((operand, true))
      else if sym == defn.Any_!= || sym == defn.Object_ne then Some((operand, false))
      else None

  end CompareNull

  /** An extractor for null-trackable references */
  object TrackedRef:
    def unapply(tree: Tree)(using Context): Option[TermRef] = tree.typeOpt match
      case ref: TermRef if isTracked(ref) => Some(ref)
      case _ => None
  end TrackedRef

  def isTracked(ref: TermRef)(using Context) =
    ref.isStable
    || { val sym = ref.symbol
        !ref.usedOutOfOrder
        && sym.span.exists
        && ctx.compilationUnit != null // could be null under -Ytest-pickler
        && ctx.compilationUnit.assignmentSpans.contains(sym.span.start)
      }

  /** The nullability context to be used after a case that matches pattern `pat`.
  *  If `pat` is `null`, this will assert that the selector `sel` is not null afterwards.
  */
  def afterPatternContext(sel: Tree, pat: Tree)(using Context) = (sel, pat) match
    case (TrackedRef(ref), Literal(Constant(null))) => ctx.addNotNullRefs(Set(ref))
    case _ => ctx

  /** The nullability context to be used for the guard and rhs of a case with
  *  given pattern `pat`. If the pattern can only match non-null values, this
  *  will assert that the selector `sel` is not null in these regions.
  */
  def caseContext(sel: Tree, pat: Tree)(using Context): Context = sel match
    case TrackedRef(ref) if matchesNotNull(pat) => ctx.addNotNullRefs(Set(ref))
    case _ => ctx

  private def matchesNotNull(pat: Tree)(using Context): Boolean = pat match
    case _: Typed | _: UnApply => true
    case Alternative(pats) => pats.forall(matchesNotNull)
    // TODO: Add constant pattern if the constant type is not nullable
    case _ => false

  extension (infos: List[NotNullInfo])

    /** Do the current not-null infos imply that `ref` is not null?
    *  Not-null infos are as a history where earlier assertions and retractions replace
    *  later ones (i.e. it records the assignment history in reverse, with most recent first)
    */
    @tailrec def impliesNotNull(ref: TermRef): Boolean = infos match
      case info :: infos1 =>
        if info.asserted.contains(ref) then true
        else if info.retracted.contains(ref) then false
        else infos1.impliesNotNull(ref)
      case _ =>
        false

    /** Add `info` as the most recent entry to the list of null infos. Assertions
    *  or retractions in `info` supersede infos in existing entries of `infos`.
    */
    def extendWith(info: NotNullInfo) =
      if info.isEmpty
        || info.asserted.forall(infos.impliesNotNull(_))
            && !info.retracted.exists(infos.impliesNotNull(_))
      then infos
      else info :: infos

    /** Retract all references to mutable variables */
    def retractMutables(using Context) =
      val mutables = infos.foldLeft(Set[TermRef]())((ms, info) =>
        ms.union(info.asserted.filter(_.symbol.is(Mutable))))
      infos.extendWith(NotNullInfo(Set(), mutables))

  end extension

  extension (ref: TermRef)
    def usedOutOfOrder(using Context): Boolean =
      val refSym = ref.symbol
      val refOwner = refSym.owner

      @tailrec def recur(s: Symbol): Boolean =
        s != NoSymbol
        && s != refOwner
        && (s.isOneOf(Lazy | Method) // not at the rhs of lazy ValDef or in a method (or lambda)
            || s.isClass // not in a class
              // TODO: need to check by-name parameter
            || recur(s.owner))

      refSym.is(Mutable) // if it is immutable, we don't need to check the rest conditions
      && refOwner.isTerm
      && recur(ctx.owner)
  end extension

  extension (tree: Tree)

    /* The `tree` with added nullability attachment */
    def withNotNullInfo(info: NotNullInfo): tree.type =
      if !info.isEmpty then tree.putAttachment(NNInfo, info)
      tree

    /* The nullability info of `tree` */
    def notNullInfo(using Context): NotNullInfo =
      stripInlined(tree).getAttachment(NNInfo) match
        case Some(info) if !ctx.erasedTypes => info
        case _ => NotNullInfo.empty

    /* The nullability info of `tree`, assuming it is a condition that evaluates to `c` */
    def notNullInfoIf(c: Boolean)(using Context): NotNullInfo =
      val cond = tree.notNullConditional
      if cond.isEmpty then tree.notNullInfo
      else tree.notNullInfo.seq(NotNullInfo(if c then cond.ifTrue else cond.ifFalse, Set()))

    /** The paths that are known to be not null if the condition represented
    *  by `tree` yields `true` or `false`. Two empty sets if `tree` is not
    *  a condition.
    */
    def notNullConditional(using Context): NotNullConditional =
      stripBlock(tree).getAttachment(NNConditional) match
        case Some(cond) if !ctx.erasedTypes => cond
        case _ => NotNullConditional.empty

    /** The current context augmented with nullability information of `tree` */
    def nullableContext(using Context): Context =
      val info = tree.notNullInfo
      if info.isEmpty then ctx else ctx.addNotNullInfo(info)

    /** The current context augmented with nullability information,
    *  assuming the result of the condition represented by `tree` is the same as
    *  the value of `c`.
    */
    def nullableContextIf(c: Boolean)(using Context): Context =
      val info = tree.notNullInfoIf(c)
      if info.isEmpty then ctx else ctx.addNotNullInfo(info)

    /** The context to use for the arguments of the function represented by `tree`.
    *  This is the current context, augmented with nullability information
    *  of the left argument, if the application is a boolean `&&` or `||`.
    */
    def nullableInArgContext(using Context): Context = tree match
      case Select(x, _) if !ctx.erasedTypes =>
        if tree.symbol == defn.Boolean_&& then x.nullableContextIf(true)
        else if tree.symbol == defn.Boolean_|| then x.nullableContextIf(false)
        else ctx
      case _ => ctx
  end extension
end Nullables
Nullables.scala, tab size = 3 
/** Operations for implementing a flow analysis for nullability */
object Nullables:
   import ast.tpd._

   /** A set of val or var references that are known to be not null, plus a set of
   *   variable references that are not known (anymore) to be not null
   */
   case class NotNullInfo(asserted: Set[TermRef], retracted: Set[TermRef]):
      assert((asserted & retracted).isEmpty)

      def isEmpty = this eq NotNullInfo.empty

      def retractedInfo = NotNullInfo(Set(), retracted)

      /** The sequential combination with another not-null info */
      def seq(that: NotNullInfo): NotNullInfo =
         if this.isEmpty then that
         else if that.isEmpty then this
         else NotNullInfo(
            this.asserted.union(that.asserted).diff(that.retracted),
            this.retracted.union(that.retracted).diff(that.asserted))

      /** The alternative path combination with another not-null info. Used to merge
      *   the nullability info of the two branches of an if.
      */
      def alt(that: NotNullInfo): NotNullInfo =
         NotNullInfo(this.asserted.intersect(that.asserted), this.retracted.union(that.retracted))

   object NotNullInfo:
      val empty = new NotNullInfo(Set(), Set())
      def apply(asserted: Set[TermRef], retracted: Set[TermRef]): NotNullInfo =
         if asserted.isEmpty && retracted.isEmpty then empty
         else new NotNullInfo(asserted, retracted)
   end NotNullInfo

   /** A pair of not-null sets, depending on whether a condition is `true` or `false` */
   case class NotNullConditional(ifTrue: Set[TermRef], ifFalse: Set[TermRef]):
      def isEmpty = this eq NotNullConditional.empty

   object NotNullConditional:
      val empty = new NotNullConditional(Set(), Set())
      def apply(ifTrue: Set[TermRef], ifFalse: Set[TermRef]): NotNullConditional =
         if ifTrue.isEmpty && ifFalse.isEmpty then empty
         else new NotNullConditional(ifTrue, ifFalse)
   end NotNullConditional

   /** An attachment that represents conditional flow facts established
   *   by this tree, which represents a condition.
   */
   private[typer] val NNConditional = Property.StickyKey[NotNullConditional]

   /** An attachment that represents unconditional flow facts established
      *   by this tree.
      */
   private[typer] val NNInfo = Property.StickyKey[NotNullInfo]

   /** An extractor for null comparisons */
   object CompareNull:

      /** Matches one of
      *
      *      tree == null, tree eq null, null == tree, null eq tree
      *      tree != null, tree ne null, null != tree, null ne tree
      *
      *   The second boolean result is true for equality tests, false for inequality tests
      */
      def unapply(tree: Tree)(using Context): Option[(Tree, Boolean)] =
         tree match
         case Apply(Select(l, _), Literal(Constant(null)) :: Nil) =>
            testSym(tree.symbol, l)
         case Apply(Select(Literal(Constant(null)), _), r :: Nil) =>
            testSym(tree.symbol, r)
         case _ =>
            None

      private def testSym(sym: Symbol, operand: Tree)(using Context) =
         if sym == defn.Any_== || sym == defn.Object_eq then Some((operand, true))
         else if sym == defn.Any_!= || sym == defn.Object_ne then Some((operand, false))
         else None

   end CompareNull

   /** An extractor for null-trackable references */
   object TrackedRef:
      def unapply(tree: Tree)(using Context): Option[TermRef] =
         tree.typeOpt match
         case ref: TermRef if isTracked(ref) => Some(ref)
         case _ => None
   end TrackedRef

   def isTracked(ref: TermRef)(using Context) =
      ref.isStable
      || {
         val sym = ref.symbol
         !ref.usedOutOfOrder
         && sym.span.exists
         && ctx.compilationUnit != null // could be null under -Ytest-pickler
         && ctx.compilationUnit.assignmentSpans.contains(sym.span.start)
      }

   /** The nullability context to be used after a case that matches pattern `pat`.
   *   If `pat` is `null`, this will assert that the selector `sel` is not null afterwards.
   */
   def afterPatternContext(sel: Tree, pat: Tree)(using Context) =
      (sel, pat) match
      case (TrackedRef(ref), Literal(Constant(null))) => ctx.addNotNullRefs(Set(ref))
      case _ => ctx

   /** The nullability context to be used for the guard and rhs of a case with
   *   given pattern `pat`. If the pattern can only match non-null values, this
   *   will assert that the selector `sel` is not null in these regions.
   */
   def caseContext(sel: Tree, pat: Tree)(using Context): Context =
      sel match
      case TrackedRef(ref) if matchesNotNull(pat) => ctx.addNotNullRefs(Set(ref))
      case _ => ctx

   private def matchesNotNull(pat: Tree)(using Context): Boolean =
      pat match
      case _: Typed | _: UnApply => true
      case Alternative(pats) => pats.forall(matchesNotNull)
      // TODO: Add constant pattern if the constant type is not nullable
      case _ => false

   extension (infos: List[NotNullInfo])

      /** Do the current not-null infos imply that `ref` is not null?
      *   Not-null infos are as a history where earlier assertions and retractions replace
      *   later ones (i.e. it records the assignment history in reverse, with most recent first)
      */
      @tailrec def impliesNotNull(ref: TermRef): Boolean =
         infos match
         case info :: infos1 =>
            if info.asserted.contains(ref) then true
            else if info.retracted.contains(ref) then false
            else infos1.impliesNotNull(ref)
         case _ =>
            false

      /** Add `info` as the most recent entry to the list of null infos. Assertions
      *   or retractions in `info` supersede infos in existing entries of `infos`.
      */
      def extendWith(info: NotNullInfo) =
         if info.isEmpty
            || info.asserted.forall(infos.impliesNotNull(_))
               && !info.retracted.exists(infos.impliesNotNull(_))
         then infos
         else info :: infos

      /** Retract all references to mutable variables */
      def retractMutables(using Context) =
         val mutables = infos.foldLeft(Set[TermRef]())((ms, info) =>
            ms.union(info.asserted.filter(_.symbol.is(Mutable))))
         infos.extendWith(NotNullInfo(Set(), mutables))

   end extension

   extension (ref: TermRef)
      def usedOutOfOrder(using Context): Boolean =
         val refSym = ref.symbol
         val refOwner = refSym.owner

         @tailrec def recur(s: Symbol): Boolean =
            s != NoSymbol
            && s != refOwner
            && (s.isOneOf(Lazy | Method) // not at the rhs of lazy ValDef or in a method (or lambda)
               || s.isClass // not in a class
                     // TODO: need to check by-name parameter
               || recur(s.owner)
               )

         refSym.is(Mutable) // if it is immutable, we don't need to check the rest conditions
         && refOwner.isTerm
         && recur(ctx.owner)
   end extension

   extension (tree: Tree)

      /* The `tree` with added nullability attachment */
      def withNotNullInfo(info: NotNullInfo): tree.type =
         if !info.isEmpty then tree.putAttachment(NNInfo, info)
         tree

      /* The nullability info of `tree` */
      def notNullInfo(using Context): NotNullInfo =
         stripInlined(tree).getAttachment(NNInfo) match
         case Some(info) if !ctx.erasedTypes => info
         case _ => NotNullInfo.empty

      /* The nullability info of `tree`, assuming it is a condition that evaluates to `c` */
      def notNullInfoIf(c: Boolean)(using Context): NotNullInfo =
         val cond = tree.notNullConditional
         if cond.isEmpty then tree.notNullInfo
         else tree.notNullInfo.seq(NotNullInfo(if c then cond.ifTrue else cond.ifFalse, Set()))

      /** The paths that are known to be not null if the condition represented
      *   by `tree` yields `true` or `false`. Two empty sets if `tree` is not
      *   a condition.
      */
      def notNullConditional(using Context): NotNullConditional =
         stripBlock(tree).getAttachment(NNConditional) match
            case Some(cond) if !ctx.erasedTypes => cond
            case _ => NotNullConditional.empty

      /** The current context augmented with nullability information of `tree` */
      def nullableContext(using Context): Context =
         val info = tree.notNullInfo
         if info.isEmpty then ctx else ctx.addNotNullInfo(info)

      /** The current context augmented with nullability information,
      *   assuming the result of the condition represented by `tree` is the same as
      *   the value of `c`.
      */
      def nullableContextIf(c: Boolean)(using Context): Context =
         val info = tree.notNullInfoIf(c)
         if info.isEmpty then ctx else ctx.addNotNullInfo(info)

      /** The context to use for the arguments of the function represented by `tree`.
      *   This is the current context, augmented with nullability information
      *   of the left argument, if the application is a boolean `&&` or `||`.
      */
      def nullableInArgContext(using Context): Context = tree match
         case Select(x, _) if !ctx.erasedTypes =>
            if tree.symbol == defn.Boolean_&& then x.nullableContextIf(true)
            else if tree.symbol == defn.Boolean_|| then x.nullableContextIf(false)
            else ctx
         case _ => ctx
   end extension
end Nullables
Nullables.scala, tab size = 4 
/** Operations for implementing a flow analysis for nullability */
object Nullables:
    import ast.tpd._

    /** A set of val or var references that are known to be not null, plus a set of
    *  variable references that are not known (anymore) to be not null
    */
    case class NotNullInfo(asserted: Set[TermRef], retracted: Set[TermRef]):
        assert((asserted & retracted).isEmpty)

        def isEmpty = this eq NotNullInfo.empty

        def retractedInfo = NotNullInfo(Set(), retracted)

        /** The sequential combination with another not-null info */
        def seq(that: NotNullInfo): NotNullInfo =
            if this.isEmpty then that
            else if that.isEmpty then this
            else NotNullInfo(
                this.asserted.union(that.asserted).diff(that.retracted),
                this.retracted.union(that.retracted).diff(that.asserted))

        /** The alternative path combination with another not-null info. Used to merge
        *    the nullability info of the two branches of an if.
        */
        def alt(that: NotNullInfo): NotNullInfo =
            NotNullInfo(this.asserted.intersect(that.asserted), this.retracted.union(that.retracted))

    object NotNullInfo:
        val empty = new NotNullInfo(Set(), Set())
        def apply(asserted: Set[TermRef], retracted: Set[TermRef]): NotNullInfo =
            if asserted.isEmpty && retracted.isEmpty then empty
            else new NotNullInfo(asserted, retracted)
    end NotNullInfo

    /** A pair of not-null sets, depending on whether a condition is `true` or `false` */
    case class NotNullConditional(ifTrue: Set[TermRef], ifFalse: Set[TermRef]):
        def isEmpty = this eq NotNullConditional.empty

    object NotNullConditional:
        val empty = new NotNullConditional(Set(), Set())
        def apply(ifTrue: Set[TermRef], ifFalse: Set[TermRef]): NotNullConditional =
            if ifTrue.isEmpty && ifFalse.isEmpty then empty
            else new NotNullConditional(ifTrue, ifFalse)
    end NotNullConditional

    /** An attachment that represents conditional flow facts established
    *    by this tree, which represents a condition.
    */
    private[typer] val NNConditional = Property.StickyKey[NotNullConditional]

    /** An attachment that represents unconditional flow facts established
        *    by this tree.
        */
    private[typer] val NNInfo = Property.StickyKey[NotNullInfo]

    /** An extractor for null comparisons */
    object CompareNull:

        /** Matches one of
        *
        *        tree == null, tree eq null, null == tree, null eq tree
        *        tree != null, tree ne null, null != tree, null ne tree
        *
        *    The second boolean result is true for equality tests, false for inequality tests
        */
        def unapply(tree: Tree)(using Context): Option[(Tree, Boolean)] =
            tree match
            case Apply(Select(l, _), Literal(Constant(null)) :: Nil) =>
                testSym(tree.symbol, l)
            case Apply(Select(Literal(Constant(null)), _), r :: Nil) =>
                testSym(tree.symbol, r)
            case _ =>
                None

        private def testSym(sym: Symbol, operand: Tree)(using Context) =
            if sym == defn.Any_== || sym == defn.Object_eq then Some((operand, true))
            else if sym == defn.Any_!= || sym == defn.Object_ne then Some((operand, false))
            else None

    end CompareNull

    /** An extractor for null-trackable references */
    object TrackedRef:
        def unapply(tree: Tree)(using Context): Option[TermRef] =
            tree.typeOpt match
            case ref: TermRef if isTracked(ref) => Some(ref)
            case _ => None
    end TrackedRef

    def isTracked(ref: TermRef)(using Context) =
        ref.isStable
        || {
            val sym = ref.symbol
            !ref.usedOutOfOrder
            && sym.span.exists
            && ctx.compilationUnit != null // could be null under -Ytest-pickler
            && ctx.compilationUnit.assignmentSpans.contains(sym.span.start)
        }

    /** The nullability context to be used after a case that matches pattern `pat`.
    *    If `pat` is `null`, this will assert that the selector `sel` is not null afterwards.
    */
    def afterPatternContext(sel: Tree, pat: Tree)(using Context) =
        (sel, pat) match
        case (TrackedRef(ref), Literal(Constant(null))) => ctx.addNotNullRefs(Set(ref))
        case _ => ctx

    /** The nullability context to be used for the guard and rhs of a case with
    *    given pattern `pat`. If the pattern can only match non-null values, this
    *    will assert that the selector `sel` is not null in these regions.
    */
    def caseContext(sel: Tree, pat: Tree)(using Context): Context =
        sel match
        case TrackedRef(ref) if matchesNotNull(pat) => ctx.addNotNullRefs(Set(ref))
        case _ => ctx

    private def matchesNotNull(pat: Tree)(using Context): Boolean =
        pat match
        case _: Typed | _: UnApply => true
        case Alternative(pats) => pats.forall(matchesNotNull)
        // TODO: Add constant pattern if the constant type is not nullable
        case _ => false

    extension (infos: List[NotNullInfo])

        /** Do the current not-null infos imply that `ref` is not null?
        *    Not-null infos are as a history where earlier assertions and retractions replace
        *    later ones (i.e. it records the assignment history in reverse, with most recent first)
        */
        @tailrec def impliesNotNull(ref: TermRef): Boolean =
            infos match
            case info :: infos1 =>
                if info.asserted.contains(ref) then true
                else if info.retracted.contains(ref) then false
                else infos1.impliesNotNull(ref)
            case _ =>
                false

        /** Add `info` as the most recent entry to the list of null infos. Assertions
        *    or retractions in `info` supersede infos in existing entries of `infos`.
        */
        def extendWith(info: NotNullInfo) =
            if info.isEmpty
                || info.asserted.forall(infos.impliesNotNull(_))
                    && !info.retracted.exists(infos.impliesNotNull(_))
            then infos
            else info :: infos

        /** Retract all references to mutable variables */
        def retractMutables(using Context) =
            val mutables = infos.foldLeft(Set[TermRef]())((ms, info) =>
                ms.union(info.asserted.filter(_.symbol.is(Mutable))))
            infos.extendWith(NotNullInfo(Set(), mutables))

    end extension

    extension (ref: TermRef)
        def usedOutOfOrder(using Context): Boolean =
            val refSym = ref.symbol
            val refOwner = refSym.owner

            @tailrec def recur(s: Symbol): Boolean =
                s != NoSymbol
                && s != refOwner
                && (s.isOneOf(Lazy | Method) // not at the rhs of lazy ValDef or in a method (or lambda)
                    || s.isClass // not in a class
                            // TODO: need to check by-name parameter
                    || recur(s.owner)
                    )

            refSym.is(Mutable) // if it is immutable, we don't need to check the rest conditions
            && refOwner.isTerm
            && recur(ctx.owner)
    end extension

    extension (tree: Tree)

        /* The `tree` with added nullability attachment */
        def withNotNullInfo(info: NotNullInfo): tree.type =
            if !info.isEmpty then tree.putAttachment(NNInfo, info)
            tree

        /* The nullability info of `tree` */
        def notNullInfo(using Context): NotNullInfo =
            stripInlined(tree).getAttachment(NNInfo) match
            case Some(info) if !ctx.erasedTypes => info
            case _ => NotNullInfo.empty

        /* The nullability info of `tree`, assuming it is a condition that evaluates to `c` */
        def notNullInfoIf(c: Boolean)(using Context): NotNullInfo =
            val cond = tree.notNullConditional
            if cond.isEmpty then tree.notNullInfo
            else tree.notNullInfo.seq(NotNullInfo(if c then cond.ifTrue else cond.ifFalse, Set()))

        /** The paths that are known to be not null if the condition represented
        *    by `tree` yields `true` or `false`. Two empty sets if `tree` is not
        *    a condition.
        */
        def notNullConditional(using Context): NotNullConditional =
            stripBlock(tree).getAttachment(NNConditional) match
                case Some(cond) if !ctx.erasedTypes => cond
                case _ => NotNullConditional.empty

        /** The current context augmented with nullability information of `tree` */
        def nullableContext(using Context): Context =
            val info = tree.notNullInfo
            if info.isEmpty then ctx else ctx.addNotNullInfo(info)

        /** The current context augmented with nullability information,
        *    assuming the result of the condition represented by `tree` is the same as
        *    the value of `c`.
        */
        def nullableContextIf(c: Boolean)(using Context): Context =
            val info = tree.notNullInfoIf(c)
            if info.isEmpty then ctx else ctx.addNotNullInfo(info)

        /** The context to use for the arguments of the function represented by `tree`.
        *    This is the current context, augmented with nullability information
        *    of the left argument, if the application is a boolean `&&` or `||`.
        */
        def nullableInArgContext(using Context): Context = tree match
            case Select(x, _) if !ctx.erasedTypes =>
                if tree.symbol == defn.Boolean_&& then x.nullableContextIf(true)
                else if tree.symbol == defn.Boolean_|| then x.nullableContextIf(false)
                else ctx
            case _ => ctx
    end extension
end Nullables

To my eyes, there’s a clear winner: tabsize 3. tabsize 2 is a bit too small, and tabsize 4 feels clunky. But 3 is perfect. There are also some specific reasons why 3 is better than 4, namely:

  • it does not align with def or val.
  • it does align with if and && and ||.

You have to see for yourself why that makes a difference.

That only leaves the problem that tabsize 3 will be even harder to establish than 4 since it is so uncommon…

7 Likes
284

I can live with 3 spaces, and agree with that it’s easier to read. It’s wider (bad) but easier to read (good).

I don’t mind it’s uncommonness - if its stated the preferred width I think it can be established.

But I would not myself want to use 4 spaces, even if it was the officially preferred width.

3 Likes