Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make completions available for newly-added symbols in unsaved files #1908

Closed
andyw8 opened this issue Apr 10, 2024 · 2 comments · Fixed by #2941
Closed

Make completions available for newly-added symbols in unsaved files #1908

andyw8 opened this issue Apr 10, 2024 · 2 comments · Fixed by #2941
Assignees
@vinistock
Labels
bug Something isn't working pinned This issue or pull request is pinned and won't be marked as stale server This pull request should be included in the server gem's release notes

Comments

@andyw8
Copy link
Contributor

andyw8 commented Apr 10, 2024 •
edited
Loading

Currently we only index when a file is saved, meaning that completions will be unavailable for newly-added symbols within the same file.

For example if you write:

class Widget
end

then type Wid, autocomplete will not suggest Widget as it's not yet indexed.

This could be unexpected, especially if people are coming from other editor setups which behave differently.

A couple of options that come to mind:

  • Index the current file on changes
  • Index the current file on-the-fly when the completion is triggered

For options we need to be a wary of the impact on performance while editing.

We should also consider the case of multiple unsaved files, with one referencing the other.
@andyw8 andyw8 added bug Something isn't working server This pull request should be included in the server gem's release notes labels Apr 10, 2024
@vinistock vinistock added the pinned This issue or pull request is pinned and won't be marked as stale label Apr 15, 2024
@vinistock vinistock mentioned this issue Apr 22, 2024
Open
@vinistock vinistock self-assigned this Jul 19, 2024
This was referenced Jul 22, 2024
Closed
Closed
@andyw8 andyw8 changed the title Completions are unavailable until file is saved Make completions available for newly-added symbols in unsaved files Oct 24, 2024
@andyw8
Copy link
Contributor Author

andyw8 commented Nov 21, 2024

(this applies to more Ruby LSP features than just completion)

This was referenced Nov 26, 2024
Merged
Merged
vinistock added a commit that referenced this issue Nov 27, 2024
@vinistock
Move URI extension to indexer (#2915)
a6842f1 
### Motivation

Move our URI extension to the indexer, so that we can get rid of `IndexablePath` and use URIs only (which is the first step to address #1908).

### Implementation

This PR just moves the extension to the indexer.
@andyw8
Copy link
Contributor Author

andyw8 commented Nov 27, 2024

Note: When this is solved, remove the note that was added in #2923

vinistock added a commit that referenced this issue Nov 27, 2024
@vinistock
Replace IndexablePath with URI (#2916)
f386d41 
### Motivation

Second step for #1908

This PR gets rid of `IndexablePath` and standardizes on using URI. This is necessary because `IndexablePath` is coupled with file paths on disk, which is incompatible with the URI scheme for unsaved files (`untitled:Untitled-1`) - a scenario where the files don't exist on disk.

### Implementation

The important changes are just in the URI extension file. Everything else is essentially just renaming things from `IndexablePath` to URI.

**Note**: this PR takes a slightly different approach than my previous attempt. I previously tried to create a separate URI class for the indexer, fully disconnected from the original `URI::Generic` class. That turned out to be a terrible idea that lead to an enormous amount of duplication, so the approach here is just to extend `URI::Generic` a bit more to handle require paths.

### Automated Tests

Added a couple of new tests for the URI extension.
This was referenced Nov 27, 2024
Merged
Merged
vinistock added a commit that referenced this issue Nov 28, 2024
@vinistock
Store URIs in entries instead of file paths (#2926)
6172ba4 
### Motivation

Another step towards #1908

Entries need to store URIs instead of file paths. Otherwise, we cannot represent an unsaved file (since there's no file path associated to it, only a URI).

### Implementation

This PR changes entries to store URIs, but still maintains the assumption that every entry is backed by a file path on disk. This allows us to minimize the number of changes for this PR.

Once this goes in, then we can start looking into indexing unsaved files, which use the `untitled` scheme instead of `file` and then we will need to handle the possibility of the file path being `nil`.
vinistock added a commit that referenced this issue Nov 28, 2024
@vinistock
Use Entry#uri where possible instead of file path (#2927)
ce50f49 
### Motivation

Another step towards #1908

This PR starts using the entry's URI directly instead of rebuilding them from the file paths. This allows us to do less work, but is also an important step so that features will actually work once we index unsaved files.

### Implementation

Started using the entry's URI for building responses, like definition locations.
@vinistock vinistock mentioned this issue Nov 28, 2024
Merged
vinistock added a commit that referenced this issue Nov 29, 2024
@vinistock
Start accepting any URI scheme in the index (#2934)
cb47bb7 
### Motivation

Another step towards #1908

After the recent changes, we can now make the index treat entries coming from any URIs appropriately.

### Implementation

The main aspect of change is changing the `@files_to_entries` variable into `@uris_to_entries`. Since entries can now be discovered in URIs not backed by a file on disk, we need to be able to add, remove and handle changes based on URIs.

Additionally, for synchronizing changes in the index, we need to accept the source now, which will be kept in memory for unsaved files.

**Note**: this is a breaking change because it means that the `entries_for` API now _needs_ to accept a URI instead of a file path.

### Automated Tests

Added a test that verifies all of the important functionality for entries discovered in unsaved URIs.
@vinistock vinistock mentioned this issue Nov 29, 2024
Merged
vinistock added a commit that referenced this issue Jan 7, 2025
@vinistock
Index documents on modification (#2941)
cf1235e 
### Motivation

Closes #1908

This PR starts indexing files upon modification in addition to on saves.

### Implementation

The idea is to use the declaration listener together with all of the other listeners, so that we collect declaration changes within the same AST traversal. This allows us to catch any declaration modifications made before saving a file.

The biggest concern with this change is performance, but I think the next PR in this stack is doing a decent job at minimizing reindexes.

### Automated Tests

Added tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vinistock vinistock

Labels
bug Something isn't working pinned This issue or pull request is pinned and won't be marked as stale server This pull request should be included in the server gem's release notes
Projects
None yet
Milestone
No milestone
2 participants
@andyw8 @vinistock