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',