Kotlin code with nullable fields in data classes breaks SimplifyBooleanExpression

[ZzAve]

What version of OpenRewrite are you using?

I am using

• OpenRewrite v8.27.4
• rewrite-kotlin v1.18-0-SNAPSHOT

How are you running OpenRewrite?

I've added a test to my fork of this repository that shows the issue
00d7a3e

What is the smallest, simplest way to reproduce the problem?

data class Todo(val completed: Boolean?)
fun main() {
  val todo = Todo(null) 
  val isCompleted: Boolean = todo.completed == true
}

What did you expect to see?

No changes

data class Todo(val completed: Boolean?)
fun main() {
  val todo = Todo(null) 
  val isCompleted: Boolean = todo.completed == true
}

What did you see instead?

data class Todo(val completed: Boolean?)
fun main() {
  val todo = Todo(null)
   val isCompleted: Boolean = todo.completed
}

What is the full stack trace of any errors you encountered?

[Expected recipe to complete in 0 cycles, but took at least one more cycle. Between the last two executed cycles there were changes to "openRewriteFile.kt"] 
expected: 
  "data class Todo(val completed: Boolean?)
  fun main() {
      val todo = Todo(null)
      val isCompleted: Boolean = todo.completed == true
  }"
 but was: 
  "data class Todo(val completed: Boolean?)
  fun main() {
      val todo = Todo(null)
      val isCompleted: Boolean = todo.completed
  }"
<Click to see difference>

org.opentest4j.AssertionFailedError: [Expected recipe to complete in 0 cycles, but took at least one more cycle. Between the last two executed cycles there were changes to "openRewriteFile.kt"] 
expected: 
  "data class Todo(val completed: Boolean?)
  fun main() {
      val todo = Todo(null)
      val isCompleted: Boolean = todo.completed == true
  }"
 but was: 
  "data class Todo(val completed: Boolean?)
  fun main() {
      val todo = Todo(null)
      val isCompleted: Boolean = todo.completed
  }"
	at org.openrewrite.test.LargeSourceSetCheckingExpectedCycles.afterCycle(LargeSourceSetCheckingExpectedCycles.java:96)
	at org.openrewrite.RecipeScheduler.runRecipeCycles(RecipeScheduler.java:97)
	at org.openrewrite.RecipeScheduler.scheduleRun(RecipeScheduler.java:41)
	at org.openrewrite.Recipe.run(Recipe.java:340)
	at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:375)
	at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:133)
	at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:128)
	at org.openrewrite.staticanalysis.kotlin.SimplifyBooleanExpressionTest.nullableVariable2(SimplifyBooleanExpressionTest.java:116)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

Are you interested in contributing a fix to OpenRewrite?

I'm happy to fiddle around trying to see if I can fix the issue myself.

[timtebeek]

Thanks for the clear report & good seeing you're exploring OpenRewrite some more. Feel free to open a draft PR if you're exploring a fix as well. Always great to have runnable examples of what should work.

[ZzAve]

I have quite a hard time to be honest getting the nullability information from the LST. I was hoping I could find some of it in the markers, but alas. Might this be related to openrewrite/rewrite#3221 .

IIUC, with nullability information, I think the SimplifyBooleanExpression.java#shouldSimplifyEqualsOn can be textended to

            // Comparing Kotlin nullable type `?` with true/false can not be simplified,
            // e.g. `X?.fun() == true` is not equivalent to `X?.fun()`
            @Override
            protected boolean shouldSimplifyEqualsOn(@Nullable J j) {
                if (j == null) {
                    return true;
                }
                System.out.println(TreeVisitingPrinter.printTree(getCursor()));
                if (j instanceof J.MethodInvocation) {
                    J.MethodInvocation m = (J.MethodInvocation) j;
                    return !m.getMarkers().findFirst(IsNullSafe.class).isPresent();
                }

                if (j instanceof J.FieldAccess) {
                    J.FieldAccess f = (J.FieldAccess) j;
                    return !f.getMarkers().findFirst(IsNullable.class).isPresent();
                }

                return true;
            }

If you have others ideas on how this could be solved; I'm open to suggestions!

[timtebeek]

Had a look into this one; I think we might need to dive into rewrite-kotlin; here's a test I've prepped for FieldAccessTest there.

@Test
void dataClassFieldAccessRetainsNullableMarker() {
    // language=kotlin
    rewriteRun(
      spec -> spec.recipe(toRecipe(() -> new KotlinIsoVisitor<>() {
          @Override
          public J.FieldAccess visitFieldAccess(J.FieldAccess fieldAccess, ExecutionContext ctx) {
              if ("completed".equals(fieldAccess.getSimpleName())) {
                  // TODO Type is not nullable, but field is nullable
              }
              return super.visitFieldAccess(fieldAccess, ctx);
          }
      })),
      kotlin(
        """
          data class Todo(val completed: Boolean?)
          fun main() {
            val todo = Todo(null)
            val isCompleted = todo.completed
          }
          """
      )
    );
}

I can't immediately see an indication that the accessed field should be of the nullable Boolean type. Perhaps we should log this as an issue there , unless I missed anything. Any quick insights @knutwannheden ?

Using the above I've traced it to this point where we're not assigning the nullable Boolean type.
https://github.com/openrewrite/rewrite-kotlin/blob/b26d6faeb9069bb70b3f61f1d64d53a13dbdad2c/src/main/java/org/openrewrite/kotlin/internal/KotlinTreeParserVisitor.java#L2530-L2539