Global context autocompletion in the Ivy Language Service #39250

alxhub · 2020-10-13T19:37:14Z

No description provided.

alxhub · 2020-11-12T20:12:32Z

Reviewers: only the last commit is relevant, the others are part of #39594 and will go away after rebase.

packages/language-service/ivy/completions.ts

packages/language-service/ivy/test/completions_spec.ts

ayazhafiz · 2020-11-20T01:46:52Z

packages/language-service/ivy/test/completions_spec.ts

+    it('should be able to complete a property binding', () => {
+      const {ngLS, fileName, cursor} =
+          setup('<h1 [model]="ti¦"></h1>', `title!: string; hero!: number;`);
+      const completions = ngLS.getCompletionsAtPosition(fileName, cursor, /* options */ undefined);
+      expectContain(completions, ts.ScriptElementKind.memberVariableElement, ['title', 'hero']);
+    });


Does an empty property binding work? [model]=""

Added a test

packages/language-service/ivy/test/completions_spec.ts

ayazhafiz · 2020-11-20T01:49:13Z

packages/language-service/ivy/test/completions_spec.ts

+    });
+
+    it('should return completions inside an event binding', () => {
+      const {ngLS, fileName, cursor} = setup(` <button (click)='t¦'></button>`, `title!: string;`);


Similarly, does an empty event binding work?

It doesn't, currently, due to the way the template parser produces a weird, invalid token. I added a excluded test, which will be enabled after my refactoring of template_target.

packages/language-service/ivy/test/completions_spec.ts

ayazhafiz · 2020-11-20T01:49:39Z

packages/language-service/ivy/test/completions_spec.ts

+    });
+
+    it('should return completions inside the RHS of a two-way binding', () => {
+      const {ngLS, fileName, cursor} = setup(`<h1 [(model)]="t¦"></h1>`, `title!: string;`);


Similarly, does an empty two-way binding work?

It took a bit of fixing, but I got this working, due to the broken way that such bindings are parsed.

ayazhafiz

Other than my outstanding comments LGTM, this is awesome! adding Keen and Andrew as reviewers as well as they are probably interested too.

packages/language-service/ivy/completions.ts

atscott · 2020-11-25T21:32:32Z

packages/language-service/ivy/completions.ts

+      // Although this completion is "global" in the sense of an Angular expression (there is no
+      // explicit receiver), it is not "global" in a TypeScript sense since Angular expressions have
+      // the component as an implicit receiver.
+      isGlobalCompletion: false,


optional bikeshed (though at this point, I'm leaning towards keeping this how it is because I can't think of a better name and it'd be a pain to rename):

This made me think about how this isn't really a "global" completion, but more like a "component scope" completion.

This is a TypeScript-defined field in ts.CompletionInfo, we have no control over the name.

Right, what I meant was to possibly rename the entire implementation here to not say "global" because a "global" completion, as defined by this typescript field, is something else. But I'm also fine leaving this as-is.

Ahhh, yeah, I see what you mean.

atscott · 2020-11-25T21:34:24Z

packages/language-service/ivy/completions.ts

+      // Entries that reference a symbol in the template context refer either to local references or
+      // variables.
+      const symbol = templateTypeChecker.getSymbolOfNode(entry.node, this.component) as
+              TemplateDeclarationSymbol |


Suggestion: maybe we should update the overloads of getSymbolOfNode to return the more narrow types for TmplAstVariable and TmplAstReference inputs?

I don't think that would actually work in this circumstance, as TypeScript will match a signature based on the union input type (TmplAstVariable|TmplAstReference), and not "split" the union to match each branch individually and then create a union of the return types. So we'll need the cast either way.

packages/language-service/ivy/completions.ts

atscott · 2020-11-25T21:40:26Z

packages/language-service/ivy/language_service.ts

+    const program = this.strategy.getProgram();
+    const compiler = this.compilerFactory.getOrCreateWithChangedFile(fileName);
+    const templateInfo = getTemplateInfoAtPosition(fileName, position, compiler);
+    if (templateInfo === undefined) {
+      return undefined;
+    }
+    const positionDetails = getTargetAtPosition(templateInfo.template, position);
+    if (positionDetails === null) {
+      return undefined;
+    }


Suggestion: these ~10 lines appear to be exact duplicates of each other in each of these completion functions. Is it possible to refactor to a helper method? (I think this is also pretty much the same work as all of the other "getXAtPosition" with fileName and position)

Done - getCompletionBuilder is now a thing.

packages/language-service/ivy/test/completions_spec.ts

atscott · 2020-11-25T21:45:36Z

packages/language-service/ivy/test/completions_spec.ts

+      contents: 'Placeholder template',
+    }
+  ]);
+  const {nodes, cursor} = env.overrideTemplateWithCursor(codePath, 'AppCmp', templateWithCursor);


suggestion: You actually kind of don't need overrideTemplateWithCursor if you expose extractCursorInfo.
You can instead do

const {text: templateText, cursor} = extractCursortInfo(templateWithCursor); ... { name: templatePath, contents: templateText }

Especially given that you don't need the overridden template nodes.

I kind of prefer this because it eliminates some of the possible string errors (i.e. mistyping the class name) and doesn't create the file initially with placeholder text.

👍 but will leave this as a future refactoring. I'd prefer to find a way not to "decouple" the extractCursorInfo operation from the path/content it's representing.

