Skip to content

Add new Pin::FactoryParameter#1063

Open
lekemula wants to merge 13 commits intocastwide:masterfrom
lekemula:factory-parameters
Open

Add new Pin::FactoryParameter#1063
lekemula wants to merge 13 commits intocastwide:masterfrom
lekemula:factory-parameters

Conversation

@lekemula
Copy link
Contributor

@lekemula lekemula commented Sep 3, 2025

Relates to: lekemula/solargraph-rspec#18 and lekemula/solargraph-rspec#14

Problem

I would like to add support for RSpec shared examples via solargraph-rspec plugin: solargraph-rspec#18

Current limitation: When using RSpec shared examples like this:

RSpec.shared_examples "some example" do |parameter|
  # example implementation
end

RSpec.describe SomeClass do
  include_examples "some example", "parameter1"
  include_examples "some example", "parameter2" 
end

Solargraph cannot:

  • Navigate to the shared example definition when clicking on "some example"
  • Provide autocompletion for available shared example names

Solution

This PR introduces a new Pin::FactoryParameter class that enables plugins (particularly solargraph-rspec) to register method parameters with specific values and their corresponding definitions.

How it works

The new pin allows plugins to define factory-like method parameters:

pin = Solargraph::Pin::FactoryParameter.new(
  method_name: 'it_behaves_like',
  method_namespace: 'RSpec::Core::ExampleGroup', 
  method_scope: :class,
  param_name: 'name',
  value: 'some shared example name', # or symbol :some_shared_example_name
  return_type: nil,
  decl: :arg,
  location: PinFactory.build_location(location_range, source_map.source.filename)
)

See FactoryParameter#initialize method documentation what each parameter means.

Key benefits

  1. Go-to-definition support: Jump directly to shared example definitions
  2. Type inference: Better return type resolution for factory methods
  3. Parameter completion: Allow users to get autocompletion as they type
  4. Extensibility: Foundation for supporting other Ruby DSLs

Why a new pin type?

This approach creates a flexible foundation that can support multiple Ruby DSL patterns beyond RSpec:

Immediate applications

  • RSpec shared examples: include_examples "example_name"
  • RSpec shared contexts: include_context "context_name"

Future possibilities

  • Factory Bot: create(:model_name) with go-to-definition to factory definitions
  • Rails associations: belongs_to :model_name with navigation to model files
  • Rails validations: validates :method_name with navigation to method definitions
  • Custom DSLs: Any method that accepts string/symbol parameters referencing definitions

Scope of this PR

This PR focuses on implementing the core infrastructure and two primary features:

Go-to-definition support - Navigate from parameter values to their definitions
Return type inference - Lay groundwork for better type resolution

These features directly address the most requested functionality from the solargraph-rspec.

Future enhancements

  • Parameter autocompletion: Show available factory parameter values while typing
  • Refactor require calls: Leverage existing Solargraph::Source::Chain::Parameter infrastructure
  • Enhanced validation: Warn when referencing non-existent factory parameters

Questions for review

  1. API design: Is Pin::FactoryParameter an appropriate name and structure?
  2. Performance: Are the lookup and indexing mechanisms efficient for large codebases?

method_call_chain = NodeChainer.chain(send_node, @filename, nil, [])
literal = Chain::Literal.new(lit, n)
arg_index = send_node.children[2..-1].index(n)
result.push Chain::Parameter.new(literal, method_call_chain)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially, I wanted to make this work as a chain of: method_call > parameter; however, I struggled to find a good way to pass the last method call resolution to the Chain::Parameter as a context here:

