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

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 {
+  sourceSpan(start: number, artificialEndIndex?: number): AbsoluteSourceSpan {
-    const serial = `${start}@${this.inputIndex}`;
+    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();
+        let nameId = this.expectIdentifierOrKeyword();
-        const nameSpan = this.sourceSpan(nameStart);
+        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 =
+        result = new BindingPipe(
-            new BindingPipe(this.span(start), this.sourceSpan(start), result, name, args, nameSpan);
+            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);
       }

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];
+}

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
+    it('should complete an empty pipe binding', () => {
-    // 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);
       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', () => {

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.BindingPipe);
-    expect(node).toBeInstanceOf(e.Interpolation);
   });
 
   it('should locate binding pipe without identifier',