Index documents on modification

jekyll/index.markdown

@@ -158,10 +158,6 @@ Sorry, your browser doesn't support embedded videos. This video demonstrates the
 
 ### Completion
 
-{: .note }
-There is currently a technical limitation that [code is only indexed when saved](https://github.com/Shopify/ruby-lsp/issues/1908).
-This means if you add, for example, a new method to its called, its completion will not be available until you save that file.
-
 The completion feature provides users with completion candidates when the text they type matches certain indexed components. This helps speed up coding by reducing the need to type out full method names or constants.
 It also allows developers to discover constants or methods that are available to them.
 

lib/ruby_indexer/lib/ruby_indexer/index.rb

@@ -48,6 +48,8 @@ def initialize
       )
 
       @configuration = T.let(RubyIndexer::Configuration.new, Configuration)
+
+      @initial_indexing_completed = T.let(false, T::Boolean)
     end
 
     # Register an included `hook` that will be executed when `module_name` is included into any namespace
@@ -56,8 +58,8 @@ def register_included_hook(module_name, &hook)
       (@included_hooks[module_name] ||= []) << hook
     end
 
-    sig { params(uri: URI::Generic).void }
-    def delete(uri)
+    sig { params(uri: URI::Generic, skip_require_paths_tree: T::Boolean).void }
+    def delete(uri, skip_require_paths_tree: false)
       key = uri.to_s
       # For each constant discovered in `path`, delete the associated entry from the index. If there are no entries
       # left, delete the constant from the index.
@@ -80,6 +82,7 @@ def delete(uri)
       end
 
       @uris_to_entries.delete(key)
+      return if skip_require_paths_tree
 
       require_path = uri.require_path
       @require_paths_tree.delete(require_path) if require_path
@@ -357,12 +360,13 @@ def index_all(uris: @configuration.indexable_uris, &block)
       # When troubleshooting an indexing issue, e.g. through irb, it's not obvious that `index_all` will augment the
       # existing index values, meaning it may contain 'stale' entries. This check ensures that the user is aware of this
       # behavior and can take appropriate action.
-      # binding.break
-      if @entries.any?
+      if @initial_indexing_completed
         raise IndexNotEmptyError,
           "The index is not empty. To prevent invalid entries, `index_all` can only be called once."
       end
 
+      @initial_indexing_completed = true
+
       RBSIndexer.new(self).index_ruby_core
       # Calculate how many paths are worth 1% of progress
       progress_step = (uris.length / 100.0).ceil
@@ -618,17 +622,24 @@ def class_variable_completion_candidates(name, owner_name)
     end
 
     # Synchronizes a change made to the given URI. This method will ensure that new declarations are indexed, removed
-    # declarations removed and that the ancestor linearization cache is cleared if necessary
-    sig { params(uri: URI::Generic, source: String).void }
-    def handle_change(uri, source)
+    # declarations removed and that the ancestor linearization cache is cleared if necessary. If a block is passed, the
+    # consumer of this API has to handle deleting and inserting/updating entries in the index instead of passing the
+    # document's source (used to handle unsaved changes to files)
+    sig do
+      params(uri: URI::Generic, source: T.nilable(String), block: T.nilable(T.proc.params(index: Index).void)).void
+    end
+    def handle_change(uri, source = nil, &block)
       key = uri.to_s
       original_entries = @uris_to_entries[key]
 
-      delete(uri)
-      index_single(uri, source)
+      if block
+        block.call(self)
+      else
+        delete(uri)
+        index_single(uri, T.must(source))
+      end
 
       updated_entries = @uris_to_entries[key]
-
       return unless original_entries && updated_entries
 
       # A change in one ancestor may impact several different others, which could be including that ancestor through

lib/ruby_indexer/test/index_test.rb

@@ -2061,7 +2061,8 @@ def test_build_non_redundant_name
     end
 
     def test_prevents_multiple_calls_to_index_all
-      # For this test class, `index_all` is already called once in `setup`.
+      @index.index_all
+
       assert_raises(Index::IndexNotEmptyError) do
         @index.index_all
       end

lib/ruby_lsp/server.rb

@@ -463,7 +463,14 @@ def run_combined_requests(message)
       document_link = Requests::DocumentLink.new(uri, parse_result.comments, dispatcher)
       code_lens = Requests::CodeLens.new(@global_state, uri, dispatcher)
       inlay_hint = Requests::InlayHints.new(document, T.must(@store.features_configuration.dig(:inlayHint)), dispatcher)
