fix(language-service): Resolve template variable in nested ngFor (#33676

)

This commit fixes a bug whereby template variables in nested scope are
not resolved properly and instead are simply typed as `any`.

PR closes angular/vscode-ng-language-service#144

PR Close #33676

packages/compiler-cli/src/diagnostics/expression_diagnostics.ts

@@ -90,52 +90,77 @@ function getDefinitionOf(info: DiagnosticTemplateInfo, ast: TemplateAst): Defini
   }
 }
 
+/**
+ * Resolve the specified `variable` from the `directives` list and return the
+ * corresponding symbol. If resolution fails, return the `any` type.
+ * @param variable template variable to resolve
+ * @param directives template context
+ * @param query
+ */
+function findSymbolForVariableInDirectives(
+    variable: VariableAst, directives: DirectiveAst[], query: SymbolQuery): Symbol {
+  for (const d of directives) {
+    // Get the symbol table for the directive's StaticSymbol
+    const table = query.getTemplateContext(d.directive.type.reference);
+    if (!table) {
+      continue;
+    }
+    const symbol = table.get(variable.value);
+    if (symbol) {
+      return symbol;
+    }
+  }
+  return query.getBuiltinType(BuiltinType.Any);
+}
+
+/**
+ * Resolve all variable declarations in a template by traversing the specified
+ * `path`.
+ * @param info
+ * @param path template AST path
+ */
 function getVarDeclarations(
     info: DiagnosticTemplateInfo, path: TemplateAstPath): SymbolDeclaration[] {
-  const result: SymbolDeclaration[] = [];
-
-  let current = path.tail;
-  while (current) {
-    if (current instanceof EmbeddedTemplateAst) {
-      for (const variable of current.variables) {
-        const name = variable.name;
-
-        // Find the first directive with a context.
-        const context =
-            current.directives.map(d => info.query.getTemplateContext(d.directive.type.reference))
-                .find(c => !!c);
-
-        // Determine the type of the context field referenced by variable.value.
-        let type: Symbol|undefined = undefined;
-        if (context) {
-          const value = context.get(variable.value);
-          if (value) {
-            type = value.type !;
-            let kind = info.query.getTypeKind(type);
-            if (kind === BuiltinType.Any || kind == BuiltinType.Unbound) {
-              // The any type is not very useful here. For special cases, such as ngFor, we can do
-              // better.
-              type = refinedVariableType(type, info, current);
-            }
-          }
-        }
-        if (!type) {
-          type = info.query.getBuiltinType(BuiltinType.Any);
-        }
-        result.push({
-          name,
-          kind: 'variable', type, get definition() { return getDefinitionOf(info, variable); }
-        });
+  const results: SymbolDeclaration[] = [];
+  for (let current = path.head; current; current = path.childOf(current)) {
+    if (!(current instanceof EmbeddedTemplateAst)) {
+      continue;
+    }
+    const {directives, variables} = current;
+    for (const variable of variables) {
+      let symbol = findSymbolForVariableInDirectives(variable, directives, info.query);
+      const kind = info.query.getTypeKind(symbol);
+      if (kind === BuiltinType.Any || kind === BuiltinType.Unbound) {
+        // For special cases such as ngFor and ngIf, the any type is not very useful.
+        // We can do better by resolving the binding value.
+        const symbolsInScope = info.query.mergeSymbolTable([
+          info.members,
+          // Since we are traversing the AST path from head to tail, any variables
+          // that have been declared so far are also in scope.
+          info.query.createSymbolTable(results),
+        ]);
+        symbol = refinedVariableType(symbolsInScope, info.query, current);
       }
+      results.push({
+        name: variable.name,
+        kind: 'variable',
+        type: symbol, get definition() { return getDefinitionOf(info, variable); },
+      });
     }
-    current = path.parentOf(current);
   }
-
-  return result;
+  return results;
 }
 
+/**
+ * Resolve a more specific type for the variable in `templateElement` by inspecting
+ * all variables that are in scope in the `mergedTable`. This function is a special
+ * case for `ngFor` and `ngIf`. If resolution fails, return the `any` type.
+ * @param mergedTable symbol table for all variables in scope
+ * @param query
+ * @param templateElement
+ */
 function refinedVariableType(
-    type: Symbol, info: DiagnosticTemplateInfo, templateElement: EmbeddedTemplateAst): Symbol {
+    mergedTable: SymbolTable, query: SymbolQuery, templateElement: EmbeddedTemplateAst): Symbol {
   // Special case the ngFor directive
   const ngForDirective = templateElement.directives.find(d => {
     const name = identifierName(d.directive.type);
@@ -144,9 +169,9 @@ function refinedVariableType(
   if (ngForDirective) {
     const ngForOfBinding = ngForDirective.inputs.find(i => i.directiveName == 'ngForOf');
     if (ngForOfBinding) {
-      const bindingType = new AstType(info.members, info.query, {}).getType(ngForOfBinding.value);
+      const bindingType = new AstType(mergedTable, query, {}).getType(ngForOfBinding.value);
       if (bindingType) {
-        const result = info.query.getElementType(bindingType);
+        const result = query.getElementType(bindingType);
         if (result) {
           return result;
         }
@@ -160,15 +185,15 @@ function refinedVariableType(
   if (ngIfDirective) {
     const ngIfBinding = ngIfDirective.inputs.find(i => i.directiveName === 'ngIf');
     if (ngIfBinding) {
-      const bindingType = new AstType(info.members, info.query, {}).getType(ngIfBinding.value);
+      const bindingType = new AstType(mergedTable, query, {}).getType(ngIfBinding.value);
       if (bindingType) {
         return bindingType;
       }
     }
   }
 
   // We can't do better, return any
-  return info.query.getBuiltinType(BuiltinType.Any);
+  return query.getBuiltinType(BuiltinType.Any);
 }
 
 function getEventDeclaration(info: DiagnosticTemplateInfo, includeEvent?: boolean) {

packages/language-service/test/completions_spec.ts

@@ -283,6 +283,20 @@ describe('completions', () => {
       const completions = ngLS.getCompletionsAt(PARSING_CASES, marker.start);
       expectContain(completions, CompletionKind.PROPERTY, ['name', 'age', 'street']);
     });
+
+    it('should be able to resolve variable in nested loop', () => {
+      mockHost.override(TEST_TEMPLATE, `
+        <div *ngFor="let leagueMembers of league">
+          <div *ngFor="let member of leagueMembers">
+            {{member.~{position}}}
+          </div>
+        </div>
+      `);
+      const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'position');
+      const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start);
+      // member variable of type Hero has properties 'id' and 'name'.
+      expectContain(completions, CompletionKind.PROPERTY, ['id', 'name']);
+    });
   });
 
   describe('data binding', () => {

packages/language-service/test/diagnostics_spec.ts

@@ -173,6 +173,23 @@ describe('diagnostics', () => {
     expect(diagnostics).toEqual([]);
   });
 
+  it('should report diagnostic for invalid property in nested ngFor', () => {
+    const content = mockHost.override(TEST_TEMPLATE, `
+      <div *ngFor="let leagueMembers of league">
+        <div *ngFor="let member of leagueMembers">
+          {{member.xyz}}
+        </div>
+      </div>
+    `);
+    const diagnostics = ngLS.getDiagnostics(TEST_TEMPLATE);
+    expect(diagnostics.length).toBe(1);
+    const {messageText, start, length} = diagnostics[0];
+    expect(messageText)
+        .toBe(`Identifier 'xyz' is not defined. 'Hero' does not contain such a member`);
+    expect(start).toBe(content.indexOf('member.xyz'));
+    expect(length).toBe('member.xyz'.length);
+  });
+
   describe('with $event', () => {
     it('should accept an event', () => {
       const fileName = '/app/test.ng';

packages/language-service/test/project/app/parsing-cases.ts

@@ -189,6 +189,7 @@ export class TemplateReference {
   title = 'Some title';
   hero: Hero = {id: 1, name: 'Windstorm'};
   heroes: Hero[] = [this.hero];
+  league: Hero[][] = [this.heroes];
   anyValue: any;
   myClick(event: any) {}
 }