Skip to content

Fix splat target in multiple assignment losing its element type - #1325

Open
apiology wants to merge 12 commits into
castwide:masterfrom
apiology:fix-masgn-splat
Open

Fix splat target in multiple assignment losing its element type#1325
apiology wants to merge 12 commits into
castwide:masterfrom
apiology:fix-masgn-splat

Conversation

@apiology

Copy link
Copy Markdown
Contributor

This PR was written by Claude Code on behalf of @apiology.

A splat target in a multiple assignment loses its inferred type entirely:

class Repro
  # @param mutator [Array<Symbol, BasicObject>]
  # @return [void]
  def call(mutator)
    command, *args = mutator
    args.fetch(0)  # Unresolved call to fetch
  end
end

Cause: MasgnNode#process looks up the pin for each left-hand-side entry by matching its node location against existing local/instance-variable pins. For a splat entry (*args, parsed as s(:splat, s(:lvasgn, :args))), the code used the splat node's location (*args) instead of the inner assignment target's location (args). These never match, so args never gets a mass_assignment set on it and stays permanently untyped.

Fix:

  • Unwrap :splat nodes to their inner assignment target before computing the lookup location (and skip anonymous splats, which have no target).
  • Track whether an entry is a splat alongside its right-hand-side node and index.
  • In BaseVariable#probe, a splat entry now collects the union of the source array's element parameter types and binds Array<...> of that union, rather than binding a single (and previously entirely absent) element type.

Non-splat multiple assignment (a, b = pair) is untouched — this repo's ComplexType::UniqueType#tuple? is currently disabled (hard-coded false, added deliberately in #1201 "Ignore literal values in type inference"), so positional/tuple-accurate element typing isn't implemented for any multi-assignment position, splat or not; that limitation is out of scope here.

Test plan:

  • bundle exec rspec spec/pin/base_variable_spec.rb — new specs for splat and non-splat masgn cases
  • bundle exec rspec — full suite, 0 failures
  • bin/solargraph typecheck --level strong on repro — 0 problems (previously "Unresolved call to fetch")
  • bundle exec rubocop on changed files — clean

A splat lhs entry in a masgn (command, *args = mutator) parses as
s(:splat, s(:lvasgn, :args)). The lookup location was computed from
the splat node itself, one column off from where the inner lvasgn
pin actually lives, so mass_assignment was never set and the splat
variable stayed permanently untyped.
apiology added a commit to apiology/solargraph that referenced this pull request Aug 22, 2026
Comment thread spec/pin/base_variable_spec.rb
Comment thread lib/solargraph/pin/base_variable.rb Outdated
Comment thread lib/solargraph/pin/base_variable.rb
Replace the literal Array/Set/Enumerable name check in multiple-
assignment splat handling with a check that also structurally
conforms a type against Enumerable via ApiMap, so a user-defined
class that includes Enumerable (or Hash, Range, etc) is
recognized, not just those three exact type names. Add a spec
covering a generic-parameterized custom Enumerable class.
The prior commit declared type as ComplexType::UniqueType, but the
callers pass a plain ComplexType (from return_types_from_node) -
a genuine typecheck error the earlier Solargraph hook skip
papered over instead of catching.
Array<Integer, String> (no tuple parens) declares a union of
possible element types, not a positional tuple. The non-splat
mass-assignment branch was taking only the first param
(all_params.first) regardless of position, so a, b = pair always
resolved both targets to the first union member. Take the whole
union instead, matching what the type actually declares. Update
the covering spec to assert on #to_s rather than #tag, since #tag
delegates to the first union member only and would have masked
this exact bug.
The else branch of the splat handling in BaseVariable#probe
(types.flat_map returning [] when a mass-assignment source type is
neither a tuple nor splattable) had no test.
Exercises the pin-not-found branch (attribute-writer masgn target,
e.g. a.b, c = 1, 2) that only logged a debug message and continued.
Vince's pending review on PR 1325 flagged several comments as
over the 1-3 line budget, one YARD @PARAM left undocumented, and
one @sg-ignore explanation as unclear.

