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

Always push namespace onto the stack #2019

Merged
merged 1 commit into from
May 9, 2024
Merged

Conversation

st0012
Copy link
Member

@st0012 st0012 commented May 7, 2024

Motivation

Found this when working on visibility tracking: Currently, the indexer doesn't index classes and modules that have dynamic namespace. Similarly, those classes and modules' names are not pushed onto the namespace stack. However, the stack is currently popped on every class or module exit, which could lead to an incorrect stack popping.

Implementation

This PR fixes the issue by always pushing the namespace to the stack, even when the namespace is dynamic. This also means that we'll start indexing dynamic namespaces' methods with the correct owner. So it actually enables go-to-definition on dynamic namespaces self-receiver methods too.

Automated Tests

Manual Tests

@st0012 st0012 added bugfix This PR will fix an existing bug server This pull request should be included in the server gem's release notes labels May 7, 2024
@st0012 st0012 requested a review from a team as a code owner May 7, 2024 21:41
@st0012 st0012 requested review from andyw8 and vinistock May 7, 2024 21:41
@vinistock
Copy link
Member

I think we actually need to always push and pop the stack. If we don't, we will end up pushing methods into the wrong owner. Consider this

module Foo
  var = Bar
  module var::Baz
    def qux; end
  end
end

If we don't push something into the stack, the qux method will be associated with the Foo module, which is incorrect. I think we can take a similar strategy that we used for code lens.

If we find a dynamic declaration, we can either push something into the stack that we can easily identify like <dynamic declaration> or use some boolean to remember we're inside a dynamic context. Then we need to discard every method definition we encounter while we're in a dynamic declaration.

Currently, the indexer doesn't index classes and modules that have dynamic
namespace. Similarly, those classes and modules' names are not pushed onto the
namespace stack. However, the stack is currently popped on every class or module
exit, which could lead to an incorrect stack popping.

This commit fixes the issue by always pushing the namespace to the stack, even
when the namespace is dynamic. This also means that we'll start indexing dynamic
namespaces' methods with the correct owner.
@st0012 st0012 force-pushed the fix-incorrect-stack-popping branch from 961ead4 to 7bc874e Compare May 8, 2024 18:26
@st0012 st0012 changed the title Only pop namespace stack when the class or module is pushed onto it Always push namespace onto the stack May 8, 2024
@st0012 st0012 added the enhancement New feature or request label May 8, 2024
@st0012
Copy link
Member Author

st0012 commented May 8, 2024

@vinistock I've updated the implementation to always push to the stack.

@st0012 st0012 merged commit f4a2c16 into main May 9, 2024
40 checks passed
@st0012 st0012 deleted the fix-incorrect-stack-popping branch May 9, 2024 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR will fix an existing bug 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.

2 participants