From a8c5c8ed2da82fdc4c202b8f9a3a8ed78918d6d8 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Thu, 7 Jan 2021 14:16:05 -0800 Subject: [PATCH] fix(language-service): recognize incomplete pipe bindings with whitespace (#40346) The Language Service uses the source span of AST nodes to recognize which node a user has selected, given their cursor position in a template. This is used to trigger autocompletion. The previous source span of BindingPipe nodes created a problem when: 1) the pipe binding had no identifier (incomplete or in-progress expression) 2) the user typed trailing whitespace after the pipe character ('|') For example, the expression `{{foo | }}`. If the cursor preceded the '}' in that expression, the Language Service was unable to detect that the user was autocompleting the BindingPipe expression, since the span of the BindingPipe ended after the '|'. This commit changes the expression parser to expand the span of BindingPipe expressions with a missing identifier, to include any trailing whitespace. This allows the Language Service to correctly recognize this case as targeting the BindingPipe and complete it successfully. The `nameSpan` of the BindingPipe is also moved to be right-aligned with the end of any whitespace present in the pipe binding expression. This change allows for the disabled test in the Language Service for pipe completion in this case to be re-enabled. PR Close #40346 --- .../compiler/src/expression_parser/parser.ts | 62 +++++++++++++++---- .../test/expression_parser/parser_spec.ts | 17 ++++- .../ivy/test/completions_spec.ts | 11 +--- .../ivy/test/legacy/template_target_spec.ts | 3 +- 4 files changed, 70 insertions(+), 23 deletions(-) diff --git a/packages/compiler/src/expression_parser/parser.ts b/packages/compiler/src/expression_parser/parser.ts index fc386e7ebf..df6b60ec10 100644 --- a/packages/compiler/src/expression_parser/parser.ts +++ b/packages/compiler/src/expression_parser/parser.ts @@ -450,14 +450,27 @@ export class _ParseAST { return this.absoluteOffset + this.inputIndex; } - span(start: number) { - return new ParseSpan(start, this.currentEndIndex); + /** + * Retrieve a `ParseSpan` from `start` to the current position (or to `artificialEndIndex` if + * provided). + * + * @param start Position from which the `ParseSpan` will start. + * @param artificialEndIndex Optional ending index to be used if provided (and if greater than the + * natural ending index) + */ + span(start: number, artificialEndIndex?: number): ParseSpan { + let endIndex = this.currentEndIndex; + if (artificialEndIndex !== undefined && artificialEndIndex > this.currentEndIndex) { + endIndex = artificialEndIndex; + } + return new ParseSpan(start, endIndex); } - sourceSpan(start: number): AbsoluteSourceSpan { - const serial = `${start}@${this.inputIndex}`; + sourceSpan(start: number, artificialEndIndex?: number): AbsoluteSourceSpan { + const serial = `${start}@${this.inputIndex}:${artificialEndIndex}`; if (!this.sourceSpanCache.has(serial)) { - this.sourceSpanCache.set(serial, this.span(start).toAbsolute(this.absoluteOffset)); + this.sourceSpanCache.set( + serial, this.span(start, artificialEndIndex).toAbsolute(this.absoluteOffset)); } return this.sourceSpanCache.get(serial)!; } @@ -521,11 +534,11 @@ export class _ParseAST { return tok === EOF ? 'end of input' : `token ${tok}`; } - expectIdentifierOrKeyword(): string { + expectIdentifierOrKeyword(): string|null { const n = this.next; if (!n.isIdentifier() && !n.isKeyword()) { this.error(`Unexpected ${this.prettyPrintToken(n)}, expected identifier or keyword`); - return ''; + return null; } this.advance(); return n.toString() as string; @@ -572,15 +585,40 @@ export class _ParseAST { do { const nameStart = this.inputIndex; - const name = this.expectIdentifierOrKeyword(); - const nameSpan = this.sourceSpan(nameStart); + let nameId = this.expectIdentifierOrKeyword(); + let nameSpan: AbsoluteSourceSpan; + let fullSpanEnd: number|undefined = undefined; + if (nameId !== null) { + nameSpan = this.sourceSpan(nameStart); + } else { + // No valid identifier was found, so we'll assume an empty pipe name (''). + nameId = ''; + + // However, there may have been whitespace present between the pipe character and the next + // token in the sequence (or the end of input). We want to track this whitespace so that + // the `BindingPipe` we produce covers not just the pipe character, but any trailing + // whitespace beyond it. Another way of thinking about this is that the zero-length name + // is assumed to be at the end of any whitespace beyond the pipe character. + // + // Therefore, we push the end of the `ParseSpan` for this pipe all the way up to the + // beginning of the next token, or until the end of input if the next token is EOF. + fullSpanEnd = this.next.index !== -1 ? this.next.index : this.inputLength + this.offset; + + // The `nameSpan` for an empty pipe name is zero-length at the end of any whitespace + // beyond the pipe character. + nameSpan = new ParseSpan(fullSpanEnd, fullSpanEnd).toAbsolute(this.absoluteOffset); + } + const args: AST[] = []; while (this.consumeOptionalCharacter(chars.$COLON)) { args.push(this.parseExpression()); + + // If there are additional expressions beyond the name, then the artificial end for the + // name is no longer relevant. } const {start} = result.span; - result = - new BindingPipe(this.span(start), this.sourceSpan(start), result, name, args, nameSpan); + result = new BindingPipe( + this.span(start), this.sourceSpan(start, fullSpanEnd), result, nameId, args, nameSpan); } while (this.consumeOptionalOperator('|')); } @@ -881,7 +919,7 @@ export class _ParseAST { const start = receiver.span.start; const nameStart = this.inputIndex; const id = this.withContext(ParseContextFlags.Writable, () => { - const id = this.expectIdentifierOrKeyword(); + const id = this.expectIdentifierOrKeyword() ?? ''; if (id.length === 0) { this.error(`Expected identifier for property access`, receiver.span.end); } diff --git a/packages/compiler/test/expression_parser/parser_spec.ts b/packages/compiler/test/expression_parser/parser_spec.ts index 9b613b7274..4130961cbc 100644 --- a/packages/compiler/test/expression_parser/parser_spec.ts +++ b/packages/compiler/test/expression_parser/parser_spec.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {ASTWithSource, BindingPipe, Interpolation, ParserError, TemplateBinding, VariableBinding} from '@angular/compiler/src/expression_parser/ast'; +import {AbsoluteSourceSpan, ASTWithSource, BindingPipe, Interpolation, ParserError, TemplateBinding, VariableBinding} from '@angular/compiler/src/expression_parser/ast'; import {Lexer} from '@angular/compiler/src/expression_parser/lexer'; import {IvyParser, Parser, SplitInterpolation} from '@angular/compiler/src/expression_parser/parser'; import {expect} from '@angular/platform-browser/testing/src/matchers'; @@ -447,6 +447,17 @@ describe('parser', () => { expectBindingError(input, err); }); } + + it('should parse an incomplete pipe with a source span that includes trailing whitespace', + () => { + const bindingText = 'foo | '; + const binding = parseBinding(bindingText).ast as BindingPipe; + + // The sourceSpan should include all characters of the input. + expect(rawSpan(binding.sourceSpan)).toEqual([0, bindingText.length]); + // The nameSpan should be positioned at the end of the input. + expect(rawSpan(binding.nameSpan)).toEqual([bindingText.length, bindingText.length]); + }); }); it('should only allow identifier or keyword as formatter names', () => { @@ -1180,3 +1191,7 @@ function checkActionWithError(text: string, expected: string, error: string) { checkAction(text, expected); expectActionError(text, error); } + +function rawSpan(span: AbsoluteSourceSpan): [number, number] { + return [span.start, span.end]; +} diff --git a/packages/language-service/ivy/test/completions_spec.ts b/packages/language-service/ivy/test/completions_spec.ts index 3ab849be76..be61b17aef 100644 --- a/packages/language-service/ivy/test/completions_spec.ts +++ b/packages/language-service/ivy/test/completions_spec.ts @@ -569,18 +569,13 @@ describe('completions', () => { expectReplacementText(completions, text, 'some'); }); - // TODO(alxhub): currently disabled as the template targeting system identifies the cursor - // position as the entire Interpolation node, not the BindingPipe node. This happens because the - // BindingPipe node's span ends at the '|' character. To make this case work, the targeting - // system will need to artificially expand the BindingPipe's span to encompass any trailing - // spaces, which will be done in a future PR. - xit('should complete an empty pipe binding', () => { - const {ngLS, fileName, cursor, text} = setup(`{{ foo | ¦ }}`, '', SOME_PIPE); + it('should complete an empty pipe binding', () => { + const {ngLS, fileName, cursor, text} = setup(`{{foo | ¦}}`, '', SOME_PIPE); const completions = ngLS.getCompletionsAtPosition(fileName, cursor, /* options */ undefined); expectContain( completions, unsafeCastDisplayInfoKindToScriptElementKind(DisplayInfoKind.PIPE), ['somePipe']); - expectReplacementText(completions, text, 'some'); + expectReplacementText(completions, text, ''); }); it('should not return extraneous completions', () => { diff --git a/packages/language-service/ivy/test/legacy/template_target_spec.ts b/packages/language-service/ivy/test/legacy/template_target_spec.ts index 56b62ade24..87eed427c1 100644 --- a/packages/language-service/ivy/test/legacy/template_target_spec.ts +++ b/packages/language-service/ivy/test/legacy/template_target_spec.ts @@ -497,8 +497,7 @@ describe('getTargetAtPosition for expression AST', () => { const {context} = getTargetAtPosition(nodes, position)!; const {node} = context as SingleNodeTarget; expect(isExpressionNode(node!)).toBe(true); - // TODO: We want this to be a BindingPipe. - expect(node).toBeInstanceOf(e.Interpolation); + expect(node).toBeInstanceOf(e.BindingPipe); }); it('should locate binding pipe without identifier',