-
Notifications
You must be signed in to change notification settings - Fork 339
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
Change method to work with explicit nulls #5802
Conversation
None | ||
} | ||
} yield value | ||
Option(obj).flatMap { data => |
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 you explain a bit more why this part was problematic? It seems the same code but without the for comp
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.
import scala.language.unsafeNulls
has some limitations and this is one of it. I think the details can be found here. scala/scala3#18775 (comment)
The issue with for comprehension is that after desugar it runs the yield function in another map so:
for {
x <- Option(test.fromJson[T]())
} yield (x)
gets desugared into
Option.apply(test.fromJson[T]()).map(x => x)
Thus, according to rules and what was described in dotty issue, as last method is not a direct call to Java method but rather an anonymous function x => x
which is basically new symbol, thus the type checking won't be run another time. My changes ensure that Java method is run at the end, thus works with the rules of unsafe 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.
Should we also add explicit nulls here so that we don't have a regression in the future?
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 was thinking on how to do this, but basically it is not so trivial especially as we cross compile it to both 2 and 3, and what is more we use version 2.13 as a "dependency from source" in dotty. Explicit nulls wasn't present in Scala 2 afaik
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.
We do compile it under Scala 3 though, wouldn't it be enough to add it when the version is Scala 3?
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.
Yeah, but then we have to add the import scala.language.unsafeNull
to every file to compile under Scala 3. And this flag is not present in scala 2. There is also another option to add .nn
to every place where explicit nulls is applied but it is also not present for Scala 2.
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.
LGTM!
Change required to work with
-Yexplicit-nulls
according to scala/scala3#18775This is a blocker for adding
-Yexpliict-nulls
in the presentation compiler module that will guarantee soundness. scala/scala3#18776This also may require a minor release.