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.

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

Track method's visibility during indexing #2093

Merged
merged 3 commits into from
May 30, 2024
Merged

Track method's visibility during indexing #2093

merged 3 commits into from
May 30, 2024

Conversation

st0012
Copy link
Member

@st0012 st0012 commented May 28, 2024

Motivation

Closes #1941

Implementation

  • Adding a visibility field to the Member entry, which is a superclass of Accessor, Method, and Alias.
  • In the DeclarationListener, create a nested stack structure to track the visibility of the current scope.

Nested stack

Every namespace will have its own visibility stack, for example:

class Foo
  class Bar
  end
end

# [[<top-level visibility stack>], [<Foo visibility stack>], [<Foo::Bar visibility stack>]]

And at each level (scope), we simply push new visibility to its stack whenever we see a modifier.

 # [[:public]]

def foo; end # :public

private # [[:public, :private]]

def bar; end # :private

protected # [[:public, :private, :protected]]

def baz; end :protected

The only exception is when we face private def foo; end, which will pop the stack when we leave the declaration.

I chose a different data structure than test CodeLens's because it assumes methods are always inside a class, which would create a problem here. And I also feel the way it swaps visibilities is harder to understand than the nested stack I use here.

One drawback of this design is that if one scope level has many visibility modifiers, it'd create a big visibility stack at that level, like:

private
private
private
# repeat 997 more times, then we'd have [[:private * 1000]]

However, I think it's an edge case that we can ignore in order to get a simpler implementation.

Automated Tests

I added 2 new test cases for this change.

Manual Tests

@st0012 st0012 added the enhancement New feature or request label May 28, 2024
@st0012 st0012 self-assigned this May 28, 2024
@st0012 st0012 force-pushed the index-visibility branch 2 times, most recently from b5df983 to 563477c Compare May 28, 2024 19:36
@st0012 st0012 added the server This pull request should be included in the server gem's release notes label May 28, 2024
@st0012 st0012 marked this pull request as ready for review May 28, 2024 20:01
@st0012 st0012 requested a review from a team as a code owner May 28, 2024 20:01
@st0012 st0012 requested review from andyw8 and vinistock May 28, 2024 20:01
@st0012 st0012 changed the title [WIP] Track method's visibility during indexing Track method's visibility during indexing May 28, 2024
lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb Outdated Show resolved Hide resolved
lib/ruby_indexer/lib/ruby_indexer/entry.rb Outdated Show resolved Hide resolved
lib/ruby_indexer/lib/ruby_indexer/entry.rb Outdated Show resolved Hide resolved
super(name, file_path, location, comments)
def initialize(name, file_path, location, comments, visibility, owner) # rubocop:disable Metrics/ParameterLists
super(name, file_path, location, comments, visibility)
@visibility = visibility
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If visibility is already set on the parent, we don't have to set it here.

@st0012 st0012 force-pushed the index-visibility branch 2 times, most recently from 782fe10 to d1e165a Compare May 29, 2024 14:51
@st0012
Copy link
Member Author

st0012 commented May 29, 2024

@vinistock I've changed the way visibility is set on entries and reduced the amount of changes needed.

@st0012 st0012 requested a review from vinistock May 29, 2024 14:52
Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Alex. I would reduce the indirection in favour of more direct code.

This commit supports visibility tracking for methods in the index by:

- Adding a visibility field to the Member entry, which is a superclass of
  Accessor, Method, and Alias.
- In the DeclarationListener, create a nested stack structure to track
  the visibility of the current scope.
Copy link
Contributor

@Morriar Morriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! 🎉

@st0012 st0012 merged commit b8f17bf into main May 30, 2024
33 checks passed
@st0012 st0012 deleted the index-visibility branch May 30, 2024 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request server This pull request should be included in the server gem's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Save method visibilities in the index
3 participants