sigmundch / dep-member-interceptors Goto Github PK

If the metaclass proposal goes through, it may make sense to pass in the class object as the target for static interceptors. Or we could stick with null for now and leave that open for refinement later.

To me, the biggest reason to use annotation syntax for interceptors is that it lets a library encapsulate the usage of interception. My awesome framework may use @foo as a regular annotation in 1.2.3 and then switch to making @foo an interceptor in 1.3.0 without breaking my users or even requiring them to know.

Imagine that I have a class with an intercepted field:

class X {
  @interceptor var field;
}

what happens if I extend it and overload intercepted field:

class Y extends X {
  var _field;
  get field => _field;
  set field(value) { _field = value; }
}

as it is specified right now interceptor stops working, it will not intercept access to the field in Y. Should be make interceptors contagious, e.g. they are always pushed down to bottom most overriding implementation? Seems like the best choice to me.

(This would slightly complicate semantics of the super invocation that would have to skip interception to avoid executing interceptor twice).

We can also make it configurable and let interceptor implementor choose.

Similarly what happens if Y implements X?

Should interceptor be part of the interface, e.g. should it be "inherited" as well? This is espeically important for interceptors that are "type like", e.g. @notnull because these type like interceptors are obvious part of the interface and not of the implementation.

Related question, how do I intercept method below:

library xyz;

class PublicInterface {
  factory PublicInterface() => new _PrivateImplementation();

  method();
}

class _PrivateImplementation implements PublicInterface {
  method() { /* ... */ };
}

there is a potential ordering issue with how to combine interceptors that are annotated on the side via @ApplyInterceptorTo

Original comment from @mraleph:

If I import the same library twice into two other different libraries and on both imports I apply interceptors to the same member --- then it's not entirely clear how that should "stack".

Of course there is an obvious order induced by the order of import statements - but this order is somewhat hidden from the programmer.

The current proposal indicates that interceptors are an implementation detail, not part of the interface See #11 for motivation on why to make it part of the interface.

The name is a bit strange because "tag" isn't used anywhere else. I don't think it's really relevant to this proposal anyway, but how about @ApplyAnnotationTo or just @Annotate?

The design doc includes a long discussion about syntax alternatives.

Let's use this issue to keep the syntax discussion in one place.

If an interceptor needs more state than the target member (or state of a different type) - it looks like with the current proposal - one needs to fall back using globals (e.g. global Maps) (possibly indexed by target object and/or interceptor arguments). The @memoize interceptor is an example of this (e.g. @memorize fib(a,b) ). Is this correct?

In order to guarantee that the additional state has the same lifetime as the target itself (i.e. the state should be GCed at the same time), it becomes more complicated, since one may need to use weak maps, ...

It would be nice if an interceptor could introduce new state without relying on globals.

The proposal says:

and the _$foo field will be final.

But the example doesn't show a field at all. It might be nice if the interceptor can choose whether or not the intercepted field gets backing state or not.

I think contract validation is definitely a useful example to show, but I worry that if we use @nonnull as the hypothetic contract that the discussion will rathole on non-nullability. I, of course, care a lot about non-nullability, but it's not relevant to this proposal. :)

How about @nonnegative or something?

In terms of the actual implementation, we need to make sure that there is some path for dart2js to implement this without an unacceptable amount of code bloat. Specifically, I am concerned about having to create a class which extends Member for each field which is annotated, like this example from the getter section:

class _$nameMember extends Member {
  const _$nameMember() : super(#name);
  get(target) => target._$name;
  set(target, value) { target._$name = value; }
  invoke(target, positional, named) =>
      Function.apply(target._$name, positional, named);
}

I think there are a few ways to get around this, but just want to make sure its highlighted as something which should be watched.

The Member and Interceptor interfaces would be added to "dart:core".

It definitely needs to be in some "dart:" library, but I wonder if this proposal will be less contentious if it's in a separate "dart:" library. Not "dart:mirrors", of course.

Currently the proposal is the following:

abstract class WriteInterceptor {
  set(target, value, Member member);
}

abstract class InvokeInterceptor {
  invoke(target, List positionalArguments, Map<Symbol,dynamic> namedArguments,
      Member member);
}

I would like to propose that we change member to always be the 2nd argument:

abstract class WriteInterceptor {
  set(target, Member member, value);
}

abstract class InvokeInterceptor {
  invoke(target, Member member, List positionalArguments,
      Map<Symbol,dynamic> namedArguments);
}

This is more intuitive, because it looks more like the calls its actually intercepting. For example when assigning a property:

foo.bar = 'baz';

The set method would be called with foo, a member whose name is #bar, and the value 'baz', in that order. With the current semantics 'baz' would come before the member with name #bar, which isn't as intuitive.