-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Migrating the main compiler to explicit nulls #14032
Conversation
76f341c
to
319e445
Compare
|
||
/** A map from (name-) offsets of all local variables in this compilation unit | ||
* that can be tracked for being not null to the list of spans of assignments | ||
* to these variables. | ||
*/ | ||
def assignmentSpans(using Context): Map[Int, List[Span]] = | ||
if myAssignmentSpans == null then myAssignmentSpans = Nullables.assignmentSpans | ||
myAssignmentSpans | ||
myAssignmentSpans.nn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this is needed because the flow typing only applies for local variables, not for class fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Perhaps we could do something for private
fields, but it would still be unsound in the presence of threads.
An alternative would be
if myAssignmentSpans == null then
val ret = Nullables.assignmentSpans
myAssignmentSpans = ret
ret
else
myAssignmentSpans
But that's more verbose than the .nn
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could try this one
import scala.util.chaining.*
if myAssignmentSpans == null then Nullables.assignmentSpans.tap(myAssignmentSpans = _)
else myAssignmentSpans
One of the difficulty during migration is: some fields are treated as non-nullable at a lot of places, but they are compared with null value at several locations (for detecting uninitialization or invalid value). One example is However, adding a default value will not always work. For types of symbols, Some examples are:
|
f0e4a8e
to
fc1eca4
Compare
Currently there are some errors in
It seems the difference is caused by |
fc1eca4
to
cd0dbd7
Compare
@smarter Do you have any idea about the pickling difference errors? Thanks |
No precise idea, I think you'd have to minimize it to figure it out: somehow an expression of type TermSymbol has its type dealiased before pickling but on unpickling the recomputed type preserves the alias. Usually we try to preserve type aliases but there is no guarantee we do so if we can't figure this out we could also change the pickling tests to normalize by always dealiasing (we already do various normalizations like this in https://github.com/lampepfl/dotty/blob/2849aed4b1a14755a5fde3ea417f4b8bfa16685f/compiler/src/dotty/tools/dotc/printing/PlainPrinter.scala#L48) |
366bea8
to
b9c7e75
Compare
@smarter I can reproduce the error with only mutable variable and
class Test {
var x: String = null
def f = x.nn
} I add a simple test in @Test def myPicklingTest: Unit = {
implicit val testGroup: TestGroup = TestGroup("testPicklingWithCompiler")
aggregateTests(
compileFile("compiler/src/dotty/tools/Test.scala", picklingWithCompilerOptions)
).limitThreads(4).checkCompile()
}
We don't have the error when x is |
b632188
to
0d52503
Compare
This is likely because the type seen before pickling contains a skolem (when I tweak the printer to not strip prefix and print after posttyper I see |
@@ -303,7 +306,7 @@ object Implicits: | |||
*/ | |||
class ContextualImplicits( | |||
val refs: List[ImplicitRef], | |||
val outerImplicits: ContextualImplicits, | |||
val outerImplicits: ContextualImplicits | Null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this ever null other than for NoContext? If not, is there anything cleaner we could do?
29148d2
to
aa39739
Compare
2f8d297
to
e08d48b
Compare
4b64eae
to
48eba1c
Compare
test performance please |
performance test scheduled: 1 job(s) in queue, 0 running. |
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/14032/ to see the changes. Benchmarks is based on merging with main (29f9d33) |
a393925
to
54b7aa5
Compare
To enable explicit nulls and disable unsafe nulls globally, I think we need to modify some code in scala-js, because the code of scala-js is included during bootstrapped compiling.
(We can simply add |
Hum, but that's not so easy to do. These files still cross-compile all the way back to Scala 2.11.12. So there's no way we can add that import upstream. I see two paths forward:
|
Hi Yaoyu, I am trying to get a clean changeset for the whole PR. Can you rebase over current main? Then I could squash all commits and see all changes together. Thanks! |
ef51a6a
to
451bc85
Compare
@odersky I have rebased and squashed my commits. |
1c72384
to
c53297c
Compare
test performance please |
performance test scheduled: 1 job(s) in queue, 0 running. |
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/14032/ to see the changes. Benchmarks is based on merging with main (727395c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise looking good now. I suggest to apply the final changes, leave notes where this cannot be done, and merge. We can improve the codebase incrementally from there.
I am going to leave some notes about painpoints of null checking in a separate issue.
while e != null do | ||
if isEqual(e, x) then return e | ||
if isEqual(e.uncheckedNN, x) then return e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have thought that flow typing would work here so that uncheckedNN
is not needed? But we can resolve that later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for other uses of uncheckedNN
in HashSet and HashMap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment. We will doule-check every uncheckedNN
once we update the reference compiler.
@@ -338,7 +337,7 @@ object Implicits: | |||
if monitored then record(s"check uncached eligible refs in irefCtx", refs.length) | |||
val ownEligible = filterMatching(tp) | |||
if isOuterMost then ownEligible | |||
else combineEligibles(ownEligible, outerImplicits.uncachedEligible(tp)) | |||
else combineEligibles(ownEligible, outerImplicits.nn.uncachedEligible(tp)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale of using sometimes uncheckedNN
and sometimes nn
on outerImplicits
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used uncheckedNN
in def level
because I know outerImplicits
is non-nullable immediately from the if condition. I should change uncheckedNN
to nn
in other places.
@@ -129,77 +135,71 @@ abstract class WeakHashSet[A <: AnyRef](initialCapacity: Int = 8, loadFactor: Do | |||
tableLoop(0) | |||
} | |||
|
|||
def lookup(elem: A): A | Null = elem match { | |||
case null => throw new NullPointerException("WeakHashSet cannot hold nulls") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was this changed? I think it's better if WeakHashSets don't contains nulls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since A <: AnyRef
, the type can ensure the value is not null
. I can revert the change for now, and add a TODO to delete the case when we can enable explicit nulls for regular compiling.
@odersky Some of the Once the reference compiler is updated, we can drop these |
@odersky The changes have been applied |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look good now! Thanks for all the hard work. This is a big step towards full null checking.
Very cool to see this landing 👏 |
This PR tries to migrating the main compiler to explicit nulls.
Changes from #13975, #13976, and #14411 are included in this PR.
The explicit nulls currently is only enabled for bootstrap compile. We can test the changes using
scala3-bootstrapped/compile
.We enable
unsafeNulls
in files to migrate step by step.The first step of migration is the core parts (core, ast, typer, and tranform). The backend and some other modules are kept with
unsafeNulls
enabled, because they have a lot of interaction with Java libraries.