From b4d0b70a2854f6486658450d2d0144c161c557c1 Mon Sep 17 00:00:00 2001 From: ayazhafiz Date: Sun, 9 Feb 2020 12:29:45 -0800 Subject: [PATCH 1/5] feat(language-service): improve non-callable error message This commit improves the context of a non-callable function error message by providing the affected call target and its non-callable type. --- packages/language-service/src/completions.ts | 4 ++-- .../src/diagnostic_messages.ts | 2 +- packages/language-service/src/diagnostics.ts | 1 + .../src/expression_diagnostics.ts | 23 +++++++++++-------- .../language-service/src/expression_type.ts | 14 +++++++---- packages/language-service/src/expressions.ts | 22 ++++++++++-------- .../language-service/src/locate_symbol.ts | 9 ++++---- .../src/typescript_symbols.ts | 5 +++- packages/language-service/src/utils.ts | 3 ++- .../language-service/test/diagnostics_spec.ts | 13 +++++++++++ 10 files changed, 62 insertions(+), 34 deletions(-) diff --git a/packages/language-service/src/completions.ts b/packages/language-service/src/completions.ts index 6233f013b9644..300aa60808af8 100644 --- a/packages/language-service/src/completions.ts +++ b/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); } diff --git a/packages/language-service/src/diagnostic_messages.ts b/packages/language-service/src/diagnostic_messages.ts index 8eb3c40cd6f13..885d781bd2992 100644 --- a/packages/language-service/src/diagnostic_messages.ts +++ b/packages/language-service/src/diagnostic_messages.ts @@ -58,7 +58,7 @@ export const Diagnostic: Record = { }, call_target_not_callable: { - message: 'Call target is not callable', + message: `Call target '%1' has non-callable type '%2'.`, kind: 'Error', }, diff --git a/packages/language-service/src/diagnostics.ts b/packages/language-service/src/diagnostics.ts index fa07382e38fd8..d437a918ea224 100644 --- a/packages/language-service/src/diagnostics.ts +++ b/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, }); } diff --git a/packages/language-service/src/expression_diagnostics.ts b/packages/language-service/src/expression_diagnostics.ts index 3a5460782e51b..0af26418be9c6 100644 --- a/packages/language-service/src/expression_diagnostics.ts +++ b/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) { - const scope = this.getExpressionScope(this.path, event); - const analyzer = new AstType(scope, this.info.query, {event}); + private diagnoseExpression(ast: AST, offset: number, inEvent: boolean) { + const scope = this.getExpressionScope(this.path, inEvent); + 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); diff --git a/packages/language-service/src/expression_type.ts b/packages/language-service/src/expression_type.ts index 732eaca64c0ff..fc70154a57de4 100644 --- a/packages/language-service/src/expression_type.ts +++ b/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,8 @@ export class AstType implements AstVisitor { return this.resolvePropertyRead(this.query.getNonNullableType(this.getType(ast.receiver)), ast); } + private sourceOf(ast: AST): string { return this.source.substring(ast.span.start, ast.span.end); } + private _anyType: Symbol|undefined; private get anyType(): Symbol { let result = this._anyType; diff --git a/packages/language-service/src/expressions.ts b/packages/language-service/src/expressions.ts index 03df9d4ba7b5a..8d56d99a02cc7 100644 --- a/packages/language-service/src/expressions.ts +++ b/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, SymbolQuery, SymbolTable} from './types'; +import {BuiltinType, Span, Symbol, SymbolQuery, SymbolTable, TemplateSource} from './types'; import {inSpan} from './utils'; type AstPath = AstPathBase; @@ -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.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 diff --git a/packages/language-service/src/locate_symbol.ts b/packages/language-service/src/locate_symbol.ts index 138d31e4e5ba5..1f1cbc286fd16 100644 --- a/packages/language-service/src/locate_symbol.ts +++ b/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 = - getExpressionSymbol(scope, ast.value, templatePosition, info.template.query); + const result = getExpressionSymbol(scope, ast.value, templatePosition, info.template); 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) { diff --git a/packages/language-service/src/typescript_symbols.ts b/packages/language-service/src/typescript_symbols.ts index ba7d088c8c4c9..69812f295fc1f 100644 --- a/packages/language-service/src/typescript_symbols.ts +++ b/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; } diff --git a/packages/language-service/src/utils.ts b/packages/language-service/src/utils.ts index 33ac9be37a37f..6d00eace56774 100644 --- a/packages/language-service/src/utils.ts +++ b/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, }; } diff --git a/packages/language-service/test/diagnostics_spec.ts b/packages/language-service/test/diagnostics_spec.ts index 7d23a8bb08bce..87566248aaf00 100644 --- a/packages/language-service/test/diagnostics_spec.ts +++ b/packages/language-service/test/diagnostics_spec.ts @@ -1023,4 +1023,17 @@ 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, `{{myClick()()}}`); + + 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()()'); + }); + }); }); From a5a879f6100923a8a232016ea4658e05d96ca87b Mon Sep 17 00:00:00 2001 From: ayazhafiz Date: Tue, 10 Mar 2020 18:30:07 -0700 Subject: [PATCH 2/5] fixup! feat(language-service): improve non-callable error message --- packages/language-service/src/expressions.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/language-service/src/expressions.ts b/packages/language-service/src/expressions.ts index 8d56d99a02cc7..594a3c565b17b 100644 --- a/packages/language-service/src/expressions.ts +++ b/packages/language-service/src/expressions.ts @@ -8,7 +8,7 @@ import {AST, ASTWithSource, AstPath as AstPathBase, RecursiveAstVisitor} from '@angular/compiler'; import {AstType} from './expression_type'; -import {BuiltinType, Span, Symbol, SymbolQuery, SymbolTable, TemplateSource} from './types'; +import {BuiltinType, Span, Symbol, SymbolTable, TemplateSource} from './types'; import {inSpan} from './utils'; type AstPath = AstPathBase; From 852229bfe392d6a19245541b4e976a0c0a288ada Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Thu, 12 Mar 2020 19:02:43 -0700 Subject: [PATCH 3/5] fixup! feat(language-service): improve non-callable error message --- packages/language-service/src/expression_type.ts | 4 +++- packages/language-service/test/diagnostics_spec.ts | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/language-service/src/expression_type.ts b/packages/language-service/src/expression_type.ts index fc70154a57de4..07dfa4f0a62cf 100644 --- a/packages/language-service/src/expression_type.ts +++ b/packages/language-service/src/expression_type.ts @@ -362,7 +362,9 @@ export class AstType implements AstVisitor { return this.resolvePropertyRead(this.query.getNonNullableType(this.getType(ast.receiver)), ast); } - private sourceOf(ast: AST): string { return this.source.substring(ast.span.start, ast.span.end); } + private sourceOf(ast: AST): string { + return this.source.substring(ast.sourceSpan.start, ast.sourceSpan.end); + } private _anyType: Symbol|undefined; private get anyType(): Symbol { diff --git a/packages/language-service/test/diagnostics_spec.ts b/packages/language-service/test/diagnostics_spec.ts index 87566248aaf00..10e5716189a05 100644 --- a/packages/language-service/test/diagnostics_spec.ts +++ b/packages/language-service/test/diagnostics_spec.ts @@ -1026,7 +1026,9 @@ describe('diagnostics', () => { describe('function calls', () => { it('should report error for non-callable function call', () => { - mockHost.override(TEST_TEMPLATE, `{{myClick()()}}`); + mockHost.override(TEST_TEMPLATE, ` +

{{myClick()()}}

+ `); const content = mockHost.readFile(TEST_TEMPLATE) !; const diags = ngLS.getSemanticDiagnostics(TEST_TEMPLATE); From 9aaba498745439588c118acd34723679c98147fe Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Fri, 13 Mar 2020 17:46:19 -0700 Subject: [PATCH 4/5] fixup! feat(language-service): improve non-callable error message --- packages/language-service/test/mocks.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/language-service/test/mocks.ts b/packages/language-service/test/mocks.ts index bab59960540f6..b58ea156519dd 100644 --- a/packages/language-service/test/mocks.ts +++ b/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, }; } } From 341543e65e56a08d87b16f627e138971e6e954f1 Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Fri, 13 Mar 2020 18:35:18 -0700 Subject: [PATCH 5/5] fixup! feat(language-service): improve non-callable error message --- packages/language-service/src/expression_type.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/language-service/src/expression_type.ts b/packages/language-service/src/expression_type.ts index 07dfa4f0a62cf..5277ca3c55273 100644 --- a/packages/language-service/src/expression_type.ts +++ b/packages/language-service/src/expression_type.ts @@ -362,6 +362,11 @@ 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); }