From c8d7cd8ef46de11f5e4e7c757bb9ee2f12cb8cd6 Mon Sep 17 00:00:00 2001 From: Stephen Adams Date: Mon, 22 Jun 2015 13:16:05 -0700 Subject: [PATCH] Fix for issue 23432. Get the correct receiver in noSuchMethod stubs. noSuchMethod stubs for selectors with Interceptor calling convention should pass the receiver to noSuchMethod (not 'this'). The actual receiver is selected by Object.noSuchMethod and Interceptor.noSuchMethod. R=floitsch@google.com Review URL: https://codereview.chromium.org//1181063005. --- .../src/js_emitter/class_stub_generator.dart | 8 ++- .../js_emitter/old_emitter/nsm_emitter.dart | 62 ++++++++++++------- .../compiler/js_lib/interceptors.dart | 18 +++++- .../_internal/compiler/js_lib/js_helper.dart | 2 +- .../dart2js_extra/dart2js_extra.status | 1 - 5 files changed, 62 insertions(+), 29 deletions(-) diff --git a/pkg/compiler/lib/src/js_emitter/class_stub_generator.dart b/pkg/compiler/lib/src/js_emitter/class_stub_generator.dart index e21ba1daf0b..54b2a1bcdc5 100644 --- a/pkg/compiler/lib/src/js_emitter/class_stub_generator.dart +++ b/pkg/compiler/lib/src/js_emitter/class_stub_generator.dart @@ -158,14 +158,16 @@ class ClassStubGenerator { String internalName = namer.invocationMirrorInternalName(selector); assert(backend.isInterceptedName(Compiler.NO_SUCH_METHOD)); + bool isIntercepted = backend.isInterceptedName(selector.name); jsAst.Expression expression = - js('''this.#noSuchMethodName(this, + js('''this.#noSuchMethodName(#receiver, #createInvocationMirror(#methodName, #internalName, #type, #arguments, #namedArguments))''', - {'noSuchMethodName': namer.noSuchMethodName, + {'receiver': isIntercepted ? r'$receiver' : 'this', + 'noSuchMethodName': namer.noSuchMethodName, 'createInvocationMirror': backend.emitter.staticFunctionAccess( backend.getCreateInvocationMirror()), @@ -179,7 +181,7 @@ class ClassStubGenerator { 'namedArguments': new jsAst.ArrayInitializer(argNames)}); jsAst.Expression function; - if (backend.isInterceptedName(selector.name)) { + if (isIntercepted) { function = js(r'function($receiver, #) { return # }', [parameterNames, expression]); } else { diff --git a/pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart b/pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart index 94ac7ca8701..201a369d717 100644 --- a/pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart +++ b/pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart @@ -278,14 +278,12 @@ class NsmEmitter extends CodeEmitterHelper { ' objectClassObject = objectClassObject[1];')); } - List sliceOffsetArguments = + dynamic isIntercepted = // jsAst.Expression or bool. firstNormalSelector == 0 - ? [] - : (firstNormalSelector == shorts.length - ? [js.number(1)] - : [js('(j < #) ? 1 : 0', js.number(firstNormalSelector))]); - - var sliceOffsetParams = sliceOffsetArguments.isEmpty ? [] : ['sliceOffset']; + ? false + : firstNormalSelector == shorts.length + ? true + : js('j < #', js.number(firstNormalSelector)); statements.add(js.statement(''' // If we are loading a deferred library the object class will not be in @@ -294,31 +292,51 @@ class NsmEmitter extends CodeEmitterHelper { if (objectClassObject) { for (var j = 0; j < shortNames.length; j++) { var type = 0; - var short = shortNames[j]; - if (short[0] == "${namer.getterPrefix[0]}") type = 1; - if (short[0] == "${namer.setterPrefix[0]}") type = 2; + var shortName = shortNames[j]; + if (shortName[0] == "${namer.getterPrefix[0]}") type = 1; + if (shortName[0] == "${namer.setterPrefix[0]}") type = 2; // Generate call to: // // createInvocationMirror(String name, internalName, type, // arguments, argumentNames) // - objectClassObject[short] = (function(name, short, - type, #sliceOffsetParams) { - return function() { - return this.#noSuchMethodName(this, - #createInvocationMirror(name, short, type, - Array.prototype.slice.call(arguments, - #sliceOffsetParams), - [])); - } - })(#names[j], short, type, #sliceOffsetArguments); + + // This 'if' is either a static choice or dynamic choice depending on + // [isIntercepted]. + if (#isIntercepted) { + objectClassObject[shortName] = + (function(name, shortName, type) { + return function(receiver) { + return this.#noSuchMethodName( + receiver, + #createInvocationMirror(name, shortName, type, + // Create proper Array with all arguments except first + // (receiver). + Array.prototype.slice.call(arguments, 1), + [])); + } + })(#names[j], shortName, type); + } else { + objectClassObject[shortName] = + (function(name, shortName, type) { + return function() { + return this.#noSuchMethodName( + // Object.noSuchMethodName ignores the explicit receiver + // argument. We could pass anything in place of [this]. + this, + #createInvocationMirror(name, shortName, type, + // Create proper Array with all arguments. + Array.prototype.slice.call(arguments, 0), + [])); + } + })(#names[j], shortName, type); + } } }''', { - 'sliceOffsetParams': sliceOffsetParams, 'noSuchMethodName': namer.noSuchMethodName, 'createInvocationMirror': createInvocationMirror, 'names': minify ? 'shortNames' : 'longNames', - 'sliceOffsetArguments': sliceOffsetArguments})); + 'isIntercepted': isIntercepted})); return statements; } diff --git a/sdk/lib/_internal/compiler/js_lib/interceptors.dart b/sdk/lib/_internal/compiler/js_lib/interceptors.dart index 8573b747442..cf4d39b2936 100644 --- a/sdk/lib/_internal/compiler/js_lib/interceptors.dart +++ b/sdk/lib/_internal/compiler/js_lib/interceptors.dart @@ -250,8 +250,8 @@ findInterceptorForType(Type type) { * interceptor, methods of that name on plain unintercepted classes also use the * interceptor calling convention. The plain classes are _self-interceptors_, * and for them, `getInterceptor(r)` returns `r`. Methods on plain - * unintercepted classes have a redundant `receiver` argument and should ignore - * it in favour of `this`. + * unintercepted classes have a redundant `receiver` argument and, to enable + * some optimizations, must ignore `receiver` in favour of `this`. * * In the case of mixins, a method may be placed on both an intercepted class * and an unintercepted class. In this case, the method must use the `receiver` @@ -294,6 +294,20 @@ abstract class Interceptor { String toString() => Primitives.objectToHumanReadableString(this); + // [Interceptor.noSuchMethod] is identical to [Object.noSuchMethod]. However, + // each copy is compiled differently. The presence of the method on an + // Interceptor class forces [noSuchMethod] to use interceptor calling + // convention. In the [Interceptor] version, `this` is the explicit receiver + // argument. In the [Object] version, as Object is not an intercepted class, + // `this` is the JavaScript receiver, and the explicit receiver is ignored. + // The noSuchMethod stubs for selectors that use the interceptor calling + // convention do not know the calling convention and forward `this` and + // `receiver` to one of these noSuchMethod implementations which selects the + // correct Dart receiver. + // + // We don't allow [noSuchMethod] on intercepted classes (that would force all + // calls to use interceptor calling convention). If we did allow it, the + // interceptor context would select the correct `this`. dynamic noSuchMethod(Invocation invocation) { throw new NoSuchMethodError( this, diff --git a/sdk/lib/_internal/compiler/js_lib/js_helper.dart b/sdk/lib/_internal/compiler/js_lib/js_helper.dart index 4270ab576d2..5d23f3a8c8f 100644 --- a/sdk/lib/_internal/compiler/js_lib/js_helper.dart +++ b/sdk/lib/_internal/compiler/js_lib/js_helper.dart @@ -210,7 +210,7 @@ String S(value) { return 'null'; } var res = value.toString(); - if (res is !String) throw argumentErrorValue(value); + if (res is !String) throw _argumentError(value); return res; } diff --git a/tests/compiler/dart2js_extra/dart2js_extra.status b/tests/compiler/dart2js_extra/dart2js_extra.status index 5af67cd586a..6236c815b59 100644 --- a/tests/compiler/dart2js_extra/dart2js_extra.status +++ b/tests/compiler/dart2js_extra/dart2js_extra.status @@ -4,7 +4,6 @@ [ $compiler == dart2js ] 16407_test: Fail # Issue 16407 -23432_test: Fail # Issue 23432 class_test: Fail statements_test: Fail typed_locals_test: Fail