overrides check with a protected java methods throws IllegalStateException

[yigit]

override-bug.zip

See attached sample for repro (simply run its tests)

If I have a java class like this:

public class TestClass {
  protected void test() {}
}

And try to check if test overrides Any.equals, it throws IllegalStateException.

// processor
        override fun process(resolver: Resolver) {
            val testClass = resolver.getClassDeclarationByName(
                resolver.getKSNameFromString("TestClass")
            )!!
            val any = resolver.getClassDeclarationByName(
                resolver.getKSNameFromString("kotlin.Any")
            )!!
            val equalsMethod = any.getAllFunctions().first {
                it.simpleName.asString() == "equals"
            }
            val testMethod = testClass.getDeclaredFunctions().first {
                it.simpleName.asString() == "test"
            }
            check(testMethod.overrides(equalsMethod) == false) { // << throws
                "should not override"
            }
        }

e: java.lang.IllegalStateException: unhandled visibility: protected/*protected and package*/
	at org.jetbrains.kotlin.ksp.symbol.impl.UtilsKt.toKSModifiers(utils.kt:122)
	at org.jetbrains.kotlin.ksp.symbol.impl.binary.KSFunctionDeclarationDescriptorImpl$modifiers$2.invoke(KSFunctionDeclarationDescriptorImpl.kt:101)
	at org.jetbrains.kotlin.ksp.symbol.impl.binary.KSFunctionDeclarationDescriptorImpl$modifiers$2.invoke(KSFunctionDeclarationDescriptorImpl.kt:22)
	at kotlin.SynchronizedLazyImpl.getValue(LazyJVM.kt:74)
	at org.jetbrains.kotlin.ksp.symbol.impl.binary.KSFunctionDeclarationDescriptorImpl.getModifiers(KSFunctionDeclarationDescriptorImpl.kt)
	at org.jetbrains.kotlin.ksp.symbol.impl.binary.KSFunctionDeclarationDescriptorImpl.overrides(KSFunctionDeclarationDescriptorImpl.kt:47)

[yigit]

Seems like it has nothing to do w/ Any or equals.
If the overriding method has protected, it fails.

e.g. with the following code:

        val source1 = SourceFile.java("TestClass.java", """
            public class TestClass extends Base {
                protected void test() {}
            }
        """.trimIndent())
        val base = SourceFile.java("Base.java", """
            public class BaseClass {
                protected void test() {}
            }
        """.trimIndent())

Checking TestClass.test overrides BaseClass.test also throws.
If I change the protected in TestClass to public, it works fine.

[yigit]

some more information.

Seems like it is an issue specific to KSFunctionDeclarationDescriptorImpl while it works fine with KSFunctionDeclarationJavaImpl.

More specifically, if I get a class via resolver.getClassDeclarationByName, it returns KSClassDeclarationDescriptorImpl whereas if i get it via a kotlin subclass (symbol.superTypes.first().resolve()?.declaration), then it returns KSClassDeclarationJavaImpl and it works properly.

So seems like an issue in resolver.
I think the fix is straightforward to extend the check to handle protected but i'm not sure if that'll just hide another bug.
Why does the resolver return two different implementations based on the call site?

[yigit]

I tried extending the javaModifiers test and the value returned from java are a bit different:

C: PUBLIC ABSTRACT
staticStr: PRIVATE
s1: FINAL JAVA_TRANSIENT
i1: PROTECTED JAVA_STATIC JAVA_VOLATILE
intFun: JAVA_SYNCHRONIZED JAVA_DEFAULT
foo: ABSTRACT JAVA_STRICT
abstractVoidMethod: PROTECTED

If I visit the declaration by directly getting it via:

resolver.getClassDeclarationByName(
      resolver.getKSNameFromString("C")
)

it prints:

C: ABSTRACT PUBLIC
abstractVoidMethod: OPEN PROTECTED
hashCode: OPEN PUBLIC OVERRIDE
equals: OPEN PUBLIC OPERATOR OVERRIDE
toString: OPEN PUBLIC OVERRIDE
intFun: OPEN
foo: ABSTRACT
staticStr: FINAL PRIVATE
s1: FINAL
i1: FINAL PROTECTED JAVA_STATIC

In some ways, getting it via resolver seems to be returning a more complete information (e.g. OPEN modifier is added).

[neetopia]

some more information.

Seems like it is an issue specific to KSFunctionDeclarationDescriptorImpl while it works fine with KSFunctionDeclarationJavaImpl.

More specifically, if I get a class via resolver.getClassDeclarationByName, it returns KSClassDeclarationDescriptorImpl whereas if i get it via a kotlin subclass (symbol.superTypes.first().resolve()?.declaration), then it returns KSClassDeclarationJavaImpl and it works properly.

So seems like an issue in resolver.
I think the fix is straightforward to extend the check to handle protected but i'm not sure if that'll just hide another bug.
Why does the resolver return two different implementations based on the call site?

They should always be same implementation, I noticed this issue last week but since the fix was rather small and inside the implementation detail therefore I didn't make it an issue, was planning to address it myself, sorry for the confusion.

[neetopia]

I tried extending the javaModifiers test and the value returned from java are a bit different:

C: PUBLIC ABSTRACT
staticStr: PRIVATE
s1: FINAL JAVA_TRANSIENT
i1: PROTECTED JAVA_STATIC JAVA_VOLATILE
intFun: JAVA_SYNCHRONIZED JAVA_DEFAULT
foo: ABSTRACT JAVA_STRICT
abstractVoidMethod: PROTECTED

If I visit the declaration by directly getting it via:

resolver.getClassDeclarationByName(
      resolver.getKSNameFromString("C")
)

it prints:

C: ABSTRACT PUBLIC
abstractVoidMethod: OPEN PROTECTED
hashCode: OPEN PUBLIC OVERRIDE
equals: OPEN PUBLIC OPERATOR OVERRIDE
toString: OPEN PUBLIC OVERRIDE
intFun: OPEN
foo: ABSTRACT
staticStr: FINAL PRIVATE
s1: FINAL
i1: FINAL PROTECTED JAVA_STATIC

In some ways, getting it via resolver seems to be returning a more complete information (e.g. OPEN modifier is added).

It's true that descriptors contains most detailed information as they are fully resolved, but they are not representing the original code structure, like the implicit OPEN for Java symbols, and hence it will be nice if users won't rely on this buggy behavior of getClassDeclarationByName always returns descriptor based implementation.

[yigit]

i think both are probably fine from developer's perspective as long as it is consistent. but when they are inconsistent, it will be really hard to workaround as most of the code in the symbol processor does not necessarily know how the KSClassDeclaration was acquired.

[neetopia]

i think both are probably fine from developer's perspective as long as it is consistent. but when they are inconsistent, it will be really hard to workaround as most of the code in the symbol processor does not necessarily know how the KSClassDeclaration was acquired.

If consistent mean .isOpen() will return always return true for an open class regardless of JavaImpl or DescriptorImpl, they are consistent by our design. We are making modifiers only include explicit modifier just to make sure there is a way to still inspect code. Developers should look at utility functions like isOpen rather than relying on modifiers which can differ by platform.

[yigit]

I agree with that.
actually i was leaning for the same thing as i'm prototyping the abstraction for room.
I found some places where we unnecessarily access elements APIs. Trying to guard them beyond an abstraction to avoid modelling too much of the elements API.

[neetopia]

android/kotlin#51 includes a fix to this reproduce project.