Add "find references" capability to Ivy integrated Language Service #39768

atscott · 2020-11-19T21:54:48Z

Commit 1:
In order to map the pipe's transform method in the type check block
directly back to the pipe name in the template source, we need to
include the BindingPipe's nameSpan with the ts.methodAccess for
the pipe's transform method.

Note that this is specifically relevant to the Language Service's "find
references" feature. As an example, with something like -2.5 | number:'1.0-0',,
when calling "find references" on the 'number' pipe we want the text
span of the reference to just be number rather than the entire binding
pipe's source -2.5 | number:'1.0-0'.

Commit 2:
This commit adds "find references" functionality to the Ivy integrated
language service. The basic approach is as follows:

Generate shims for all files to ensure we find references in shims
throughout the entire program
Determine if the position for the reference request is within a
template.

Yes, it is in a template: Find which node in the template AST the
position refers to. Then find the position in the shim file for that
template node. Pass the shim file and position in the shim file along
to step 3.
No, the request for references was made outside a template: Forward
the file and position to step 3.

(getReferencesAtTypescriptPosition): Call the native TypeScript LS
getReferencesAtPosition. For each reference that is in a shim file, map those
back to a template location, otherwise return it as-is.

Close angular/vscode-ng-language-service#29

ayazhafiz

This is very nice!

packages/language-service/ivy/references.ts

ayazhafiz · 2020-11-20T00:12:15Z

packages/language-service/ivy/references.ts

