Skip to content

Commit 7797671

Browse files
atscottatscott
committed
fix(language-service): get quick info at local var location to align with TS semantics and support type narrowing
Previously, the Language Service fetched Quick Info and definitions for template variables (such as `@let` declarations) using mapping to their `initializerLocation` (the right-hand side expression). This aggressively bubbled the type, JSDoc, and definition identity of the initializer backwards onto the variable itself. This approach had two flaws: 1. It broke type narrowing because the LS read the original un-narrowed type from the source expression rather than the type of the narrowed intermediate variable in the Type Check Block. 2. It deviated from native TypeScript semantics, where a local `let` binding (`let address = hero.address`) does not inherit the docstrings or `(property)` kind of its initializer, acting solely as a local inferred variable. By using `localVarLocation` rather than `initializerLocation` for LetDeclaration Quick Info and Type Definitions, these intermediate variables now properly preserve type narrowing within templates and flawlessly match the standard behavior expected of TypeScript block variables. `VariableSymbol.initializerLocation` is retained solely to map the value spans of structural directive contexts (e.g., `exportAs` strings). fixes #65491 (cherry picked from commit 75ac120)
1 parent 0b08e29 commit 7797671

7 files changed

Lines changed: 42 additions & 40 deletions

File tree

‎packages/compiler-cli/src/ngtsc/typecheck/api/symbols.ts‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -274,11 +274,6 @@ export interface LetDeclarationSymbol {
274274
* The location in the shim file for the identifier of the `@let` declaration.
275275
*/
276276
localVarLocation: TcbLocation;
277-
278-
/**
279-
* The location in the shim file of the `@let` declaration's initializer expression.
280-
*/
281-
initializerLocation: TcbLocation;
282277
}
283278

284279
/**

‎packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -646,9 +646,9 @@ export class SymbolBuilder {
646646
return {
647647
tsType: nodeValueSymbol.tsType,
648648
tsSymbol: nodeValueSymbol.tsSymbol,
649-
initializerLocation: nodeValueSymbol.tcbLocation,
650649
kind: SymbolKind.Variable,
651650
declaration: variable,
651+
initializerLocation: nodeValueSymbol.tcbLocation,
652652
localVarLocation: {
653653
tcbPath: this.tcbPath,
654654
isShimFile: this.tcbIsShim,
@@ -727,7 +727,7 @@ export class SymbolBuilder {
727727
return null;
728728
}
729729

730-
const nodeValueSymbol = this.getSymbolOfTsNode(node.initializer!);
730+
const nodeValueSymbol = this.getSymbolOfTsNode(node.name);
731731

732732
if (nodeValueSymbol === null) {
733733
return null;
@@ -736,7 +736,6 @@ export class SymbolBuilder {
736736
return {
737737
tsType: nodeValueSymbol.tsType,
738738
tsSymbol: nodeValueSymbol.tsSymbol,
739-
initializerLocation: nodeValueSymbol.tcbLocation,
740739
kind: SymbolKind.LetDeclaration,
741740
declaration: decl,
742741
localVarLocation: {

‎packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -201,12 +201,6 @@ runInEachFileSystem(() => {
201201
assertVariableSymbol(symbol);
202202
expect(program.getTypeChecker().typeToString(symbol.tsType!)).toEqual('any');
203203
expect(symbol.declaration.name).toEqual('contextFoo');
204-
205-
// Ensure we can map the shim locations back to the template
206-
const initializerMapping = templateTypeChecker.getSourceMappingAtTcbLocation(
207-
symbol.initializerLocation,
208-
)!;
209-
expect(initializerMapping.span.toString()).toEqual('bar');
210204
const localVarMapping = templateTypeChecker.getSourceMappingAtTcbLocation(
211205
symbol.localVarLocation,
212206
)!;
@@ -2025,11 +2019,6 @@ runInEachFileSystem(() => {
20252019
expect(program.getTypeChecker().typeToString(symbol.tsType!)).toEqual('string');
20262020
expect(symbol.declaration.name).toEqual('message');
20272021

2028-
// Ensure we can map the shim locations back to the template
2029-
const initializerMapping = templateTypeChecker.getSourceMappingAtTcbLocation(
2030-
symbol.initializerLocation,
2031-
)!;
2032-
expect(initializerMapping.span.toString()).toEqual(`'The value is ' + value`);
20332022
const localVarMapping = templateTypeChecker.getSourceMappingAtTcbLocation(
20342023
symbol.localVarLocation,
20352024
)!;

‎packages/language-service/src/definitions.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,7 @@ export class DefinitionBuilder {
172172
});
173173
}
174174
}
175-
if (symbol.kind === SymbolKind.Variable || symbol.kind === SymbolKind.LetDeclaration) {
175+
if (symbol.kind === SymbolKind.Variable) {
176176
definitions.push(
177177
...this.getDefinitionsForSymbols({tcbLocation: symbol.initializerLocation}),
178178
);
@@ -281,7 +281,7 @@ export class DefinitionBuilder {
281281
case SymbolKind.Variable:
282282
case SymbolKind.LetDeclaration: {
283283
definitions.push(
284-
...this.getTypeDefinitionsForSymbols({tcbLocation: symbol.initializerLocation}),
284+
...this.getTypeDefinitionsForSymbols({tcbLocation: symbol.localVarLocation}),
285285
);
286286
break;
287287
}

‎packages/language-service/src/quick_info.ts‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -148,21 +148,24 @@ export class QuickInfoBuilder {
148148
);
149149
}
150150

151-
private getQuickInfoForVariableSymbol(symbol: VariableSymbol): ts.QuickInfo {
152-
const info = this.getQuickInfoFromTypeDefAtLocation(symbol.initializerLocation);
153-
return createQuickInfo(
154-
symbol.declaration.name,
155-
DisplayInfoKind.VARIABLE,
156-
getTextSpanOfNode(this.node),
157-
undefined /* containerName */,
158-
this.typeChecker.typeToString(symbol.tsType),
159-
info?.documentation,
160-
info?.tags,
161-
);
151+
private getQuickInfoForVariableSymbol(symbol: VariableSymbol): ts.QuickInfo | undefined {
152+
const quickInfo = this.getQuickInfoAtTcbLocation(symbol.localVarLocation);
153+
if (quickInfo === undefined || quickInfo.displayParts === undefined) {
154+
return quickInfo;
155+
}
156+
157+
for (const part of quickInfo.displayParts) {
158+
if (part.kind === 'localName') {
159+
part.text = symbol.declaration.name;
160+
break;
161+
}
162+
}
163+
164+
return updateQuickInfoKind(quickInfo, DisplayInfoKind.VARIABLE);
162165
}
163166

