fix(compiler-cli): only consider used pipes for inline type-check requirement (#46807)

JoostK · thePunderWoman · commit 99697dae668d · 2022-07-12T21:09:18.000Z
After a bugfix in #46096, the compiler is now better capable of detecting pipes which require an inline type constructor. However, there is an issue in how all pipes are considered when verifying the inline type-ctor requirement: it should only check actually used pipes. Fixes #46747 PR Close #46807
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts
@@ -263,8 +263,16 @@ export class TypeCheckContextImpl implements TypeCheckContext {
       templateDiagnostics,
     });
 
+    const usedPipes: Reference<ClassDeclaration<ts.ClassDeclaration>>[] = [];
+    for (const name of boundTarget.getUsedPipes()) {
+      if (!pipes.has(name)) {
+        continue;
+      }
+      usedPipes.push(pipes.get(name)!);
+    }
+
     const inliningRequirement =
-        requiresInlineTypeCheckBlock(ref, shimData.file, pipes, this.reflector);
+        requiresInlineTypeCheckBlock(ref, shimData.file, usedPipes, this.reflector);
 
     // If inlining is not supported, but is required for either the TCB or one of its directive
     // dependencies, then exit here with an error.
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/tcb_util.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/tcb_util.ts
@@ -73,7 +73,7 @@ export enum TcbInliningRequirement {
 
 export function requiresInlineTypeCheckBlock(
     ref: Reference<ClassDeclaration<ts.ClassDeclaration>>, env: ReferenceEmitEnvironment,
-    usedPipes: Map<string, Reference<ClassDeclaration<ts.ClassDeclaration>>>,
+    usedPipes: Reference<ClassDeclaration<ts.ClassDeclaration>>[],
     reflector: ReflectionHost): TcbInliningRequirement {
   // In order to qualify for a declared TCB (not inline) two conditions must be met:
   // 1) the class must be suitable to be referenced from `env` (e.g. it must be exported)
@@ -85,7 +85,7 @@ export function requiresInlineTypeCheckBlock(
     // Condition 2 is false, the class has constrained generic types. It should be checked with an
     // inline TCB if possible, but can potentially use fallbacks to avoid inlining if not.
     return TcbInliningRequirement.ShouldInlineForGenericBounds;
-  } else if (Array.from(usedPipes.values()).some(pipeRef => !env.canReferenceType(pipeRef))) {
+  } else if (usedPipes.some(pipeRef => !env.canReferenceType(pipeRef))) {
     // If one of the pipes used by the component is not exported, a non-inline TCB will not be able
     // to import it, so this requires an inline TCB.
     return TcbInliningRequirement.MustInline;
diff --git a/packages/language-service/test/diagnostic_spec.ts b/packages/language-service/test/diagnostic_spec.ts
@@ -296,6 +296,48 @@ describe('getSemanticDiagnostics', () => {
     expect(diags.length).toBe(0);
   });
 
+  it('should exclude unused pipes that would otherwise require an inline TCB', () => {
+    const files = {
+      // Declare an external package that exports `MyPipeModule` without also exporting `MyPipe`
+      // from its public API. This means that `MyPipe` cannot be imported using the package name
+      // as module specifier.
+      'node_modules/pipe/pipe.d.ts': `
+        import {ɵɵDirectiveDeclaration} from '@angular/core';
+
+        export declare class MyPipe {
+          static ɵpipe: ɵɵPipeDeclaration<MyPipe, "myPipe", false>;
+        }
+      `,
+      'node_modules/pipe/index.d.ts': `
+        import {ɵɵNgModuleDeclaration} from '@angular/core';
+        import {MyPipe} from './pipe';
+
+        export declare class MyPipeModule {
+          static ɵmod: ɵɵNgModuleDeclaration<MyPipeModule, [typeof MyPipe], never, [typeof MyPipe]>;
+        }
+      `,
+      'app.ts': `
+        import {Component, NgModule} from '@angular/core';
+        import {MyPipeModule} from 'pipe';
+
+        @Component({
+          template: 'Simple template that does not use "myPipe"',
+        })
+        export class MyComponent {}
+
+        @NgModule({
+          declarations: [MyComponent],
+          imports: [MyPipeModule],
+        })
+        export class MyModule {}
+      `
+    };
+
+    const project = env.addProject('test', files);
+    const diags = project.getDiagnosticsForFile('app.ts');
+    expect(diags.length).toBe(0);
+  });
+
   it('logs perf tracing', () => {
     const files = {
       'app.ts': `
diff --git a/packages/language-service/testing/src/project.ts b/packages/language-service/testing/src/project.ts
@@ -68,7 +68,7 @@ export class Project {
       const dirPath = fs.dirname(filePath);
       fs.ensureDir(dirPath);
       fs.writeFile(filePath, contents);
-      if (projectFilePath.endsWith('.ts')) {
+      if (projectFilePath.endsWith('.ts') && !projectFilePath.endsWith('.d.ts')) {
         entryFiles.push(filePath);
       }
     }

Original file line number	Diff line number	Diff line change
`@@ -68,7 +68,7 @@ export class Project {`
`68`	`68`	`const dirPath = fs.dirname(filePath);`
`69`	`69`	`fs.ensureDir(dirPath);`
`70`	`70`	`fs.writeFile(filePath, contents);`
`71`		`- if (projectFilePath.endsWith('.ts')) {`
	`71`	`+ if (projectFilePath.endsWith('.ts') && !projectFilePath.endsWith('.d.ts')) {`
`72`	`72`	`entryFiles.push(filePath);`
`73`	`73`	`}`
`74`	`74`	`}`