Skip to content

Infer element types for array literals and NilClass for nil literals - #1226

Closed
apiology wants to merge 12 commits into
castwide:masterfrom
apiology:fix-array-nil-literal-inference
Closed

Infer element types for array literals and NilClass for nil literals#1226
apiology wants to merge 12 commits into
castwide:masterfrom
apiology:fix-array-nil-literal-inference

Conversation

@apiology

@apiology apiology commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Two pre-existing type-inference gaps identified while investigating solargraph-rspec plugin CI failures (see #1224):

  • Array literals (e.g. [1, 2, 3]) inferred as bare Array instead of Array<Integer> — Chain::Array#resolve never inferred element types from the literal's children.
  • nil literals inferred as the internal pseudo-type tag "nil" instead of NilClass when passed through simplify_literals (used for e.g. hover/display purposes).

Changes

  • lib/solargraph/source/chain/array.rb: infer each array element's type and attach the union as the Array's generic parameter; falls back to a bare Array when the literal is empty or an element's type can't be determined.
  • lib/solargraph/complex_type/unique_type.rb: simplify_literals now maps the nil pseudo-type to NilClass, matching how other literals are simplified to their class name. Widened to_rbs's nil check to also recognize NilClass so RBS output still renders the nil keyword type rather than ::NilClass.
  • Removed two pending markers in specs that this fix resolves (spec/complex_type/conforms_to_spec.rb, spec/type_checker/levels/strict_spec.rb) and updated two simple_tags assertions that encoded the old "nil" output.

Test plan

  • bundle exec rake spec — full suite green except 2 pre-existing local-machine-only failures in shell_spec.rb (Gem::ConflictError from a stale globally-installed solargraph gem conflicting with bundler; reproduces identically on unmodified master, not a CI concern).
  • Verified against the downstream apiology/solargraph-rspec plugin test suite (the two target specs — infers type for some_array and infers type for some_nil — now pass; no new failures elsewhere).

@apiology
apiology marked this pull request as ready for review July 29, 2026 22:39
apiology and others added 10 commits July 29, 2026 20:13
@apiology
Revert "Ignore literal values in type inference (castwide#1201)"
56bbdc1 
This reverts commit 8c40692.

Restoring as a base to fix the specious-inference bugs from castwide#1196
directly instead of leaving literal/tuple inference disabled.
@apiology @claude
Restore tuple/literal element inference with safe (union-based) indexing
8ffb757 
Fixes castwide#1196. PR castwide#1201 disabled all array/tuple element-type inference
after finding several cases of specious (wrong-looking-precise)
results. This restores the inference but reworks tuple indexing to
avoid the specious cases instead of giving up on element typing
altogether:

- UniqueType#resolve_generics: fixed ancestor-generics resolution so
  that methods inherited from Array/Enumerable (e.g. #last, #first,
  #each) resolve their generic (e.g. Elem) to the union of a tuple's
  element types, instead of incorrectly indexing into the tuple's own
  positional generics. This also fixes generic defaults (e.g. Tuple's
  C = A | B) being returned as unresolved placeholders instead of
  being resolved against the same context.

- rbs/fills/tuple/tuple.rbs: dropped the literal-indexed overloads for
  #[], #at, and #fetch. Precise positional access (e.g. array[0] ->
  exactly the first element's type) depends on tracking a variable's
  literal value through reassignment, non-literal indices, and
  mutating calls like #unshift - which is exactly what produced the
  wrong answers in castwide#1196. All indexed access now returns the union of
  the tuple's element types instead, which is less precise but never
  wrong.

Verified against all four repro cases from the issue: each now
returns a safe union type instead of an incorrect specific type.

Two pre-existing, unrelated spec failures remain (both
"Hash superclass with untyped value and alias finds superclass method
pin parameter type", expecting Symbol but getting ::Hash::_Key) -
confirmed present on stock master prior to this change, likely from an
RBS version drift in Hash's core signatures.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014cK1bh4FZiYhuVZUt3H9L8
@apiology @claude
Use skip instead of pending for version-sensitive tuple spec
0f7a1ad 
The "understands tuples inherit from regular arrays" spec has
historically flip-flopped between skip and pending, with the note
"Results vary on Ruby versions" - it depends on core RBS signatures
that differ across Ruby/RBS combos. On CI's ruby 3.3/rbs 3.10.0
combo, the resolve_generics fix in this PR happens to make the block
pass, which fails a pending example (RSpec's "FIXED" convention.
Reverting to skip, matching the test's prior state, since pending's
fail-on-unexpected-pass semantics don't fit a genuinely
version-dependent result.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014cK1bh4FZiYhuVZUt3H9L8
EOF
)
@apiology
Track a variable's literal value through reassignment for tuple indexing
1c2220a 
Follow-up to the previous commit, which restored tuple/literal element
inference but deliberately kept Tuple#[]/#at/#fetch union-only because
precise indexing depended on tracking a variable's literal value
through reassignment - exactly what produced the wrong answers in
castwide#1196. This tracks it, so precise indexing can come back safely.

Root cause (confirmed by direct reproduction): `index = 0; index += 1;
array[index]` resolved `index` back to the stale literal `0`, because:

- `index += 1` desugars to a self-referential `index = index + 1`
  (OpasgnNode#process_vasgn_target). Resolving its own RHS re-entered
  variable lookup with the same self-referential assignment as a
  candidate, producing a merged pin whose #identity (a location-based
  string) collided with the identity Chain's recursion guard had
  already pushed for the very same lookup - so the guard mistook a
  legitimate recursive resolution for a cycle and silently dropped it.

Two fixes, both required (verified independently - either alone either
still drops the value or causes unbounded recursion):

- Pin::BaseVariable#return_types_from_node: when resolving one
  assignment's RHS, exclude the pin(s) that assignment itself belongs
  to from the candidates available to resolve references within that
  RHS, keyed on AST node identity (robust for both `a = a` and the
  desugared `index += 1`, unlike a position-based check - the
  desugared self-reference's synthesized location can't be
  distinguished from the assignment's own start).
- Pin::Base#identity: include presence in the fingerprint alongside
  location, since a merged multi-assignment pin and its earliest
  constituent assignment share the same #choose-d location but differ
  in presence - this is what caused the false collision above.

rbs/fills/tuple/tuple.rbs restores the literal-indexed overloads for
#[]/#at/#fetch/#first, with the non-literal catch-all changed from
unsafe (nil/void) to the safe union of all element types.

Doing this also exposed a second, independent bug: Pin::Parameter
#compatible_arg? treats any Integer as "compatible" with a
literal-0-typed parameter (correct for general call-validity, wrong
for overload *selection* - it made the first literal overload always
win over the safe catch-all for any argument merely assignable to it,
including a plain non-literal Integer with no reassignment involved at
all). Source::Chain::Call#literal_param_arg_matches? adds an exact-
match requirement used only for overload selection when the candidate
overload's parameter is a genuine value literal (excluding nil/true/
false, which are singletons, not multi-valued dispatch literals -
needed so ordinary nilable params like String#split's `(Regexp |
string | nil pattern)` aren't affected).

Restoring literal-indexed overloads is a deliberate, accepted
trade-off: it also reopens the castwide#1196 `#unshift` mutation case (a
literal index into a tuple that was mutated after creation can again
return a wrong, not just imprecise, answer), since nothing here or in
the previous commit tracks mutation. That's documented in tuple.rbs's
top comment and covered by a spec that asserts the known-wrong result
so it reads as deliberate rather than an oversight.

Full spec suite green (1649 examples, 0 failures, 48 pending) and
rubocop clean on all changed lines. The project's own self-typecheck
(overcommit's Solargraph hook) reports 12 pre-existing problems
unrelated to this change - confirmed identical on the unmodified base
commit via a throwaway comparison worktree, consistent with local RBS
4.1.0 vs CI's pinned <=4.0.2 (the same class of drift castwide#1224 already
documented for Hash::_Key).
@apiology @claude
Restore review-flagged spec comments and a dropped completions test
f5e1afb 
Addresses PR review feedback on castwide#1223:

- Restore the six "@todo Ideally this would be X - RBS isn't
  sophisticated enough to express this" comments in the tuple specs.
  These predate this PR entirely (they're from castwide/master's
  already-pending versions of these same tests) and document a real,
  separate, still-present limitation: indexing a tuple past its
  declared type arguments falls back to the generic default union
  (e.g. C = A | B) rather than nil, because RBS has no way to express
  "index out of range". My earlier rewrite of these tests dropped the
  comments; the values were already correct (verified unchanged), so
  this only restores the comments.
- Restore 'combines types from tuples in completions', which was
  dropped (not adapted) when tuple.rbs was first reverted to
  union-only, before this session. Updated its first assertion (which
  checked a literal `foo[0]` index) from the union-based expectation
  to the now-precise 'String', and its completion check to no longer
  expect Integer#abs alongside String#upcase there - both follow
  directly from the literal-indexed overloads this PR restores. The
  second assertion (block param completion via #each, still a union)
  is unchanged.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014cK1bh4FZiYhuVZUt3H9L8
@apiology @claude
Add regression specs for two more specious-inference cases
698834a 
Fred raised two additional examples on PR castwide#1223: a scalar
reassignment union that still shows the stale pre-+= literal, and a
plain Array's inferred element type not tracking a later #push.

Both reproduce identically on master, so neither is caused by this
PR. The first is the same general "sequential assignment"
flow-narrowing gap already tracked as pending since PR castwide#863 (see the
pre-existing "replaces type with reassignments" spec). The second is
the same "no mutation tracking" limitation already documented and
accepted for tuples/#unshift in this PR, generalized to plain arrays
via the separate literal-array inference path.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LCTZUQv2ijSU5Cqgw4CU1f
@apiology @claude
Simplify redundant literal items out of variable-pin unions
67bd423 
Pin::BaseVariable#probe unions the return types of every assignment
to a variable in scope. When one assignment's type is a literal
(e.g. `0` from `x = 0`) and another is that literal's own
non-literal base type (e.g. `Integer` from `x += 1`, which already
correctly widens away the literal per the earlier reassignment fix),
the literal adds no information the base type doesn't already carry
- keeping both just reads as if the literal value were still
reachable after a later, wider assignment. Drop such redundant
literal items so `x = 0; x += 1; x` infers as `Integer` instead of
`0, Integer`.

This does not touch the general "sequential assignment" narrowing
gap (unioning across *all* assignments regardless of position,
tracked since PR castwide#863) - it only removes items that were always
redundant given another item already in the same union.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LCTZUQv2ijSU5Cqgw4CU1f
@apiology @claude
Widen tuple.rbs return types for position-shifting mutators
edb944d 
Array's mutating methods that can shift, replace, or reorder a
tuple's existing positions (unshift/prepend, insert, delete_if,
keep_if, reject!, select!/filter!, compact!, flatten!, uniq!, sort!,
sort_by!, reverse!, rotate!, shuffle!, replace, fill, clear,
collect!/map!) were inherited from core Array unmodified, so their
RBS-declared `-> self` return type kept the precise (and, after such
a call, wrong) Tuple type. #push/#<</#concat are deliberately
excluded - appending past the known arity can't invalidate an
already-known position.

Overriding these to return the widened, position-erased
`Array[union-of-all-elements]` type instead of `self` means
capturing the result via reassignment (`array = array.unshift(x)`)
now falls back to the safe union, reusing this PR's existing
reassignment-tracking machinery. It does nothing for the more common
bare-statement form (`array.unshift(x)`, no reassignment) - that's
still the documented castwide#1196 scenario-4 limitation, unaffected and
still covered by its own spec.

Note for reviewers: RBS's own maintainers hit this same wall and
retreated from it for the general core-type case - see
ruby/rbs@aae95840 ("Use monomorphic
versions of in-place modifying methods"), which reverted an earlier
attempt at a non-self, generically-typed #collect! because "it's not
possible to represent side-effects and the receiver type changing."
Our case differs in a way that keeps it sound: we're not
introducing a fresh polymorphic type var the way that attempt did,
just narrowing to a type that's already a strict superset of every
possible resulting position - the tuple's own declared union.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LCTZUQv2ijSU5Cqgw4CU1f
@apiology @claude
Check restarg argument types against the receiver's element type
8e94b1c 
TypeChecker#signature_argument_problems_for used to bail out on any
signature with a restarg parameter, skipping type checking entirely
for the rest of the call. That's why `y = [1]; y.push 'two'` (Fred's
second example on castwide#1223) went unflagged even though `push` expects an
Integer.

Restarg params are now checked argument-by-argument against the
restarg's declared type, resolved against the receiver's actual
generic parameters (e.g. `Integer` for an `Array<Integer>` receiver).
Trailing positional parameters and an implicit kwargs hash appended
to the call's arguments are excluded from the restarg's own checks.

This surfaced a real bug in RbsTranslator#to_parameter_pin: restarg
and kwrestarg parameters had their per-element type discarded and
hardcoded to bare `Array` / `Hash{Symbol => Object}`, so there was
never any element type to check against in the first place. Fixed to
preserve the real per-element type, falling back to the old bare
Array/Hash only when the element type is genuinely untyped (e.g. an
inline `#: (*bar) -> bool` annotation with no declared element type).
Two specs in spec/pin/method_spec.rb asserted the old erased-to-bare
behavior and are updated to reflect the now-tracked type.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LCTZUQv2ijSU5Cqgw4CU1f
@apiology @claude
Fix push-argument spec to not depend on RBS param name
1e9556d 
CI runs the matrix against RBS 3.10.0 through 4.0.2, where
Array#push's restarg parameter is named differently (e.g. `obj`)
than in the RBS version used locally (`objects`). The parameter
name is an incidental detail of the core RBS declaration, not
something this PR's type-checking logic controls, so match on the
substance of the message instead of the exact name.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LCTZUQv2ijSU5Cqgw4CU1f
@apiology
apiology marked this pull request as draft August 2, 2026 14:41
apiology and others added 2 commits August 2, 2026 13:25
@apiology @claude
Address PR review comments on castwide#1223
42f5643 
- Add UniqueType#singleton? predicate for nil/true/false, replacing
  the hardcoded name array in dispatch_literal?
- Merge literal_param_arg_matches? into Pin::Parameter#compatible_arg?
  (its only caller) instead of threading a second, redundant typify
  call through Source::Chain::Call
- Add ComplexType#without_redundant_literals, pulling the
  literal/non-literal union dedup out of Pin::BaseVariable#probe and
  into the type hierarchy
- rbs_translator.rb: fix @param type [RBS::Types::Bases::Base]
  annotations that were actually too narrow (the real RBS type is
  RBS::Types::t, a union most RBS type classes do not inherit
  Bases::Base from). Removes 3 of the sg-ignore comments entirely.
  The remaining case/when-narrowing sg-ignores in type_to_tag now
  reference castwide/solargraph issue 1241, filed to track that the
  type checker does not narrow a case subject's type inside each
  branch
- shell.rb: revert the unrelated cache_core rebuild-condition
  one-liner, out of scope for this PR
- type_checker.rb: clarify that receiver_type generic resolution is
  currently restarg-specific, not yet generalized to fixed-arity
  params
- chain_spec.rb: assert the non-literal (simplify_literals) type of
  true is Boolean, alongside the existing literal-type assertion

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019zMih8CMx6rXoSkYehxFH3
@apiology
Infer element types for array literals and NilClass for nil literals
e6f894a 
Array literal chains (`[1, 2, 3]`) previously resolved to a bare
`Array` with no element type. Infer each child's type and attach it
as the Array's generic parameter, falling back to plain `Array` when
empty or undefined.

`simplify_literals` left the `nil` pseudo-type tag as-is instead of
converting it to `NilClass` like other literals are converted to their
class names. This also surfaced two previously-pending specs
(NilClass/nil conformance, and passing a NilClass value to a `nil`
parameter) that now pass.
@apiology
apiology force-pushed the fix-array-nil-literal-inference branch from 63837f2 to e6f894a Compare August 2, 2026 19:17
@apiology apiology mentioned this pull request Aug 2, 2026
Draft
@apiology

apiology commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Closing in favor of apiology#40, which is the same branch rebased onto #1223 (apiology-1196-literal-inference) and opened as a PR against that branch directly. GitHub does not allow setting a PRs base to a branch that only exists in the head fork, so this could not be done as a stacked PR here upstream — the fork-internal PR shows just the incremental diff instead of a diff duplicating #1223s changes.

Will re-target the fork PR to master and reopen upstream once #1223 merges.

🤖 Generated with Claude Code

@apiology apiology closed this Aug 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@apiology