Index constants #893

st0012 · 2023-08-16T12:18:45Z

Motivation

Having constants in the index would be helpful for both go-to-definition and hover features.

Closes #883

Implementation

Since the goal is to index constants' definitions, I introduced new methods to handle Constant(Path)WriteNode and Constant(Path)OperatorOrWriteNode. Those nodes will be added to the index as RubyIndexer::Index::Constant with fully qualified names.

Automated Tests

I added lib/ruby_indexer/test/constant_test.rb for it.

Manual Tests

github-actions · 2023-08-16T12:33:24Z

Benchmark results in seconds (slowest at top)

          textDocument/completion average: 0.32872 std_dev: 0.0131
          textDocument/diagnostic average: 0.061307 std_dev: 0.013851
          textDocument/definition average: 0.007225 std_dev: 0.003727
      textDocument/selectionRange average: 0.00588 std_dev: 0.001005
        textDocument/documentLink average: 0.003708 std_dev: 0.001001
      textDocument/documentSymbol average: 0.003671 std_dev: 0.000702
 textDocument/semanticTokens/full average: 0.003651 std_dev: 0.000778
            textDocument/codeLens average: 0.003638 std_dev: 0.000673
   textDocument/documentHighlight average: 0.003595 std_dev: 0.000777
        textDocument/foldingRange average: 0.003458 std_dev: 0.000665
textDocument/semanticTokens/range average: 0.002287 std_dev: 0.000451
           textDocument/inlayHint average: 0.00193 std_dev: 0.000492
               textDocument/hover average: 0.001852 std_dev: 0.000464
               codeAction/resolve average: 0.001848 std_dev: 0.000426
          textDocument/formatting average: 0.001205 std_dev: 0.000513
    textDocument/onTypeFormatting average: 0.00117 std_dev: 0.000339
          textDocument/codeAction average: 0.001169 std_dev: 0.000302


================================================================================
Comparison with main branch:

 textDocument/semanticTokens/full unchanged
textDocument/semanticTokens/range unchanged
      textDocument/documentSymbol unchanged
        textDocument/foldingRange unchanged
          textDocument/formatting unchanged
          textDocument/diagnostic unchanged
        textDocument/documentLink unchanged
           textDocument/inlayHint unchanged
      textDocument/selectionRange unchanged
   textDocument/documentHighlight unchanged
               textDocument/hover unchanged
          textDocument/codeAction unchanged
    textDocument/onTypeFormatting unchanged
               codeAction/resolve unchanged
          textDocument/completion unchanged
            textDocument/codeLens unchanged
          textDocument/definition unchanged


================================================================================
Missing benchmarks:

RubyLsp::Requests::ShowSyntaxTree

vinistock

Looking great!!

vinistock · 2023-08-16T13:49:18Z

lib/ruby_indexer/lib/ruby_indexer/visitor.rb

+    end
+    def add_constant(node)
+      comments = collect_comments(node)
+      @index << Index::Entry::Constant.new(fully_qualify_name(node.name), @file_path, node.location, comments)


Do we not need the same logic for the name here? And why not?

fully_qualified_name = name.start_with?("::") ? name.delete_prefix("::") : fully_qualify_name(name)

Because if there's ::, either in the middle of as a prefix, it'd be a ConstantPath* node instead.

vinistock · 2023-08-16T13:50:47Z

lib/ruby_indexer/lib/ruby_indexer/visitor.rb

+      ).void
+    end
+    def add_constant_with_path(node)
+      name = node.target.location.slice


This method is almost identical to the other one. Is there a way we can write this to share the same implementation between the two?

ConstantWriteNode and ConstantPathWriteNode needs to be indexed differently in 2 places:

How name is retrieved

Whether :: needs to be handled

Consider these, I prefer having them separated for now instead of checking node type inside the shared method.

We could move the :: handling inside fully_qualify_name, which probably fits the responsibility better.

And we can also get the name for the constant with node.location.slice. It's just a suggestion though, it might allow some cleanup.

I wonder if the :: prefix should always be deleted in Index#<<. It seems it's more a concern of the index itself rather than the visitor.

If I was to use an index manually, shouldn't I benefit from this behavior instead of having to reimplement it myself every time?

Yeah, that might be the case. Maybe we can move the handling of the prefix to the index.

Ok, I will open another PR for this as it affects class/module indexing too.

lib/ruby_indexer/test/constant_test.rb

vinistock · 2023-08-16T17:04:04Z

lib/ruby_indexer/lib/ruby_indexer/visitor.rb

+      ).void
+    end
+    def add_constant_with_path(node)
+      name = node.target.location.slice


We could move the :: handling inside fully_qualify_name, which probably fits the responsibility better.

And we can also get the name for the constant with node.location.slice. It's just a suggestion though, it might allow some cleanup.

lib/ruby_indexer/test/test_case.rb

…d-patch-b823885b92 Bump the minor-and-patch group with 4 updates

st0012 added the enhancement New feature or request label Aug 16, 2023

st0012 added this to the 2023-Q3 milestone Aug 16, 2023

st0012 self-assigned this Aug 16, 2023

st0012 requested a review from a team as a code owner August 16, 2023 12:18

st0012 requested review from andyw8 and Morriar August 16, 2023 12:18

st0012 force-pushed the index-constants branch from b8b9d11 to 69f0d22 Compare August 16, 2023 12:51

vinistock reviewed Aug 16, 2023

View reviewed changes

st0012 force-pushed the index-constants branch from 69f0d22 to 856a54d Compare August 16, 2023 14:43

st0012 requested a review from vinistock August 16, 2023 14:45

vinistock approved these changes Aug 16, 2023

View reviewed changes

st0012 added 2 commits August 16, 2023 21:52

Support indexing constants

56cef3c

Extract RubyIndexer::TestCase

646d2c6

st0012 force-pushed the index-constants branch from 856a54d to 646d2c6 Compare August 16, 2023 21:01

st0012 commented Aug 16, 2023

View reviewed changes

lib/ruby_indexer/test/test_case.rb Show resolved Hide resolved

st0012 merged commit 40fac28 into main Aug 17, 2023

st0012 deleted the index-constants branch August 17, 2023 13:53

st0012 mentioned this pull request Aug 17, 2023

Simplify constants' :: prefix handling #904

Merged

vinistock pushed a commit that referenced this pull request Feb 28, 2024

Merge pull request #893 from Shopify/dependabot/npm_and_yarn/minor-an…

f817417

…d-patch-b823885b92 Bump the minor-and-patch group with 4 updates

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Index constants #893

Index constants #893

st0012 commented Aug 16, 2023

github-actions bot commented Aug 16, 2023 •

edited

Loading

vinistock left a comment

vinistock Aug 16, 2023

st0012 Aug 16, 2023 •

edited

Loading

vinistock Aug 16, 2023

st0012 Aug 16, 2023

vinistock Aug 16, 2023

Morriar Aug 16, 2023

vinistock Aug 16, 2023

st0012 Aug 16, 2023

vinistock Aug 16, 2023

Index constants #893

Index constants #893

Conversation

st0012 commented Aug 16, 2023

Motivation

Implementation

Automated Tests

Manual Tests

github-actions bot commented Aug 16, 2023 • edited Loading

vinistock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

st0012 Aug 16, 2023 • edited Loading

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

github-actions bot commented Aug 16, 2023 •

edited

Loading

st0012 Aug 16, 2023 •

edited

Loading