Why is shadowed extension not an error?


#1

Suppose we use an external library that defines
class C
Then we write a very simple extension function
fun C.foo = println("extension")

Much later on, a new version of the library comes out which defines
class C { fun foo = println("library") }

The evolution of the library clearly broke my code.
This is normal considering how extensions work,
Still, the compiler and IDE only show warnings, and let you compile something that most of the time won’t work as originally intended.
From my limited experience i can already foresee developpers being careless about warnings or tests and letting this mistake pass through in production with potentially dramatic consequences.

Is there any reason a shadowed extension is just a warning and not an error ?
I can’t see any situation where this would be an intended behaviour, is there any ?


#2

I guess this could be problematic with upcoming libraries like android-ktx - if you would like to add your own method that shadows extension, your only real option would be to remove this library


#3

I’m not sure I understood your point correctly, this seems to be a different situation, where the roles are “reversed” (user is the one adding a method and 3d-party lib is the one making use of an extension) ?

For example if you have a situation where you extend a class that have some extensions implemented somewhere :
open class C
fun C.foo() = println("base class extension")

If I add a method to a derived class C1 :
class C1 : C() { fun foo() = println("member func of derived class") }

Then these calls :
val c1 = C1()
c1.foo()
val c1asC : C = C1()
c1asC.foo()

will print :
member func of derived class
base class extension

This seems legit to me because when you are calling foo on c1asC the method foo does not exists in C. Also you do not break the library that relied on C.foo() as an extension.

On the other hand, if you have the possibility to alter C and make something like :
class C { fun foo() = println("new C method") }

Here, you will definitely break the library behaviour since the library rely on C not having a foo() method for its extension to work.

TL;DR : What I complain about is that the extension developper will only get a warning when the class he added his extension to has changed. I expect sloppy developpers not caring about warnings and letting this kind of otherwise difficult to detect errors go into production. Couldn’t we change the default behaviour of the compiler to an error to avoid this ?

I realize we won’t fix the problem of developpers ignoring warnings or not implementing proper testing but should we not try to limit the problem where we can ?

(Sorry for the long post)


#4

You can add in gradle:
compileKotlin {
kotlinOptions.allWarningsAsErrors = true
} :slight_smile:

Please note that extensions are resolved statically, and require explicit import - so you can for example have situation like this (in one file):
class A
fun A.foo

class C : A { fun foo }

class B : A

val asA : A = C()
val asC : C = C()
val b : B = B()

asA.foo() <- will call extension as extensions are resolved statically and asA known type is A which doesn’t have member function foo
asC.foo() <- will call class function (member function always wins)
b.foo() <- will call extension

This is legit, although indeed asC.foo() can be slightly confusing. But if instead of a warning there was an error, you would have problem to call asA.foo or b.foo


#5

I agree that since an extension function that has the same signature of another native function is completely ignored, it should be treated as an error.

What I do to minimize the risk, is starting all my extension functions with “ef”. In your example, it would look like this:

fun C.efFoo = println("extension")

If it becomes an error in the future, I will stop doing it as I really don’t like the aesthetics of my “solution”


#6

asA.foo() or b.foo() calls should not give you any warning nor error since there are not ambiguous.

On the other hand, the asC.foo() call only gives you a warning, which I think should be an error.
This could avoid people being careless and letting wrong code go into production.

(Also I know you can treat all warning as error, the point is to make this specific warning an error by default)


#7

Well, I’m still not sure that it should be an error. Yes, in some situations as mentioned someone may not notice that imported extension will not take effect as class already has function/property with the same name, but if it was error and in one file you use both class C and B (from my example) you will loose option to use extension on class B (as otherwise C overshadowing extension would cause error)

For example - I added extension visible to View (similar thing will be added to Kotlin Android extensions now). But what if I use some custom component (not created by myself) that also added property visible? I wouldn’t be able to use my extension in any file where I have this component, even for other views.

I think it’s similar case to unsafe cast etc - in general it’s recommended to avoid it, it’s good to be warned by compiler, but there are legitimate cases where you need to use it anyhow. In such case you add @SuppressWarning annotation to note that you’re aware of it but can’t find better way to deal at the moment.