New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(language-service): improve non-callable error message #35271
Changes from all commits
b4d0b70
a5a879f
852229b
9aaba49
341543e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,21 +12,21 @@ 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 { | ||
private readonly diagnostics: ng.Diagnostic[] = []; | ||
|
||
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 !); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Alternatively, we could fix the typings upstream. Right now, many parameters are just typed If we go with the cast, we'll have to account for the other case which is a PropertyRead receiver. Please see next comment =) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, you're right. I'll see if we can type the ast specially. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem with pulling out the name is it only contains the name for the most local AST. So for something like |
||
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; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have to pass in the entire template, because the AST we got from the TemplateAst is actually ASTWithSource, so we already have the expression string here. This info combined with the span of the expression node should be sufficient for us to retrieve the string of interest.
Before we do that though, it might be better to update the type for all ASTs in
template_ast.ts
from AST to ASTWithSource. Please let me know if you need more clarification, thank you!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this will work, because AstType may visit child nodes of the AST, which may not be
ASTWithSource
s. We could pass the source of just the expression, but I am weary of this it would require using more relative spans. What do you think?