return [] if undefined?
# working_pin is the surrounding closure pin for the link
# being processed, whose #binder method will provide the LHS /
# 'self type' of the next link (same as the #return_type method
# --the type of the result so far).
#
# @todo ProxyType uses 'type' for the binder, but '
working_pin = name_pin
links[0..-2].each do |link|
pins = link.resolve(api_map, working_pin, locals)
type = infer_from_definitions(pins, working_pin, api_map, locals)
if type.undefined?
logger.debug { "Chain#define(links=#{links.map(&:desc)}, name_pin=#{name_pin.inspect}, locals=#{locals}) => [] - undefined type from #{link.desc}" }
return []
end
# We continue to use the context from the head pin, in case
# we need it to, for instance, provide context for a block
# evaluation. However, we use the last link's return type
# for the binder, as this is chaining off of it, and the
# binder is now the lhs of the rhs we are evaluating.
working_pin = Pin::ProxyType.anonymous(name_pin.context, binder: type, closure: name_pin, source: :chain)
logger.debug { "Chain#define(links=#{links.map(&:desc)}, name_pin=#{name_pin.inspect}, locals=#{locals}) - after processing #{link.desc}, new working_pin=#{working_pin} with binder #{working_pin.binder}" }
end
links.last.last_context = working_pin
links.last.resolve(api_map, working_pin, locals)

So I ended up with parameter(method_call_chain).

Maybe I don't understand the Pin::ProxyType well enough, so if you have a suggestion on how to make it work, I'd be more than happy to refactor this into an "appropriate chain".


module Solargraph
module Pin
class FactoryParameter < Base
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to other naming suggestions!

map = Solargraph::SourceMap.map(source)
cursor = map.cursor_at(Solargraph::Position.new(2, 10)) # :bar
expect(cursor.chain.links.last).to be_a(Solargraph::Source::Chain::Parameter)
expect(cursor.chain.links.map(&:word)).to eq(['foo(.., <::Symbol>, ..)'])
Copy link
Contributor Author

@lekemula lekemula Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm NOT very happy with this "word notation": foo(.., <::Symbol>, ..), other ideas?

Another one that I thought of was: foo(name=<::Symbol>), maybe?

@lekemula lekemula marked this pull request as ready for review September 3, 2025 21:38
@ShadiestGoat
Copy link
Contributor

Hi - can you explain what you mean by "factory like"? I don't think I get it

@lekemula
Copy link
Contributor Author

lekemula commented Sep 4, 2025

what you mean by "factory like"? I don't think I get it

Good question @ShadiestGoat! By factory-like methods I mean the methods which build other objects (classical definition), a typical example being FactoryBot.create(:model).

And in Ruby, this extends to DSL class methods that "build the class", by adding methods (eg, Rails model associations) or customising its behaviour (eg, Rails validations).

This was the initial idea behind the FactoryParameter naming.

Does that make sense?

@ShadiestGoat
Copy link
Contributor

what you mean by "factory like"? I don't think I get it

Good question @ShadiestGoat! By factory-like methods I mean the methods which build other objects (classical definition), a typical example being FactoryBot.create(:model).

And in Ruby, this extends to DSL class methods that "build the class", by adding methods (eg, Rails model associations) or customising its behaviour (eg, Rails validations).

Does that make sense?

Yes thank you for clarifying. One more question - I don't think I understood how to actually use them? In the case of factory bot for eg? What do the various params do?

@lekemula
Copy link
Contributor Author

lekemula commented Sep 4, 2025

I don't think I understood how to actually use them? In the case of factory bot for eg? What do the various params do?

I actually use the FactoryBot case to describe it in spec/source/chain_spec.rb( see it 'infers types from factory methods' do) spec, have you checked it by any chance?

Also see FactoryParameter#initialize method documentation.

Let me know if the spec is still unclear.

@ShadiestGoat
Copy link
Contributor

Oh that's great - I have not had a chance to look at any code, so I didn't catch that. I was just going of off the PR description.

As a side note, maybe it'd be good to link to it in the description, for silly people like me?

@ShadiestGoat
Copy link
Contributor

ShadiestGoat commented Sep 5, 2025

Very interesting. I wonder how much better this is than a method override as I proposed in my factory bot pr? Also, I might've just missed this in my quick glance, but is this able to determine other parts of the signature based on the value? Like other params available?

Obviously I'm no maintainer here and I haven't touched this codebase in like at least a month so I have no say, but given examples from other languages like typescript, it makes more sense to me to do overrides w/ different signatures than a different type of pin. Like imo the signature detection and use of it should improve rather than adding smt like this. What do you think?

Like the fundamental problem is that solargraph's completion & validation for constants/literals is lacking & that override go to definition is not supported. Those are the problems that need to be fixed. Especially since this is currently valid code according to sg (afaik):

# @param arg ['foo']
def example(arg)
end

