From c03e5804237b7a2fc6fcd8d617fc09319b75c3af Mon Sep 17 00:00:00 2001 From: Enea Jahollari Date: Wed, 19 Jun 2024 01:24:58 +0200 Subject: [PATCH] fix(compiler-cli): correctly get the type of nested function call expressions This PR fixes a bug where the type of a nested function call expression was incorrectly being returned as null. --- .../nullish_coalescing_not_nullable_spec.ts | 23 +++++ .../typecheck/src/template_symbol_builder.ts | 11 ++- ...ecker__get_symbol_of_template_node_spec.ts | 31 ++++++- .../language-service/src/template_target.ts | 1 + .../test/legacy/template_target_spec.ts | 9 ++ .../language-service/test/quick_info_spec.ts | 85 +++++++++++++++++-- .../test/references_and_rename_spec.ts | 36 ++++++++ .../test/signature_help_spec.ts | 24 ++++++ 8 files changed, 205 insertions(+), 15 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/nullish_coalescing_not_nullable/nullish_coalescing_not_nullable_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/nullish_coalescing_not_nullable/nullish_coalescing_not_nullable_spec.ts index b0f9b869360b..74670669ea87 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/nullish_coalescing_not_nullable/nullish_coalescing_not_nullable_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/nullish_coalescing_not_nullable/nullish_coalescing_not_nullable_spec.ts @@ -125,6 +125,29 @@ runInEachFileSystem(() => { expect(diags.length).toBe(0); }); + it('should not produce diagnostics for nullish coalescing in a chain', () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([ + { + fileName, + templates: { + 'TestCmp': `{{ var1 !== '' && (items?.length ?? 0) > 0 }}`, + }, + source: 'export class TestCmp { var1 = "text"; items: string[] | undefined = [] }', + }, + ]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, + program.getTypeChecker(), + [nullishCoalescingNotNullableFactory], + {strictNullChecks: true} /* options */, + ); + const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); + expect(diags.length).toBe(0); + }); + it('should not produce nullish coalescing warning for the any type', () => { const fileName = absoluteFrom('/main.ts'); const {program, templateTypeChecker} = setup([ diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts index fe0129548038..9d71f5bbf188 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts @@ -8,6 +8,7 @@ import { AST, + ASTWithName, ASTWithSource, BindingPipe, ParseSourceSpan, @@ -708,9 +709,13 @@ export class SymbolBuilder { let withSpan = expression.sourceSpan; - // The `name` part of a `PropertyWrite` and a non-safe `Call` does not have its own + // The `name` part of a `PropertyWrite` and `ASTWithName` do not have their own // AST so there is no way to retrieve a `Symbol` for just the `name` via a specific node. - if (expression instanceof PropertyWrite) { + // Also skipping SafePropertyReads as it breaks nullish coalescing not nullable extended diagnostic + if ( + expression instanceof PropertyWrite || + (expression instanceof ASTWithName && !(expression instanceof SafePropertyRead)) + ) { withSpan = expression.nameSpan; } @@ -770,6 +775,8 @@ export class SymbolBuilder { let tsSymbol: ts.Symbol | undefined; if (ts.isPropertyAccessExpression(node)) { tsSymbol = this.getTypeChecker().getSymbolAtLocation(node.name); + } else if (ts.isCallExpression(node)) { + tsSymbol = this.getTypeChecker().getSymbolAtLocation(node.expression); } else { tsSymbol = this.getTypeChecker().getSymbolAtLocation(node); } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts index e3c210461abf..388b5f7bebfb 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts @@ -758,7 +758,7 @@ runInEachFileSystem(() => { it('should get a symbol for function on a component used in an input binding', () => { const fileName = absoluteFrom('/main.ts'); - const templateString = `
`; + const templateString = `
`; const {templateTypeChecker, program} = setup([ { fileName, @@ -766,6 +766,7 @@ runInEachFileSystem(() => { source: ` export class Cmp { helloWorld() { return ''; } + nested = { helloWorld1() { return ''; } }; }`, }, ]); @@ -777,6 +778,13 @@ runInEachFileSystem(() => { assertExpressionSymbol(symbol); expect(program.getTypeChecker().symbolToString(symbol.tsSymbol!)).toEqual('helloWorld'); expect(program.getTypeChecker().typeToString(symbol.tsType)).toEqual('() => string'); + + const nestedSymbol = templateTypeChecker.getSymbolOfNode(nodes[0].inputs[1].value, cmp)!; + assertExpressionSymbol(nestedSymbol); + expect(program.getTypeChecker().symbolToString(nestedSymbol.tsSymbol!)).toEqual( + 'helloWorld1', + ); + expect(program.getTypeChecker().typeToString(nestedSymbol.tsType)).toEqual('string'); }); it('should get a symbol for binary expressions', () => { @@ -1118,8 +1126,15 @@ runInEachFileSystem(() => { const {templateTypeChecker, program} = setup([ { fileName, - templates: {'Cmp': '
'}, - source: `export class Cmp { toString(v: any): string { return String(v); } }`, + templates: { + 'Cmp': '
', + }, + source: ` + export class Cmp { + toString(v: any): string { return String(v); } + nested = { toString(v: any): string { return String(v); } }; + } + `, }, ]); const sf = getSourceFileOrError(program, fileName); @@ -1128,8 +1143,16 @@ runInEachFileSystem(() => { const callSymbol = templateTypeChecker.getSymbolOfNode(node.inputs[0].value, cmp)!; assertExpressionSymbol(callSymbol); // Note that the symbol returned is for the return value of the Call. - expect(callSymbol.tsSymbol).toBeNull(); + expect(callSymbol.tsSymbol).toBeTruthy(); + expect(callSymbol.tsSymbol?.getName()).toEqual('toString'); expect(program.getTypeChecker().typeToString(callSymbol.tsType)).toBe('string'); + + const nestedCallSymbol = templateTypeChecker.getSymbolOfNode(node.inputs[1].value, cmp)!; + assertExpressionSymbol(nestedCallSymbol); + // Note that the symbol returned is for the return value of the Call. + expect(nestedCallSymbol.tsSymbol).toBeTruthy(); + expect(nestedCallSymbol.tsSymbol?.getName()).toEqual('toString'); + expect(program.getTypeChecker().typeToString(nestedCallSymbol.tsType)).toBe('string'); }); it('should get a symbol for SafeCall expressions', () => { diff --git a/packages/language-service/src/template_target.ts b/packages/language-service/src/template_target.ts index fda0c8d8c49e..bf102f06e883 100644 --- a/packages/language-service/src/template_target.ts +++ b/packages/language-service/src/template_target.ts @@ -229,6 +229,7 @@ export function getTargetAtPosition( } const candidate = path[path.length - 1]; + // Walk up the result nodes to find the nearest `TmplAstTemplate` which contains the targeted // node. let context: TmplAstTemplate | null = null; diff --git a/packages/language-service/test/legacy/template_target_spec.ts b/packages/language-service/test/legacy/template_target_spec.ts index bab2bd836f73..42bf19ddf3a3 100644 --- a/packages/language-service/test/legacy/template_target_spec.ts +++ b/packages/language-service/test/legacy/template_target_spec.ts @@ -155,6 +155,15 @@ describe('getTargetAtPosition for template AST', () => { expect(node).toBeInstanceOf(e.PropertyRead); }); + it('should locate bound event nested value', () => { + const {errors, nodes, position} = parse(``); + expect(errors).toBe(null); + const {context} = getTargetAtPosition(nodes, position)!; + const {node} = context as SingleNodeTarget; + expect(isExpressionNode(node!)).toBe(true); + expect(node).toBeInstanceOf(e.PropertyRead); + }); + it('should locate element children', () => { const {errors, nodes, position} = parse(`
`); expect(errors).toBe(null); diff --git a/packages/language-service/test/quick_info_spec.ts b/packages/language-service/test/quick_info_spec.ts index c896eebd9fac..84caa111bdac 100644 --- a/packages/language-service/test/quick_info_spec.ts +++ b/packages/language-service/test/quick_info_spec.ts @@ -14,7 +14,7 @@ import {createModuleAndProjectWithDeclarations, LanguageServiceTestEnv, Project} function quickInfoSkeleton(): {[fileName: string]: string} { return { 'app.ts': ` - import {Component, Directive, EventEmitter, Input, NgModule, Output, Pipe, PipeTransform, model} from '@angular/core'; + import {Component, Directive, EventEmitter, Input, NgModule, Output, Pipe, PipeTransform, model, signal} from '@angular/core'; import {CommonModule} from '@angular/common'; export interface Address { @@ -60,6 +60,18 @@ function quickInfoSkeleton(): {[fileName: string]: string} { setTitle(newTitle: string) {} trackByFn!: any; name!: any; + someObject = { + someProp: 'prop', + someSignal: signal(0), + someMethod: (): number => 1, + nested: { + helloWorld: () => { + return { + nestedMethod: () => 1 + } + } + } + }; } @Directive({ @@ -331,14 +343,6 @@ describe('quick info', () => { }); }); - it('should work for $event from native element', () => { - expectQuickInfo({ - templateOverride: `
`, - expectedSpanText: '$event', - expectedDisplayString: '(parameter) $event: MouseEvent', - }); - }); - it('should work for click output from native element', () => { expectQuickInfo({ templateOverride: `
`, @@ -422,6 +426,23 @@ describe('quick info', () => { }); }); + it('should work for accessed function calls', () => { + expectQuickInfo({ + templateOverride: `
`, + expectedSpanText: 'someMethod', + expectedDisplayString: '(property) someMethod: () => number', + }); + }); + + it('should work for accessed very nested function calls', () => { + expectQuickInfo({ + templateOverride: `
`, + expectedSpanText: 'helloWorld', + expectedDisplayString: + '(property) helloWorld: () => {\n nestedMethod: () => number;\n}', + }); + }); + it('should find members in an attribute interpolation', () => { expectQuickInfo({ templateOverride: `
`, @@ -481,6 +502,44 @@ describe('quick info', () => { expect(toText(info.documentation)).toEqual('Documentation for myFunc.'); }); + it('should work for safe signal calls', () => { + const files = { + 'app.ts': `import {Component, Signal} from '@angular/core'; + @Component({template: '
'}) + export class AppCmp { + something!: { + /** Documentation for value. */ + value: Signal; + }; + }`, + }; + const project = createModuleAndProjectWithDeclarations(env, 'test_project', files); + const appFile = project.openFile('app.ts'); + appFile.moveCursorToText('something?.va¦lue()'); + const info = appFile.getQuickInfoAtPosition()!; + expect(toText(info.displayParts)).toEqual('(property) value: Signal'); + expect(toText(info.documentation)).toEqual('Documentation for value.'); + }); + + it('should work for signal calls', () => { + const files = { + 'app.ts': `import {Component, signal} from '@angular/core'; + @Component({template: '
'}) + export class AppCmp { + something = { + /** Documentation for value. */ + value: signal(0) + }; + }`, + }; + const project = createModuleAndProjectWithDeclarations(env, 'test_project', files); + const appFile = project.openFile('app.ts'); + appFile.moveCursorToText('something.va¦lue()'); + const info = appFile.getQuickInfoAtPosition()!; + expect(toText(info.displayParts)).toEqual('(property) value: WritableSignal\n() => number'); + expect(toText(info.documentation)).toEqual('Documentation for value.'); + }); + it('should work for accessed properties in writes', () => { expectQuickInfo({ templateOverride: `
`, @@ -719,6 +778,14 @@ describe('quick info', () => { expectedDisplayString: '(variable) aliasName: [{ readonly name: "name"; }]', }); }); + + it('if block alias variable', () => { + expectQuickInfo({ + templateOverride: `@if (someObject.some¦Signal(); as aliasName) {}`, + expectedSpanText: 'someSignal', + expectedDisplayString: '(property) someSignal: WritableSignal\n() => number', + }); + }); }); describe('let declarations', () => { diff --git a/packages/language-service/test/references_and_rename_spec.ts b/packages/language-service/test/references_and_rename_spec.ts index 5dabd3e50fb5..045bcad008b8 100644 --- a/packages/language-service/test/references_and_rename_spec.ts +++ b/packages/language-service/test/references_and_rename_spec.ts @@ -170,6 +170,42 @@ describe('find references and rename locations', () => { }); }); + describe('when cursor in on argument to a nested function call in an external template', () => { + let appFile: OpenBuffer; + + beforeEach(() => { + const files = { + 'app.ts': ` + import {Component} from '@angular/core'; + @Component({template: '
'}) + export class AppCmp { + title = ''; + nested = { + setTitle(s: string) {} + } + }`, + }; + env = LanguageServiceTestEnv.setup(); + const project = createModuleAndProjectWithDeclarations(env, 'test', files); + appFile = project.openFile('app.ts'); + appFile.moveCursorToText('(ti¦tle)'); + }); + + it('gets member reference in ts file', () => { + const refs = getReferencesAtPosition(appFile)!; + expect(refs.length).toBe(2); + + assertTextSpans(refs, ['title']); + }); + + it('finds rename location in ts file', () => { + const refs = getRenameLocationsAtPosition(appFile)!; + expect(refs.length).toBe(2); + + assertTextSpans(refs, ['title']); + }); + }); + describe('when cursor is on $event in method call arguments', () => { let file: OpenBuffer; diff --git a/packages/language-service/test/signature_help_spec.ts b/packages/language-service/test/signature_help_spec.ts index 9da96af9ff40..cbb7453cd34b 100644 --- a/packages/language-service/test/signature_help_spec.ts +++ b/packages/language-service/test/signature_help_spec.ts @@ -131,6 +131,30 @@ describe('signature help', () => { expect(items.argumentIndex).toEqual(1); expect(items.items.length).toEqual(1); }); + + it('should handle a single argument if the function is nested', () => { + const main = setup(` + import {Component} from '@angular/core'; + @Component({ + template: '{{ someObj.foo("test") }}', + }) + export class MainCmp { + someObj = { + foo(alpha: string, beta: number): string { + return 'blah'; + } + } + } + `); + main.moveCursorToText('foo("test"¦)'); + + const items = main.getSignatureHelpItems()!; + expect(items).toBeDefined(); + expect(getText(main.contents, items.applicableSpan)).toEqual('"test"'); + expect(items.argumentCount).toEqual(1); + expect(items.argumentIndex).toEqual(0); + expect(items.items.length).toEqual(1); + }); }); function setup(mainTs: string): OpenBuffer {