+
+    const entries: ts.ReferenceEntry[] = [];
+    for (const ref of refs) {
+      if (ref.fileName.endsWith('ngtypecheck.ts')) {


maybe we should expose a isShimFilePath on the type check strategy? Can be a todo.

👍 Added TODO

We do have such a method, but you'll have to convert the filename to a source file first:

angular/packages/language-service/ivy/adapters.ts

Lines 33 to 35 in 23c36a2

isShim(sf: ts.SourceFile): boolean {

return isShim(sf);

}

@kyliau - Alex, pointed me to the isShim helper as well, but it doesn't work because we store the data on a specific instance of the SourceFile object. We can't do isShim(program.getSourceFile(fileName)) because that returns a different object, without the shim marker.

One potential solution would be to expose an isShim method on the TemplateTypeChecker that makes use of the getFileAndShimRecordsForPath method and returns true if it's able to find the file in its records.

// cc @alxhub

isShim won't work because the Language Service generates shims in a completely different way than the shim generator that isShim is designed for.

ayazhafiz · 2020-11-20T00:14:08Z

packages/language-service/ivy/references.ts

+        // If the request for a reference is from a template, we want to include regular TS file
+        // results because the TS LS will not find those from the template string/external template.
+        // Otherwise, we skip them because the TS LS *will* include those in the results.
+        if (requestMadeFromTemplate) {
+          entries.push(ref);
+        }


I think the editor should be doing this, not us. In my mind TS references are still in the scope of an Angular app, and relying on TS to give them back as supplemental assumes a TS LS is enabled simultaneously as ours, which is reasonable but not something we should rely on IMO. What do you think?

Yea, I wasn't really sure how to handle this. I did double check and confirmed that VSCode will not deduplicate results. If we want to change this, we'll need to address it in some way now rather than knowingly produce results that will appear as duplicates.

How about we de-duplicate on the language server (extension) side? This way we are more "near the editor", which ideally should be responsible for this, the language service itself does not have to be concerned with this. And we can upstream a bug report to vscode folks as well to see what they think. Curious if @kyliau @alxhub have thoughts too.

Update: I looked into this more because I remembered I had some vague recollection of how this worked, kind of like some itch bugging me. Eventually I found microsoft/vscode#39890 which I saw long ago, which suggests that vscode should deduplicate "identical" results. I then looked at VSCode's reference provider code, found here, and it looks like it considers equal references those in the same file and the same span. So it seems that if VSCode is not deduplicating results this should be a bug downstream, as it seems it is their intention to do so.

_{Btw, for a while we (when I was working with TS folks in g3) had a similar kind of problem hacking around codesearch's UI with Kythe. Eventually we "learned our lesson" and I think Mikita is doing some work to land a better fix on the codesearch side, but anyway, now similar kinds of problems have become a small hobby horse.}

Wonderful, thanks for the research! I like not having to include any logic for that on our side.

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

atscott

Can you also add a spec of usage of the input in a TS script, aside from its declaration? (If that works)

I think I understood what you meant and added a test, but I'm not entirely sure. See should work for inputs referenced from some other place

atscott · 2020-11-20T16:59:12Z

packages/language-service/ivy/references.ts

+        // If the request for a reference is from a template, we want to include regular TS file
+        // results because the TS LS will not find those from the template string/external template.
+        // Otherwise, we skip them because the TS LS *will* include those in the results.
+        if (requestMadeFromTemplate) {
+          entries.push(ref);
+        }


Wonderful, thanks for the research! I like not having to include any logic for that on our side.

packages/language-service/ivy/references.ts

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

packages/compiler-cli/src/ngtsc/testing/fake_common/index.ts

ayazhafiz · 2020-11-20T22:19:56Z

packages/language-service/ivy/language_service.ts

+    const compiler = this.compilerFactory.getOrCreateWithChangedFile(fileName);
+    const results = new ReferenceBuilder(this.strategy, this.tsLS, compiler)
+                        .get(absoluteFrom(fileName), position);
+    this.compilerFactory.registerLastKnownProgram();
+    return results;


aside, no action required: we should create a withCompiler or similar wrapper in this class, having to make sure we register the last known program is making me shiver a bit

ayazhafiz · 2020-11-20T22:24:20Z

packages/language-service/ivy/references.ts

+    shimReferenceEntry: ts.ReferenceEntry,
+    templateTypeChecker: TemplateTypeChecker): ts.ReferenceEntry|null {
+  const mapping = templateTypeChecker.getTemplateMappingAtShimLocation({
+    shimPath: absoluteFrom(shimReferenceEntry.fileName),


Here and on line 130 and 132: consider using the resolve method on the project serverHost rather than the compiler filesystem. LSParseConfigHost in the adapters file provides this functionality as well. As discussed before this generally should not be a problem, but I think it would be good to be consistent in what abstraction we use to resolve paths.

@alxhub - Can you provide your input here. I only added the absoluteFrom after a nudge from you to do it. Otherwise, these would just pass around the string fileNames.

Added TODO after discussion with Alex. I think absoluteFrom is fine for now since we haven't really considered or tested remote files at all. We should be resolving to some sort of branded path before passing a path along to the compiler and I'm not really sure what that would look like at the moment.

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

packages/compiler-cli/src/ngtsc/testing/fake_common/index.ts

packages/language-service/ivy/references.ts

kyliau · 2020-11-25T17:07:48Z

packages/language-service/ivy/references.ts

+
+    const entries: ts.ReferenceEntry[] = [];
+    for (const ref of refs) {
+      if (ref.fileName.endsWith('ngtypecheck.ts')) {


We do have such a method, but you'll have to convert the filename to a source file first:

angular/packages/language-service/ivy/adapters.ts

Lines 33 to 35 in 23c36a2

isShim(sf: ts.SourceFile): boolean {

return isShim(sf);

}

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

kyliau · 2020-11-25T17:25:23Z

packages/language-service/ivy/references.ts

+          entries.push(entry);
+        }
+      } else {
+        entries.push(ref);


Should we handle the case where get references is initiated from TS code (not Angular template), so TS references are already handled by the native TypeScript extension?
I saw we already had a discussion on this, but if we could reduce the result set sent from the server without too much complexity, then we should do that.

Yea...so we should just decide what we want to do here. I had the logic to exclude the result but removed it after this discussion: #39768 (comment). It's not difficult to manage either way, but we do need to make a call.

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

In order to map the pipe's `transform` method in the type check block directly back to the pipe name in the template source, we need to include the `BindingPipe`'s `nameSpan` with the `ts.methodAccess` for the pipe's transform method. Note that this is specifically relevant to the Language Service's "find references" feature. As an example, with something like `-2.5 | number:'1.0-0'`,, when calling "find references" on the 'number' pipe we want the text span of the reference to just be `number` rather than the entire binding pipe's source `-2.5 | number:'1.0-0'`.

In order to map the a safe property read's method access in the type check block directly back to the property in the template source, we need to include the `SafePropertyRead`'s `nameSpan` with the `ts.propertyAccess` for the pipe's transform method. Note that this is specifically relevant to the Language Service's "find references" feature. As an example, with something like `{{a?.value}}`, when calling "find references" on the 'value' we want the text span of the reference to just be `value` rather than the entire source `a?.value`.

packages/language-service/ivy/references.ts

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

alxhub · 2020-11-30T22:41:15Z

packages/language-service/ivy/references.ts

+      }
+      case SymbolKind.Expression: {
+        const {shimPath, positionInShimFile} = symbol.shimLocation;
+        return this.getReferencesAtTypescriptPosition(shimPath, positionInShimFile);


It might be valuable to validate that the expression here is actually of a kind that makes sense to search for references on - a PropertyRead or MethodCall?

Hmm, I think it might be better to just make the request and allow the TSLS to just return no results. In addition to PropertyRead and MethodCall, there is also BindingPipe (though this would change to its own symbol type after #39555) and LiteralPrimitive for keyed reads (something['a']). I could go both ways, but I'm leaning towards keeping this as-is to avoid the extra logic where it will already return no results (as far as I know).

packages/language-service/ivy/references.ts

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

…rated LS This commit adds "find references" functionality to the Ivy integrated language service. The basic approach is as follows: 1. Generate shims for all files to ensure we find references in shims throughout the entire program 2. Determine if the position for the reference request is within a template. * Yes, it is in a template: Find which node in the template AST the position refers to. Then find the position in the shim file for that template node. Pass the shim file and position in the shim file along to step 3. * No, the request for references was made outside a template: Forward the file and position to step 3. 3. (`getReferencesAtTypescriptPosition`): Call the native TypeScript LS `getReferencesAtPosition`. For each reference that is in a shim file, map those back to a template location, otherwise return it as-is.

…cked shim The Language Service "find references" currently uses the `ngtypecheck.ts` suffix to determine if a file is a shim file. Instead, a better API would be to expose a method in the template type checker that does this verification so that the LS does not have to "know" about the typecheck suffix. This also fixes an issue (albeit unlikely) whereby a file in the user's program that _actually_ is named with the `ngtypecheck.ts` suffix would have been interpreted as a shim file.

jessicajaniuk · 2020-12-01T23:03:13Z

Presubmit

In order to map the a safe property read's method access in the type check block directly back to the property in the template source, we need to include the `SafePropertyRead`'s `nameSpan` with the `ts.propertyAccess` for the pipe's transform method. Note that this is specifically relevant to the Language Service's "find references" feature. As an example, with something like `{{a?.value}}`, when calling "find references" on the 'value' we want the text span of the reference to just be `value` rather than the entire source `a?.value`. PR Close #39768

There were two issues with the current TCB: 1. The logic for only wrapping the right hand side of the property write if it was not already a parenthesized expression was incorrect. A parenthesized expression could still have a trailing comment, and if that were the case, that span comment would still be ambiguous, as explained by the comment in the code before `wrapForTypeChecker`. 2. The right hand side of keyed writes was not wrapped in parens at all PR Close #39768

…rated LS (#39768) This commit adds "find references" functionality to the Ivy integrated language service. The basic approach is as follows: 1. Generate shims for all files to ensure we find references in shims throughout the entire program 2. Determine if the position for the reference request is within a template. * Yes, it is in a template: Find which node in the template AST the position refers to. Then find the position in the shim file for that template node. Pass the shim file and position in the shim file along to step 3. * No, the request for references was made outside a template: Forward the file and position to step 3. 3. (`getReferencesAtTypescriptPosition`): Call the native TypeScript LS `getReferencesAtPosition`. For each reference that is in a shim file, map those back to a template location, otherwise return it as-is. PR Close #39768

…cked shim (#39768) The Language Service "find references" currently uses the `ngtypecheck.ts` suffix to determine if a file is a shim file. Instead, a better API would be to expose a method in the template type checker that does this verification so that the LS does not have to "know" about the typecheck suffix. This also fixes an issue (albeit unlikely) whereby a file in the user's program that _actually_ is named with the `ngtypecheck.ts` suffix would have been interpreted as a shim file. PR Close #39768

angular-automatic-lock-bot · 2021-01-02T16:47:44Z

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 Nov 19, 2020

pullapprove bot requested review from AndrewKushnir and ayazhafiz November 19, 2020 21:54

atscott added action: review The PR is still awaiting reviews from at least one requested reviewer target: minor This PR is targeted for the next minor release and removed cla: yes labels Nov 19, 2020

google-cla bot added the cla: yes label Nov 19, 2020

atscott requested review from kyliau and alxhub and removed request for AndrewKushnir November 19, 2020 21:55

atscott added this to PRs In Review in Ivy Language Service Nov 19, 2020

atscott force-pushed the referencesandrename branch from 7ad04ef to f61f30c Compare November 19, 2020 22:10

ayazhafiz reviewed Nov 20, 2020

View reviewed changes

atscott force-pushed the referencesandrename branch 4 times, most recently from ebc979f to 935ccff Compare November 20, 2020 20:27

atscott commented Nov 20, 2020

View reviewed changes

atscott force-pushed the referencesandrename branch from 935ccff to 1ba2df8 Compare November 20, 2020 22:03

atscott requested a review from ayazhafiz November 20, 2020 22:09

atscott force-pushed the referencesandrename branch from 1ba2df8 to 5f06438 Compare November 20, 2020 22:10

ayazhafiz reviewed Nov 20, 2020

View reviewed changes

kyliau reviewed Nov 25, 2020

View reviewed changes

atscott force-pushed the referencesandrename branch 4 times, most recently from 31127cb to 7772bca Compare November 25, 2020 23:24

atscott added 2 commits November 25, 2020 15:24

atscott force-pushed the referencesandrename branch from d3fb395 to 8817ad6 Compare November 26, 2020 01:32

kyliau approved these changes Nov 30, 2020

View reviewed changes

packages/language-service/ivy/references.ts Show resolved Hide resolved

packages/language-service/ivy/test/references_spec.ts Show resolved Hide resolved

atscott force-pushed the referencesandrename branch from 8817ad6 to 7bd5119 Compare November 30, 2020 20:05

alxhub reviewed Nov 30, 2020

View reviewed changes

packages/language-service/ivy/references.ts Outdated Show resolved Hide resolved

alxhub reviewed Nov 30, 2020

View reviewed changes

packages/language-service/ivy/test/references_spec.ts Outdated Show resolved Hide resolved

alxhub reviewed Nov 30, 2020

View reviewed changes

packages/language-service/ivy/test/references_spec.ts Outdated Show resolved Hide resolved

alxhub approved these changes Nov 30, 2020

View reviewed changes

jessicajaniuk added the area: language-service Issues related to Angular's VS Code language service label Dec 1, 2020

ngbot bot added this to the needsTriage milestone Dec 1, 2020

atscott force-pushed the referencesandrename branch from 7bd5119 to a6ae381 Compare December 1, 2020 02:24

atscott force-pushed the referencesandrename branch from a6ae381 to 521931c Compare December 1, 2020 02:32

atscott added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 1, 2020

jessicajaniuk added action: merge The PR is ready for merge by the caretaker and removed action: merge The PR is ready for merge by the caretaker labels Dec 2, 2020

mhevery closed this in 1a5e5f8 Dec 2, 2020

alxhub moved this from PRs In Review to Done in Ivy Language Service Dec 8, 2020

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

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "find references" capability to Ivy integrated Language Service #39768

Add "find references" capability to Ivy integrated Language Service #39768

atscott commented Nov 19, 2020 •

edited by kyliau

ayazhafiz left a comment

ayazhafiz Nov 20, 2020

atscott Nov 20, 2020

kyliau Nov 25, 2020

atscott Nov 25, 2020

alxhub Nov 30, 2020

ayazhafiz Nov 20, 2020

atscott Nov 20, 2020

ayazhafiz Nov 20, 2020

ayazhafiz Nov 20, 2020 •

edited

atscott Nov 20, 2020

atscott left a comment

atscott Nov 20, 2020

ayazhafiz Nov 20, 2020

ayazhafiz Nov 20, 2020

atscott Nov 25, 2020

atscott Dec 1, 2020

kyliau Nov 25, 2020

kyliau Nov 25, 2020

atscott Nov 25, 2020

alxhub Nov 30, 2020

atscott Dec 1, 2020

jessicajaniuk commented Dec 1, 2020

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

Add "find references" capability to Ivy integrated Language Service #39768

Add "find references" capability to Ivy integrated Language Service #39768

Conversation

atscott commented Nov 19, 2020 • edited by kyliau

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

ayazhafiz Nov 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atscott 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

jessicajaniuk commented Dec 1, 2020

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

atscott commented Nov 19, 2020 •

edited by kyliau

ayazhafiz Nov 20, 2020 •

edited