fix(ivy): skip field inheritance if InheritDefinitionFeature is prese…

…nt on parent def (#34244)

The main logic of the `InheritDefinitionFeature` is to go through the prototype chain of a given Component and merge all Angular-specific information onto that Component def. The problem happens in case there is a Component in a hierarchy that also contains the `InheritDefinitionFeature` (i.e. it extends some other Component), so it inherits all Angular-specific information from its super class. As a result, the root Component may end up having duplicate information inherited from different Components in hierarchy.

Let's consider the following structure: `GrandChild` extends `Child` that extends `Base` and the `Base` class has a `HostListener`. In this scenario `GrandChild` and `Child` will have `InheritDefinitionFeature` included into the `features` list. The processing will happend in the following order:

- `Child` inherits `HostListener` from the `Base` class
- `GrandChild` inherits `HostListener` from the `Child` class
- since `Child` has a parent, `GrandChild` also inherits from the `Base` class

The result is that the `GrandChild` def has duplicated host listener, which is not correct.

This commit introduces additional logic that checks whether we came across a def that has `InheritDefinitionFeature` feature (which means that this def already inherited information from its super classes). If that's the case, we skip further fields-related inheritance logic, but keep going though the prototype chain to look for super classes that contain other features (like NgOnChanges), that we need to invoke for a given Component def.

PR Close #34244

packages/core/src/render3/features/inherit_definition_feature.ts

@@ -25,6 +25,7 @@ export function getSuperType(type: Type<any>): Type<any>&
  */
 export function ɵɵInheritDefinitionFeature(definition: DirectiveDef<any>| ComponentDef<any>): void {
   let superType = getSuperType(definition.type);
+  let shouldInheritFields = true;
 
   while (superType) {
     let superDef: DirectiveDef<any>|ComponentDef<any>|undefined = undefined;
@@ -40,38 +41,40 @@ export function ɵɵInheritDefinitionFeature(definition: DirectiveDef<any>| Comp
     }
 
     if (superDef) {
-      // Some fields in the definition may be empty, if there were no values to put in them that
-      // would've justified object creation. Unwrap them if necessary.
-      const writeableDef = definition as any;
-      writeableDef.inputs = maybeUnwrapEmpty(definition.inputs);
-      writeableDef.declaredInputs = maybeUnwrapEmpty(definition.declaredInputs);
-      writeableDef.outputs = maybeUnwrapEmpty(definition.outputs);
-
-      // Merge hostBindings
-      const superHostBindings = superDef.hostBindings;
-      superHostBindings && inheritHostBindings(definition, superHostBindings);
-
-      // Merge queries
-      const superViewQuery = superDef.viewQuery;
-      const superContentQueries = superDef.contentQueries;
-      superViewQuery && inheritViewQuery(definition, superViewQuery);
-      superContentQueries && inheritContentQueries(definition, superContentQueries);
-
-      // Merge inputs and outputs
-      fillProperties(definition.inputs, superDef.inputs);
-      fillProperties(definition.declaredInputs, superDef.declaredInputs);
-      fillProperties(definition.outputs, superDef.outputs);
-
-      // Inherit hooks
-      // Assume super class inheritance feature has already run.
-      definition.afterContentChecked =
-          definition.afterContentChecked || superDef.afterContentChecked;
-      definition.afterContentInit = definition.afterContentInit || superDef.afterContentInit;
-      definition.afterViewChecked = definition.afterViewChecked || superDef.afterViewChecked;
-      definition.afterViewInit = definition.afterViewInit || superDef.afterViewInit;
-      definition.doCheck = definition.doCheck || superDef.doCheck;
-      definition.onDestroy = definition.onDestroy || superDef.onDestroy;
-      definition.onInit = definition.onInit || superDef.onInit;
+      if (shouldInheritFields) {
+        // Some fields in the definition may be empty, if there were no values to put in them that
+        // would've justified object creation. Unwrap them if necessary.
+        const writeableDef = definition as any;
+        writeableDef.inputs = maybeUnwrapEmpty(definition.inputs);
+        writeableDef.declaredInputs = maybeUnwrapEmpty(definition.declaredInputs);
+        writeableDef.outputs = maybeUnwrapEmpty(definition.outputs);
+
+        // Merge hostBindings
+        const superHostBindings = superDef.hostBindings;
+        superHostBindings && inheritHostBindings(definition, superHostBindings);
+
+        // Merge queries
+        const superViewQuery = superDef.viewQuery;
+        const superContentQueries = superDef.contentQueries;
+        superViewQuery && inheritViewQuery(definition, superViewQuery);
+        superContentQueries && inheritContentQueries(definition, superContentQueries);
+
+        // Merge inputs and outputs
+        fillProperties(definition.inputs, superDef.inputs);
+        fillProperties(definition.declaredInputs, superDef.declaredInputs);
+        fillProperties(definition.outputs, superDef.outputs);
+
+        // Inherit hooks
+        // Assume super class inheritance feature has already run.
+        definition.afterContentChecked =
+            definition.afterContentChecked || superDef.afterContentChecked;
+        definition.afterContentInit = definition.afterContentInit || superDef.afterContentInit;
+        definition.afterViewChecked = definition.afterViewChecked || superDef.afterViewChecked;
+        definition.afterViewInit = definition.afterViewInit || superDef.afterViewInit;
+        definition.doCheck = definition.doCheck || superDef.doCheck;
+        definition.onDestroy = definition.onDestroy || superDef.onDestroy;
+        definition.onInit = definition.onInit || superDef.onInit;
+      }
 
       // Run parent features
       const features = superDef.features;
@@ -81,6 +84,16 @@ export function ɵɵInheritDefinitionFeature(definition: DirectiveDef<any>| Comp
           if (feature && feature.ngInherit) {
             (feature as DirectiveDefFeature)(definition);
           }
+          // If `InheritDefinitionFeature` is a part of the current `superDef`, it means that this
+          // def already has all the necessary information inherited from its super class(es), so we
+          // can stop merging fields from super classes. However we need to iterate through the
+          // prototype chain to look for classes that might contain other "features" (like
+          // NgOnChanges), which we should invoke for the original `definition`. We set the
+          // `shouldInheritFields` flag to indicate that, essentially skipping fields inheritance
+          // logic and only invoking functions from the "features" list.
+          if (feature === ɵɵInheritDefinitionFeature) {
+            shouldInheritFields = false;
+          }
         }
       }
     }
@@ -104,7 +117,6 @@ function maybeUnwrapEmpty(value: any): any {
 function inheritViewQuery(
     definition: DirectiveDef<any>| ComponentDef<any>, superViewQuery: ViewQueriesFunction<any>) {
   const prevViewQuery = definition.viewQuery;
-
   if (prevViewQuery) {
     definition.viewQuery = (rf, ctx) => {
       superViewQuery(rf, ctx);
@@ -119,7 +131,6 @@ function inheritContentQueries(
     definition: DirectiveDef<any>| ComponentDef<any>,
     superContentQueries: ContentQueriesFunction<any>) {
   const prevContentQueries = definition.contentQueries;
-
   if (prevContentQueries) {
     definition.contentQueries = (rf, ctx, directiveIndex) => {
       superContentQueries(rf, ctx, directiveIndex);
@@ -134,17 +145,12 @@ function inheritHostBindings(
     definition: DirectiveDef<any>| ComponentDef<any>,
     superHostBindings: HostBindingsFunction<any>) {
   const prevHostBindings = definition.hostBindings;
-  // If the subclass does not have a host bindings function, we set the subclass host binding
-  // function to be the superclass's (in this feature). We should check if they're the same here
-  // to ensure we don't inherit it twice.
-  if (superHostBindings !== prevHostBindings) {
-    if (prevHostBindings) {
-      definition.hostBindings = (rf: RenderFlags, ctx: any, elementIndex: number) => {
-        superHostBindings(rf, ctx, elementIndex);
-        prevHostBindings(rf, ctx, elementIndex);
-      };
-    } else {
-      definition.hostBindings = superHostBindings;
-    }
+  if (prevHostBindings) {
+    definition.hostBindings = (rf: RenderFlags, ctx: any, elementIndex: number) => {
+      superHostBindings(rf, ctx, elementIndex);
+      prevHostBindings(rf, ctx, elementIndex);
+    };
+  } else {
+    definition.hostBindings = superHostBindings;
   }
 }

packages/core/test/acceptance/inherit_definition_feature_spec.ts

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {Component, ContentChildren, Directive, EventEmitter, HostBinding, Input, OnChanges, Output, QueryList, ViewChildren} from '@angular/core';
+import {Component, ContentChildren, Directive, EventEmitter, HostBinding, HostListener, Input, OnChanges, Output, QueryList, ViewChildren} from '@angular/core';
 import {TestBed} from '@angular/core/testing';
 import {By} from '@angular/platform-browser';
 import {onlyInIvy} from '@angular/private/testing';
@@ -4225,6 +4225,71 @@ describe('inheritance', () => {
       expect(foundQueryList !.length).toBe(5);
     });
 
+    it('should inherit host listeners from base class once', () => {
+      const events: string[] = [];
+
+      @Component({
+        selector: 'app-base',
+        template: 'base',
+      })
+      class BaseComponent {
+        @HostListener('click')
+        clicked() { events.push('BaseComponent.clicked'); }
+      }
+
+      @Component({
+        selector: 'app-child',
+        template: 'child',
+      })
+      class ChildComponent extends BaseComponent {
+        // additional host listeners are defined here to have `hostBindings` function generated on
+        // component def, which would trigger `hostBindings` functions merge operation in
+        // InheritDefinitionFeature logic (merging Child and Base host binding functions)
+        @HostListener('focus')
+        focused() {}
+
+        clicked() { events.push('ChildComponent.clicked'); }
+      }
+
+      @Component({
+        selector: 'app-grand-child',
+        template: 'grand-child',
+      })
+      class GrandChildComponent extends ChildComponent {
+        // additional host listeners are defined here to have `hostBindings` function generated on
+        // component def, which would trigger `hostBindings` functions merge operation in
+        // InheritDefinitionFeature logic (merging GrandChild and Child host binding functions)
+        @HostListener('blur')
+        blurred() {}
+
+        clicked() { events.push('GrandChildComponent.clicked'); }
+      }
+
+      @Component({
+        selector: 'root-app',
+        template: `
+          <app-base></app-base>
+          <app-child></app-child>
+          <app-grand-child></app-grand-child>
+        `,
+      })
+      class RootApp {
+      }
+
+      const components = [BaseComponent, ChildComponent, GrandChildComponent];
+      TestBed.configureTestingModule({
+        declarations: [RootApp, ...components],
+      });
+      const fixture = TestBed.createComponent(RootApp);
+      fixture.detectChanges();
+
+      components.forEach(component => {
+        fixture.debugElement.query(By.directive(component)).nativeElement.click();
+      });
+      expect(events).toEqual(
+          ['BaseComponent.clicked', 'ChildComponent.clicked', 'GrandChildComponent.clicked']);
+    });
+
     xdescribe(
         'what happens when...',
         () => {