Skip to content

Conversation

hamzaremmal
Copy link
Member

@hamzaremmal hamzaremmal commented Jul 18, 2025

The PR enables explicit nulls for stdlib. The detailed review guide is at: #23566 (comment)

@hamzaremmal
Copy link
Member Author

@noti0na1 As agreed to, please push the changes to explicitly null check the stdlib here.

@noti0na1 noti0na1 force-pushed the explicit-nulls-stdlib branch from 2fe2627 to 2d178a7 Compare August 26, 2025 14:17
@noti0na1 noti0na1 marked this pull request as ready for review September 1, 2025 12:20
@noti0na1 noti0na1 requested a review from a team as a code owner September 1, 2025 12:20
@noti0na1 noti0na1 force-pushed the explicit-nulls-stdlib branch from cc174ce to 99df51b Compare September 1, 2025 14:49
@noti0na1
Copy link
Member

noti0na1 commented Sep 1, 2025

@hamzaremmal Do you know how the stdlib is compiled at the first step? When I do scalac, it seems compiling my new code using the old library.

@noti0na1 noti0na1 requested a review from Copilot September 2, 2025 12:04
Copilot

This comment was marked as outdated.

noti0na1 added a commit that referenced this pull request Sep 2, 2025
When we compute `afterPatternContext`, the wrong `ctx` is passed to the
function.

```scala
def f(s: AnyRef | Null) = s match
  case null => 0
  case s: String => s.length
  case _ =>
    val a: AnyRef = s
    a.toString.length
```

Will be useful for #23566
* @return A Scala `Iterator` view of the argument.
*/
def asScala[A](i: ju.Iterator[A]): Iterator[A] = i match {
def asScala[A](i: ju.Iterator[A] | Null): Iterator[A] | Null = i match {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could do some kind of trick like mapNull, with match types, to give asScala a null-polymorphic signature, so that it returns a non-null when you pass it a non-null. That would reduce the need for many .nns not only within the library itself, but probably also in code that uses the library.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to add a match type as shown in @sjrd 's comment. But this means the new type would show up at the type signature, and adding a new public definition to stdlib is impossible at this point...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I should just forbid null like the wrappers wrapRefArray

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forbidding nulls would break the current contract. That's a big no-no.

I also agree that we shouldn't introduce the match type I suggested at this time. We can reconsider in a future release, as we get closer to general adoption of explicit nulls.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue for non-explicit-nulls users, forbidding nulls will not change any contract. It is just a more strict type for explicit-nulls.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would make the body inconsistent with its typing. Constant-folding in the compiler could for example mis-"optimize" x == null as false because the type of x is not nullable. So even non-explicit-nulls users could get affected, indirectly.

In general I don't think the types under explicit-nulls should be any more restrictive than the existing (sometimes tacit 🤷‍♂️) contract. Typing should better describe the contract; not make the contract stricter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have to make a decision here. Of course we will ensure the behaviour of the body will not change, no matter passing null or not. Take asScala as an example:

  1. Keep the original type: def asScala[A](i: ju.Iterator[A]): Iterator[A]. No effect for non-explicit-nulls in terms of typing. Not able to pass null in explicit nulls, more convenient to have a chain of collection operations.
  2. Change the type to: def asScala[A](i: ju.Iterator[A] | Null): Iterator[A] | Null. Still no effect for non-explicit-nulls. More precise to the original behaviour and document. Have to add .nn at more places in explicit-nulls.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After experimenting with the wrapper, I personally think 1 is batter choice for the library.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we disagree, I added this question as an agenda item for tomorrow's core meeting.

@noti0na1 noti0na1 force-pushed the explicit-nulls-stdlib branch from 216877c to 2a854a1 Compare September 8, 2025 12:47
@noti0na1
Copy link
Member

Why does the MiMa check a java class?

Error:  scala-library-nonbootstrapped: Failed binary compatibility check against org.scala-lang:fat-stdlib:3.7.3! Found 2 potential problems (filtered 5132)
Error:   * abstract method get(java.lang.Object)java.lang.Object in class java.util.Dictionary does not have a correspondent in current version
Error:     filter with: ProblemFilters.exclude[DirectAbstractMethodProblem]("java.util.Dictionary.get")
Error:   * abstract method remove(java.lang.Object)java.lang.Object in class java.util.Dictionary does not have a correspondent in current version
Error:     filter with: ProblemFilters.exclude[DirectAbstractMethodProblem]("java.util.Dictionary.remove")
Error:  
Error:  Filters in MiMaFilters.Scala3Library are used in this check.

@sjrd
Copy link
Member

sjrd commented Sep 15, 2025

Why should it not? Java classes are also part of the ABI of our artifacts.

Even tasty-mima checks Java classes!

@noti0na1
Copy link
Member

Why should it not? Java classes are also part of the ABI of our artifacts.

Oh, I thought we only check scala code.

@noti0na1
Copy link
Member

I added java.util.Dictionary to MiMaFilter temporarily, and the check passed.

So I guess at least the stdlib itself is good?

@noti0na1
Copy link
Member

Strange tailrac errors only during scala-library-sjs/compile:

[info] compiling 650 Scala sources and 55 Java sources to /Users/Work/dotty/library-js/target/scala-library/classes ...
[error] -- Error: /Users/Work/dotty/library/src/scala/collection/concurrent/TrieMap.scala:60:26 
[error] 60 |        else GCAS_Complete(/*READ*/mainnode, ct)
[error]    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]    |             Cannot rewrite recursive call: it is not in tail position
[error] -- Error: /Users/Work/dotty/library/src/scala/collection/concurrent/TrieMap.scala:73:28 
[error] 73 |          else GCAS_Complete(m, ct)
[error]    |               ^^^^^^^^^^^^^^^^^^^^
[error]    |               Cannot rewrite recursive call: it is not in tail position
[error] -- Error: /Users/Work/dotty/library/src/scala/collection/concurrent/TrieMap.scala:77:23 
[error] 77 |          GCAS_Complete(/*READ*/mainnode, ct)
[error]    |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]    |          Cannot rewrite recursive call: it is not in tail position
[error] -- Error: /Users/Work/dotty/library/src/scala/collection/concurrent/TrieMap.scala:179:53 
[error] 179 |              if (startgen eq in.gen) in.rec_insertif(k, v, hc, cond, fullEquals, lev + 5, this, startgen, ct)
[error]     |                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]     |             Cannot rewrite recursive call: it is not in tail position
[error] -- Error: /Users/Work/dotty/library/src/scala/collection/concurrent/TrieMap.scala:181:72 
[error] 181 |                if (GCAS(cn, cn.renewed(startgen, ct), ct)) rec_insertif(k, v, hc, cond, fullEquals, lev, parent, startgen, ct)
[error]     |                                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]     |             Cannot rewrite recursive call: it is not in tail position
[error] -- Error: /Users/Work/dotty/library/src/scala/runtime/MethodCache.scala:73:33 
[error] 73 |      case x: PolyMethodCache => x findInternal forReceiver
[error]    |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]    |               Cannot rewrite recursive call: it is not in tail position