example('bar')

@lekemula
Copy link
Contributor Author

lekemula commented Sep 5, 2025

Thank you so much @ShadiestGoat for your feedback - you’ve raised some excellent questions!

On the question of “why introduce a new pin instead of using overload signatures?”: I see overloads as most useful when a method has a finite set of known input values (for example, enum-like parameters such as @param [:value1, :value2, …]). In our case, with calls like RSpec.include_examples(example_name=) or FactoryBot#create(factory_name=), the parameter values are effectively open-ended (@param [String, Symbol]) and depend on definitions elsewhere — e.g. RSpec.shared_examples or FactoryBot.define :factory_name.

As far as I know, no type system can fully capture this sort of dynamic binding. That’s why I believe it’s more appropriate for a solargraph plugin to provide that "conventional connection" via a new FactoryParameter pin.

Also, I might’ve just missed this in my quick glance, but is this able to determine other parts of the signature based on the value? Like other params available?

That was outside the scope of this particular PR (it wasn’t needed for my case), but the current implementation could certainly be extended to support something along those lines.

As for:

Like the fundamental problem is that solargraph's completion & validation for constants/literals is lacking & that override go to definition is not supported. Those are the problems that need to be fixed. Especially since this is currently valid code according to sg (afaik):

I agree with you here - but I see that as a separate issue, more aligned with the case I mentioned earlier where values are known ahead of time from the signature itself.

I’m not dismissing your suggestion at all; in fact, I’d love to hear what @castwide thinks as well. I just wanted to clarify my reasoning for introducing a new pin instead of relying on overloads, as you did in your FactoryBot PR (which I really appreciate ❤️). And please don’t take this PR as a blocker for your work there!

TL;DR: Using overloads here felt at least semantically incorrect, and on top of that, they seemed harder to make work (at least per my current understanding of the codebase).

@lekemula
Copy link
Contributor Author

lekemula commented Sep 5, 2025

From the practical perspective, another reason why I favoured the new pin over overload is that with the later:

RSpec.include_examples("some shared example")
      # ^^^^^^^^^^^^ Uses needs to click on the method name to jump to the definition
      # We lose the method definition

Whereas with the new pin

RSpec.include_examples("some shared example")
      #                 ^^^^^^^^^^^^^^^^^^^ User clicks on the param
      # ^^^^^^^^^^^^ User can still jump to "real method definition"

@ShadiestGoat
Copy link
Contributor

That's fair - thank you for the expansion. I do agree that it's not exactly ergonomic to use overloads.

Your last point sounds like a really cool use case (though I'm not 100% convinced that it can't be done with overloads).

I'm not sure I understand about your first point - rspec shared examples, factory models, or even rails belongs_to aren't open ended from the perspective of a plugin. Like a plugin that'd implement factory params would know what are the available values, right?

PS: Just like you, I'm also not suggesting that this is a bad implementation/idea, I'm trying to explore this idea/issue further. I really love the rspec plugin and it has saved me tons of headaches at work ❤️

@lekemula
Copy link
Contributor Author

lekemula commented Sep 5, 2025

Your last point sounds like a really cool use case (though I'm not 100% convinced that it can't be done with overloads).

You're actually right here, it's rather a question of should we, and later how hard and efficiently we could do that.

I'm not sure I understand about your first point - rspec shared examples, factory models, or even rails belongs_to aren't open ended from the perspective of a plugin. Like a plugin that'd implement factory params would know what are the available values, right?

I meant from the perspective of the method definition. I think, @overload is primarily meant to express the acceptable params prior to its usage, and let alone the usage of other related methods.

Let me use Hash#[] as an example. We could in theory track the key assignments of a given variable, and suggest the possible keys and/or their return values eg. my_hash["my_key"], but would the @overload of Hash#[] or invention of a "imaginary subclass" of a hash with overloaded [] with the possible keys, be the right way? It surely would do the trick, but IMO it's "a trick" nonetheless.

That is why I think it makes more sense to have a separate construct for this purpose. (Not 100% sure though!)

Maybe I'm being over-pedantic here.

I really love the rspec plugin and it has saved me tons of headaches at work ❤️

This really warms my heart to hear! 🥰

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants