From 5eed2478b5efd6db1e09f72fdf178175f897df71 Mon Sep 17 00:00:00 2001 From: Vince Broz Date: Mon, 14 Oct 2024 09:25:36 -0400 Subject: [PATCH 1/6] Fix "Not enough arguments to" false positives on kwarg type checking --- lib/solargraph/type_checker.rb | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/lib/solargraph/type_checker.rb b/lib/solargraph/type_checker.rb index 059caaeff..fda3a1e86 100644 --- a/lib/solargraph/type_checker.rb +++ b/lib/solargraph/type_checker.rb @@ -254,6 +254,11 @@ def call_problems result end + # @param chain [Solargraph::Source::Chain] + # @param api_map [Solargraph::ApiMap] + # @param block_pin [Solargraph::Pin::Base] + # @param locals [Array] + # @param location [Solargraph::Location] def argument_problems_for chain, api_map, block_pin, locals, location result = [] base = chain @@ -279,9 +284,14 @@ def argument_problems_for chain, api_map, block_pin, locals, location errors = [] sig.parameters.each_with_index do |par, idx| argchain = base.links.last.arguments[idx] - if argchain.nil? && par.decl == :arg - errors.push Problem.new(location, "Not enough arguments to #{pin.path}") - next + if argchain.nil? + if par.decl == :arg + errors.push Problem.new(location, "Not enough arguments to #{pin.path}") + next + else + last = base.links.last.arguments.last + argchain = last if last && last.node.type == :kwsplat + end end if argchain if par.decl != :arg From 3d4456ac67a3bf42a37c4a89fdda13effd8d2b12 Mon Sep 17 00:00:00 2001 From: Vince Broz Date: Mon, 14 Oct 2024 09:34:36 -0400 Subject: [PATCH 2/6] Add tests, including issue failing on main branch --- spec/type_checker/levels/strict_spec.rb | 47 +++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/spec/type_checker/levels/strict_spec.rb b/spec/type_checker/levels/strict_spec.rb index 22bbd3631..c3ef2301f 100644 --- a/spec/type_checker/levels/strict_spec.rb +++ b/spec/type_checker/levels/strict_spec.rb @@ -376,6 +376,53 @@ def bar(baz) expect(checker.problems.first.message).to include('Not enough arguments') end + it 'reports solo missing kwarg' do + checker = type_checker(%( + class Foo + def bar(baz:) + end + end + Foo.new.bar + )) + expect(checker.problems).to be_one + expect(checker.problems.first.message).to include('Missing keyword arguments') + end + + it 'reports not enough kwargs' do + checker = type_checker(%( + class Foo + def bar(foo:, baz:) + end + end + Foo.new.bar(foo: 100) + )) + expect(checker.problems).to be_one + expect(checker.problems.first.message).to include('Missing keyword argument') + expect(checker.problems.first.message).to include('baz') + end + + it 'accepts passed kwargs' do + checker = type_checker(%( + class Foo + def bar(baz:) + end + end + Foo.new.bar(baz: 123) + )) + expect(checker.problems).to be_empty + end + + it 'accepts multiple passed kwargs' do + checker = type_checker(%( + class Foo + def bar(baz:, bing:) + end + end + Foo.new.bar(baz: 123, bing: 456) + )) + expect(checker.problems).to be_empty + end + it 'requires strict return tags' do checker = type_checker(%( class Foo From 60a16f88e71975210526314da97475fb15e2f018 Mon Sep 17 00:00:00 2001 From: Vince Broz Date: Mon, 14 Oct 2024 09:47:01 -0400 Subject: [PATCH 3/6] Acccount for older Ruby kwarg handling --- lib/solargraph/type_checker.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/solargraph/type_checker.rb b/lib/solargraph/type_checker.rb index fda3a1e86..b78484fc2 100644 --- a/lib/solargraph/type_checker.rb +++ b/lib/solargraph/type_checker.rb @@ -290,7 +290,7 @@ def argument_problems_for chain, api_map, block_pin, locals, location next else last = base.links.last.arguments.last - argchain = last if last && last.node.type == :kwsplat + argchain = last if last && [:kwsplat, :HASH].include?(last.node.type) end end if argchain From f7b5d2428e3bf7c0f440bf91b9a5c32f6d575273 Mon Sep 17 00:00:00 2001 From: Vince Broz Date: Mon, 14 Oct 2024 10:29:33 -0400 Subject: [PATCH 4/6] Convert kwarg_problems_for to focus on one parameter --- lib/solargraph/type_checker.rb | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/lib/solargraph/type_checker.rb b/lib/solargraph/type_checker.rb index b78484fc2..ef6d663de 100644 --- a/lib/solargraph/type_checker.rb +++ b/lib/solargraph/type_checker.rb @@ -327,30 +327,29 @@ def argument_problems_for chain, api_map, block_pin, locals, location result end - def kwarg_problems_for argchain, api_map, block_pin, locals, location, pin, params, first + def kwarg_problems_for argchain, api_map, block_pin, locals, location, pin, params, idx result = [] kwargs = convert_hash(argchain.node) - pin.signatures.first.parameters[first..-1].each_with_index do |par, cur| - idx = first + cur - argchain = kwargs[par.name.to_sym] - if par.decl == :kwrestarg || (par.decl == :optarg && idx == pin.parameters.length - 1 && par.asgn_code == '{}') - result.concat kwrestarg_problems_for(api_map, block_pin, locals, location, pin, params, kwargs) - else - if argchain - data = params[par.name] - if data.nil? - # @todo Some level (strong, I guess) should require the param here - else - ptype = data[:qualified] - next if ptype.undefined? + par = pin.signatures.first.parameters[idx] + argchain = kwargs[par.name.to_sym] + if par.decl == :kwrestarg || (par.decl == :optarg && idx == pin.parameters.length - 1 && par.asgn_code == '{}') + result.concat kwrestarg_problems_for(api_map, block_pin, locals, location, pin, params, kwargs) + else + if argchain + data = params[par.name] + if data.nil? + # @todo Some level (strong, I guess) should require the param here + else + ptype = data[:qualified] + unless ptype.undefined? argtype = argchain.infer(api_map, block_pin, locals) if argtype.defined? && ptype && !any_types_match?(api_map, ptype, argtype) result.push Problem.new(location, "Wrong argument type for #{pin.path}: #{par.name} expected #{ptype}, received #{argtype}") end end - elsif par.decl == :kwarg - result.push Problem.new(location, "Call to #{pin.path} is missing keyword argument #{par.name}") end + elsif par.decl == :kwarg + result.push Problem.new(location, "Call to #{pin.path} is missing keyword argument #{par.name}") end end result From ad3724c8adfd3b824c07c956dcc43663acac531b Mon Sep 17 00:00:00 2001 From: Vince Broz Date: Mon, 14 Oct 2024 10:34:35 -0400 Subject: [PATCH 5/6] Verify newer Ruby versions as well - kwargs are a moving target --- .github/workflows/rspec.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/rspec.yml b/.github/workflows/rspec.yml index cbc69c804..9ce44af37 100644 --- a/.github/workflows/rspec.yml +++ b/.github/workflows/rspec.yml @@ -22,7 +22,7 @@ jobs: runs-on: ubuntu-latest strategy: matrix: - ruby-version: ['2.6', '2.7', '3.0', '3.1'] + ruby-version: ['2.6', '2.7', '3.0', '3.1', '3.2', '3.3'] steps: - uses: actions/checkout@v3 From 19b24769041fa8aaa2c368f787c757f2b244371b Mon Sep 17 00:00:00 2001 From: Vince Broz Date: Mon, 14 Oct 2024 10:43:46 -0400 Subject: [PATCH 6/6] Drop Ruby 3.3 from matrix due to https://github.com/castwide/solargraph/issues/726 --- .github/workflows/rspec.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/rspec.yml b/.github/workflows/rspec.yml index 9ce44af37..7f1c09e1a 100644 --- a/.github/workflows/rspec.yml +++ b/.github/workflows/rspec.yml @@ -22,7 +22,7 @@ jobs: runs-on: ubuntu-latest strategy: matrix: - ruby-version: ['2.6', '2.7', '3.0', '3.1', '3.2', '3.3'] + ruby-version: ['2.6', '2.7', '3.0', '3.1', '3.2'] steps: - uses: actions/checkout@v3