dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.11k stars 1.56k forks source link

Annotations conflict with inherited variables #32189

Open jxson opened 6 years ago

jxson commented 6 years ago

If a class has inherited the variable name proxy an error occurs because Dart thinks proxy is the annotation and not the varible defined in the parent class.

It's possible to get around this by appending "super", e.g. super.proxy works.

Here is a basic example:


class Proxy {
  void bar() {
    print('bar called!');
  }
}

abstract class ParentWithProxy {
  final Proxy proxy = new Proxy();
}

class Child extends ParentWithProxy {
  void foo() {
    proxy.bar();
  }
}

void main() {
  Child child = new Child()
  child.foo();
}

This will fail with:

Unhandled exception:
NoSuchMethodError: Class '_Proxy' has no instance method 'bar'.
Receiver: Instance of '_Proxy'
Tried calling: bar()
#0      Object.noSuchMethod (dart:core-patch/dart:core/object_patch.dart:46)
#1      Child.foo (file:///<snip>/weird-inheritence-bug/main.dart:14:11)
#2      main (file:///<snip>/weird-inheritence-bug/main.dart:20:7)
#3      _startIsolate.<anonymous closure> (dart:isolate-patch/dart:isolate/isolate_patch.dart:279)
#4      _RawReceivePortImpl._handleMessage (dart:isolate-patch/dart:isolate/isolate_patch.dart:165)

I ran into this while trying to refactor/dry up some code in Fuchsia where the analyzer shows this error:

[dart] 'proxy' is deprecated and shouldn't be used.
(deprecated) Object proxy
dart.core

The annotation @proxy marks a class as implementing members dynamically through noSuchMethod.

The annotation applies to any class. It is inherited by subclasses from both superclass and interfaces.

If a class is annotated with @proxy, or it implements any class that is annotated, then the class is considered to implement any member with regard to static type analysis. As such, it is not a static type warning to access any member of the object which is not implemented by the class, or to call a method with a different number of parameters than it is declared with.

The annotation does not change which classes the annotated class implements, and does not prevent static warnings for assigning an object to a variable with a static type not implemented by the object.

If desired I can point you to the code in Fuchsia, where I ran into this.

kevmoo commented 6 years ago

I could see this being "by design" but it's certainly annoying. @eernstg @leafpetersen @lrhn ?

jxson commented 6 years ago

Interesting, it made sense once I saw what was causing it but it did take a little spelunking in dart:core to track it down. Minimally the error message could be improved if the behavior is "by design".

Thanks for the quick reply!

eernstg commented 6 years ago

It's certainly by design, but it is a non-trivial trade-off. The reason why identifier lookup selects the top-level declaration (imported from 'dart:core', typically implicitly) rather than the inherited declaration is that the lookup is ordered to search the lexically enclosing scopes first, and only then treats an identifier id as if it had been this.id (which allows access to inherited features, plus features that are just "promised" via implements and subsequently implemented by a subclass).

One relevant rationale for doing it this way is that it gives priority to the declaration which is "in sight":

const x = 42;
class B extends A { 
  get foo => x;
}

// Far away in some other file
class A {
  String x;
}

The point is that lexical lookup is the simple concept that we can grasp just by getting into a habit of looking at an identifier x and then searching for its "meaning" (its declaration) by looking out into the physically enclosing blocks of code. An inherited name could be far away, and if it's an abstract declaration coming from one of many classes that this class implements, possibly indirectly, then it's going to be much more work to find that declaration than it is to look at enclosing blocks.

Note that this may be more important in Dart than it is in other languages, e.g., Java, because Dart allows top-level declarations of substance (variables, functions) and not just types.

But this distance based rationale breaks down when the inherited declaration is just 2 inches up from the identifier that we're trying to look up, and the global declaration that accidentally "captures" that slot in the binding environment has been imported, and that import was implicit!

So your example is really a worst case for the kind of reasoning that led to the prioritization of the lexical scope.

I think there's a cultural side to this as well: It may be convenient to provide features like proxy as top-level declarations in some library L that clients may import, but it's also a huge tax on the "name budget" in client code because that declaration is now shadowing inherited declarations of the same name, in every class in every library that imports L without a prefix. So the Dart culture should be (and generally is, I think!) aware of the cost that comes with top-level declarations, especially for lowercase names. There should be very few, and they should be completely obvious (well-known).

Finally, if there is a clash like the one you experienced then the type system helps telling developers that something does not mean what they thought it meant, and a suitable key combination would typically take them to the declaration of any given name and reveal the issue.

lrhn commented 6 years ago

Another reason for choosing this particular trade-off is that the fix is easy: You can always prefix with this. to get the non-default behavior. If it had been the other way around, we would need some kind of scope override to be able to target a top-level declaration through an inherited member.

jxson commented 6 years ago

I agree, the fix is simple. However the error message doesn't help with discovering what went wrong and how to address it.

I suggest this issue's action item should be to improve the error message for both the exception and the analyzer warning. E.g. the message about the annotation @proxy being deprecated is misleading, instead the body of the message could include a hint for using this.proxy to access the intended object.