Allow syntax for classes that implement all members (regression from Dart1)

[Hixie]

It was previously possible to create a class that was typed, i.e. had known methods and such that the analyzer could type-check, while still allowing any unknown methods to be accessed, as in:

@proxy
class Test {
  String foo(int a) => a.toString();

  @override
  dynamic noSuchMethod(Invocation invocation) {
    if (invocation.isGetter)
      return invocation.memberName.toString();
    return super.noSuchMethod(invocation);
  }
}

void main() {   
  Test t = new Test();
  print(t.foo(2)); // prints '2'
  print(t.bar); // prints 'Symbol("bar")'
  bool x = t.foo('y'); // two analyzer warnings, one for the argument and one for the assignment, and a	runtime error
}

This no longer works (and there's no equivalent). This breaks existing code. The nearest equivalent workaround is to use dynamic everywhere, but that loses critical type checking and means the code has to be full of casts, which defeats the entire point of having a class like this (making the code succint and readable).

cc @leafpetersen

[Hixie]

As far as I can tell, implementing this is trivial -- just silence warnings of the form The getter 'bar' isn't defined for the class 'Test' for any class that has the @proxy annotation.

[leafpetersen]

means the code has to be full of casts,

What do you mean by this? There should be no explicit casts at all, if you're using dynamic. If you're trying to cast to specific interfaces, that would never have worked in Dart 1 either (at least not checked mode). Just marking something with @proxy didn't make it assignable to everything - just made it be treated as if it had all methods.

[leafpetersen]

Following up on the feature request, let me start by saying that I really do think that @proxy was a terrible feature. It was a pit of failure. An easy way to do a quick hack, that quickly resulted in unmaintainable code. I can't tell you how many garbage tests we found in internal code that were garbage because someone at some point had used @proxy to mock a class API. When that class API changed or was deleted, the static analysis didn't point them to the tests that were mocking the old API, and so the zombie mocks just stayed around forever, testing nothing, or testing the wrong things. Going through and deleting uses of @proxy internally was one of the changes that I can say with the most confidence universally made things better.

If you want to mock a specific interface (or set of compatible interfaces even), you can do so just by using no such method and specifying what methods you want to implement. You will also get the benefit that they will be assignable to the types you're mocking!

If you want to mock all interfaces dynamically (I encourage you not to, see above, but your business) then dynamic works as well as @proxy.

The only use case we've lost is the ability to mock specific interfaces statically, plus everything else in the universe dynamically, using a single class. This seems.. niche. Do you have good examples of why this is important? (Seriously, I want to understand the use cases here).

I do think that we should have a good way to handle more general mocking, and I would like it to be lightweight and easy enough to use for quick hacks. But I really do feel that we're better off without a primrose path feature like @proxy - it's not actually very useful (no assignability) and it's almost certainly something you will regret using later on in a project lifecycle.

cc @lrhn

[lrhn]

The effect of having @proxy anywhere in your interface hierarchy was, in Dart 2 terms, that all invocations of unknown members were dynamic. The only way to get dynamic invocation in Dart 2 is to have a static type of dynamic. So, to solve the problem, you can just write dynamic t = new Test();, and the program will act like in Dart 1.

There is indeed no type safety if you do that, you can't be sure that the object you are using is actually an instance of Test. The @proxy annotation allowed you to have a static type of Test and act like Test for members of Test, and an effective behavior as if the type was dynamic for anything not declared by Test.

I do see a use for that feature, even if it is a highly dynamic feature - it's just slightly less dynamic than actual dynamic - so if you could declare a variable as dynamic Test, it could enforce the Test type, but allows any invocation on top of that. I'd just not make it a feature of the class, just like you can't implement dynamic, it's a property of the object access, not the object itself.

[eernstg]

tl;dr @proxy was a second-class feature, and we do have a faithful replacement. rd;lt

@Hixie, I'd like to make the point that the use of variables of type dynamic is not just a superficial workaround, it's equivalent to the Dart 1 @proxy feature at the foundation. There are some differences (superficial ones ;-), but there is a language construct that we have discussed already a long time ago which would make them equivalent at all levels except the syntax. So here we go:

The situation in Dart 1 with @proxy is very similar to the situation with an extension that we have discussed several times: Considering dynamic to be a modifier with default argument Object, and hence allowing things like dynamic<C> for any C.

That would mean "checking statically that invocations of members of C are appropriate, but allowing arbitrary usages of other members, with no errors". For instance,

main() {
  dynamic<List<int>> xs = ...;
  num n = xs.length; // OK, `List<int>` has a `length`, `int` is assignable to `num`.
  xs.add(n); // OK, with downcast (so it's failing with `--no-implicit-casts`).
  xs.whatEverYouWant(whatever: 42); // OK: invoking a "dynamic" member, no checks.
}

The point is that this allows you to use xs in its scope as if it had had some other type (or several other types). But it's a shallow property: Even though you may be able to treat xs as if it were a String, and it might "work" (based on noSuchMethod, and whatnot), you won't be able to assign it to a variable of type String, and it won't make xs is String true. So that magic ability to "have many types" is a second-class property: it breaks down very quickly if you actually try to use it as if it had one of those types.

A Dart 1 class C with @proxy gave you exactly the same amount of functionality. But it was deceptive, in a sense, because it seemed like instances of C were somehow "more dynamic" than instances of other classes, and that isn't true. The more forgiving static analysis is applied in client code when the static type of an expression is C (or a subtype thereof), and the instances of C (and subtypes) do not have any extra powers/permissions at all.

So using the type dynamic is obviously second-class (it influences the static analysis but not the properties of the actual instances being operated upon), and the same thing would hold for dynamic<C> or a similar generalization. Since @proxy class C ... was also a second-class mechanism all the time, switching to use dynamic/dynamic<C> is a perfectly valid replacement for Dart 1 @proxy, not a regression.

[Hixie]

Consider this code:
https://github.com/seamonkeysocial/cruisemonkey/blob/master/lib/src/network/network.dart#L112

What I would like it to look like is:

  Future<Calendar> _getCalendar() async {
    final Json data = Json.parse(await _request('get', '/api/v2/event.json'));
    try {
      final Json values = data.event.asIterable().single;
      if (values.status != 'ok')
        throw const FormatException('status invalid');
      if (values.total_count != values.events.asIterable().length)
        throw const FormatException('total_count invalid');
      return new Calendar(events: values.events.asIterable().map<Event>((Json value) {
        return new Event(
          id: value.id.toString(),
          title: value.title.toString(),
          official: value.official.toBoolean(),
          description: value['description']?.toString(),
          location: value.location.toString(),
          startTime: DateTime.parse(value.start_time.toString()),
          endTime: DateTime.parse(value.end_time.toString()),
        );
      }).toList());
    } on FormatException {
      return null;
    } on NoSuchMethodError {
      return null;
    }
  }

In particular, note the removal of several casts, and the replacement of "dynamic" with "Json", allowing for much better type checking.

(The Json class is implemented here: https://github.com/seamonkeysocial/cruisemonkey/blob/master/lib/src/json.dart)

dynamic<C>

I would be fine with having the Json class somehow implement dynamic<Json> or whatever. I wouldn't want to have to explicitly say dynamic<Json> every time Json appears in the snippet above; that's even worse than having it be dynamic like today, in terms of code ugliness.

[matanlurey]

In the case where .name is just used as a dynamic lookup anyway, couldn't you just add an operator overload for []?

abstract class Json {
  operator[](String key) => /* TODO: Implement. */
}

Then you'd write:

try {
  final Json values = data['event'].asIterable().single;

... instead of:

try {
  final Json values = data.event.asIterable().single;

Same amount of "type safety" without any of the overhead of this feature.

[Hixie]

Not doing that is the entire point of the Json class.

[matanlurey]

Do you really value ['event'] -> .event so much to need this feature?

Sorry just seems silly.

[Hixie]

Sure. It's the difference between Json code being barely readable and Json code being too ugly to want to use Json. :-)

[matanlurey]

Personally json['event'] is more readable than json.event, where I might mis-spell as eveent and get a runtime exception. This not working (i.e. typed (non-dynamic) classes allowing arbitrary .something that might fail) is part of the reason folks like Dart.

[Hixie]

You're just as likely to misspell json['eveent'] and get a runtime exception.
I agree that it shouldn't work arbitrarily. Ideally we'd be able to say much more fine-grained things, like "I implement every getter, returning a Json, and every setter, accepting any of these types..., and I don't implement any methods", but that's a feature request for another day. This bug is just about not regressing the very little support we already had for something in this direction.

[matanlurey]

I sympathize that you're personally unhappy, but I don't think this will change.

Personally I think this bug should be closed as WAI, the Dart2 team spent a lot of time and effort deciding this already, removed it from the spec, updated the tools, etc. Perhaps better metaprogramming in Dart 2.1+ could help alleviate some of this pain, but until then dynamic seems just fine to me.

[Hixie]

This is what the code above would look like with square brackets. It becomes much harder to read, and additionally, it loses the key distinction between "this should be here and if it isn't it's an error in the data" and "this field is optional".

  Future<Calendar> _getCalendar() async {
    final Json data = Json.parse(await _request('get', '/api/v2/event.json'));
    try {
      final Json values = data['event'].asIterable().single;
      if (values['status'] != 'ok')
        throw const FormatException('status invalid');
      if (values['total_count'] != values.events.asIterable().length)
        throw const FormatException('total_count invalid');
      return new Calendar(events: values['events'].asIterable().map<Event>((Json value) {
        return new Event(
          id: value['id'].toString(),
          title: value['title'].toString(),
          official: value['official'].toBoolean(),
          description: value['description']?.toString(),
          location: value['location'].toString(),
          startTime: DateTime.parse(value['start_time'].toString()),
          endTime: DateTime.parse(value['end_time'].toString()),
        );
      }).toList());
    } on FormatException {
      return null;
    } on NoSuchMethodError {
      return null;
    }
  }

This is a regression that makes existing code much uglier and fixing it would be simple. I don't really understand why we wouldn't want to fix it.

Fixing this only requires two things: when a class is specially marked, have the analyzer act just like it does today if the static type is dynamic in any case where today it would say that a member is not defined (i.e. just don't complain about unknown members on such classes), and have the compiler call noSuchMethod when it otherwise wouldn't know what to call on such a class. As far as I can tell, that's all you need.

[matanlurey]

I think this is a place an in-person discussion with the team is going to be a lot of more productive (for everyone) than a GitHub issue. I'm happy to set one up if you'd like.

[leafpetersen]

Thanks for expanding on the use case, it's helpful.

[zoechi]

@Hixie I'm wondering why you use JSON directly at all. I always deserialize it to a class before I use it. This way I never have to use x['foo']. Sure it requires some boilerplate code first, but code generation (for example using built_value or json_serializable generates that automatically.

[Hixie]

This is the code that deserialises it.

[natebosch]

The new semantics of noSuchMethod preclude the possibility of an @proxy annotation with these characteristics. noSuchMethod is only called for methods that are declared on the statically known interfaces.

As far as I know changing noSuchMethod back to the previous semantics is not feasible so I don't think we have any action we can take on this issue.

[Hixie]

I don't think this should be closed. It's a valid request to not regress a feature that people have previously depended upon.

As far as I can tell it would be simple to implement. It's essentially asking for a way to say you implement every method, getter, and setter. This is commonly implemented in many languages.

[natebosch]

I had a misunderstanding about noSuchMethod and it turns out this isn't as impossible as I thought it was.

[eernstg]

I don't think it would be difficult to implement support for dynamic<Json> as described here, or to allow something like class Json implements dynamic {...} to have a very similar semantics (that would presumably extend to all subtypes, which may or may not be useful), or to reintroduce @proxy with the Dart 1 semantics. So I believe we're basically discussing whether we should do it, not whether it's within reach to get it implemented in the context of Dart 2.

Given that it would enable usage of some code that used to have compile-time errors, I believe it is a non-breaking change.

However, the decision to eliminate the special treatment of @proxy indicates that we'd need to change our minds. ;-)

In order to reconsider the situation I think @Hixie's two code snippets are a good starting point for getting an idea about how much it matters: this example vs. this one.

So we're basically discussing whether the avoidance of a lot of ['/'] brackets is such a relief with respect to readability and writeability that it's worth letting that override the reasons for removing the special treatment of @proxy. (OK, other examples could have a different profile, but @Hixie's example is concerned with a scenario that has the potential to be relevant for a substantial amount of code.) No matter the outcome I think it's fine to have this issue as the context for taking that discussion.

[matanlurey]

One small point, I don't believe this issue is customer-flutter but rather just a personal request by Hixie. I say that just because I imagine we'd prefer focusing on Flutter customer issues at this time.

(From a completely personal preference, I wouldn't want implements dynamic supported at all in the context of the web compilers - we are trying to go the other way and introduce language options to completely disable all dynamic dispatch)

[Hixie]

Yeah sorry if this wasn't clear earlier, this is definitely not something Flutter is trying to do. We try to avoid JSON in general. :-)

[Hixie]

This has now gotten even worse, because you can't implicitly cast from dynamic anymore as far as I can tell. So now you have to say something like Iterable<dynamic> foo = bar.asIterable() as Iterable<dynamic> instead of just being able to say Iterable<dynamic> foo = bar.asIterable().

[leafpetersen]

bar.asIterable() has type dynamic? This is still a valid implicit cast. Do you have the --no-implicit-casts option turned on?

[Hixie]

bar has static type dynamic and runtime type Json. I have implicit-casts: false in my analysis_options.yaml, but I've had that there for months, is it expected that its behaviour changed recently?

[leafpetersen]

Ah, I think I know what may have happened. There was a regression, I think only in dev releases between 2.0.0 and 2.1.0, that caused --no-implicit-casts to allow all declaration casts. That is, the intention was that --no-implicit-casts implied --no-declaration-casts, but for a period of time it was treated as implying --declaration-casts. This meant that:

  dynamic d;
  int x = d; // Allowed, declaration cast
  ((int x) => x)(d); // Rejected

I take it you would like the no implicit cast option to still allow casts from dynamic?

[Hixie]

ah, interesting.

I think it's fine to not have casts from dynamic in general (and require as for those). What I'd like is something that allows arguments to calls into methods or setters implemented by noSuchMethod, and return values from such methods or getters, to be implicitly cast, as per the original issue filed above.

(Also, separately, I'd like assert(foo is bar) to implicitly cast foo to bar in the analyzer.)

[jamesderlin]

(Also, separately, I'd like assert(foo is bar) to implicitly cast foo to bar in the analyzer.)

Also see #35019 (sort of).