From 1c20a62611f59052b5170baaec9e83c434a86b07 Mon Sep 17 00:00:00 2001 From: Tim Blasi Date: Tue, 29 Mar 2016 10:55:06 -0700 Subject: [PATCH] feat(dart): Add a dev-mode check for undeclared lifecycle interfaces Add a check in ReflectionCapabilities#interfaces which determines if the passed-in type implements a Lifecycle Interface but does not declare that it does so. See https://goo.gl/b07Kii for details. Closes #6849 --- .../reflection/reflection_capabilities.dart | 111 +++++++++++++++++- .../test/core/reflection/reflector_spec.ts | 22 ++++ 2 files changed, 130 insertions(+), 3 deletions(-) diff --git a/modules/angular2/src/core/reflection/reflection_capabilities.dart b/modules/angular2/src/core/reflection/reflection_capabilities.dart index e390aa6ac8..e5709670d8 100644 --- a/modules/angular2/src/core/reflection/reflection_capabilities.dart +++ b/modules/angular2/src/core/reflection/reflection_capabilities.dart @@ -1,9 +1,12 @@ library reflection.reflection_capabilities; -import 'package:angular2/src/facade/lang.dart'; -import 'types.dart'; import 'dart:mirrors'; + +import 'package:angular2/src/core/linker/interfaces.dart'; +import 'package:angular2/src/facade/lang.dart'; + import 'platform_reflection_capabilities.dart'; +import 'types.dart'; var DOT_REGEX = new RegExp('\\.'); @@ -272,7 +275,9 @@ class ReflectionCapabilities implements PlatformReflectionCapabilities { } List interfaces(type) { - return _interfacesFromMirror(reflectType(type)); + final clazz = reflectType(type); + _assertDeclaresLifecycleHooks(clazz); + return _interfacesFromMirror(clazz); } List _interfacesFromMirror(classMirror) { @@ -324,3 +329,103 @@ class ReflectionCapabilities implements PlatformReflectionCapabilities { return '${(reflectClass(type).owner as LibraryMirror).uri}'; } } + +final _lifecycleHookMirrors = [ + reflectType(AfterContentChecked), + reflectType(AfterContentInit), + reflectType(AfterViewChecked), + reflectType(AfterViewInit), + reflectType(DoCheck), + reflectType(OnChanges), + reflectType(OnDestroy), + reflectType(OnInit), +]; + +/// Checks whether [clazz] implements lifecycle ifaces without declaring them. +/// +/// Due to Dart implementation details, lifecycle hooks are only called when a +/// class explicitly declares that it implements the associated interface. +/// See https://goo.gl/b07Kii for details. +void _assertDeclaresLifecycleHooks(ClassMirror clazz) { + final missingDeclarations = []; + for (var iface in _lifecycleHookMirrors) { + if (!_checkDeclares(clazz, iface: iface) && + _checkImplements(clazz, iface: iface)) { + missingDeclarations.add(iface); + } + } + if (missingDeclarations.isNotEmpty) { + throw new MissingInterfaceError(clazz, missingDeclarations); + } +} + +/// Returns whether [clazz] declares that it implements [iface]. +/// +/// Returns `false` if [clazz] implements [iface] but does not declare it. +/// Returns `false` if [clazz]'s superclass declares that it +/// implements [iface]. +bool _checkDeclares(ClassMirror clazz, {ClassMirror iface: null}) { + if (iface == null) { + throw new ArgumentError.notNull('iface'); + } + return clazz.superinterfaces.contains(iface); +} + +/// Returns whether [clazz] implements [iface]. +/// +/// Returns `true` if [clazz] implements [iface] and does not declare it. +/// Returns `true` if [clazz]'s superclass implements [iface]. +/// +/// This is an approximation of a JavaScript feature check: +/// ```js +/// var matches = true; +/// for (var prop in iface) { +/// if (iface.hasOwnProperty(prop)) { +/// matches = matches && clazz.hasOwnProperty(prop); +/// } +/// } +/// return matches; +/// ``` +bool _checkImplements(ClassMirror clazz, {ClassMirror iface: null}) { + if (iface == null) { + throw new ArgumentError.notNull('iface'); + } + + var matches = true; + iface.declarations.forEach((symbol, declarationMirror) { + if (!matches) return; + if (declarationMirror.isConstructor || declarationMirror.isPrivate) return; + matches = clazz.declarations.keys.contains(symbol); + }); + if (!matches && clazz.superclass != null) { + matches = _checkImplements(clazz.superclass, iface: iface); + } + if (!matches && clazz.mixin != clazz) { + matches = _checkImplements(clazz.mixin, iface: iface); + } + + return matches; +} + +/// Error thrown when a class implements a lifecycle iface it does not declare. +class MissingInterfaceError extends Error { + final ClassMirror clazz; + final List missingDeclarations; + + MissingInterfaceError(this.clazz, this.missingDeclarations); + + @override + String toString() { + final buf = new StringBuffer(); + buf.write('${clazz.simpleName} implements '); + if (missingDeclarations.length == 1) { + buf.write('an interface but does not declare it: '); + } else { + buf.write('interfaces but does not declare them: '); + } + buf.write( + missingDeclarations.map((d) => d.simpleName.toString()).join(', ')); + buf.write('. See https://goo.gl/b07Kii for more info.'); + return buf.toString(); + } +} diff --git a/modules/angular2/test/core/reflection/reflector_spec.ts b/modules/angular2/test/core/reflection/reflector_spec.ts index 73f5f24802..bf5b295f54 100644 --- a/modules/angular2/test/core/reflection/reflector_spec.ts +++ b/modules/angular2/test/core/reflection/reflector_spec.ts @@ -7,6 +7,7 @@ import { beforeEach, browserDetection } from 'angular2/testing_internal'; +import {OnInit} from 'angular2/core'; import {Reflector, ReflectionInfo} from 'angular2/src/core/reflection/reflection'; import {ReflectionCapabilities} from 'angular2/src/core/reflection/reflection_capabilities'; import { @@ -65,6 +66,19 @@ class SuperClassImplementingInterface implements Interface2 {} class ClassImplementingInterface extends SuperClassImplementingInterface implements Interface {} +// Classes used to test our runtime check for classes that implement lifecycle interfaces but do not +// declare them. +// See https://github.com/angular/angular/pull/6879 and https://goo.gl/b07Kii for details. +class ClassDoesNotDeclareOnInit { + ngOnInit() {} +} + +class SuperClassImplementingOnInit implements OnInit { + ngOnInit() {} +} + +class SubClassDoesNotDeclareOnInit extends SuperClassImplementingOnInit {} + export function main() { describe('Reflector', () => { var reflector; @@ -214,6 +228,14 @@ export function main() { var p = reflector.interfaces(ClassWithDecorators); expect(p).toEqual([]); }); + + it("should throw for undeclared lifecycle interfaces", + () => { expect(() => reflector.interfaces(ClassDoesNotDeclareOnInit)).toThrowError(); }); + + it("should throw for class inheriting a lifecycle impl and not declaring the interface", + () => { + expect(() => reflector.interfaces(SubClassDoesNotDeclareOnInit)).toThrowError(); + }); }); }