Fix NameError raised by RubyIndexer's abstract method guard - #4197
Open
aryrabelo wants to merge 1 commit into
Open
Fix NameError raised by RubyIndexer's abstract method guard#4197aryrabelo wants to merge 1 commit into
aryrabelo wants to merge 1 commit into
Conversation
…f relying on RubyLsp's constant
Author
|
CLA signed. |
Author
|
I have signed the CLA. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
RubyIndexer::Entry::Member#signaturesis an abstract method that raisesAbstractMethodInvokedError. That constant is not defined anywhere underRubyIndexer. It exists only asRubyLsp::AbstractMethodInvokedErrorinlib/ruby_lsp/utils.rb, andRubyIndexeris a sibling module ofRubyLsp, not nested inside it. So when the guard fires, it raisesNameErrorinstead of the intended error:This is the only one of the 17
raise AbstractMethodInvokedErrorsites in the repository that lives outsidelib/ruby_lsp/. The other 16 are insidemodule RubyLspand resolve correctly.On severity, so the diff is not read as more urgent than it is: this is not reachable from the code as it stands.
Memberis abstract, and both of its concrete subclasses implementsignatures,Accessorwith a method andMethodwith anattr_reader. The guard is latent. The value of fixing it is that it still works for the case it exists to catch, a future subclass that forgets to implement the method, and thatrescue AbstractMethodInvokedErrorwould not catch aNameError.Implementation
AbstractMethodInvokedErroris now defined insideRubyIndexer::Entry, so the bare constant resolves lexically fromEntry::Member.I deliberately did not point the
raiseatRubyLsp::AbstractMethodInvokedError. Todaylib/ruby_indexer/lib/has no code dependency onRubyLspat all, the only two mentions being arubocop:disabledirective inenhancement.rband a prose comment inindex.rb, and referencing it would create the first one.RubyIndexeralready declares its own error classes in exactly this shape:UnresolvableAliasError,NonExistingNamespaceErrorandIndexNotEmptyErrorall sit at the top ofclass Indexinindex.rb, scoped to the class that raises them.Automated Tests
No test added. The repository has no test covering any of the 17 abstract method guards, so adding one here would introduce a pattern that does not exist yet, and it would need a throwaway
Membersubclass whose only purpose is to trigger the raise.I did verify the change both ways rather than by inspection. A standalone minitest asserting that the guard raises the intended error fails against the current nesting, with
NameError: uninitialized constant ..., and passes against the patched nesting. If you would rather have that inlib/ruby_indexer/test/, say so and I will add it.Manual Tests
The guard is not reachable through the editor, so there is nothing to exercise in a VS Code instance. The failure mode reproduces directly:
With this change the same snippet raises
RubyIndexer::Entry::AbstractMethodInvokedError.