feat(language-service): improve non-callable error message (#35271)

This commit improves the context of a non-callable function error
message by providing the affected call target and its non-callable type.

PR Close #35271

packages/language-service/src/completions.ts

@@ -492,7 +492,7 @@ class ExpressionVisitor extends NullTemplateVisitor {
   visitBoundText(ast: BoundTextAst) {
     if (inSpan(this.position, ast.value.sourceSpan)) {
       const completions = getExpressionCompletions(
-          this.getExpressionScope(), ast.value, this.position, this.info.template.query);
+          this.getExpressionScope(), ast.value, this.position, this.info.template);
       if (completions) {
         this.addSymbolsToCompletions(completions);
       }
@@ -501,7 +501,7 @@ class ExpressionVisitor extends NullTemplateVisitor {
 
   private processExpressionCompletions(value: AST) {
     const symbols = getExpressionCompletions(
-        this.getExpressionScope(), value, this.position, this.info.template.query);
+        this.getExpressionScope(), value, this.position, this.info.template);
     if (symbols) {
       this.addSymbolsToCompletions(symbols);
     }

packages/language-service/src/diagnostic_messages.ts

@@ -58,7 +58,7 @@ export const Diagnostic: Record<DiagnosticName, DiagnosticMessage> = {
   },
 
   call_target_not_callable: {
-    message: 'Call target is not callable',
+    message: `Call target '%1' has non-callable type '%2'.`,
     kind: 'Error',
   },
 

packages/language-service/src/diagnostics.ts

@@ -40,6 +40,7 @@ export function getTemplateDiagnostics(ast: AstResult): ng.Diagnostic[] {
     offset: template.span.start,
     query: template.query,
     members: template.members,
+    source: ast.template.source,
   });
 }
 

packages/language-service/src/expression_diagnostics.ts

@@ -21,6 +21,7 @@ export interface DiagnosticTemplateInfo {
   members: SymbolTable;
   htmlAst: Node[];
   templateAst: TemplateAst[];
+  source: string;
 }
 
 export function getTemplateExpressionDiagnostics(info: DiagnosticTemplateInfo): ng.Diagnostic[] {
@@ -103,7 +104,7 @@ function getVarDeclarations(
           // that have been declared so far are also in scope.
           info.query.createSymbolTable(results),
         ]);
-        symbol = refinedVariableType(variable.value, symbolsInScope, info.query, current);
+        symbol = refinedVariableType(variable.value, symbolsInScope, info, current);
       }
       results.push({
         name: variable.name,
@@ -143,11 +144,11 @@ function getVariableTypeFromDirectiveContext(
  * case for `ngFor` and `ngIf`. If resolution fails, return the `any` type.
  * @param value variable value name
  * @param mergedTable symbol table for all variables in scope
- * @param query
+ * @param info available template information
  * @param templateElement
  */
 function refinedVariableType(
-    value: string, mergedTable: SymbolTable, query: SymbolQuery,
+    value: string, mergedTable: SymbolTable, info: DiagnosticTemplateInfo,
     templateElement: EmbeddedTemplateAst): Symbol {
   if (value === '$implicit') {
     // Special case: ngFor directive
@@ -159,9 +160,10 @@ function refinedVariableType(
       const ngForOfBinding = ngForDirective.inputs.find(i => i.directiveName == 'ngForOf');
       if (ngForOfBinding) {
         // Check if there is a known type for the ngFor binding.
-        const bindingType = new AstType(mergedTable, query, {}).getType(ngForOfBinding.value);
+        const bindingType =
+            new AstType(mergedTable, info.query, {}, info.source).getType(ngForOfBinding.value);
         if (bindingType) {
-          const result = query.getElementType(bindingType);
+          const result = info.query.getElementType(bindingType);
           if (result) {
             return result;
           }
@@ -184,7 +186,8 @@ function refinedVariableType(
       const ngIfBinding = ngIfDirective.inputs.find(i => i.directiveName === 'ngIf');
       if (ngIfBinding) {
         // Check if there is a known type bound to the ngIf input.
-        const bindingType = new AstType(mergedTable, query, {}).getType(ngIfBinding.value);
+        const bindingType =
+            new AstType(mergedTable, info.query, {}, info.source).getType(ngIfBinding.value);
         if (bindingType) {
           return bindingType;
         }
@@ -193,7 +196,7 @@ function refinedVariableType(
   }
 
   // We can't do better, return any
-  return query.getBuiltinType(BuiltinType.Any);
+  return info.query.getBuiltinType(BuiltinType.Any);
 }
 
 function getEventDeclaration(
@@ -338,9 +341,9 @@ class ExpressionDiagnosticsVisitor extends RecursiveTemplateAstVisitor {
     return ast.sourceSpan.start.offset;
   }
 
-  private diagnoseExpression(ast: AST, offset: number, event: boolean) {
+  private diagnoseExpression(ast: AST, offset: number, inEvent: boolean) {
-    const scope = this.getExpressionScope(this.path, event);
+    const scope = this.getExpressionScope(this.path, inEvent);
-    const analyzer = new AstType(scope, this.info.query, {event});
+    const analyzer = new AstType(scope, this.info.query, {inEvent}, this.info.source);
     for (const diagnostic of analyzer.getDiagnostics(ast)) {
       diagnostic.span = this.absSpan(diagnostic.span, offset);
       this.diagnostics.push(diagnostic);

packages/language-service/src/expression_type.ts

@@ -12,7 +12,7 @@ import {Diagnostic, createDiagnostic} from './diagnostic_messages';
 import {BuiltinType, Signature, Symbol, SymbolQuery, SymbolTable} from './symbols';
 import * as ng from './types';
 
-export interface ExpressionDiagnosticsContext { event?: boolean; }
+export interface ExpressionDiagnosticsContext { inEvent?: boolean; }
 
 // AstType calculatetype of the ast given AST element.
 export class AstType implements AstVisitor {
@@ -20,13 +20,13 @@ export class AstType implements AstVisitor {
 
   constructor(
       private scope: SymbolTable, private query: SymbolQuery,
-      private context: ExpressionDiagnosticsContext) {}
+      private context: ExpressionDiagnosticsContext, private source: string) {}
 
   getType(ast: AST): Symbol { return ast.visit(this); }
 
   getDiagnostics(ast: AST): ng.Diagnostic[] {
     const type: Symbol = ast.visit(this);
-    if (this.context.event && type.callable) {
+    if (this.context.inEvent && type.callable) {
       this.diagnostics.push(
           createDiagnostic(ast.span, Diagnostic.callable_expression_expected_method_call));
     }
@@ -206,14 +206,16 @@ export class AstType implements AstVisitor {
     const args = ast.args.map(arg => this.getType(arg));
     const target = this.getType(ast.target !);
     if (!target || !target.callable) {
-      this.diagnostics.push(createDiagnostic(ast.span, Diagnostic.call_target_not_callable));
+      this.diagnostics.push(createDiagnostic(
+          ast.span, Diagnostic.call_target_not_callable, this.sourceOf(ast.target !), target.name));
       return this.anyType;
     }
     const signature = target.selectSignature(args);
     if (signature) {
       return signature.result;
     }
-    // TODO: Consider a better error message here.
+    // TODO: Consider a better error message here. See `typescript_symbols#selectSignature` for more
+    // details.
     this.diagnostics.push(
         createDiagnostic(ast.span, Diagnostic.unable_to_resolve_compatible_call_signature));
     return this.anyType;
@@ -360,6 +362,15 @@ export class AstType implements AstVisitor {
     return this.resolvePropertyRead(this.query.getNonNullableType(this.getType(ast.receiver)), ast);
   }
 
+  /**
+   * Gets the source of an expession AST.
+   * The AST's sourceSpan is relative to the start of the template source code, which is contained
+   * at this.source.
+   */
+  private sourceOf(ast: AST): string {
+    return this.source.substring(ast.sourceSpan.start, ast.sourceSpan.end);
+  }
+
   private _anyType: Symbol|undefined;
   private get anyType(): Symbol {
     let result = this._anyType;

packages/language-service/src/expressions.ts

@@ -8,8 +8,7 @@
 
 import {AST, ASTWithSource, AstPath as AstPathBase, RecursiveAstVisitor} from '@angular/compiler';
 import {AstType} from './expression_type';
-
+import {BuiltinType, Span, Symbol, SymbolTable, TemplateSource} from './types';
-import {BuiltinType, Span, Symbol, SymbolQuery, SymbolTable} from './types';
 import {inSpan} from './utils';
 
 type AstPath = AstPathBase<AST>;
@@ -38,13 +37,16 @@ function findAstAt(ast: AST, position: number, excludeEmpty: boolean = false): A
 }
 
 export function getExpressionCompletions(
-    scope: SymbolTable, ast: AST, position: number, query: SymbolQuery): Symbol[]|undefined {
+    scope: SymbolTable, ast: AST, position: number, templateInfo: TemplateSource): Symbol[]|
+    undefined {
   const path = findAstAt(ast, position);
   if (path.empty) return undefined;
   const tail = path.tail !;
   let result: SymbolTable|undefined = scope;
 
-  function getType(ast: AST): Symbol { return new AstType(scope, query, {}).getType(ast); }
+  function getType(ast: AST): Symbol {
+    return new AstType(scope, templateInfo.query, {}, templateInfo.source).getType(ast);
+  }
 
   // If the completion request is in a not in a pipe or property access then the global scope
   // (that is the scope of the implicit receiver) is the right scope as the user is typing the
@@ -66,7 +68,7 @@ export function getExpressionCompletions(
       if (position >= ast.exp.span.end &&
           (!ast.args || !ast.args.length || position < (<AST>ast.args[0]).span.start)) {
         // We are in a position a pipe name is expected.
-        result = query.getPipes();
+        result = templateInfo.query.getPipes();
       }
     },
     visitPrefixNot(ast) {},
@@ -81,7 +83,7 @@ export function getExpressionCompletions(
     },
     visitQuote(ast) {
       // For a quote, return the members of any (if there are any).
-      result = query.getBuiltinType(BuiltinType.Any).members();
+      result = templateInfo.query.getBuiltinType(BuiltinType.Any).members();
     },
     visitSafeMethodCall(ast) {
       const receiverType = getType(ast.receiver);
@@ -106,12 +108,14 @@ export function getExpressionCompletions(
  */
 export function getExpressionSymbol(
     scope: SymbolTable, ast: AST, position: number,
-    query: SymbolQuery): {symbol: Symbol, span: Span}|undefined {
+    templateInfo: TemplateSource): {symbol: Symbol, span: Span}|undefined {
   const path = findAstAt(ast, position, /* excludeEmpty */ true);
   if (path.empty) return undefined;
   const tail = path.tail !;
 
-  function getType(ast: AST): Symbol { return new AstType(scope, query, {}).getType(ast); }
+  function getType(ast: AST): Symbol {
+    return new AstType(scope, templateInfo.query, {}, templateInfo.source).getType(ast);
+  }
 
   let symbol: Symbol|undefined = undefined;
   let span: Span|undefined = undefined;
@@ -139,7 +143,7 @@ export function getExpressionSymbol(
     visitPipe(ast) {
       if (inSpan(position, ast.nameSpan, /* exclusive */ true)) {
         // We are in a position a pipe name is expected.
-        const pipes = query.getPipes();
+        const pipes = templateInfo.query.getPipes();
         symbol = pipes.get(ast.name);
 
         // `nameSpan` is an absolute span, but the span expected by the result of this method is

packages/language-service/src/locate_symbol.ts

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {AST, Attribute, BoundDirectivePropertyAst, CssSelector, DirectiveAst, ElementAst, EmbeddedTemplateAst, ExpressionBinding, RecursiveTemplateAstVisitor, SelectorMatcher, StaticSymbol, TemplateAst, TemplateAstPath, VariableBinding, templateVisitAll, tokenReference} from '@angular/compiler';
+import {AST, Attribute, BoundDirectivePropertyAst, CssSelector, DirectiveAst, ElementAst, EmbeddedTemplateAst, RecursiveTemplateAstVisitor, SelectorMatcher, StaticSymbol, TemplateAst, TemplateAstPath, VariableBinding, templateVisitAll, tokenReference} from '@angular/compiler';
 import * as tss from 'typescript/lib/tsserverlibrary';
 
 import {AstResult} from './common';
@@ -73,7 +73,7 @@ function locateSymbol(ast: TemplateAst, path: TemplateAstPath, info: AstResult):
         } else {
           const dinfo = diagnosticInfoFromTemplateInfo(info);
           const scope = getExpressionScope(dinfo, path);
-          result = getExpressionSymbol(scope, ast, templatePosition, info.template.query);
+          result = getExpressionSymbol(scope, ast, templatePosition, info.template);
         }
         if (result) {
           symbol = result.symbol;
@@ -149,8 +149,7 @@ function locateSymbol(ast: TemplateAst, path: TemplateAstPath, info: AstResult):
           if (inSpan(expressionPosition, ast.value.span)) {
             const dinfo = diagnosticInfoFromTemplateInfo(info);
             const scope = getExpressionScope(dinfo, path);
-            const result =
+            const result = getExpressionSymbol(scope, ast.value, templatePosition, info.template);
-                getExpressionSymbol(scope, ast.value, templatePosition, info.template.query);
             if (result) {
               symbol = result.symbol;
               span = offsetSpan(result.span, ast.sourceSpan.start.offset);
@@ -217,7 +216,7 @@ function getSymbolInMicrosyntax(info: AstResult, path: TemplateAstPath, attribut
     if (inSpan(path.position, tb.value?.ast.sourceSpan)) {
       const dinfo = diagnosticInfoFromTemplateInfo(info);
       const scope = getExpressionScope(dinfo, path);
-      result = getExpressionSymbol(scope, tb.value !, path.position, info.template.query);
+      result = getExpressionSymbol(scope, tb.value !, path.position, info.template);
     } else if (inSpan(path.position, tb.sourceSpan)) {
       const template = path.first(EmbeddedTemplateAst);
       if (template) {

packages/language-service/src/typescript_symbols.ts

@@ -220,7 +220,10 @@ function signaturesOf(type: ts.Type, context: TypeContext): Signature[] {
 
 function selectSignature(type: ts.Type, context: TypeContext, types: Symbol[]): Signature|
     undefined {
-  // TODO: Do a better job of selecting the right signature.
+  // TODO: Do a better job of selecting the right signature. TypeScript does not currently support a
+  // Type Relationship API (see https://github.com/angular/vscode-ng-language-service/issues/143).
+  // Consider creating a TypeCheckBlock host in the language service that may also act as a
+  // scratchpad for type comparisons.
   const signatures = type.getCallSignatures();
   return signatures.length ? new SignatureWrapper(signatures[0], context) : undefined;
 }

packages/language-service/src/utils.ts

@@ -97,7 +97,8 @@ export function diagnosticInfoFromTemplateInfo(info: AstResult): DiagnosticTempl
     query: info.template.query,
     members: info.template.members,
     htmlAst: info.htmlAst,
-    templateAst: info.templateAst
+    templateAst: info.templateAst,
+    source: info.template.source,
   };
 }
 

packages/language-service/test/diagnostics_spec.ts

@@ -1023,4 +1023,19 @@ describe('diagnostics', () => {
       expect(content.substring(start !, start ! + length !)).toBe(expression);
     }
   });
+
+  describe('function calls', () => {
+    it('should report error for non-callable function call', () => {
+      mockHost.override(TEST_TEMPLATE, `
+        <p>{{myClick()()}}</p>
+      `);
+
+      const content = mockHost.readFile(TEST_TEMPLATE) !;
+      const diags = ngLS.getSemanticDiagnostics(TEST_TEMPLATE);
+      expect(diags.length).toBe(1);
+      const {messageText, start, length} = diags[0] !;
+      expect(messageText).toBe(`Call target 'myClick()' has non-callable type 'void'.`);
+      expect(content.substring(start !, start ! + length !)).toBe('myClick()()');
+    });
+  });
 });

packages/language-service/test/mocks.ts

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {AotSummaryResolver, CompileMetadataResolver, CompilerConfig, DEFAULT_INTERPOLATION_CONFIG, DirectiveNormalizer, DirectiveResolver, DomElementSchemaRegistry, HtmlParser, I18NHtmlParser, InterpolationConfig, JitSummaryResolver, Lexer, NgAnalyzedModules, NgModuleResolver, ParseTreeResult, Parser, PipeResolver, ResourceLoader, StaticReflector, StaticSymbol, StaticSymbolCache, StaticSymbolResolver, StaticSymbolResolverHost, SummaryResolver, TemplateParser, analyzeNgModules, createOfflineCompileUrlResolver} from '@angular/compiler';
+import {AotSummaryResolver, CompileMetadataResolver, CompilerConfig, DirectiveNormalizer, DirectiveResolver, DomElementSchemaRegistry, HtmlParser, I18NHtmlParser, JitSummaryResolver, Lexer, NgAnalyzedModules, NgModuleResolver, ParseTreeResult, Parser, PipeResolver, ResourceLoader, StaticReflector, StaticSymbol, StaticSymbolCache, StaticSymbolResolver, StaticSymbolResolverHost, TemplateParser, analyzeNgModules, createOfflineCompileUrlResolver} from '@angular/compiler';
 import {Directory, MockAotContext} from '@angular/compiler-cli/test/mocks';
 import {setup} from '@angular/compiler-cli/test/test_support';
 import {ViewEncapsulation, ɵConsole as Console} from '@angular/core';
@@ -238,7 +238,8 @@ export function getDiagnosticTemplateInfo(
           fileName: templateFile,
           offset: 0, query, members,
           htmlAst: compiledTemplate.htmlAst,
-          templateAst: compiledTemplate.templateAst
+          templateAst: compiledTemplate.templateAst,
+          source: sourceFile.text,
         };
       }
     }