feat(compiler): add support for shorthand property declarations in templates

[crisbeto]

Adds support for shorthand property declarations inside Angular templates. E.g. writing {foo, bar} instead of {foo: foo, bar: bar}.

Fixes #10277.

[jessicajaniuk]

LGTM 🍪

[JoostK]

The change generally looks good, with a comment on parse spans.

I think this syntax requires special attention in the language service when renaming a property, as the shorthand syntax may need to be expanded in that case. @atscott WDYT about this case?

[crisbeto]

I've addressed the span issue @JoostK.

[crisbeto]

Addressed the latest set of feedback and fixed the test failure.

[atscott]

I think we should add more tests around each part of the system that supports the language service:

The "template target" code ensures that we can map a position in a template to a target node in the TCB. This code is tested in packages/language-service/ivy/test/legacy/template_target_spec.ts (note that these tests do not pass so there needs to be an update to the template target code):

  describe('object literal shorthand', () => {
    fit('with one item', () => {
      debugger;
      const {errors, nodes, position} = parse(`{{ {va¦l1} }}`);
      expect(errors).toBe(null);
      const {context} = getTargetAtPosition(nodes, position)!;
      expect(context.kind).toBe(TargetNodeKind.RawExpression);
      // Maybe this should be multinode target so we target both the key and the value of the object
      // literal map
      const {node} = context as SingleNodeTarget;
      expect(node).toBeInstanceOf(e.PropertyRead);
      expect((node as e.PropertyRead).name).toBe('val1');
    });

    fit('with two shorthands', () => {
      const {errors, nodes, position} = parse(`{{ {val1, va¦l2} }}`);
      expect(errors).toBe(null);
      const {context} = getTargetAtPosition(nodes, position)!;
      expect(context.kind).toBe(TargetNodeKind.RawExpression);
      const {node} = context as SingleNodeTarget;
      expect(node).toBeInstanceOf(e.PropertyRead);
      expect((node as e.PropertyRead).name).toBe('val2');
    });

    fit('with shorthand and regular syntax', () => {
      const {errors, nodes, position} = parse(`{{ {val1: 'val1', va¦l2} }}`);
      expect(errors).toBe(null);
      const {context} = getTargetAtPosition(nodes, position)!;
      expect(context.kind).toBe(TargetNodeKind.RawExpression);
      const {node} = context as SingleNodeTarget;
      expect(node).toBeInstanceOf(e.PropertyRead);
      expect((node as e.PropertyRead).name).toBe('val2');
    });
  });

This brings up an immediate question: Should we just target the property read node via a SingleNodeTarget? The way the TCB is currently generated, I think that's all we can do.

We should add some tests around the TCB generation in packages/compiler-cli/src/ngtsc/typecheck/test/span_comments_spec.ts. This is the second part of the question above: What do we do about the divergent representations in the type check block vs what's in the template? The template code is written as {val1} and then generated as {"val1": ((ctx).val1 /*span*/) /*span*/} in the TCB. The language service leverages the TCB for most of the actions we perform. Because the TCB output is different than the representation in the template, it would be hard to get everything fully correct (renames, go to definition, etc.). This might just be something we have to be okay with and just do actions off of the property read. We might need to add a new template target type that can detect this case and not allow renaming. Or we just allow a rename from the property read and be okay that we get it wrong where template shorthand is used (which hopefully isn't many places and at least the compiler will detect them and complain).

After the template node is found, the language service builds a symbol for the template node. This part is tested in packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts. Please add a test there to ensure we build the correct symbol for this case.

Lastly, we have specific functions in the language service for LSP operations (quick info, definitions, etc) using the template symbol information. Generally if the symbol was built correctly, the other parts should also work. Here's a test for quick info (packages/language-service/ivy/test/quick_info_spec.ts):

  // TODO(atscott): This should be able to be added to the previous section as long as the project
  // has a different name than the one in the beforeEach but something goes wrong in the test
  // environment
  describe('non-skeleton project', () => {
    beforeEach(() => {
      initMockFileSystem('Native');
      env = LanguageServiceTestEnv.setup();
    });
    fit('should work for shorthand object literal', () => {
      project = env.addProject(
          'test', {
            'app.ts': `
            import {Component, NgModule} from '@angular/core';
            import {CommonModule} from '@angular/common';

            @Component({
              selector: 'some-cmp',
              templateUrl: './app.html',
            })
            export class SomeCmp {
              val1 = 'one';
              val2 = 2;

              doSomething(obj: {val1: string, val2: number}) {

              }
            }

            @NgModule({
              declarations: [SomeCmp],
              imports: [CommonModule],
            })
            export class AppModule{
            }
       `,
            'app.html': `{{doSomething({val1, val2})}}`,
          },
          {strictTemplates: true});
      env.expectNoSourceDiagnostics();
      project.expectNoSourceDiagnostics();

      const template = project.openFile('app.html');
      template.moveCursorToText('val¦1');
      const quickInfo = template.getQuickInfoAtPosition();
      expect(toText(quickInfo!.displayParts)).toEqual('(property) val1: string');
      template.moveCursorToText('val¦2');
      const quickInfo2 = template.getQuickInfoAtPosition();
      expect(toText(quickInfo!.displayParts)).toEqual('(property) val2: number');
    });
  });

You propably want to add a similar test to definitions_spec in the same folder to ensure "go to definition" works properly as well as the references_and_rename_spec.

[crisbeto]

@atscott all of the feedback should be covered in the latest fixup commit. I've also included more tests for safe keyed reads since I didn't get the chance to add them before #41911 got merged in.

[crisbeto]

The latest set of feedback has been addressed.

[alxhub]

This test appears unrelated to everything else in this PR. Is it here because prior designs broke this feature?

Either way, I would recommend extracting the addition of this test as a separate commit prior to the main commit, to avoid confusion.

[crisbeto]

These are tests that I missed in my safe keyed reads PR (#41911). I decided to add them here since this PR touches the same test files. I would rather keep them as is for now, because untangling them from the various fixup commits will take a while and may involve having to squash the commits.

[alxhub]

I think we should squash the commits and then extract these tests into a separate commit, honestly.

It's not a big deal, but I think in the name of having a clean history, it makes sense not to bundle unrelated tests.

To avoid another round trip and blocking this PR, I'll go ahead and do that for you since I think it's ready to merge otherwise.

[alxhub]

If you wouldn't mind, I think we could also use a test here for a case like:

<div #foo></div>
{{ m({foo}) }}

just to verify that a shorthand reference to a property defined in the template itself still works.

[alxhub]

(and apologies for the last-minute change requests! this PR is 👍 otherwise, this is just a confusing part of the system and i want to make sure it's as clear as possible for people in the future)

[crisbeto]

I've addressed the latest set of feedback @alxhub.

[google-cla]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[alxhub]

(I've pushed an update that splits out the extra tests for keyed reads into a separate commit, to keep the history clean)

[dylhunn]

@AndrewKushnir suggests running a TGP, I will do so today.

[dylhunn]

This CL is on the 1pm TAP train, should have results this afternoon.

[dylhunn]

TGP is green.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}