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

Add ability to linearize ancestors #2024

Merged
merged 2 commits into from
May 30, 2024
Merged

Conversation

vinistock
Copy link
Member

Motivation

Step towards #1333

Add the ability to linearize ancestors for a given name.

Implementation

  1. Changed the way we remember included, prepend and extended modules. We were throwing everything in separate arrays, but in reality we need to remember the exact order between prepends and includes to ensure we linearize it the right way
  2. Added the linearization algorithm

The algorithm works like this:

class Foo < Bar
  include A
  prepend B
end
  1. Start with the namespace itself [Foo]
  2. Handle the include. We must linearize the ancestors of A and ensure we're removing any duplicate modules that have already been prepended or included
  3. Then handle the prepend. We must linearize the ancestors of B and only remove duplicates if they are present in the prepended modules. It is allowed to have duplicates if the module was included

Looking at the tests will better clarify the edge cases.

Automated Tests

Added a bunch of tests and tried to cover all corner cases I could think of. Please let me know if you can think of other cases.

@vinistock vinistock added enhancement New feature or request server This pull request should be included in the server gem's release notes labels May 8, 2024
@vinistock vinistock self-assigned this May 8, 2024
@vinistock vinistock requested a review from a team as a code owner May 8, 2024 20:28
@vinistock vinistock requested review from andyw8 and st0012 May 8, 2024 20:28
@vinistock vinistock force-pushed the vs/add_ancestors_linearization branch 2 times, most recently from 68c5a00 to cac4824 Compare May 9, 2024 18:51
lib/ruby_indexer/lib/ruby_indexer/index.rb Show resolved Hide resolved
lib/ruby_indexer/lib/ruby_indexer/index.rb Outdated Show resolved Hide resolved
lib/ruby_indexer/lib/ruby_indexer/index.rb Outdated Show resolved Hide resolved
lib/ruby_indexer/lib/ruby_indexer/index.rb Outdated Show resolved Hide resolved
lib/ruby_indexer/lib/ruby_indexer/index.rb Outdated Show resolved Hide resolved
lib/ruby_indexer/test/classes_and_modules_test.rb Outdated Show resolved Hide resolved
lib/ruby_indexer/lib/ruby_indexer/index.rb Outdated Show resolved Hide resolved
lib/ruby_indexer/test/index_test.rb Show resolved Hide resolved
lib/ruby_indexer/test/index_test.rb Outdated Show resolved Hide resolved
lib/ruby_indexer/lib/ruby_indexer/index.rb Show resolved Hide resolved
@vinistock vinistock force-pushed the vs/add_ancestors_linearization branch 3 times, most recently from 9a57b65 to 0f56a52 Compare May 29, 2024 17:54
@vinistock vinistock requested a review from Morriar May 29, 2024 17:54
Copy link
Contributor

@andyw8 andyw8 left a comment

Choose a reason for hiding this comment

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

🎉

lib/ruby_indexer/lib/ruby_indexer/entry.rb Outdated Show resolved Hide resolved
@@ -369,13 +369,13 @@ class ConstantPathReferences
RUBY

foo = T.must(@index["Foo"][0])
assert_equal(["A1", "A2", "A3", "A4", "A5", "A6"], foo.included_modules)
assert_equal(["A1", "A2", "A3", "A4", "A5", "A6"], foo.modules.map(&:module_name))
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to have foo.ancestor_names?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unless we discover a use case for the list of unresolved and unlinearized ancestors, I would vote against adding an API for that. It could potentially give the wrong impression to users of the index that the list is the linearized list of ancestors, which it isn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

Add a mixin_operations_module_names.

lib/ruby_indexer/lib/ruby_indexer/entry.rb Outdated Show resolved Hide resolved
@@ -31,6 +32,9 @@ def initialize

# Holds all require paths for every indexed item so that we can provide autocomplete for requires
@require_paths_tree = T.let(PrefixTree[IndexablePath].new, PrefixTree[IndexablePath])

# Holds the linearized ancestors list for every namespace
@ancestors = T.let({}, T::Hash[String, T::Array[String]])
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's go with this approach for now but I think we'll need a graph instead to be able to implement requests such as Type Hierarchy Subtypes. Maybe something like a poset.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess the trade off here will also involve performance. If building the poset takes longer and requires more memory, then it could potentially not be worth it (since we use linearization all the time for go to definition, hover and completion).

Even if it's the more correct implementation, it might be better to pay a higher price and search for subtypes only when someone is requesting it (which not as common as the other features).

lib/ruby_indexer/lib/ruby_indexer/entry.rb Outdated Show resolved Hide resolved
@vinistock
Copy link
Member Author

@st0012 raised a great point of circular dependencies involving aliases. Will add a test for the following

module A
end

ALIAS = A

module A
  include ALIAS
end

@vinistock vinistock force-pushed the vs/add_ancestors_linearization branch from 401c668 to 9566119 Compare May 30, 2024 17:59
@vinistock vinistock merged commit 162fda2 into main May 30, 2024
33 checks passed
@vinistock vinistock deleted the vs/add_ancestors_linearization branch May 30, 2024 18:13
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.

4 participants