fix(ivy): proper component resolution in case of inheritance (#28439)

Ivy allows Components to extend Directives (but not the other way around) and as a result we may have Component and Directive annotations present at the same time. The logic that resolves annotations to pick the necessary one didn't take this into account and as a result Components were recognized as Directives (and vice versa) in case of inheritance. This change updates the resolution logic by picking known annotation that is the nearest one (in inheritance tree) and compares it with expected type. That should help avoid mis-classification of Components/Directives during resolution.

PR Close #28439

packages/core/src/render3/jit/module.ts

@@ -345,9 +345,12 @@ function setScopeOnDeclaredComponents(moduleType: Type<any>, ngModule: NgModule)
  */
 export function patchComponentDefWithScope<C>(
     componentDef: ComponentDef<C>, transitiveScopes: NgModuleTransitiveScopes) {
-  componentDef.directiveDefs = () => Array.from(transitiveScopes.compilation.directives)
+  componentDef.directiveDefs = () =>
-                                         .map(dir => getDirectiveDef(dir) || getComponentDef(dir) !)
+      Array.from(transitiveScopes.compilation.directives)
-                                         .filter(def => !!def);
+          .map(
+              dir => dir.hasOwnProperty(NG_COMPONENT_DEF) ? getComponentDef(dir) ! :
+                                                            getDirectiveDef(dir) !)
+          .filter(def => !!def);
   componentDef.pipeDefs = () =>
       Array.from(transitiveScopes.compilation.pipes).map(pipe => getPipeDef(pipe) !);
 }

packages/core/test/linker/inheritance_integration_spec.ts

@@ -0,0 +1,93 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {Component, Directive, HostBinding} from '@angular/core';
+import {TestBed} from '@angular/core/testing';
+import {describe, expect, it} from '@angular/core/testing/src/testing_internal';
+import {modifiedInIvy, onlyInIvy} from '@angular/private/testing';
+
+@Directive({selector: '[directiveA]'})
+class DirectiveA {
+}
+
+@Directive({selector: '[directiveB]'})
+class DirectiveB {
+  @HostBinding('title')
+  title = 'DirectiveB Title';
+}
+
+@Component({selector: 'component-a', template: 'ComponentA Template'})
+class ComponentA {
+}
+
+@Component(
+    {selector: 'component-extends-directive', template: 'ComponentExtendsDirective Template'})
+class ComponentExtendsDirective extends DirectiveA {
+}
+
+class ComponentWithNoAnnotation extends ComponentA {}
+
+@Directive({selector: '[directiveExtendsComponent]'})
+class DirectiveExtendsComponent extends ComponentA {
+  @HostBinding('title')
+  title = 'DirectiveExtendsComponent Title';
+}
+
+class DirectiveWithNoAnnotation extends DirectiveB {}
+
+@Component({selector: 'my-app', template: '...'})
+class App {
+}
+
+describe('Inheritance logic', () => {
+  it('should handle Components that extend Directives', () => {
+    TestBed.configureTestingModule({declarations: [ComponentExtendsDirective, App]});
+    const template = '<component-extends-directive></component-extends-directive>';
+    TestBed.overrideComponent(App, {set: {template}});
+    const fixture = TestBed.createComponent(App);
+    fixture.detectChanges();
+    expect(fixture.nativeElement.firstChild.innerHTML).toBe('ComponentExtendsDirective Template');
+  });
+
+  it('should handle classes with no annotations that extend Components', () => {
+    TestBed.configureTestingModule({declarations: [ComponentWithNoAnnotation, App]});
+    const template = '<component-a></component-a>';
+    TestBed.overrideComponent(App, {set: {template}});
+    const fixture = TestBed.createComponent(App);
+    fixture.detectChanges();
+    expect(fixture.nativeElement.firstChild.innerHTML).toBe('ComponentA Template');
+  });
+
+  it('should handle classes with no annotations that extend Directives', () => {
+    TestBed.configureTestingModule({declarations: [DirectiveWithNoAnnotation, App]});
+    const template = '<div directiveB></div>';
+    TestBed.overrideComponent(App, {set: {template}});
+    const fixture = TestBed.createComponent(App);
+    fixture.detectChanges();
+    expect(fixture.nativeElement.firstChild.title).toBe('DirectiveB Title');
+  });
+
+  modifiedInIvy('View Engine allows Directives to extend Components')
+      .it('should handle Directives that extend Components', () => {
+        TestBed.configureTestingModule({declarations: [DirectiveExtendsComponent, App]});
+        const template = '<div directiveExtendsComponent>Some content</div>';
+        TestBed.overrideComponent(App, {set: {template}});
+        const fixture = TestBed.createComponent(App);
+        fixture.detectChanges();
+        expect(fixture.nativeElement.firstChild.title).toBe('DirectiveExtendsComponent Title');
+      });
+
+  onlyInIvy('Ivy does not allow Directives to extend Components')
+      .it('should throw in case a Directive tries to extend a Component', () => {
+        TestBed.configureTestingModule({declarations: [DirectiveExtendsComponent, App]});
+        const template = '<div directiveExtendsComponent>Some content</div>';
+        TestBed.overrideComponent(App, {set: {template}});
+        expect(() => TestBed.createComponent(App))
+            .toThrowError('Directives cannot inherit Components');
+      });
+});

packages/core/test/test_bed_spec.ts

@@ -257,42 +257,67 @@ describe('TestBed', () => {
   });
 
   onlyInIvy('patched ng defs should be removed after resetting TestingModule')
-      .it('make sure we restore ng defs to their initial states', () => {
+      .describe('resetting ng defs', () => {
-        @Pipe({name: 'somePipe', pure: true})
+        it('should restore ng defs to their initial states', () => {
-        class SomePipe {
+          @Pipe({name: 'somePipe', pure: true})
-          transform(value: string): string { return `transformed ${value}`; }
+          class SomePipe {
-        }
+            transform(value: string): string { return `transformed ${value}`; }
+          }
 
-        @Directive({selector: 'someDirective'})
+          @Directive({selector: 'someDirective'})
-        class SomeDirective {
+          class SomeDirective {
-          someProp = 'hello';
+            someProp = 'hello';
-        }
+          }
 
-        @Component({selector: 'comp', template: 'someText'})
+          @Component({selector: 'comp', template: 'someText'})
-        class SomeComponent {
+          class SomeComponent {
-        }
+          }
 
-        @NgModule({declarations: [SomeComponent]})
+          @NgModule({declarations: [SomeComponent]})
-        class SomeModule {
+          class SomeModule {
-        }
+          }
 
-        TestBed.configureTestingModule({imports: [SomeModule]});
+          TestBed.configureTestingModule({imports: [SomeModule]});
 
-        // adding Pipe and Directive via metadata override
+          // adding Pipe and Directive via metadata override
-        TestBed.overrideModule(
+          TestBed.overrideModule(
-            SomeModule, {set: {declarations: [SomeComponent, SomePipe, SomeDirective]}});
+              SomeModule, {set: {declarations: [SomeComponent, SomePipe, SomeDirective]}});
-        TestBed.overrideComponent(
+          TestBed.overrideComponent(
-            SomeComponent, {set: {template: `<span someDirective>{{'hello' | somePipe}}</span>`}});
+              SomeComponent,
-        TestBed.createComponent(SomeComponent);
+              {set: {template: `<span someDirective>{{'hello' | somePipe}}</span>`}});
+          TestBed.createComponent(SomeComponent);
 
-        const defBeforeReset = (SomeComponent as any).ngComponentDef;
+          const defBeforeReset = (SomeComponent as any).ngComponentDef;
-        expect(defBeforeReset.pipeDefs().length).toEqual(1);
+          expect(defBeforeReset.pipeDefs().length).toEqual(1);
-        expect(defBeforeReset.directiveDefs().length).toEqual(2);  // directive + component
+          expect(defBeforeReset.directiveDefs().length).toEqual(2);  // directive + component
 
-        TestBed.resetTestingModule();
+          TestBed.resetTestingModule();
 
-        const defAfterReset = (SomeComponent as any).ngComponentDef;
+          const defAfterReset = (SomeComponent as any).ngComponentDef;
-        expect(defAfterReset.pipeDefs().length).toEqual(0);
+          expect(defAfterReset.pipeDefs().length).toEqual(0);
-        expect(defAfterReset.directiveDefs().length).toEqual(1);  // component
+          expect(defAfterReset.directiveDefs().length).toEqual(1);  // component
+        });
+
+        it('should cleanup ng defs for classes with no ng annotations (in case of inheritance)',
+           () => {
+             @Component({selector: 'someDirective', template: '...'})
+             class SomeComponent {
+             }
+
+             class ComponentWithNoAnnotations extends SomeComponent {}
+
+             TestBed.configureTestingModule({declarations: [ComponentWithNoAnnotations]});
+             TestBed.createComponent(ComponentWithNoAnnotations);
+
+             expect(ComponentWithNoAnnotations.hasOwnProperty('ngComponentDef')).toBeTruthy();
+             expect(SomeComponent.hasOwnProperty('ngComponentDef')).toBeTruthy();
+
+             TestBed.resetTestingModule();
+
+             expect(ComponentWithNoAnnotations.hasOwnProperty('ngComponentDef')).toBeFalsy();
+
+             // ngComponentDef should be preserved on super component
+             expect(SomeComponent.hasOwnProperty('ngComponentDef')).toBeTruthy();
+           });
       });
 });

packages/core/testing/src/r3_test_bed.ts

@@ -336,7 +336,18 @@ export class TestBedRender3 implements Injector, TestBed {
 
     // restore initial component/directive/pipe defs
     this._initiaNgDefs.forEach((value: [string, PropertyDescriptor], type: Type<any>) => {
-      Object.defineProperty(type, value[0], value[1]);
+      const [prop, descriptor] = value;
+      if (!descriptor) {
+        // Delete operations are generally undesirable since they have performance implications on
+        // objects they were applied to. In this particular case, situations where this code is
+        // invoked should be quite rare to cause any noticable impact, since it's applied only to
+        // some test cases (for example when class with no annotations extends some @Component) when
+        // we need to clear 'ngComponentDef' field on a given class to restore its original state
+        // (before applying overrides and running tests).
+        delete (type as any)[prop];
+      } else {
+        Object.defineProperty(type, prop, descriptor);
+      }
     });
     this._initiaNgDefs.clear();
     clearResolutionOfComponentResourcesQueue();

packages/core/testing/src/resolvers.ts

@@ -37,9 +37,21 @@ abstract class OverrideResolver<T> implements Resolver<T> {
   }
 
   getAnnotation(type: Type<any>): T|null {
-    // We should always return the last match from filter(), or we may return superclass data by
+    const annotations = reflection.annotations(type);
-    // mistake.
+    // Try to find the nearest known Type annotation and make sure that this annotation is an
-    return reflection.annotations(type).filter(a => a instanceof this.type).pop() || null;
+    // instance of the type we are looking for, so we can use it for resolution. Note: there might
+    // be multiple known annotations found due to the fact that Components can extend Directives (so
+    // both Directive and Component annotations would be present), so we always check if the known
+    // annotation has the right type.
+    for (let i = annotations.length - 1; i >= 0; i--) {
+      const annotation = annotations[i];
+      const isKnownType = annotation instanceof Directive || annotation instanceof Component ||
+          annotation instanceof Pipe || annotation instanceof NgModule;
+      if (isKnownType) {
+        return annotation instanceof this.type ? annotation : null;
+      }
+    }
+    return null;
   }
 
   resolve(type: Type<any>): T|null {