164167
private getQuickInfoForLetDeclarationSymbol(symbol: LetDeclarationSymbol): ts.QuickInfo {
165-
const info = this.getQuickInfoFromTypeDefAtLocation(symbol.initializerLocation);
168+
const info = this.getQuickInfoAtTcbLocation(symbol.localVarLocation);
166169
return createQuickInfo(
167170
symbol.declaration.name,
168171
DisplayInfoKind.LET,

‎packages/language-service/src/utils/display_parts.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ export function getSymbolDisplayInfo(
8080
const quickInfo =
8181
symbol.kind === SymbolKind.Reference
8282
? getQuickInfoFromTypeDefAtLocation(tsLS, symbol.targetLocation)
83-
: getQuickInfoFromTypeDefAtLocation(tsLS, symbol.initializerLocation);
83+
: getQuickInfoFromTypeDefAtLocation(tsLS, symbol.localVarLocation);
8484
return {
8585
kind,
8686
displayParts,

‎packages/language-service/test/grp3/quick_info_spec.ts‎

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -401,7 +401,7 @@ describe('quick info', () => {
401401
expectedSpanText: 'hero',
402402
expectedDisplayString: '(variable) hero: Hero',
403403
});
404-
expect(toText(documentation)).toEqual('The most heroic being.');
404+
expect(toText(documentation)).toEqual('');
405405
});
406406

407407
it('should work for ReadonlyArray members (#36191)', () => {
@@ -416,7 +416,7 @@ describe('quick info', () => {
416416
expectQuickInfo({
417417
templateOverride: `<div *ngFor="let name of constNames">{{na¦me}}</div>`,
418418
expectedSpanText: 'name',
419-
expectedDisplayString: '(variable) name: { readonly name: "name"; }',
419+
expectedDisplayString: '(variable) name: {\n readonly name: "name";\n}',
420420
});
421421
});
422422

@@ -438,15 +438,15 @@ describe('quick info', () => {
438438
expectQuickInfo({
439439
templateOverride: `<div *ngFor="let name of constNames">{{\`Hello \${na¦me}\`}}</div>`,
440440
expectedSpanText: 'name',
441-
expectedDisplayString: '(variable) name: { readonly name: "name"; }',
441+
expectedDisplayString: '(variable) name: {\n readonly name: "name";\n}',
442442
});
443443
});
444444

445445
it('should work for tagged template literals', () => {
446446
expectQuickInfo({
447447
templateOverride: `<div *ngFor="let name of constNames">{{someTag\`Hello \${na¦me}\`}}</div>`,
448448
expectedSpanText: 'name',
449-
expectedDisplayString: '(variable) name: { readonly name: "name"; }',
449+
expectedDisplayString: '(variable) name: {\n readonly name: "name";\n}',
450450
});
451451
});
452452
});
@@ -867,7 +867,15 @@ describe('quick info', () => {
867867
expectQuickInfo({
868868
templateOverride: `@if (constNames; as al¦iasName) {}`,
869869
expectedSpanText: 'aliasName',
870-
expectedDisplayString: '(variable) aliasName: [{ readonly name: "name"; }]',
870+
expectedDisplayString: '(variable) aliasName: [{\n readonly name: "name";\n}]',
871+
});
872+
});
873+
874+
it('if block alias variable narrowed', () => {
875+
expectQuickInfo({
876+
templateOverride: `@if (signalValue; as al¦iasName) {}`,
877+
expectedSpanText: 'aliasName',
878+
expectedDisplayString: '(variable) aliasName: string | undefined',
871879
});
872880
});
873881

@@ -883,7 +891,7 @@ describe('quick info', () => {
883891
expectQuickInfo({
884892
templateOverride: `@if (false) {} @else if (constNames; as al¦iasName) {}`,
885893
expectedSpanText: 'aliasName',
886-
expectedDisplayString: '(variable) aliasName: [{ readonly name: "name"; }]',
894+
expectedDisplayString: '(variable) aliasName: [{\n readonly name: "name";\n}]',
887895
});
888896
});
889897
});
@@ -896,6 +904,14 @@ describe('quick info', () => {
896904
expectedDisplayString: `(let) name: "Frodo"`,
897905
});
898906
});
907+
908+
it('should get quick info for a let declaration initialized with a narrowed property', () => {
909+
expectQuickInfo({
910+
templateOverride: `@if (signalValue) { @let na¦me = signalValue; {{name}} }`,
911+
expectedSpanText: `@let name = signalValue`,
912+
expectedDisplayString: `(let) name: string`,
913+
});
914+
});
899915
});
900916

901917
it('should work for object literal with shorthand property declarations', () => {

0 commit comments

Comments
 (0)