-
Notifications
You must be signed in to change notification settings - Fork 154
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
Conversation
68c5a00
to
cac4824
Compare
9a57b65
to
0f56a52
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
@@ -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)) |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
@@ -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]]) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
@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 |
5e520c3
to
401c668
Compare
401c668
to
9566119
Compare
Motivation
Step towards #1333
Add the ability to linearize ancestors for a given name.
Implementation
The algorithm works like this:
[Foo]
A
and ensure we're removing any duplicate modules that have already been prepended or includedB
and only remove duplicates if they are present in the prepended modules. It is allowed to have duplicates if the module was includedLooking 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.