transformer implementation of invokeSetter throws NoSuchCapability error instead of NoSuchMethod error about reflectable.dart

[eernstg]

We know, and this is a very interesting problem! ;-)

With pre-transform code, the 'dart:mirrors' based invocation can use the built-in semantics to determine that a given method is not supported by the given receiver, such that we can get the standard noSuchMethodError. With post-transform code the situation is as follows: An invocation of a method 'message' has been requested, and there is no generated code for performing such an invocation. This could be because the capabilities specified that only a subset of the available methods should be supported, and this one is not among them, or it could be because it is a non-existing method. We cannot distinguish these two situations without storing information about all methods that we do not support.

That is not a very attractive thing to do, given that we don't want to spend a lot of resources (program size) on things for which we do not have any capabilities.

This is a known problem, and it is not obvious how we could solve it in a good way. So we are not going to have a solution at this point.

Do you think it would be better to always throw a NoSuchMethodError, even though it might as well be a NoSuchCapability problem?

Another issue which is related is that we cannot invoke noSuchMethod, because that method needs a Symbol indicating which selector (method name) to use for the invocation, and we don't want to introduce dynamic creation of Symbol values. (Essentially, the runtime state must then include a complete mapping from String values to the corresponding minified Symbol values, which would take up a significant amount of space, and it would potentially also be an unintended-deobfuscation issue).

Keeping the issue open as a reminder (would label it, but there is no "not right now" label).

[jakemac53]

We cannot distinguish these two situations without storing information about all methods that we do not support.

What if instead of storing information about each method, you just stored information about the capabilities that were originally supplied? This info would be relatively compact and should allow you to distinguish the two cases?

Do you think it would be better to always throw a NoSuchMethodError, even though it might as well be a NoSuchCapability problem?

This is a really hard question :(. In the case of Polymer this caused a post-transformation bug because I was catching NoSuchMethodError since it was much faster than actually checking if a setter exists. The opposite situation could easily happen as well though, where somebody was trying to catch a NoSuchCapability error and instead it threw a NoSuchMethodError. Therefore, I don't think one is better than the other.

Keeping the issue open as a reminder (would label it, but there is no "not right now" label).

You can add one here ;) https://github.com/dart-lang/reflectable/labels

[eernstg]

We'll explore that idea. Keeping the issue open (without the "not right now", just open, but we'll fix some more bugs first ;).

[jakemac53]

This actually is going to be more important going forward, since we are requiring annotations on all properties/methods now. It would be really great to be able to distinguish the case where the method actually didn't exist, versus it just needing an annotation.

[eernstg]

Returning to this issue, I cannot see how we can distinguish "this method does not exist" from "this method exists, but you haven't asked for the capability to call it" by consulting things like a RegExp stored in an instance of InstanceInvokeCapability or a Type stored in a StaticInvokeMetaCapability.

Even if we know that we can invoke methods whose name matches "^fo.*ar$" and methods carrying metadata of type D or a subtype thereof, we cannot know whether or not there is a method called foobar accepting 3 positional arguments and two named arguments with names #baz and #blit even if we stare really hard at that RegExp and that Type. ;-)

We'd have to store information about every non-covered method in the target class itself as well as every superclass: name plus parameter list shape (including all the names of its named optional parameters), and then we'd have to check that each invocation where the lookup fails has a matching name and a conforming parameter list shape among the non-covered methods, in which case it's NoSuchCapabilityError; otherwise the method just isn't there, and it's noSuchMethod.

My gut feeling is that we should not store a large amount of information about stuff that the explicitly specified capabilities have rejected, given that the starting point for the package reflectable is to save space.

This gives rise to another idea: We could declare that there is no capability for reflectively invoking methods that do not exist, just like there is no capability for invoking methods that exist but fail to match the criteria (RegExp, Type of metadata), and then we would throw a NoSuchCapabilityError in both cases. That's a simple and consistent rule, and I don't think the alternatives are obviously more "correct".

Another matter is that it might be useful to be able to make queries about supported methods for a given instance/class mirror. We can do this today, indirectly: With a given instance mirror, we could obtain the corresponding class mirror, get its instanceMembers, and search for the existence of a method with a given name; similarly, we could search the staticMembers of a given class mirror. But then we'd need to manually inspect the formal parameter list and check whether the intended actual argument list conforms to the formal list, and then we can conclude "yes, we have that method" or "no, we don't".

Wouldn't it work at least equally well to simply try invoking the method as planned, and then catching NoSuchCapabilityError? ;-)

[jakemac53]

I kind of like the idea of just always throwing a NoSuchCapabilityError actually... at least everything is consistent that way.

Another matter is that it might be useful to be able to make queries about supported methods for a given instance/class mirror. We can do this today, indirectly: With a given instance mirror, we could obtain the corresponding class mirror, get its instanceMembers, and search for the existence of a method with a given name; similarly, we could search the staticMembers of a given class mirror. But then we'd need to manually inspect the formal parameter list and check whether the intended actual argument list conforms to the formal list, and then we can conclude "yes, we have that method" or "no, we don't".

I was doing basically this originally, but it was prohibitively slow. I didn't spend a lot of time trying to figure out exactly which part was slow, but just catching the error was definitely a lot faster (at least a few months ago).

[eernstg]

The approach taken in CL https://codereview.chromium.org/1485193002/ is exactly this: Always throw NoSuchCapabilityError when a method is invoked, but not present. The underlying motto would be "reflectable and noSuchMethod do not mix". That CL eliminates the associated test failures (which had been failing for many months because we had no solution until now) so we can now close this issue. ;-)

[eernstg]

We are finally making progress on this. The commit 5a835be contains a large number of changes related to the treatment of no-such-method events in transformed code. CL https://codereview.chromium.org/1512033002/ contains the corresponding changes for doing the same in untransformed code. The resulting semantics is as follows:

The core concept is a no-such-method situation. That is a reflectable invocation (that is, an invoke, invokeGetter, invokeSetter, delegate, or newInstance on an InstanceMirror, ClassMirror, or LibraryMirror) that fails because the specified method/getter/setter does not exist, or an argument list was provided which had a non-conforming number of positional arguments, or a named argument was specified whose name isn't defined for that method. When such a no-such-method situation arises, reflectable throws a ReflectableNoSuchMethodError containing a StringInvocation (just like an Invocation but with a string rather than a symbol as the memberName). This means that it is possible to catch that exception and invoke whatever-stand-in-for-noSuchMethod you wish to use.

[eernstg]

CL https://codereview.chromium.org/1512033002/ was landed as commit 45214a1. The approach of consistently throwing ReflectableNoSuchMethod in no-such-method situations for reflectable invocations is now in place, so we will close this issue.

There is a lot of new code involved, though. Please create new issues if you spot any buggy behaviors.