Confusing type validation error on tests when missing classpath entry

[pettermahlen]

What version of OpenRewrite are you using?

I am using

• openrewrite-test v8.40.3
• openrewrite-kotlin v1.23.1

How are you running OpenRewrite?

As a unit test.

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

  @Test
  fun shouldAddAdditionalMethodToInterface() {
    rewriteRun(
      kotlin("""
        import io.reactivex.rxjava3.core.Observable

        interface MyApi {
          fun myObservable(): Observable<String>
        }
      """.trimIndent(),
        """
          import io.reactivex.rxjava3.core.Observable

          interface MyApi {
              @Deprecated("Use myFlow() instead", ReplaceWith("myFlow()"))
              fun myObservable(): Observable<String>
              fun myFlow(): Flow<String>
          }
        """.trimIndent()),
    )
  }

What did you expect to see?

I expected a test failure, because I haven't implemented the rule in question.

What did you see instead?

A test parsing error.

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

LST contains missing or invalid type information
Identifier->ParameterizedType->MethodDeclaration->Block->ClassDeclaration->CompilationUnit
/*~~(Identifier type is missing or malformed)~~>*/String
java.lang.IllegalStateException: LST contains missing or invalid type information
Identifier->ParameterizedType->MethodDeclaration->Block->ClassDeclaration->CompilationUnit
/*~~(Identifier type is missing or malformed)~~>*/String
	at org.openrewrite.kotlin.Assertions.assertValidTypes(Assertions.java:234)
	at org.openrewrite.kotlin.Assertions.validateTypes(Assertions.java:56)
	at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:303)
	at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:132)
	at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:127)

Changing to

interface MyApi {
          fun myObservable(): Observable<java.lang.String>
}

Makes the problem go away. But that's not exactly reasonable, IMO, since that's not how people write Kotlin (or Java!).

It would be fantastic if the error message could be clearer, as well. Some suggestions for improvements:

1. Specify whether this is in the 'before' or 'after'
2. The line /*~~(Identifier type is missing or malformed)~~>*/String is very hard to understand - the 'String' bit looks almost like it's a bug. Why not simply say something like Identifier type is missing or malformed for type 'String'.
3. Including the place in the file where parsing failed would help; line+column.

[pettermahlen]

I think the problem is somewhere here. When I debug this, the 'String' identifier has a null type, and that's supposedly OK for type parameters, but the 'cursor' at runtime doesn't have a parent that is an instance of J.TypeParameter, but rather an instance of JRightPadded. Its parent in turn is a JContainer, which looks like a list of type parameters, if my guess is correct.

[pettermahlen]

FWIW, it's the same if you change away from a concrete type to a type parameter:

        interface MyApi<T> {
          fun myObservable(): Observable<T>
        }

[knutwannheden]

I think the problem is somewhere here. When I debug this, the 'String' identifier has a null type, and that's supposedly OK for type parameters, but the 'cursor' at runtime doesn't have a parent that is an instance of J.TypeParameter, but rather an instance of JRightPadded. Its parent in turn is a JContainer, which looks like a list of type parameters, if my guess is correct.

That analysis sounds reasonable. The method should be using getParentTreeCursor() rather than getParentCursor() which would skip over those two other nodes.

[pettermahlen]

When I try that, it looks like the value of the parent cursor is an instance of J.ParameterizedType. So the instanceof check would also have to be changed - does that seem reasonable? If so, I can create a PR with a fix.

[knutwannheden]

Yes, that sounds reasonable and a PR would be very welcome! I haven't looked at this code for a long time, but if the tests still all pass (including your new one), then I think this should be correct.

[pettermahlen]

Hm, not that easy, apparently. This test starts failing with that change, as well as a couple of others.

I get the feeling a part of the problem is that Observable is not on the classpath, because if I use List<String> as the return type, everything works. But it seems a little harsh to require adding every class referenced in source code to the classpath the parser uses? Also, I'm not even sure how to do that in a test, it's not clear to me how the classpath-related methods in KotlinParser.Builder are meant to be used. Adding RxJava as an implementation dependency in Gradle doesn't help.

[knutwannheden]

Let's see if @timtebeek can chime in here.

[pettermahlen]

Hm, looks like the classpath hunch was correct. Adding

        spec.parser(KotlinParser.builder().classpath("rxjava"))

removes that parsing issue. So it could be seen as user error, but the feedback isn't great.

[timtebeek]

Had a brief look; agree that it would be nice to differentiate between type validation checks that fail on the before or after source spec. Right now the API we have is fairly minimal, but we could for instance add a label to TypeValidation, which can then be printed along with any type validation issues found here.

rewrite-kotlin/src/main/java/org/openrewrite/kotlin/Assertions.java

Lines 209 to 237 in 0f94397

	private static void assertValidTypes(TypeValidation typeValidation, J sf) {
	if (typeValidation.identifiers() || typeValidation.methodInvocations() || typeValidation.methodDeclarations() || typeValidation.classDeclarations() ||
	typeValidation.constructorInvocations()) {
	List<FindMissingTypes.MissingTypeResult> missingTypeResults = findMissingTypes(sf);
	missingTypeResults = missingTypeResults.stream()
	.filter(missingType -> {
	if (missingType.getJ() instanceof J.Identifier) {
	return typeValidation.identifiers();
	} else if (missingType.getJ() instanceof J.ClassDeclaration) {
	return typeValidation.classDeclarations();
	} else if (missingType.getJ() instanceof J.MethodInvocation || missingType.getJ() instanceof J.MemberReference) {
	return typeValidation.methodInvocations();
	} else if (missingType.getJ() instanceof J.NewClass) {
	return typeValidation.constructorInvocations();
	} else if (missingType.getJ() instanceof J.MethodDeclaration) {
	return typeValidation.methodDeclarations();
	} else if (missingType.getJ() instanceof J.VariableDeclarations.NamedVariable) {
	return typeValidation.variableDeclarations();
	} else {
	return true;
	}
	})
	.collect(Collectors.toList());
	if (!missingTypeResults.isEmpty()) {
	throw new IllegalStateException("LST contains missing or invalid type information\n" + missingTypeResults.stream().map(v -> v.getPath() + "\n" + v.getPrintedTree())
	.collect(Collectors.joining("\n\n")));
	}
	}
	}

It's somewhat curious that you got the type validation on String and not Observable with that missing classpath entry; that would be another thing to fix on those same above lines.

[pettermahlen]

BTW, another thing to look into in this area is the behaviour when using a fully qualified import:

   fun foo(): Observable<java.lang.String>

Then the java.lang.String element is parsed as a FieldAccess (IIRC; it was something like that). Anyway, it wasn't understood to be type information.