Trims the masgn splat-unwrap comment, the splattable? docstring,
and the non-tuple-Array spec comment to budget. Documents the
new third (Boolean, is-a-splat-target) element of the
mass_assignment tuple in BaseVariable's @PARAM doc.

Re-running strong typecheck on the @sg-ignore line (Solargraph
confusing mass_node's type with the @mass_assignment tuple)
finds no problem with it removed, so it's dropped rather than
reworded - the tool limitation it recorded no longer reproduces.
The debug block in MasgnNode#process built its message only when the
global logger sat at debug level. spec_helper sets that level, but
spec/language_server/host_spec.rb resets it to WARN partway through the
suite, so the block body never ran in a full run and undercover reported
the node at 50 percent.

The new example calls the block directly through a stub instead of
depending on the ambient level, and asserts the message names the node
type of the target that had no variable pin.
apiology added a commit to apiology/solargraph that referenced this pull request Sep 5, 2026
Brings castwide#1325 up to its current head. Its original fix,
8f78bf4, is already here; seven commits are new, including real lib
work - recognizing structurally-Enumerable classes as splattable, and
giving every position the full union for a non-tuple Array target -
alongside c469abc, the coverage for the undercover node this PR owns
at masgn_node.rb:64-66.

One conflict, lib/solargraph/pin/base_variable.rb, where each side adds
a method the other lacks: override_assignments? and definite_reaches?
here, splattable? there. Both sides ended mid-method sharing the closing
end below, so they cannot simply be concatenated; kept this branch's
region and spliced splattable? in from that branch's own file.
splattable? has two live callers here, so dropping it was not an option.

Also ratchets .rubocop_todo.yml, which removes an entry rather than
adding one: Lint/DuplicateMethods was excluded for node_chainer.rb,
suppressing the duplicated hash_pairs that 269c9f8 deleted. RuboCop
had found that defect and the exclusion had hidden it.

2207 examples, 0 failures, 45 pending. Strong typecheck at the six known
baseline problems.
BaseVariable#probe splits multiple assignment into two flat_map blocks -
one for a splat target, one without - and each has an else branch for a
right-hand side that is neither a tuple nor splattable.

79de19b covered the splat one, using `command, *args = mutator`. The
non-splat branch had no test: `command, args = mutator` takes the other
path entirely, so the existing example never reaches it.

Verified by instrumenting the branch and confirming only the new example
reaches it.

spec/pin/base_variable_spec.rb: 9 examples, 0 failures.
apiology added a commit to apiology/solargraph that referenced this pull request Sep 5, 2026
Brings castwide#1325 up to 8a2a2ee, adding coverage for the
non-splat branch of BaseVariable#probe's multiple-assignment handling -
the last uncovered node undercover reported on this branch.

No conflicts. 2208 examples, 0 failures, 45 pending.
BaseVariable#probe selects a single position out of a tuple for a
non-splat multiple-assignment target, and drops the target when its
index runs past the end. No source reaches that branch:
ComplexType#tuple? returns false unconditionally on master, a shim
added by "Ignore literal values in type inference" (castwide#1201) and removed
again by the open castwide#1223.

A comment recording that would go stale silently once castwide#1223 lands. The
pending spec asserts the behaviour that PR makes correct instead, so it
fails today and breaks the build the day it starts passing, forcing
whoever lands castwide#1223 to delete the pending line.

Undercover still reports the enclosing block at 83.33 percent, with
[type.all_params[index]].compact at zero hits. Closing that needs
castwide#1223, not another example.

spec/pin/base_variable_spec.rb: 10 examples, 0 failures, 1 pending.
Review asked for one line at most on the non-tuple comment, and a
reword of the tuple one. Both now name the type form they are about,
so the pair reads as the contrast the branch actually makes:
Array(Integer, String) is positional, Array<Integer, String> is a
union.
@apiology
apiology marked this pull request as ready for review September 6, 2026 18:09
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.

1 participant