search

abeln / dotty

Scala with explicit nulls
https://github.com/abeln/dotty/wiki/scala-with-explicit-nulls
Other
23 stars 2 forks source link

Merge PR Branch to Migration Branch #36

Closed noti0na1 closed 5 years ago

noti0na1 commented 5 years ago

The new flag and NotNull list are rewritten in new style.

I made one change to the behavior of the NotNull list. I will comment it below.

We should merge #35 first and update this branch.

abeln commented 5 years ago

35 is now merged

noti0na1 commented 5 years ago

@abeln I select several projects to test (scalacheck, scalatest, betterfile). The error counts are all different. Error counts (checker framework metadata) are slightly higher (except scalatest which has a lot more errors now 46 -> 226). The Error counts (java-interop-dont-nullify-outermost) are slightly lower. I also tested the new project: intent, it has 10 and 5 errors without and with the flag respectively.

abeln commented 5 years ago

@noti0na1 thanks for checking

Could you please do the following 1) Pick any checker framework project (preferably one with fewer errors) 2) Run it with the branch before this PR and record the errors 3) Run it with the branch after this PR and record the errors 4) As per your message, there should be more errors in 3) than in 2)

What we want to understand is whether there are more errors because a problem in the merge, or because a problem in the previous branch (which we already suspect to be the case because of the error you fix in JavaNullInterop).

If you could look into that, that would be great. If you could send me the error logs for 2) and 3), I can take a look as well.

Thanks!

cc @olhotak

noti0na1 commented 5 years ago

@abeln I just found another potential bug in JavaNullMap of JavaNullInterop.

def needsNullArgs(tp: AppliedType): Boolean = !tp.classSymbol.is(JavaDefined)

// map
case appTp @ AppliedType(tycons, targs) =>
  val oldOutermostNullable = outermostLevelAlreadyNullable
  outermostLevelAlreadyNullable = false
  val targs2 = if (needsNullArgs(appTp)) targs map this else targs
  outermostLevelAlreadyNullable = oldOutermostNullable
  val appTp2 = derivedAppliedType(appTp, tycons, targs2)
  if (needsNull(tycons)) toJavaNullableUnion(appTp2) else appTp2

When nullify List[Array[String]], the result would be List[Array[String]], since List is Java Defined and needsNullArgs(appTp) == false. But I think a correct answer should be List[Array[String | JavaNull]].

A fix:

case appTp @ AppliedType(tycons, targs) =>
  val oldOutermostNullable = outermostLevelAlreadyNullable
  // change this two lines
  outermostLevelAlreadyNullable = tp.classSymbol.is(JavaDefined)
  val targs2 = targs map this
  outermostLevelAlreadyNullable = oldOutermostNullable
  val appTp2 = derivedAppliedType(appTp, tycons, targs2)
  if (needsNull(tycons)) toJavaNullableUnion(appTp2) else appTp2
abeln commented 5 years ago

@noti0na1 good find! Yes, I agree that we have a bug, and I agree with your fix. Could you please submit that fix as a separate PR to the explicit nulls branch. Could you please add your test as well as a few more that exercise increased nestedness levels?

e.g.

List[Array[List[Array[String]]]] => List[Array[List[Array[String|JavaNull]]|JavaNull]]|JavaNull

Thanks!

abeln commented 5 years ago

This will need to be updated after https://github.com/abeln/dotty/pull/37 is merged, right?

noti0na1 commented 5 years ago

@abeln updated

abeln commented 5 years ago

Did you do a full test run for this? Do all tests pass?

noti0na1 commented 5 years ago

@abeln Yes, all tests pass

abeln commented 5 years ago

Great! Thanks for doing this. When you have a chance, could you please do a full run of the community build and update the spreadsheet? We're mainly interested in the errors counts with the don't-nullify-java-types flag enabled.