-      dispatcher.dispatch(parse_result.value)
+
+      # Re-index the file as it is modified. This mode of indexing updates entries only. Require path trees are only
+      # updated on save
+      @global_state.index.handle_change(uri) do |index|
+        index.delete(uri, skip_require_paths_tree: true)
+        RubyIndexer::DeclarationListener.new(index, dispatcher, parse_result, uri, collect_comments: true)
+        dispatcher.dispatch(parse_result.value)
+      end
 
       # Store all responses retrieve in this round of visits in the cache and then return the response for the request
       # we actually received

test/server_test.rb

@@ -796,6 +796,136 @@ def test_cancelling_requests_returns_expected_error_code
     assert_equal("Request 1 was cancelled", error.message)
   end
 
+  def test_unsaved_changes_are_indexed_when_computing_automatic_features
+    uri = URI("file:///foo.rb")
+    index = @server.global_state.index
+
+    # Simulate opening a file. First, send the notification to open the file with a class inside
+    @server.process_message({
+      method: "textDocument/didOpen",
+      params: {
+        textDocument: {
+          uri: uri,
+          text: +"class Foo\nend",
+          version: 1,
+          languageId: "ruby",
+        },
+      },
+    })
+    # Fire the automatic features requests to trigger indexing
+    @server.process_message({
+      id: 1,
+      method: "textDocument/documentSymbol",
+      params: { textDocument: { uri: uri } },
+    })
+
+    entries = index["Foo"]
+    assert_equal(1, entries.length)
+
+    # Modify the file without saving
+    @server.process_message({
+      method: "textDocument/didChange",
+      params: {
+        textDocument: { uri: uri, version: 2 },
+        contentChanges: [
+          { text: "  def bar\n  end\n", range: { start: { line: 1, character: 0 }, end: { line: 1, character: 0 } } },
+        ],
+      },
+    })
+
+    # Parse the document after it was modified. This occurs automatically when we receive a text document request, to
+    # avoid parsing the document multiple times, but that depends on request coming in through the STDIN pipe, which
+    # isn't reproduced here. Parsing manually matches what happens normally
+    store = @server.instance_variable_get(:@store)
+    store.get(uri).parse!
+
+    # Trigger the automatic features again
+    @server.process_message({
+      id: 2,
+      method: "textDocument/documentSymbol",
+      params: { textDocument: { uri: uri } },
+    })
+
+    # There should still only be one entry for each declaration, but we should have picked up the new ones
+    entries = index["Foo"]
+    assert_equal(1, entries.length)
+
+    entries = index["bar"]
+    assert_equal(1, entries.length)
+  end
+
+  def test_ancestors_are_recomputed_even_on_unsaved_changes
+    uri = URI("file:///foo.rb")
+    index = @server.global_state.index
+    source = +<<~RUBY
+      module Bar; end
+
+      class Foo
+        extend Bar
+      end
+    RUBY
+
+    # Simulate opening a file. First, send the notification to open the file with a class inside
+    @server.process_message({
+      method: "textDocument/didOpen",
+      params: {
+        textDocument: {
+          uri: uri,
+          text: source,
+          version: 1,
+          languageId: "ruby",
+        },
+      },
+    })
+    # Fire the automatic features requests to trigger indexing
+    @server.process_message({
+      id: 1,
+      method: "textDocument/documentSymbol",
+      params: { textDocument: { uri: uri } },
+    })
+
+    assert_equal(["Foo::<Class:Foo>", "Bar"], index.linearized_ancestors_of("Foo::<Class:Foo>"))
+
+    # Delete the extend
+    @server.process_message({
+      method: "textDocument/didChange",
+      params: {
+        textDocument: { uri: uri, version: 2 },
+        contentChanges: [
+          { text: "", range: { start: { line: 3, character: 0 }, end: { line: 3, character: 12 } } },
+        ],
+      },
+    })
+
+    # Parse the document after it was modified. This occurs automatically when we receive a text document request, to
+    # avoid parsing the document multiple times, but that depends on request coming in through the STDIN pipe, which
+    # isn't reproduced here. Parsing manually matches what happens normally
+    store = @server.instance_variable_get(:@store)
+    document = store.get(uri)
+
+    assert_equal(<<~RUBY, document.source)
+      module Bar; end
+
+      class Foo
+
+      end
+    RUBY
+
+    document.parse!
+
+    # Trigger the automatic features again
+    @server.process_message({
+      id: 2,
+      method: "textDocument/documentSymbol",
+      params: { textDocument: { uri: uri } },
+    })
+
+    result = find_message(RubyLsp::Result, id: 2)
+    refute_nil(result)
+
+    assert_equal(["Foo::<Class:Foo>"], index.linearized_ancestors_of("Foo::<Class:Foo>"))
+  end
+
   private
 
   def with_uninstalled_rubocop(&block)