Fix local variable bindings

[ShadiestGoat]

Hey!

As part of my work on the include & factory bot, I've noticed that auto complete is a bit wonky at times for variables defined within examples and contexts. After tons of puts statements everywhere, I have to the conclusion that the reason is that the local variables aren't bound to the right block. So eg:

describe SomeClass do
  describe 'someting about it' do
    it 'should do something' do
      some_variable = method_from_an_included_module()
    end
  end
end

Solargraph can recognize the method's return type, but can't infer the type on some_variable

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.59%. Comparing base (1cf43a3) to head (8f2508a).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #16      +/-   ##
==========================================
+ Coverage   86.54%   86.59%   +0.04%     
==========================================
  Files          23       23              
  Lines        1078     1104      +26     
  Branches       77       76       -1     
==========================================
+ Hits          933      956      +23     
- Misses        145      148       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[lekemula]

Hey, nice catch! Do you mind adding a spec for this to prevent regressions in the future? Thank you in advance.

[ShadiestGoat]

Yes! Let me add them. I also realize that there is probably also another improvement for something like an include pin? Maybe even generalize this further up to remove the block corrector. I'll see what I can do

[ShadiestGoat]

Hey @lekemula I changed the way that I'm fixing the issue & added needed specs. Unfortunately, it seems to affect a spec in a correct but "not like it was before" manner. Basically, now this spec resolved MyClass as RSpec::ExampleGroups::TestSomeNamespaceTransaction::MyClass. This is correct, but different from the way it worked before (where it'd resolve it as MyClass). However what this also means is that for this to work, your change over at castwide/solargraph#1008 needs to be implemented, since the way it resolves constants is my doing confirming that each part exists. So it needs to confirm that RSpec, as the root of the name, exists. Without your change, it doesn't know about RSpec so... broken as a duck.

[lekemula]

Thanks for the specs and suggested refactoring @ShadiestGoat 👍

Let me know what do you think about: #16 (comment)

[lekemula]

Thanks for the specs and suggested refactoring @ShadiestGoat 👍

Let me know what do you think about: #16 (comment)

@ShadiestGoat I'm fine with moving forward with this despite the super-minor "breaking change" (see comment), as soon as you include the other suggestions.

Good stuff 👍

[ShadiestGoat]

Hi - sorry for the long wait. I'll apply the suggestions in a sec, but I did want to confirm - are you sure? This would break the plugin until your solargraph PR gets merged!

[lekemula]

Hi - sorry for the long wait. I'll apply the suggestions in a sec, but I did want to confirm - are you sure? This would break the plugin until your solargraph PR gets merged!

Hi, I'm fine to move forward with this because I think the issue relates to the way we write the spec, which is an edge case IMO. #16 (comment)

That is, we define the class inside:

RSpec.describe SomeNamespace::Transaction, type: :model do
  class MyClass; end
  
  let(:some_object) { MyClass.new }
end

If I understand you correctly, this would still work, right?

class MyClass; end

RSpec.describe SomeNamespace::Transaction, type: :model do
  let(:some_object) { MyClass.new }
end

[lekemula]

If I understand you correctly, this would still work, right?

I can confirm, this does work:

diff --git a/spec/solargraph/rspec/convention_spec.rb b/spec/solargraph/rspec/convention_spec.rb
index 1391374..c813b4b 100644
--- a/spec/solargraph/rspec/convention_spec.rb
+++ b/spec/solargraph/rspec/convention_spec.rb
@@ -481,11 +481,19 @@ def load_and_assert_type(let_declaration, let_name, expected_type)
     end

     it 'infers type for some_object' do
-      pending('https://github.com/castwide/solargraph/pull/1008')
-      load_and_assert_type(<<~RUBY, 'some_object', 'RSpec::ExampleGroups::TestSomeNamespaceTransaction::MyClass')
+      load_string filename, <<~RUBY
         class MyClass; end
-        let(:some_object) { MyClass.new }
+
+        RSpec.describe SomeNamespace::Transaction, type: :model do
+          let(:some_object) { MyClass.new }
+        end
       RUBY
+
+      assert_public_instance_method_inferred_type(
+        api_map,
+        "RSpec::ExampleGroups::TestSomeNamespaceTransaction#some_object",
+        'MyClass'
+      )
     end

So let's inline load_and_assert_type for now.

[lekemula]

Merging this as is and will apply #16 (comment) changes afterwards.

Thanks for the changes @ShadiestGoat!