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

ayazhafiz · 2020-02-09T20:31:32Z

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

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Feature

Does this PR introduce a breaking change?

Yes
No

ayazhafiz · 2020-02-09T20:33:23Z

One thing we should start evaluating a design for is isolating error messages in a separate (json) file with unique IDs, descriptions, and even examples. Many modern language tools like tsserver, Rust's compiler, and Shellcheck follow this model. It will also provide a natural way for us to support enabling/disabling particular error messages as users have requested.

kyliau · 2020-02-11T01:28:53Z

One thing we should start evaluating a design for is isolating error messages in a separate (json) file with unique IDs, descriptions, and even examples. Many modern language tools like tsserver, Rust's compiler, and Shellcheck follow this model. It will also provide a natural way for us to support enabling/disabling particular error messages as users have requested.

In Ivy, each error already has a unique ID, but description is still generated on the fly.
It's good idea to pull them out into JSON.

https://github.com/angular/angular/blob/master/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts

packages/language-service/test/diagnostics_spec.ts

kyliau · 2020-02-11T01:50:42Z

packages/language-service/src/expression_type.ts

@@ -197,15 +197,18 @@ export class AstType implements AstVisitor {
    const args = ast.args.map(arg => this.getType(arg));
    const target = this.getType(ast.target !);


ast.target is of type AST here, but really it's a MethodCall AST, of which the receiver is an ImplicitReceiver.
The MethodCall AST contains the name of the function call, so it looks like a better fix here is to make a safe cast to retrieve the name.

Alternatively, we could fix the typings upstream. Right now, many parameters are just typed AST. They should have been more specific.

If we go with the cast, we'll have to account for the other case which is a PropertyRead receiver. Please see next comment =)

Yeah, you're right. I'll see if we can type the ast specially.

The problem with pulling out the name is it only contains the name for the most local AST. So for something like title.toUpperCase()(), the property read AST name would only give us toUpperCase, and we would have to continue backing up and reconstruct what we think the source code looks like to get title.toUpperCase(). For this reason, I think it's better to parse out the source code from the AST span.

ayazhafiz · 2020-03-05T03:12:16Z

ptal @kyliau

kyliau · 2020-03-05T23:00:01Z

packages/language-service/src/expression_diagnostics.ts

-    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);


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!

I'm not sure this will work, because AstType may visit child nodes of the AST, which may not be ASTWithSources. 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?

TemplateAst values are currently typed as the base class AST, but they are actually constructed with ASTWithSource. Type them as such, because ASTWithSource gives more information about the consumed expression AST to downstream customers (namely, the expression AST source). Unblocks angular#35271

TemplateAst values are currently typed as the base class AST, but they are actually constructed with ASTWithSource. Type them as such, because ASTWithSource gives more information about the consumed expression AST to downstream customers (namely, the expression AST source). Unblocks #35271 PR Close #35892

kyliau · 2020-03-11T18:17:07Z

packages/language-service/src/expression_type.ts

@@ -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); }


You brought up a very good point in https://github.com/angular/angular/pull/35271/files#r390700195
Could you please double check that ast.span here is relative to the whole template?
I think it might be relative to the expression in the template.
In the test case that you wrote in packages/language-service/test/diagnostics_spec.ts the expression is the template itself so this works. Could you please test this with a template that has multiple expressions?

Good call. Fixed to use absolute spans.

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

angular-automatic-lock-bot · 2020-04-17T16:06:00Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

ayazhafiz added feature Issue that requests a new feature area: language-service Issues related to Angular's VS Code language service target: major This PR is targeted for the next major release labels Feb 9, 2020

ayazhafiz requested a review from kyliau February 9, 2020 20:31

ayazhafiz assigned kyliau Feb 9, 2020

ngbot bot added this to the needsTriage milestone Feb 9, 2020

googlebot added the cla: yes label Feb 9, 2020

kyliau suggested changes Feb 11, 2020

View reviewed changes

ayazhafiz requested a review from kyliau February 11, 2020 16:40

ayazhafiz force-pushed the i/cannot-call branch from af324b5 to 4b60ddd Compare February 11, 2020 16:40

ayazhafiz force-pushed the i/cannot-call branch 2 times, most recently from 6ac9355 to f58240b Compare March 5, 2020 03:12

kyliau reviewed Mar 5, 2020

View reviewed changes

ayazhafiz mentioned this pull request Mar 6, 2020

feat(compiler): type TemplateAst values as ASTWithSource #35892

Closed

7 tasks

ayazhafiz force-pushed the i/cannot-call branch from f58240b to 15ca665 Compare March 11, 2020 01:30

ayazhafiz requested a review from kyliau March 11, 2020 01:30

kyliau reviewed Mar 11, 2020

View reviewed changes

ayazhafiz force-pushed the i/cannot-call branch from b0e3085 to 93af139 Compare March 13, 2020 02:02

ayazhafiz requested a review from kyliau March 13, 2020 02:03

ayazhafiz and others added 4 commits March 13, 2020 17:46

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

b4d0b70

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

fixup! feat(language-service): improve non-callable error message

a5a879f

fixup! feat(language-service): improve non-callable error message

852229b

fixup! feat(language-service): improve non-callable error message

9aaba49

ayazhafiz force-pushed the i/cannot-call branch from 93af139 to 9aaba49 Compare March 14, 2020 00:46

kyliau approved these changes Mar 14, 2020

View reviewed changes

fixup! feat(language-service): improve non-callable error message

341543e

ayazhafiz added the action: merge The PR is ready for merge by the caretaker label Mar 17, 2020

AndrewKushnir closed this in acc483e Mar 17, 2020

angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ayazhafiz commented Feb 9, 2020

ayazhafiz commented Feb 9, 2020

kyliau commented Feb 11, 2020

kyliau Feb 11, 2020

ayazhafiz Feb 11, 2020

ayazhafiz Feb 11, 2020

ayazhafiz commented Mar 5, 2020

kyliau Mar 5, 2020

ayazhafiz Mar 11, 2020

kyliau Mar 11, 2020 •

edited

Loading

ayazhafiz Mar 13, 2020

angular-automatic-lock-bot bot commented Apr 17, 2020

		@@ -197,15 +197,18 @@ export class AstType implements AstVisitor {
		const args = ast.args.map(arg => this.getType(arg));
		const target = this.getType(ast.target !);

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

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

Conversation

ayazhafiz commented Feb 9, 2020

PR Checklist

PR Type

Does this PR introduce a breaking change?

ayazhafiz commented Feb 9, 2020

kyliau commented Feb 11, 2020

kyliau Feb 11, 2020

Choose a reason for hiding this comment

ayazhafiz Feb 11, 2020

Choose a reason for hiding this comment

ayazhafiz Feb 11, 2020

Choose a reason for hiding this comment

ayazhafiz commented Mar 5, 2020

kyliau Mar 5, 2020

Choose a reason for hiding this comment

ayazhafiz Mar 11, 2020

Choose a reason for hiding this comment

kyliau Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

ayazhafiz Mar 13, 2020

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Apr 17, 2020

kyliau Mar 11, 2020 •

edited

Loading