Materialize singleton classes for namespace declarations#872
Conversation
Assisted-By: devx/7c3d70e1-327c-4158-8ab5-ffccba197089
Assisted-By: devx/7c3d70e1-327c-4158-8ab5-ffccba197089
| } | ||
|
|
||
| fn is_searchable_declaration(declaration: &Declaration) -> bool { | ||
| !matches!(declaration, Declaration::Namespace(Namespace::SingletonClass(_))) |
There was a problem hiding this comment.
This simply drops all singleton class declarations from search results. For UX, we may want to keep explicit singleton class declarations from class <<, and only filter out synthetic ones.
There was a problem hiding this comment.
How about we still always list single class decls from Rubydex, and we let the client to decide whether to show them?
@vinistock WDYT?
There was a problem hiding this comment.
I'm a bit torn here. Filtering on the Rust side allows for great performance and parallelism. Additionally, the common case is almost never to search for the singleton class name, but instead the attached object name.
It's also still possible to reach the singleton class by searching for the attached object and then directly accessing.
There was a problem hiding this comment.
I’ll keep the code above for now. We can revisit it later if it causes another issue.
Assisted-By: devx/7c3d70e1-327c-4158-8ab5-ffccba197089
| } | ||
|
|
||
| fn is_searchable_declaration(declaration: &Declaration) -> bool { | ||
| !matches!(declaration, Declaration::Namespace(Namespace::SingletonClass(_))) |
There was a problem hiding this comment.
How about we still always list single class decls from Rubydex, and we let the client to decide whether to show them?
@vinistock WDYT?
vinistock
left a comment
There was a problem hiding this comment.
If we make the move to always create singleton classes eagerly, then I think the right layer for the fix would to create a singleton class whenever the class gets created. It may require studying the impact for lazily creating singleton class of singleton classes (e.g.: Foo::<Foo>::<<Foo>>).
Also, I wonder if it would still be worth trying to minimize the amount of work. So far, if no operation in the code reaches into the singleton class, we've been considering that it doesn't exist in our modelling (lazy). Maybe we should only create singleton classes for the descendants of existing singleton classes (instead of all of them).
It makes sense. Will fix the PR to do so. And we still need to keep the lazy path for meta-singleton classes anyway. Ancestor/descendant enumeration will remain broken for meta-singleton classes, but that seems acceptable.
It would be possible, but it would require a significant re-architecture of the ancestor/descendant handling. I think we should keep the eager materialization strategy for this issue for now. |
Assisted-By: devx/7c3d70e1-327c-4158-8ab5-ffccba197089
Assisted-By: devx/7c3d70e1-327c-4158-8ab5-ffccba197089
This PR materializes all singleton class declarations for class and module namespaces.
This is required for
descendantsenumeration and forfind_member_in_ancestorscorrectness. Without these declarations, a subclass singleton class such asChild::<Child>may not exist whenChildis only defined asclass Child < Parent, so inherited singleton ancestors and members cannot be found.Impacted Ruby methods
Namespace#descendantsnow includes materialized singleton class declarations.Namespace#find_membernow returns members inherited through singleton class ancestors.Other user-facing changes
Performance impact
Because this materializes singleton classes for every namespace declaration, it can add memory use and runtime cost. In practice, the measured impact was small.
Validation
origin/main:Extension.descendantslacksChild::<Child>find_memberregression fails onorigin/main:Child::<Child>returnsDeclarationNotFoundshadowenv exec -- cargo test -p rubydex find_member_in_ancestors_returns_inherited_singleton_membershadowenv exec -- bundle exec ruby -Itest test/declaration_test.rb -n /test_find_member_returns_inherited_members_on_singleton_class/shadowenv exec -- cargo test -p rubydex exact_match_empty_query_returns_allshadowenv exec -- cargo test -p rubydexshadowenv exec -- bundle exec rake ruby_testgit diff --check HEAD~2..HEAD