GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Clean up FAR aggregation by amcasey · Pull Request #48619 · microsoft/TypeScript

Conversation

@amcasey

@amcasey Andrew Casey (amcasey) commented Apr 9, 2022

Copy link
Copy Markdown
Member

We have library functions for searching within a project for symbols, references, rename locations, etc. All of that code is exactly as before - this change is about the layer that sits on top of that and aggregates the results across multiple projects. In the course of some performance investigations, I discovered that user-FAR was invoking per-project-FAR more times than I expected and that the code wasn't readable enough for me to confidently determine why or how to prevent it. This change attempts to address both issues.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@amcasey
Andrew Casey (amcasey) requested review from Andrew Branch (andrewbranch), Nathan Shively-Sanders (sandersn) and Sheetal Nandi (sheetalkamat) April 9, 2022 00:15
@typescript-bot TypeScript Bot (typescript-bot) assigned Andrew Casey (amcasey) Apr 9, 2022
@typescript-bot TypeScript Bot (typescript-bot) added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Apr 9, 2022
@amcasey Andrew Casey (amcasey) mentioned this pull request Apr 9, 2022
Clean up FAR and NavTo aggregation #47938
Closed
@amcasey Andrew Casey (amcasey) changed the title Far cleanup redux Clean up FAR aggregation Apr 9, 2022
@amcasey

Andrew Casey (amcasey) commented Apr 9, 2022

Copy link
Copy Markdown
Member Author

From #48211

Benefits:

  1. Uninverted callbacks
  2. Intelligible meaning for isDefinition
  3. Does fewer redundant searches when projects are already loaded

Costs:

  1. Does more tsconfig searches (probably negligible cost)
  2. isDefinition is computed multiple times for projects other than the initial project

Rough explanation of the new isDefinition:

  1. Start at the place the user invoked FAR (this is surprisingly hard to find)
  2. Record that location in a set of spans known to refer to the "real" symbol
  3. Find a project with a result in the known set
  4. Find the symbol at that position in that project
  5. Use that symbol to compute isDefinition for each declaration in the results for that project and add those that refer to the symbol to the set
  6. Goto (3) until no such project is found
  7. Clear isDefinition in all remaining projects
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@amcasey

Andrew Casey (amcasey) commented Apr 9, 2022

Copy link
Copy Markdown
Member Author

Sheetal Nandi (@sheetalkamat) I know Andrew Branch (@andrewbranch) prefers the reduced complexity of this simplification, but I find its correctness less intuitively obvious. What are your thoughts on reverting that simplification commit?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@amcasey Andrew Casey (amcasey) mentioned this pull request Apr 20, 2022
Don't go past import in cross-project renaming #48758
Merged
Andrew Casey (amcasey) added a commit to amcasey/TypeScript that referenced this pull request May 5, 2022
@amcasey
Harden combineProjectOutputForReferences against empty results
6d83df4 
Getting an empty result doesn't seem expected, but a deeper fix doesn't make sense until microsoft#48619 is merged.

Fixes microsoft#48963
Andrew Casey (amcasey) added a commit to amcasey/TypeScript that referenced this pull request May 5, 2022
@amcasey
Harden combineProjectOutputForReferences against empty results
dd28f12 
Getting an empty result doesn't seem expected, but a deeper fix doesn't make sense until microsoft#48619 is merged.

Fixes microsoft#48963
@amcasey Andrew Casey (amcasey) mentioned this pull request May 5, 2022
Harden combineProjectOutputForReferences against empty results #48978
Merged
Daniel Rosenwasser (DanielRosenwasser) pushed a commit that referenced this pull request May 5, 2022
@amcasey
Harden combineProjectOutputForReferences against empty results (#48978)
58114cf 
Getting an empty result doesn't seem expected, but a deeper fix doesn't make sense until #48619 is merged.

Fixes #48963
Andrew Casey (amcasey) added 11 commits May 17, 2022 09:36
@amcasey
Clean up FAR and RenameLocations
99fa17e 
This change had two goals:

1. Make the code easier to understand, primarily by simplifying the callback structure and minimizing side-effects
2. Improve performance by reducing repeated work, both FAR searches of individual projects and default tsconfig searches

This implementation attempts to preserve the merging order found in the original code (someone less relevant in the present state of using syntactic isDefinition).
@amcasey
Stop enforcing search and aggregation order
205a6e1 
...in preparation for implementing isDefinition explicitly.

Also restore convention of referring to `DocumentPosition`s as "locations".
@amcasey
Introduce LanguageService.updateIsDefinitionOfReferencedSymbols
fe0713f 
...to allow use of the checker when computing isDefinition across projects.
@amcasey
Update baselines
12a78b0
@amcasey
Tidy diff
b5fab0d
@amcasey
De-dup simplified results
5f088f6
@amcasey
Baseline cross-project isDefinition results
d9ff2cc
@amcasey
Move de-duping upstream to fix Full output
7003203
@amcasey
Add server baseline test to confirm searches are not repeated
780a84e
@amcasey
Manually merge microsoft#48758
d47ae8a
@amcasey
Update baseline for newer fix to microsoft#48963
827f5f3
@amcasey
Andrew Casey (amcasey) force-pushed the FarCleanupRedux branch from 284b0ac to 827f5f3 Compare May 17, 2022 19:41
@amcasey

Andrew Casey (amcasey) commented May 17, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

cc Gabriela Araujo Britto (@gabritto) for the merge of #48758 (the non-trivial parts of which are in d47ae8a).
gabritto reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@amcasey

Andrew Casey (amcasey) commented May 17, 2022

Copy link
Copy Markdown
Member Author

Ping Sheetal Nandi (@sheetalkamat) Andrew Branch (@andrewbranch) now that we're in 4.8, this is ready for review.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
andrewbranch
Andrew Branch (andrewbranch) approved these changes May 18, 2022
View reviewed changes
@amcasey
Andrew Casey (amcasey) merged commit 12ed012 into microsoft:main May 19, 2022
@amcasey
Andrew Casey (amcasey) deleted the FarCleanupRedux branch May 19, 2022 00:26
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request May 23, 2022
Crash when find-all-references/rename finds no references #49217
Closed
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@andrewbranch Andrew Branch (andrewbranch) andrewbranch approved these changes
@sandersn Nathan Shively-Sanders (sandersn) Awaiting requested review from sandersn
@sheetalkamat Sheetal Nandi (sheetalkamat) Awaiting requested review from sheetalkamat

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@amcasey Andrew Casey (amcasey)

Labels

Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

PR Backlog
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@amcasey @andrewbranch @typescript-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.