Extensions can't shadow members, leading to unexpected behaviour changes

[Hixie]

Suppose a library declares a type Foo:

class Foo { }

...and an application uses it alongside an extension:

import 'library.dart';

extension on Object {
  void test() {
    print("on Object");
  }
}

void main() {
  Object().test();
  Foo().test();
  Object o = Foo();
  o.test();
  Foo f = o;
  f.test();
}

This prints "on Object" four times.

Now, the following supposedly non-breaking change is made and the library is revved:

class Foo { 
  void test() {
    print("hello from Foo");
  }
}

The next time the above application is run, the output is now "on Object", "hello from Foo", "on Object", "hello from Foo", instead of "on Object" four times.

It seems like an extension should shadow members, or that the same "can't import two conflicting names" logic we have for imported members should apply here, or something.

[mraleph]

Now, the following supposedly non-breaking change is made and the library is revved:

Note that adding members is not a non-breaking change in general - because users can be implementing your types.

[Hixie]

Fair, although at least in that case the breaking change is analyzer-detected (at least if you have the lint enabled that required @OverRide). In the case above the break is silent behaviour change with no analyzer or compiler message.

[Hixie]

(Also, realistically, nobody really considers adding a novel member to a class that isn't intended to be used as an interface to be a breaking change.)

[eernstg]

@Hixie wrote:

It seems like an extension should shadow members, or that the same
"can't import two conflicting names" logic we have for imported members
should apply here, or something.

There was a long discussion in the language team about the precedence of extension methods and instance methods etc, and the conclusion was that instance methods should always prevail when both exist. Cf. #556.

[munificent]

It seems like an extension should shadow members

If we did that, then the reverse scenario would cause the same problem. If you revved a library that added a new extension method to some existing class, it would now shadow any instance method on the class that you were previously calling.

that the same "can't import two conflicting names" logic we have for imported members should apply here, or something.

With imported top level names, there really is no reasonable way the language could decide which to prefer since they are two imports of the same kind of thing. We definitely don't want import order to be meaningful. (All top level declarations and directives are essentially "simultaneous" in Dart. You don't need to do forward declarations.) So in the absence of any real way to prioritize one over the other, we were forced to make it an error.

With collisions between extensions and instance methods, the two things are different, which gives us the ability to make a prioritized choice based on that difference. There's no perfect answer, but we felt that prioritizing the instance method would do what users want most of the time and the cases where it would cause bugs would be rare. For what it's worth, C# makes the same choice, and I think Kotlin does too. (There are some differences there because those languages have overloaded, but that gave us some confidence that it was a reasonable choice.)

[Hixie]

Kotlin making this mistake is how I discovered Dart had it (it caused me problems in some Dart code I was writing, and I tried it in Dart to see how we handled it, and it turns out we don't do any better).

I really think the right thing to do here is to just make the conflict be an error. I don't think there's any way to know whether authors prefer one or the other, and it seems extremely likely that whichever one they want, they don't know that the other one exists. This means that half the time we'll pick the wrong one and debugging this would be massively frustrating.

At an absolute minimum we should have a lint to catch it.

[leafpetersen]

At an absolute minimum we should have a lint to catch it.

This seems like a great candidate for a lint/hint.

[srawlins]

I've requested this in dart-lang/sdk#58182.