Skip to content
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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 3, 2023
Merged

Change method to work with explicit nulls #5802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
157bbe9
Change method to work with explicit nulls
rochala Oct 31, 2023
fe3948d
Add flags to check for unsafeNullsCompatibility
rochala Nov 2, 2023
9c407bc
Run it on latest LTS
rochala Nov 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions build.sbt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,11 @@ lazy val mtagsShared = project
.filterNot(isScala3WithPresentationCompiler),
crossVersion := CrossVersion.full,
Compile / packageSrc / publishArtifact := true,
Compile / scalacOptions ++= {
if (scalaVersion.value == V.scala3)
List("-Yexplicit-nulls", "-language:unsafeNulls")
else Nil
},
libraryDependencies ++= List(
"org.lz4" % "lz4-java" % "1.8.0",
"com.google.protobuf" % "protobuf-java" % "3.24.3",
Expand Down
28 changes: 13 additions & 15 deletions mtags-shared/src/main/scala/scala/meta/internal/mtags/CommonMtagsEnrichments.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,21 +42,19 @@ trait CommonMtagsEnrichments {
cls: java.lang.Class[T],
gson: Option[Gson] = None
): Option[T] =
for {
data <- Option(obj)
value <-
try {
Option(
gson
.getOrElse(new Gson())
.fromJson[T](data.asInstanceOf[JsonElement], cls)
)
} catch {
case NonFatal(e) =>
logger.log(Level.SEVERE, s"decode error: $cls", e)
None
}
} yield value
Option(obj).flatMap { data =>
Copy link
Contributor

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

Copy link
Collaborator Author

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.

Copy link
Contributor

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?

Copy link
Collaborator Author

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

Copy link
Contributor

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?

Copy link
Collaborator Author

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.

try {
Option(
gson
.getOrElse(new Gson())
.fromJson[T](data.asInstanceOf[JsonElement], cls)
)
} catch {
case NonFatal(e) =>
logger.log(Level.SEVERE, s"decode error: $cls", e)
None
}
}

implicit class XtensionJEitherCross[A, B](either: JEither[A, B]) {
def asScala: Either[A, B] =
Expand Down
Loading