fix(ivy): handle overloaded constructors in ngtsc (#34590)

Currently ngtsc looks for the first `ConstructorDeclaration` when figuring out what the parameters are so that it can generate the DI instructions. The problem is that if a constructor has overloads, it'll have several `ConstructorDeclaration` members with a different number of parameters. These changes tweak the logic so it looks for the constructor implementation.

PR Close #34590

Author: crisbeto

packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts

@@ -35,8 +35,12 @@ export class TypeScriptReflectionHost implements ReflectionHost {
   getConstructorParameters(clazz: ClassDeclaration): CtorParameter[]|null {
     const tsClazz = castDeclarationToClassOrDie(clazz);
 
-    // First, find the constructor.
-    const ctor = tsClazz.members.find(ts.isConstructorDeclaration);
+    // First, find the constructor with a `body`. The constructors without a `body` are overloads
+    // whereas we want the implementation since it's the one that'll be executed and which can
+    // have decorators.
+    const ctor = tsClazz.members.find(
+        (member): member is ts.ConstructorDeclaration =>
+            ts.isConstructorDeclaration(member) && member.body !== undefined);
     if (ctor === undefined) {
       return null;
     }
@@ -564,4 +568,4 @@ function getExportedName(decl: ts.Declaration, originalId: ts.Identifier): strin
   return ts.isImportSpecifier(decl) ?
       (decl.propertyName !== undefined ? decl.propertyName : decl.name).text :
       originalId.text;
-}
+}

packages/compiler-cli/src/ngtsc/reflection/test/ts_host_spec.ts

@@ -211,6 +211,29 @@ runInEachFileSystem(() => {
         expect(args.length).toBe(1);
         expectParameter(args[0], 'bar', {moduleName: './bar', name: 'Bar'});
       });
+
+      it('should reflect the arguments from an overloaded constructor', () => {
+        const {program} = makeProgram([{
+          name: _('/entry.ts'),
+          contents: `
+            class Bar {}
+            class Baz {}
+
+            class Foo {
+              constructor(bar: Bar);
+              constructor(bar: Bar, baz?: Baz) {}
+            }
+        `
+        }]);
+        const clazz = getDeclaration(program, _('/entry.ts'), 'Foo', isNamedClassDeclaration);
+        const checker = program.getTypeChecker();
+        const host = new TypeScriptReflectionHost(checker);
+        const args = host.getConstructorParameters(clazz) !;
+        expect(args.length).toBe(2);
+        expectParameter(args[0], 'bar', 'Bar');
+        expectParameter(args[1], 'baz', 'Baz');
+      });
+
     });
 
 

packages/compiler-cli/test/compliance/r3_view_compiler_di_spec.ts

@@ -99,6 +99,41 @@ describe('compiler compliance: dependency injection', () => {
     expectEmit(result.source, def, 'Incorrect injectable definition');
   });
 
+  it('should create a factory definition for an injectable with an overloaded constructor', () => {
+    const files = {
+      app: {
+        'spec.ts': `
+          import {Injectable, Optional} from '@angular/core';
+
+          class MyDependency {}
+          class MyOptionalDependency {}
+
+          @Injectable()
+          export class MyService {
+            constructor(dep: MyDependency);
+            constructor(dep: MyDependency, @Optional() optionalDep?: MyOptionalDependency) {}
+          }
+        `
+      }
+    };
+
+    const factory = `
+      MyService.ɵfac = function MyService_Factory(t) {
+        return new (t || MyService)($r3$.ɵɵinject(MyDependency), $r3$.ɵɵinject(MyOptionalDependency, 8));
+      }`;
+
+    const def = `
+      MyService.ɵprov = $r3$.ɵɵdefineInjectable({
+        token: MyService,
+        factory: MyService.ɵfac
+      });
+    `;
+
+    const result = compile(files, angularFiles);
+    expectEmit(result.source, factory, 'Incorrect factory definition');
+    expectEmit(result.source, def, 'Incorrect injectable definition');
+  });
+
   it('should create a single factory def if the class has more than one decorator', () => {
     const files = {
       app: {

packages/compiler-cli/test/ngtsc/ngtsc_spec.ts

@@ -192,6 +192,32 @@ runInEachFileSystem(os => {
       expect(jsContents).toContain('inject(Dep, 8)');
     });
 
+    it('should compile @Injectable with constructor overloads', () => {
+      env.write('test.ts', `
+      import {Injectable, Optional} from '@angular/core';
+
+      @Injectable()
+      class Dep {}
+
+      @Injectable()
+      class OptionalDep {}
+
+      @Injectable()
+      class Service {
+        constructor(dep: Dep);
+
+        constructor(dep: Dep, @Optional() optionalDep?: OptionalDep) {}
+      }
+    `);
+      env.driveMain();
+      const jsContents = env.getContents('test.js');
+
+      expect(jsContents)
+          .toContain(
+              `Service.ɵfac = function Service_Factory(t) { ` +
+              `return new (t || Service)(i0.ɵɵinject(Dep), i0.ɵɵinject(OptionalDep, 8)); };`);
+    });
+
     it('should compile Directives without errors', () => {
       env.write('test.ts', `
         import {Directive} from '@angular/core';

packages/core/test/acceptance/di_spec.ts

@@ -982,6 +982,30 @@ describe('di', () => {
             `This will become an error in v10. Please add @Injectable() to the "MyRootService" class.`);
       }
     });
+
+    it('should inject services in constructor with overloads', () => {
+      @Injectable({providedIn: 'root'})
+      class MyService {
+      }
+
+      @Injectable({providedIn: 'root'})
+      class MyOtherService {
+      }
+
+      @Component({template: ''})
+      class MyComp {
+        constructor(myService: MyService);
+        constructor(
+            public myService: MyService, @Optional() public myOtherService?: MyOtherService) {}
+      }
+      TestBed.configureTestingModule({declarations: [MyComp]});
+      const fixture = TestBed.createComponent(MyComp);
+      fixture.detectChanges();
+
+      expect(fixture.componentInstance.myService instanceof MyService).toBe(true);
+      expect(fixture.componentInstance.myOtherService instanceof MyOtherService).toBe(true);
+    });
+
   });
 
   describe('inject', () => {