fix(ivy): don't track identifiers of ffr-resolved references (#29387)

This fix is for a bug in the ngtsc PartialEvaluator, which statically
evaluates expressions.

Sometimes, evaluating a reference requires resolving a function which is
declared in another module, and thus no function body is available. To
support this case, the PartialEvaluator has the concept of a foreign
function resolver.

This allows the interpretation of expressions like:

const router = RouterModule.forRoot([]);

even though the definition of the 'forRoot' function has no body. In
ngtsc today, this will be resolved to a Reference to RouterModule itself,
via the ModuleWithProviders foreign function resolver.

However, the PartialEvaluator also associates any Identifiers in the path
of this resolution with the Reference. This is done so that if the user
writes

const x = imported.y;

'x' can be generated as a local identifier instead of adding an import for
'y'.

This was at the heart of a bug. In the above case with 'router', the
PartialEvaluator added the identifier 'router' to the Reference generated
(through FFR) to RouterModule.

This is not correct. References that result from FFR expressions may not
have the same value at runtime as they do at compile time (indeed, this is
not the case for ModuleWithProviders). The Reference generated via FFR is
"synthetic" in the sense that it's constructed based on a useful
interpretation of the code, not an accurate representation of the runtime
value. Therefore, it may not be legal to refer to the Reference via the
'router' identifier.

This commit adds the ability to mark such a Reference as 'synthetic', which
allows the PartialEvaluator to not add the 'router' identifier down the
line. Tests are included for both the PartialEvaluator itself as well as the
resultant buggy behavior in ngtsc overall.

PR Close #29387

packages/compiler-cli/src/ngtsc/imports/src/references.ts

@@ -49,6 +49,14 @@ export class Reference<T extends ts.Node = ts.Node> {
 
   private identifiers: ts.Identifier[] = [];
 
+  /**
+   * Indicates that the Reference was created synthetically, not as a result of natural value
+   * resolution.
+   *
+   * This is used to avoid misinterpreting the Reference in certain contexts.
+   */
+  synthetic = false;
+
   private _alias: Expression|null = null;
 
   constructor(readonly node: T, bestGuessOwningModule: OwningModule|null = null) {

packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts

@@ -218,7 +218,12 @@ export class StaticInterpreter {
     const result =
         this.visitDeclaration(decl.node, {...context, ...joinModuleContext(context, node, decl)});
     if (result instanceof Reference) {
-      result.addIdentifier(node);
+      // Only record identifiers to non-synthetic references. Synthetic references may not have the
+      // same value at runtime as they do at compile time, so it's not legal to refer to them by the
+      // identifier here.
+      if (!result.synthetic) {
+        result.addIdentifier(node);
+      }
     } else if (result instanceof DynamicValue) {
       return DynamicValue.fromDynamicInput(node, result);
     }
@@ -404,7 +409,14 @@ export class StaticInterpreter {
         };
       }
 
-      return this.visitExpression(expr, context);
+      const res = this.visitExpression(expr, context);
+      if (res instanceof Reference) {
+        // This Reference was created synthetically, via a foreign function resolver. The real
+        // runtime value of the function expression may be different than the foreign function
+        // resolved value, so mark the Reference as synthetic to avoid it being misinterpreted.
+        res.synthetic = true;
+      }
+      return res;
     }
 
     const body = fn.body;

packages/compiler-cli/src/ngtsc/partial_evaluator/test/evaluator_spec.ts

@@ -12,7 +12,7 @@ import {Reference} from '../../imports';
 import {TypeScriptReflectionHost} from '../../reflection';
 import {getDeclaration, makeProgram} from '../../testing/in_memory_typescript';
 import {DynamicValue} from '../src/dynamic';
-import {PartialEvaluator} from '../src/interface';
+import {ForeignFunctionResolver, PartialEvaluator} from '../src/interface';
 import {EnumValue, ResolvedValue} from '../src/result';
 
 function makeSimpleProgram(contents: string): ts.Program {
@@ -41,11 +41,12 @@ function makeExpression(
 }
 
 function evaluate<T extends ResolvedValue>(
-    code: string, expr: string, supportingFiles: {name: string, contents: string}[] = []): T {
+    code: string, expr: string, supportingFiles: {name: string, contents: string}[] = [],
+    foreignFunctionResolver?: ForeignFunctionResolver): T {
   const {expression, checker, program, options, host} = makeExpression(code, expr, supportingFiles);
   const reflectionHost = new TypeScriptReflectionHost(checker);
   const evaluator = new PartialEvaluator(reflectionHost, checker);
-  return evaluator.evaluate(expression) as T;
+  return evaluator.evaluate(expression, foreignFunctionResolver) as T;
 }
 
 describe('ngtsc metadata', () => {
@@ -313,8 +314,34 @@ describe('ngtsc metadata', () => {
         evaluate(`enum Test { VALUE = 'test', } const value = \`a.\${Test.VALUE}.b\`;`, 'value');
     expect(value).toBe('a.test.b');
   });
+
+  it('should not attach identifiers to FFR-resolved values', () => {
+    const value = evaluate(
+        `
+    declare function foo(arg: any): any;
+    class Target {}
+
+    const indir = foo(Target);
+    const value = indir;
+  `,
+        'value', [], firstArgFfr);
+    if (!(value instanceof Reference)) {
+      return fail('Expected value to be a Reference');
+    }
+    const id = value.getIdentityIn(value.node.getSourceFile());
+    if (id === null) {
+      return fail('Expected value to have an identity');
+    }
+    expect(id.text).toEqual('Target');
+  });
 });
 
 function owningModuleOf(ref: Reference): string|null {
   return ref.bestGuessOwningModule !== null ? ref.bestGuessOwningModule.specifier : null;
 }
+
+function firstArgFfr(
+    node: Reference<ts.FunctionDeclaration|ts.MethodDeclaration|ts.FunctionExpression>,
+    args: ReadonlyArray<ts.Expression>): ts.Expression {
+  return args[0];
+}

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

@@ -1251,6 +1251,36 @@ describe('ngtsc behavioral tests', () => {
           .toContain(
               'i0.ɵNgModuleDefWithMeta<TestModule, never, [typeof i1.InternalRouterModule], never>');
     });
+
+    it('should not reference a constant with a ModuleWithProviders value in ngModuleDef imports',
+       () => {
+         env.tsconfig();
+         env.write('dep.d.ts', `
+          import {ModuleWithProviders, ɵNgModuleDefWithMeta as NgModuleDefWithMeta} from '@angular/core';
+      
+          export declare class DepModule {
+            static forRoot(arg1: any, arg2: any): ModuleWithProviders<DepModule>;
+            static ngModuleDef: NgModuleDefWithMeta<DepModule, never, never, never>;
+          }
+        `);
+         env.write('test.ts', `
+          import {NgModule, ModuleWithProviders} from '@angular/core';
+          import {DepModule} from './dep';
+    
+          @NgModule({})
+          export class Base {}
+    
+          const mwp = DepModule.forRoot(1,2);
+    
+          @NgModule({
+            imports: [mwp],
+          })
+          export class Module {}
+        `);
+         env.driveMain();
+         const jsContents = env.getContents('test.js');
+         expect(jsContents).toContain('imports: [i1.DepModule]');
+       });
   });
 
   it('should unwrap a ModuleWithProviders-like function if a matching literal type is provided for it',