scala3 icon indicating copy to clipboard operation
scala3 copied to clipboard

Pain points for explicit null checking

Open odersky opened this issue 2 years ago • 9 comments

Compiler version

3.1.3 RC1 with -Yexplicit-nulls

Minimized example

When reviewing #14032, I observed some pain points for explicit null checking that we might be able to fix. In decreasing order of importance:

  1. Flow typing does not extend to variables of enclosing methods or fields of enclosing classes. It's clearly more difficult to know when a nullability info needs to be invalidated in these cases. But there is one situation, which happens to be the most common one, where I think we should be able to check nullability: It's when we know that a variable is initially null and every assignment to the variable is a non-null value. In that case, there's nothing to invalidate: once we know a variable is non-null at some point, it will stay non-null forever. If we could exploit that knowledge, the vast majority of caches in dotty could be null-checked without needing .nn.

    The tricky bit is sequencing. To know that all assignments are non-null we need to type check the program. But that means we might have to type check dereferences to a nullable variable first. We could try to find a better phase ordering. Maybe delay flow-typing until after typer. During type checking, always proceed under the unsafeNulls assumption. During subsequent flow typing, remove inserted .nn calls if they are redundant according to flow typing rules. If any inserted .nn calls remain, issue errors unless unsafeNulls is imported.

  2. Calls to Java methods. Super annoying to have to write System.err.nn.println(...) or str.substring(a, b).nn. We should consider erring more towards unsoundness here.

  3. eq, and ne should also work for nullable types. We create a lot of complexity in comparisons since that's currently not the case.

odersky avatar Mar 05 '22 14:03 odersky

eq and ne were discussed heavily here: https://contributors.scala-lang.org/t/wip-scala-with-explicit-nulls/2761 The suggestion there was to use == and != instead, which are defined on Any, while eq and ne are defined only on AnyRef (and a null is not an AnyRef).

olhotak avatar Mar 05 '22 18:03 olhotak

There's a semantic difference between == and eq so I don't see how that suggestion could make sense. eq and ne clearly have to work on nulls as well. I realize this is tricky since eq right now is a member of AnyRef, but nevertheless we have to make it work.

odersky avatar Mar 05 '22 21:03 odersky

Can we simply add this to Predef.scala?

extension (inline x: AnyRef | Null)
  inline def eq(inline y: AnyRef | Null): Boolean =
    x.asInstanceOf[AnyRef] eq y.asInstanceOf[AnyRef]
  inline def ne(inline y: AnyRef | Null): Boolean =
    !(x eq y)

noti0na1 avatar Mar 06 '22 07:03 noti0na1

Can we simply add this to Predef.scala?

I think this should work.

odersky avatar Mar 06 '22 09:03 odersky

On point 1, if we never assign null to a variable, could we make its type non-nullable and initialize it to uninitialized instead of to null? We would not be allowed to compare it to null (to test dynamically whether it is still uninitialized), but perhaps we can find or add a workaround. Maybe if x == uninitialized instead of if x == null, but that might be adding too much magic, proliferating uninitialized a bit too much.

olhotak avatar Mar 07 '22 08:03 olhotak

Here's another possible solution for 1. The idea is to use an explicit type MonotoneNull to state that we might read null from a field but we cannot write null into it. The flow typing would then take advantage of that.

type MonotoneNull <: Null
val initialNull = null.asInstanceOf[MonotoneNull]

class C {
  private var field: String | MonotoneNull = initialNull
  if field == null then field = "foo" // OK
  field = null // error
} 

It's possible to implement initialNull without the cast with some additional complexity that could be put in a library. The cast is here to keep it simple and communicate the idea.

olhotak avatar Mar 08 '22 06:03 olhotak

That looks nice. You could also call that type Uninitialized: var field: String | Uninitialized. I think it would convey your semantics to many readers.

dwijnand avatar Mar 08 '22 09:03 dwijnand

IIUC, this is about the following pattern:

  private var myAssignmentSpans: Map[Int, List[Span]] | Null = null

  def assignmentSpans(using Context): Map[Int, List[Span]] =
    if myAssignmentSpans == null then myAssignmentSpans = Nullables.assignmentSpans
    myAssignmentSpans.nn

?

Because if that's the case, there's a non-language solution to this, that will even reduce boilerplate:

// define once:
inline def initIfNull[A](value: A | Null)(inline set: A => Unit)(inline init: => A): A =
  if value == null then
    val initValue = init
    set(initValue)
    initValue
  else
    value

// then use at will:
  def assignmentSpans(using Context): Map[Int, List[Span]] =
    initIfNull(myAssignmentSpans)(myAssignmentSpans = _)(Nullables.assignmentSpans)

sjrd avatar May 10 '22 11:05 sjrd

I just had a unit test fail in CI where bootstrapped implies explicit nulls, because of methods on String considered nullable.

It would be handy to patch return types of methods on String as Nonnull or NotNull.

I see s.substring(0) is mentioned in the comment. But "super annoying" is probably an understatement.

som-snytt avatar Sep 11 '22 21:09 som-snytt

In my own first experiments with getting some Scala 3 code of my own to compile under -Yexplicit-nulls, I found by far the biggest pain point to be Martin's point 2, "Calls to Java methods."

Currently I have to insert .nn over and over again when calling methods (often from the Java standard library) that I don't control and thus can't annotate as null-safe. For the Java standard library, perhaps the Scala 3 compiler could have a built-in list of Java stdlib methods that never return null. But also, perhaps the compiler could read a config file of my own that specifies additional methods (or even entire classes?) that I know to be null-safe.

Without improvements in this area, the feature is really very painful to enable on much ordinary Scala code, IMO.

SethTisue avatar Oct 21 '22 09:10 SethTisue

Another pain point I hit over and over again that I don't think has been mentioned before is:

I routinely get irrelevant errors after the first nullability-related failure. For example, when I write (simplified from actual code):

Iterator(" foo bar  ", " baz qux").map(_.trim.split(' ')).toArray

I get two errors:

scala> Iterator("foo bar", "baz qux").map(_.trim.split(' ')).toArray
-- [E008] Not Found Error: -----------------------------------------------------
1 |Iterator("foo bar", "baz qux").map(_.trim.split(' ')).toArray
  |                                   ^^^^^^^^^^^^
  |        value split is not a member of String | Null.
  |        Since explicit-nulls is enabled, the selection is rejected because
  |        String | Null could be null at runtime.
  |        If you want to select split without checking for a null value,
  |        insert a .nn before .split or import scala.language.unsafeNulls.
-- Error: ----------------------------------------------------------------------
1 |Iterator("foo bar", "baz qux").map(_.trim.split(' ')).toArray
  |                                                             ^
  |                                            No ClassTag available for B
  |
  |                                            where:    B is a type variable
2 errors found

The first error is desired, but the second one is unhelpful.

After issuing a nullability error, could the compiler pretend I added .nn before proceeding? Then such unhelpful follow-up errors would be avoided.

SethTisue avatar Oct 21 '22 09:10 SethTisue

But also, perhaps the compiler could read a config file of my own that specifies additional methods (or even entire classes?) that I know to be null-safe.

https://github.com/scala/scala/pull/9447 would be a good fit for that. To include the non-null annotations in a project one would only need to add a library dependency to the build.

lrytz avatar Oct 21 '22 10:10 lrytz