Skip to content

Commit

Permalink
refactor(compiler-cli): Adjust generated TCB when checkTypeOfPipes is…
Browse files Browse the repository at this point in the history
… false (#40523)

When `checkTypeOfPipes` is set to `false`, our TCB currently generates
the a statement like the following when pipes appear in the template:
`(_pipe1 as any).transform(args)`

This did enable us to get _some_ information from the Language Service
about pipes in this case because we still had access to the pipe
instance. However, because it is immediately cast to `any`, we cannot
get type information about the transform access. That means actions like "go to
definition", "find references", "quick info", etc. will return
incomplete information or fail altogether.

Instead, this commit changes the TCB to generate `(_pipe1.transform as any)(args)`.
This gives us the ability to get complete information for the LS
operations listed above.

PR Close #40523
  • Loading branch information
atscott authored and alxhub committed May 6, 2021
1 parent a86ca4f commit 459af57
Show file tree
Hide file tree
Showing 14 changed files with 119 additions and 95 deletions.
Expand Up @@ -430,19 +430,14 @@ export class SymbolBuilder {
}

private getSymbolOfPipe(expression: BindingPipe): PipeSymbol|null {
const node = findFirstMatchingNode(
this.typeCheckBlock, {withSpan: expression.sourceSpan, filter: ts.isCallExpression});
if (node === null || !ts.isPropertyAccessExpression(node.expression)) {
const methodAccess = findFirstMatchingNode(
this.typeCheckBlock,
{withSpan: expression.nameSpan, filter: ts.isPropertyAccessExpression});
if (methodAccess === null) {
return null;
}

const methodAccess = node.expression;
// Find the node for the pipe variable from the transform property access. This will be one of
// two forms: `_pipe1.transform` or `(_pipe1 as any).transform`.
const pipeVariableNode = ts.isParenthesizedExpression(methodAccess.expression) &&
ts.isAsExpression(methodAccess.expression.expression) ?
methodAccess.expression.expression.expression :
methodAccess.expression;
const pipeVariableNode = methodAccess.expression;
const pipeDeclaration = this.getTypeChecker().getSymbolAtLocation(pipeVariableNode);
if (pipeDeclaration === undefined || pipeDeclaration.valueDeclaration === undefined) {
return null;
Expand Down
Expand Up @@ -1712,17 +1712,19 @@ class TcbExpressionTranslator {

// Use an 'any' value to at least allow the rest of the expression to be checked.
pipe = NULL_AS_ANY;
} else if (this.tcb.env.config.checkTypeOfPipes) {
} else {
// Use a variable declared as the pipe's type.
pipe = this.tcb.env.pipeInst(pipeRef);
} else {
// Use an 'any' value when not checking the type of the pipe.
pipe = ts.createAsExpression(
this.tcb.env.pipeInst(pipeRef), ts.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword));
}
const args = ast.args.map(arg => this.translate(arg));
const methodAccess = ts.createPropertyAccess(pipe, 'transform');
let methodAccess: ts.Expression =
ts.factory.createPropertyAccessExpression(pipe, 'transform');
addParseSpanInfo(methodAccess, ast.nameSpan);
if (!this.tcb.env.config.checkTypeOfPipes) {
methodAccess = ts.factory.createAsExpression(
methodAccess, ts.factory.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword));
}

const result = ts.createCall(
/* expression */ methodAccess,
/* typeArguments */ undefined,
Expand Down
Expand Up @@ -992,7 +992,7 @@ describe('type check blocks', () => {
const DISABLED_CONFIG: TypeCheckingConfig = {...BASE_CONFIG, checkTypeOfPipes: false};
const block = tcb(TEMPLATE, PIPES, DISABLED_CONFIG);
expect(block).toContain('var _pipe1: i0.TestPipe = null!;');
expect(block).toContain('((_pipe1 as any).transform(((ctx).a), ((ctx).b), ((ctx).c)));');
expect(block).toContain('((_pipe1.transform as any)(((ctx).a), ((ctx).b), ((ctx).c))');
});
});

Expand Down
Expand Up @@ -722,29 +722,19 @@ runInEachFileSystem(() => {
BindingPipe;
}

it('should get symbol for pipe', () => {
setupPipesTest();
const pipeSymbol = templateTypeChecker.getSymbolOfNode(binding, cmp)!;
assertPipeSymbol(pipeSymbol);
expect(program.getTypeChecker().symbolToString(pipeSymbol.tsSymbol!))
.toEqual('transform');
expect(program.getTypeChecker().symbolToString(pipeSymbol.classSymbol.tsSymbol))
.toEqual('TestPipe');
expect(program.getTypeChecker().typeToString(pipeSymbol.tsType!))
.toEqual('(value: string, repeat: number, commaSeparate: boolean) => string[]');
});

it('should get symbol for pipe, checkTypeOfPipes: false', () => {
setupPipesTest(false);
const pipeSymbol = templateTypeChecker.getSymbolOfNode(binding, cmp)! as PipeSymbol;
assertPipeSymbol(pipeSymbol);
expect(pipeSymbol.tsSymbol).toBeNull();
expect(program.getTypeChecker().typeToString(pipeSymbol.tsType!)).toEqual('any');
expect(program.getTypeChecker().symbolToString(pipeSymbol.classSymbol.tsSymbol))
.toEqual('TestPipe');
expect(program.getTypeChecker().typeToString(pipeSymbol.classSymbol.tsType))
.toEqual('TestPipe');
});
for (const checkTypeOfPipes of [true, false]) {
it(`should get symbol for pipe, checkTypeOfPipes: ${checkTypeOfPipes}`, () => {
setupPipesTest(checkTypeOfPipes);
const pipeSymbol = templateTypeChecker.getSymbolOfNode(binding, cmp)!;
assertPipeSymbol(pipeSymbol);
expect(program.getTypeChecker().symbolToString(pipeSymbol.tsSymbol!))
.toEqual('transform');
expect(program.getTypeChecker().symbolToString(pipeSymbol.classSymbol.tsSymbol))
.toEqual('TestPipe');
expect(program.getTypeChecker().typeToString(pipeSymbol.tsType!))
.toEqual('(value: string, repeat: number, commaSeparate: boolean) => string[]');
});
}

it('should get symbols for pipe expression and args', () => {
setupPipesTest(false);
Expand Down
Expand Up @@ -71,6 +71,7 @@ i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDE
}]
}] });
class MyForwardPipe {
transform() { }
}
MyForwardPipe.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyForwardPipe, deps: [], target: i0.ɵɵFactoryTarget.Pipe });
MyForwardPipe.ɵpipe = i0.ɵɵngDeclarePipe({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyForwardPipe, name: "my_forward_pipe" });
Expand Down
Expand Up @@ -11,6 +11,7 @@ export class HostBindingComp {

@Pipe({name: 'my_forward_pipe'})
class MyForwardPipe {
transform() {}
}

@NgModule({declarations: [HostBindingComp, MyForwardPipe]})
Expand Down
Expand Up @@ -1361,7 +1361,9 @@ runInEachFileSystem(() => {
@Pipe({
name: 'dep',
})
export class DepB {}
export class DepB {
transform() {}
}
`);
env.write('module.ts', `
import {NgModule} from '@angular/core';
Expand All @@ -1385,7 +1387,9 @@ runInEachFileSystem(() => {
@Pipe({
name: 'dep',
})
export class DepA {}
export class DepA {
transform() {}
}
@Directive({
selector: 'dep',
Expand Down
16 changes: 12 additions & 4 deletions packages/compiler-cli/test/ngtsc/incremental_spec.ts
Expand Up @@ -200,7 +200,9 @@ runInEachFileSystem(() => {
import {Pipe} from '@angular/core';
@Pipe({name: 'myPipe'})
export class MyPipe {}
export class MyPipe {
transform() {}
}
`);
env.write('module.ts', `
import {NgModule, NO_ERRORS_SCHEMA} from '@angular/core';
Expand Down Expand Up @@ -238,7 +240,9 @@ runInEachFileSystem(() => {
import {Pipe} from '@angular/core';
@Pipe({name: 'foo_changed'})
export class FooPipe {}
export class FooPipe {
transform() {}
}
`);
env.driveMain();
const written = env.getFilesWrittenSinceLastFlush();
Expand Down Expand Up @@ -910,7 +914,9 @@ runInEachFileSystem(() => {
import {Pipe} from '@angular/core';
@Pipe({name: 'foo'})
export class FooPipe {}
export class FooPipe {
transform() {}
}
`);
env.write('foo_module.ts', `
import {NgModule} from '@angular/core';
Expand Down Expand Up @@ -940,7 +946,9 @@ runInEachFileSystem(() => {
import {Pipe} from '@angular/core';
@Pipe({name: 'foo'})
export class BarPipe {}
export class BarPipe {
transform() {}
}
`);
env.write('bar_module.ts', `
import {NgModule} from '@angular/core';
Expand Down
20 changes: 15 additions & 5 deletions packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
Expand Up @@ -757,7 +757,9 @@ function allTests(os: string) {
// ModuleA classes
@Pipe({name: 'number'})
class PipeA {}
class PipeA {
transform() {}
}
@NgModule({
declarations: [PipeA],
Expand All @@ -768,7 +770,9 @@ function allTests(os: string) {
// ModuleB classes
@Pipe({name: 'number'})
class PipeB {}
class PipeB {
transform() {}
}
@Component({
selector: 'app',
Expand Down Expand Up @@ -800,7 +804,9 @@ function allTests(os: string) {
// ModuleA classes
@Pipe({name: 'number'})
class PipeA {}
class PipeA {
transform() {}
}
@NgModule({
declarations: [PipeA],
Expand All @@ -811,7 +817,9 @@ function allTests(os: string) {
// ModuleB classes
@Pipe({name: 'number'})
class PipeB {}
class PipeB {
transform() {}
}
@NgModule({
declarations: [PipeB],
Expand Down Expand Up @@ -1647,7 +1655,9 @@ function allTests(os: string) {
import {Component, NgModule, Pipe} from '@angular/core';
@Pipe({name: 'test'})
export class TestPipe {}
export class TestPipe {
transform() {}
}
@Component({selector: 'test-cmp', template: '{{value | test}}'})
export class TestCmp {
Expand Down
11 changes: 5 additions & 6 deletions packages/language-service/ivy/test/definitions_spec.ts
Expand Up @@ -8,7 +8,7 @@

import {initMockFileSystem} from '@angular/compiler-cli/src/ngtsc/file_system/testing';

import {assertFileNames, createModuleAndProjectWithDeclarations, humanizeDocumentSpanLike, LanguageServiceTestEnv, OpenBuffer} from '../testing';
import {assertFileNames, assertTextSpans, createModuleAndProjectWithDeclarations, humanizeDocumentSpanLike, LanguageServiceTestEnv, OpenBuffer} from '../testing';

describe('definitions', () => {
it('gets definition for template reference in overridden template', () => {
Expand All @@ -35,7 +35,7 @@ describe('definitions', () => {
assertFileNames(Array.from(definitions!), ['app.html']);
});

it('returns the pipe class as definition when checkTypeOfPipes is false', () => {
it('returns the pipe definitions when checkTypeOfPipes is false', () => {
initMockFileSystem('Native');
const files = {
'app.ts': `
Expand All @@ -57,10 +57,9 @@ describe('definitions', () => {

const {textSpan, definitions} = getDefinitionsAndAssertBoundSpan(env, template);
expect(template.contents.substr(textSpan.start, textSpan.length)).toEqual('date');
expect(definitions!.length).toEqual(1);
const [def] = definitions!;
expect(def.textSpan).toContain('DatePipe');
expect(def.contextSpan).toContain('DatePipe');
expect(definitions.length).toEqual(3);
assertTextSpans(definitions, ['transform']);
assertFileNames(definitions, ['index.d.ts']);
});

it('gets definitions for all inputs when attribute matches more than one', () => {
Expand Down
9 changes: 7 additions & 2 deletions packages/language-service/ivy/test/quick_info_spec.ts
Expand Up @@ -559,8 +559,13 @@ describe('quick info', () => {
// checkTypeOfPipes is set to false when strict templates is false
project = env.addProject('test', quickInfoSkeleton(), {strictTemplates: false});
const templateOverride = `<p>The hero's birthday is {{birthday | da¦te: "MM/dd/yy"}}</p>`;
expectQuickInfo(
{templateOverride, expectedSpanText: 'date', expectedDisplayString: '(pipe) DatePipe'});
expectQuickInfo({
templateOverride,
expectedSpanText: 'date',
expectedDisplayString:
'(pipe) DatePipe.transform(value: string | number | Date, format?: string | undefined, timezone?: ' +
'string | undefined, locale?: string | undefined): string | null (+2 overloads)'
});
});

it('should still get quick info if there is an invalid css resource', () => {
Expand Down
60 changes: 31 additions & 29 deletions packages/language-service/ivy/test/references_and_rename_spec.ts
Expand Up @@ -754,47 +754,49 @@ describe('find references and rename locations', () => {
}
}`;

describe('when cursor is on pipe name', () => {
let file: OpenBuffer;
beforeEach(() => {
const files = {
'app.ts': `
for (const checkTypeOfPipes of [true, false]) {
describe(`when cursor is on pipe name, checkTypeOfPipes: ${checkTypeOfPipes}`, () => {
let file: OpenBuffer;
beforeEach(() => {
const files = {
'app.ts': `
import {Component} from '@angular/core';
@Component({template: '{{birthday | prefixPipe: "MM/dd/yy"}}'})
export class AppCmp {
birthday = '';
}
`,
'prefix-pipe.ts': prefixPipe
};
'prefix-pipe.ts': prefixPipe
};

env = LanguageServiceTestEnv.setup();
const project = createModuleAndProjectWithDeclarations(env, 'test', files);
file = project.openFile('app.ts');
file.moveCursorToText('prefi¦xPipe:');
});
env = LanguageServiceTestEnv.setup();
const project = createModuleAndProjectWithDeclarations(env, 'test', files);
file = project.openFile('app.ts');
file.moveCursorToText('prefi¦xPipe:');
});

it('should find references', () => {
const refs = getReferencesAtPosition(file)!;
expect(refs.length).toBe(5);
assertFileNames(refs, ['index.d.ts', 'prefix-pipe.ts', 'app.ts']);
assertTextSpans(refs, ['transform', 'prefixPipe']);
});
it('should find references', () => {
const refs = getReferencesAtPosition(file)!;
expect(refs.length).toBe(5);
assertFileNames(refs, ['index.d.ts', 'prefix-pipe.ts', 'app.ts']);
assertTextSpans(refs, ['transform', 'prefixPipe']);
});

it('should find rename locations', () => {
const renameLocations = getRenameLocationsAtPosition(file)!;
expect(renameLocations.length).toBe(2);
assertFileNames(renameLocations, ['prefix-pipe.ts', 'app.ts']);
assertTextSpans(renameLocations, ['prefixPipe']);
});
it('should find rename locations', () => {
const renameLocations = getRenameLocationsAtPosition(file)!;
expect(renameLocations.length).toBe(2);
assertFileNames(renameLocations, ['prefix-pipe.ts', 'app.ts']);
assertTextSpans(renameLocations, ['prefixPipe']);
});

it('should get rename info', () => {
const result = file.getRenameInfo() as ts.RenameInfoSuccess;
expect(result.canRename).toEqual(true);
expect(result.displayName).toEqual('prefixPipe');
it('should get rename info', () => {
const result = file.getRenameInfo() as ts.RenameInfoSuccess;
expect(result.canRename).toEqual(true);
expect(result.displayName).toEqual('prefixPipe');
});
});
});
}

describe('when cursor is on pipe name expression', () => {
it('finds rename locations and rename info', () => {
Expand Down
12 changes: 10 additions & 2 deletions packages/language-service/ivy/test/ts_utils_spec.ts
@@ -1,3 +1,11 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

import {initMockFileSystem} from '@angular/compiler-cli/src/ngtsc/file_system/testing';
import * as ts from 'typescript';
import {LanguageServiceTestEnv, OpenBuffer, Project} from '../testing';
Expand All @@ -7,7 +15,7 @@ describe('ts utils', () => {
describe('collectMemberMethods', () => {
beforeEach(() => {
initMockFileSystem('Native');
})
});

it('gets only methods in class, not getters, setters, or properties', () => {
const files = {
Expand Down Expand Up @@ -69,7 +77,7 @@ describe('ts utils', () => {
function getMemberMethodNames(project: Project, file: OpenBuffer): string[] {
const sf = project.getSourceFile('app.ts')!;
const node = findTightestNode(sf, file.cursor)!;
expect(ts.isClassDeclaration(node.parent)).toBe(true)
expect(ts.isClassDeclaration(node.parent)).toBe(true);
return collectMemberMethods(node.parent as ts.ClassDeclaration, project.getTypeChecker())
.map(m => m.name.getText())
.sort();
Expand Down

0 comments on commit 459af57

Please sign in to comment.