The same files was fine when only compiling the stdlib.

@noti0na1
Copy link
Member

Strange tailrac errors only during scala-library-sjs/compile:

It seems there are extra asInstanceOf inserted after the recursive calls, which prevents the tailrec rewriting.
Not sure if this is caused by interaction with union types, or some special handling in scalajs.

@noti0na1
Copy link
Member

As the explicit-null checks for the Scala 3 standard library progress, I’d like to invite everyone to help and perform a deep review of the changes. Please look beyond the diff: review surrounding code and any interactions that could be impacted.

Guidelines I followed during the migration:

  1. Preserve behavior and binary compatibility

    • No logic changes.
    • No new type parameters or changes to public definitions (public API remains unchanged).
  2. Careful use of .nn

    • Add .nn only when the value is logically guaranteed to be non-null at that point; it asserts non-null immediately.
    • It’s easy to overuse; double-check each .nn to ensure it can’t be null in practice.
    • Redundant .nns (e.g., on already non-null types or when passing x.nn to a nullable parameter) should be flagged by warnings and have been cleaned up.
    • If you spot any questionable .nn, please point it out.
  3. Use | Null when logic depends on null

    • Add | Null to a type if the code relies on checking for null: pattern-matching on null, comparing with null, or returning null.
    • If a function calls methods on a value without prior null checks, prefer keeping the type non-nullable.
    • Only Exception: Converters/wrappers like asScala, genericWrapArray. Even though they check for null and may return null, we intentionally keep their signatures non-nullable for now to avoid widespread .nn usage. This decision is to encourage safer non-nullable code in the future (A match-type-based solution could improve this later.)
  4. Mutable fields and initialization with = _

    • If null is used to signal “not initialized”, it’s often clearer to make the field’s type explicitly nullable.
    • If a field is initialized exactly once and/or is set to null only at end for GC, prefer keeping it non-nullable and use null.asInstanceOf[...] at those specific points.
    • Note: Flow typing doesn’t track mutable fields by default. Making them nullable can introduce many .nns. To force enabling flow typing on a mutable field when the prefix is a stable path, use @scala.annotation.stableNull.
  5. Removed T >: Null lower bounds

    • Using T >: Null to allow assigning or returning null is now discouraged.
    • It forces passing nullable types everywhere and interacts poorly with flow typing (e.g., T >: Null <: C | Null).
    • Prefer using T | Null precisely at the sites that need null.
  6. Array[T] issues

    • Array[T] shares the same apply signature for reference and primitive T. While def apply(i: Int): T is fine for primitive arrays, for reference types the slot may be uninitialized and thus return null.
    • If C is a reference type and uninitialized elements can occur, use Array[C | Null].
  7. Avoid language.unsafeNull

    • There is no usage of unsafeNull now.
    • Only use it in very limited scopes (e.g., a single method) when absolutely necessary, and prefer | Null instead.

How you can help:

  • Feel free to start with a focused set of related files or modules and note your selection in the thread so we can cover the codebase efficiently.
  • Please pay special attention to:
    • .nn correctness and necessity,
    • accurate use of | Null,
    • mutable field tracking with @stableNull,
    • preservation of binary compatibility and behavior,
    • Array[T] nullability where reference types are involved.

Thanks in advance for your thorough review!

@noti0na1 noti0na1 requested a review from bracevac September 24, 2025 12:27
@noti0na1 noti0na1 force-pushed the explicit-nulls-stdlib branch from b09ecac to 4678e50 Compare October 6, 2025 13:46
@noti0na1 noti0na1 requested a review from olhotak October 6, 2025 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

-Yexplicit-nulls no longer allows eq and ne comparsion, needs better errors message
8 participants