Avoid collecting duplicates in reference finder

[vicocamacho]

Motivation

Closes #3088

As I was playing around with the gem in Neovim I discovered some over reporting of references when a project has classes that are re-opened throughout the codebase.

Implementation

Stop adding references on class and node module enter events and enhance the way we collect constant references to add additional logic to determine whether a reference is a constant or not.

Automated Tests

Added one new test case to reference_finder_test.rb that covers for the scenario presented above.

[graphite-app]

How to use the Graphite Merge Queue

Add the label graphite-merge to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

[vinistock]

Thank you for the PR!

I think we can avoid looking backwards in previously collected locations.

The reason for the duplicates is the fact that a class/module node will match both on_class/module_node_enter and on_constant_* because there's a constant node inside of the declaration.

Instead of looking backwards, let's stop pushing references in the on_class_node_enter and on_module_node_enter events. This will already get rid of the duplication.

The only piece left in the fix will be determining if a constant is a declaration or not. In collect_constant_references, we need to check if there's a declaration for the constant we just resolved, so that we can then consider it as declaration: true.

Something like this:

entries = @index.resolve(name, @stack)
return unless entries

# Get the fully qualified name of the constant we just resolved
full_constant_name = T.must(entries.first).name

# Check if this constant is a namespace declaration
# (class or module)
declaration = @index[full_constant_name].any? { |e| e.is_a?(Entry::Namespace) }

[vicocamacho]

Thank for the feedback @vinistock.

I just pushed an update to incorporate it, please let me know of any other tweaks or adjustments.

[vinistock]

It's almost there. I think this is what we need to do. There are a couple of pieces missing, like implementing Location#== and passing the file URI to the reference finder.

def collect_constant_references(name, location)
  return unless @target.is_a?(ConstTarget)

  entries = @index.resolve(name, @stack)
  return unless entries

  # Filter down to all constant declarations that match the expected name and type
  matching_entries = entries.select do |e|
    [Entry::Namespace, Entry::Constant, Entry::ConstantAlias, Entry::UnresolvedConstantAlias].include?(e.class) &&
      e.name == @target.fully_qualified_name
  end

  # If any of the matching entries have the same location as the constant and were
  # defined in the same file, then it is that constant's declaration
  #
  # Note: we will need to implement `==` on RubyIndexer::Location to accept either
  # another `RubyIndexer::Location` or a `Prism::Location`, so that we can compare with
  # both.
  #
  # Additionally, we don't currently pass the `@uri` to the reference finder, but we will
  # need it so that we can check if the declaration was made in the same file
  declaration = matching_entries.any? do |e|
    e.name_location == location && e.uri == @uri
  end

  @references << Reference.new(name, location, declaration: declaration)
end

[vicocamacho]

@vinistock I incorporated the next round of feedback. I left one comment with one question I have for your consideration.

Please let me know of any additional feedback/changes. I appreciate all the help

[vinistock]

Just one minor modification, but this looks good

[vicocamacho]

@vinistock Just incorporated the change. I think it might be good be go

[vinistock]

The changes look good to me. There are still some linting errors. You can run linting locally with bundle exec rubocop.

Also, there's a conflict that needs to be resolved.

[vicocamacho]

Thanks! Just corrected the linting error and incorporated changes from main

[vinistock]

Thanks for your contribution!

[graphite-app]

Merge activity

• Feb 3, 10:59 AM EST: A user added this pull request to the Graphite merge queue.
• Feb 3, 11:13 AM EST: The Graphite merge queue couldn't merge this PR because it failed for an unknown reason (Stack merges are not currently supported for forked repositories. Please create a branch in the target repository in order to merge).