kyliau · 2020-11-30T19:20:05Z

packages/language-service/ivy/completions.ts

+      const symbol =
+          templateTypeChecker.getSymbolOfNode(node, this.component) as TemplateDeclarationSymbol |
+          null;
+      if (symbol === null || symbol.tsSymbol === null) {


nit: consider replacing these 4 lines with return symbol?.tsSymbol. I think it is in line with the Angular "style guide" since this function could return undefined.

I don't think that would work, since symbol.tsSymbol would be null and not undefined. You'd need symbol?.tsSymbol ?? undefined. I prefer the explicitness of the if branch, which makes it very clear that this is exactly how we want to handle both cases.

kyliau · 2020-11-30T19:22:14Z

packages/language-service/ivy/language_service.ts

+    return result;
+  }
+
+  getCompletionEntrySymbol(fileName: string, position: number, name: string): ts.Symbol|undefined {


Could you please add a comment to describe the difference between getCompletionEntryDetails and getCompletionEntrySymbol? When is one used over the other?

Oh I have no idea - these are both defined in the TypeScript API so we implement them both.

Oh, okay. For our purpose I don't think this matters much since we pick the API to call in the language server, but I'll add a card for myself to investigate how they're used in TypeScript language service.

getCompletionEntrySymbol was added because of microsoft/TypeScript#10812, but the issue doesn't give an example motivation.

…s (Ivy) This commit adds support in the Ivy Language Service for autocompletion in a global context - e.g. a {{foo|}} completion. Support is added both for the primary function `getCompletionsAtPosition` as well as the detail functions `getCompletionEntryDetails` and `getCompletionEntrySymbol`. These latter operations are not used yet as an upstream change to the extension is required to advertise and support this capability.

angular-automatic-lock-bot · 2021-01-04T16:50:55Z

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.}

google-cla bot added the cla: yes label Oct 13, 2020

atscott added the area: language-service Issues related to Angular's VS Code language service label Oct 13, 2020

ngbot bot added this to the needsTriage milestone Oct 13, 2020

alxhub force-pushed the ngtsc/ls/autocomplete-3 branch 4 times, most recently from 579c3bb to f0f92b1 Compare November 12, 2020 19:47

alxhub force-pushed the ngtsc/ls/autocomplete-3 branch 3 times, most recently from cb2f9f8 to 7406be9 Compare November 17, 2020 20:07

alxhub marked this pull request as ready for review November 17, 2020 20:08

alxhub added the target: minor This PR is targeted for the next minor release label Nov 17, 2020

pullapprove bot requested a review from ayazhafiz November 17, 2020 20:08

ayazhafiz reviewed Nov 18, 2020

View reviewed changes

alxhub force-pushed the ngtsc/ls/autocomplete-3 branch 4 times, most recently from 2c052d5 to 11b4f94 Compare November 18, 2020 20:27

ayazhafiz reviewed Nov 20, 2020

View reviewed changes

ayazhafiz approved these changes Nov 25, 2020

View reviewed changes

ayazhafiz requested review from atscott and kyliau November 25, 2020 19:19

atscott approved these changes Nov 25, 2020

View reviewed changes

kyliau approved these changes Nov 30, 2020

View reviewed changes

alxhub force-pushed the ngtsc/ls/autocomplete-3 branch 2 times, most recently from 330528d to f435d37 Compare December 3, 2020 00:14

alxhub added the action: merge The PR is ready for merge by the caretaker label Dec 3, 2020

alxhub force-pushed the ngtsc/ls/autocomplete-3 branch from f435d37 to 5469b2b Compare December 3, 2020 21:51

mhevery closed this in 28a0bcb Dec 4, 2020

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 4, 2021

pullapprove bot added the area: compiler Issues related to `ngc`, Angular's template compiler label Jan 4, 2021

ngbot bot modified the milestones: needsTriage, Backlog Jan 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Global context autocompletion in the Ivy Language Service #39250

Global context autocompletion in the Ivy Language Service #39250

alxhub commented Oct 13, 2020

alxhub commented Nov 12, 2020

ayazhafiz Nov 20, 2020

alxhub Dec 2, 2020

ayazhafiz Nov 20, 2020

alxhub Dec 2, 2020

ayazhafiz Nov 20, 2020

alxhub Dec 2, 2020

ayazhafiz left a comment

atscott Nov 25, 2020

alxhub Dec 2, 2020

atscott Dec 2, 2020

alxhub Dec 2, 2020

atscott Nov 25, 2020

alxhub Dec 2, 2020

atscott Nov 25, 2020

alxhub Dec 3, 2020

atscott Nov 25, 2020

alxhub Dec 3, 2020

kyliau Nov 30, 2020

alxhub Dec 3, 2020

kyliau Nov 30, 2020

alxhub Dec 2, 2020

kyliau Dec 2, 2020 •

edited

ayazhafiz Dec 2, 2020

angular-automatic-lock-bot bot commented Jan 4, 2021

Global context autocompletion in the Ivy Language Service #39250

Global context autocompletion in the Ivy Language Service #39250

Conversation

alxhub commented Oct 13, 2020

alxhub commented Nov 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayazhafiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyliau Dec 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Jan 4, 2021

kyliau Dec 2, 2020 •

edited