Skip to content

Pain points for explicit null checking #14622

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

Open
odersky opened this issue Mar 5, 2022 · 12 comments
Open

Pain points for explicit null checking #14622

odersky opened this issue Mar 5, 2022 · 12 comments
Assignees

Comments

@odersky
Copy link
Contributor

odersky commented Mar 5, 2022

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 odersky added stat:needs triage Every issue needs to have an "area" and "itype" label and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Mar 5, 2022
@olhotak
Copy link
Contributor

olhotak commented Mar 5, 2022

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).

@odersky
Copy link
Contributor Author

odersky commented Mar 5, 2022

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.

@noti0na1
Copy link
Member

noti0na1 commented Mar 6, 2022

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)

@odersky
Copy link
Contributor Author

odersky commented Mar 6, 2022

Can we simply add this to Predef.scala?

I think this should work.

@olhotak
Copy link
Contributor

olhotak commented Mar 7, 2022

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
Copy link
Contributor

olhotak commented Mar 8, 2022

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.

@dwijnand
Copy link
Member

dwijnand commented Mar 8, 2022

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

@sjrd
Copy link
Member

sjrd commented May 10, 2022

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)

@som-snytt
Copy link
Contributor

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.

@SethTisue
Copy link
Member

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
Copy link
Member

SethTisue commented Oct 21, 2022

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.

@lrytz
Copy link
Member

lrytz commented Oct 21, 2022

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.

scala/scala#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.

@ckipp01 ckipp01 added the stat:needs triage Every issue needs to have an "area" and "itype" label label May 15, 2023
@nicolasstucki nicolasstucki added area:nullability and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Jul 14, 2023
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

10 participants