Fix find overridee, add tests

[yigit]

This CL fixes a few problems in find overridee implementation:

a) If the method does not override anything, it would throw instead of
returning null, fixed.
b) If it overrides a method that overrides another method, it would
return null, now it returns the first actual override.
c) If it overrides a method from a GrandSuper (where super does not
override it), it would return null. Now it returns the method in
GrandSuper class.

Fixes: #150

[neetopia]

My only concern is the use of FAKE_OVERRIDE, I'll verify it with several use cases myself, and if everything looks good I'll merge this one.

[neetopia]

Sorry for delay, I just tried with following test case where I was worrying about use of FAKE_OVERRIDE and it seems to be causing issue.

interface ITF {
    fun openFoo(): Int { return 1}
    fun absFoo(): Unit
}

abstract class ABS: ITF {
    override fun absFoo(): Unit {val a = 1}
    override fun openFoo(): Int { return 2 }
}

class Foo: ABS(), ITF {
    override fun absFoo(): Unit {
        TODO("Not yet implemented")
    }
}

returns null since it got 2 of Kind DECLARATION therefore singleOrNull() check will fail and return null.

See this for repro

[yigit]

awesome, thanks for the repro, i will take a look at it.
To be clear, the desired case is:

Foo.absFoo overrides ABS.absFoo right?

I'm curious for Foo.openFoo because getDeclaredFunctions won't return it. allFunctions will return it which would technically be ABS.openFoo. Given that containing should still be ABS, i would expect overridee to return ITF.openFoo

[yigit]

oh actually it returns null not because of FAKE_OVERRIDE check but because it overrides more than 1 method.

Same problem happens with the following code as well:

interface MyInterface {
    fun absFoo(): Unit
}

interface MyInterface2 {
    fun absFoo(): Unit
}

class Subject2: MyInterface, MyInterface2 {
    override fun absFoo(): Unit = TODO()
}

I think we need to make a decision here.
Either change find Overridee to findOverridees OR have a heuristic on picking the overridee.
The previous discussion of choosing the deepest method won't help here since in the case above, they are in the same level and unrelated to each-other.

The docs for FunctionDescriptor does not clarify anything on the order of the results. Based on my experimentation, it depends on whichever is declared in the code first. (Foo: ABS(), ITF returns ABS.absFoo first, Foo: ITF, ABS() returns ITF.absFoo first.).

I looked around the compiler codebase a bit more and there is actually a overriddenTreeUniqueAsSequence method in DescriptorUtils (https://github.com/JetBrains/kotlin/blob/master/core/descriptors/src/org/jetbrains/kotlin/resolve/DescriptorUtils.kt#L274)

Seems like using that method is more appropriate here. I'll play w/ that one and look for more usages.

sample usage from compiler:
https://github.com/JetBrains/kotlin/blob/c165b8d55c9a4cf62d2d34382fc50fc107bc4c8b/compiler/backend/src/org/jetbrains/kotlin/codegen/ArgumentGenerator.kt#L141

[neetopia]

Yeah, I didn't mean that it is FAKE_OVERRIDE that caused the issue, but rather relying on FAKE_OVERRIDE can run into issues. I am not sure if this can be categorized as diamond issue though.

The other case is quite interesting which fall out of my consideration when implementing this API, let me know how that overriddenTreeUniqueAsSequence work out for you.

[yigit]

so i've been trying to create some use case w/ possible conflicts and see what KAPT generates but i couldn't create anything useful that would compile :(
So in every case, whether overridee is the class or the interface does not make a difference in the final output.

Also, overriddenTreeUniqueAsSequence does not really help because it does in order traversal.

I think for this CL, I'll change it to fix conflicts when there are more than 1 equal-level parent and turn it into true breadth first search and document it. Might also make sense to mark this API as experimental because it is likely that we need to open this API further to be more proper.
I would still really like to get in something as this is still better than the existing version.

[yigit]

another possible choice is to make this function return null if the overridden method is not clear.
that will certainly be more surprising imo but it is a possible temporary solution.

[yigit]

I updated the PR with a BFS version. I think it is also valid to go w/ the null option. In either case, I don't think this function will live for long as it is impossible to make it work w/o opening up multiple overrides case but I would like to get something in before the next release as a temporary improvement.

[neetopia]

LGTM, will merge after #156 is merged.