Fix nil-safety, return-value, and dead-code bugs across parser/type-checker/RBS layers by apiology · Pull Request #1245 · castwide/solargraph

apiology · 2026-08-02T19:11:14Z

Summary

Nil-guard bug fixes, return-value corrections, dead-code removal, and Sorbet-narrowing refactors, split out of the larger typecheck-annotation branch (#1240) so this diff is pure behavior with no comment-only noise.

Nil-guard fixes: Pin::Method#return_type_from_inline_rbs/#signatures_from_inline_rbs (crashed on unparseable inline RBS), TypeChecker#kwarg_problems_for (crashed on out-of-range param index), NodeMethods#find_recipient_node_by_text (crashed on nil name matches).
Return-value fixes (nil → sensible default): ApiMap::Store#get_path_pins/#fqns_pins, Library#references_from/#next_map, TypeChecker#arity_problems_for, YardMap::Mapper#macros_for_method_object.
Strict-boolean coercions: Host#client_supports_progress?/#prepare_rename?, NodeMethods#splatted_hash?.
Dead code removal: RbsMap::Conversions had two definitions each of build_type/parts_of_function; removed the shadowed, unreachable copies (verified byte-identical to the live ones).
Refactors: local-variable extraction replacing repeated node.children[N] calls in several node processors — behavior-preserving.

Test plan

bundle exec rspec: 1618 examples, 0 failures
Confirmed the dead-code removal is safe by diffing base vs. head file content for the surviving methods

Based on master, which doesn't carry #1240's annotations yet — rake typecheck (strong) will show pre-existing master drift unrelated to this diff.

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com

https://claude.ai/code/session_01LYQc4tRAkDEfp6vZsvHaXr

Split out of the strong-level typechecking cleanup in castwide#1240: nil-guard bug fixes, nil/false-vs-[] return-value corrections, dead duplicate method removals (rbs_map/conversions.rb had two definitions each of build_type and parts_of_function; only the second was ever live), and Sorbet-narrowing local-variable extractions. castwide#1240 stays annotation and CI-gate only; these are the actual behavior-touching hunks that were mixed into that branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LYQc4tRAkDEfp6vZsvHaXr

The 28 non-comment hunks that touched actual Ruby behavior (nil-guard fixes, return-value corrections, dead duplicate method removal, Sorbet-narrowing refactors) now live in castwide#1245. This branch reverts those hunks back to their pre-cleanup form and marks each resulting strong-typecheck gap with an @sg-ignore comment referencing castwide#1245, so this PR stays annotation- and CI-gate-only as requested. Verified via solargraph typecheck --level strong: normalized diff against the pre-revert state of this branch shows zero net-new problems introduced by the revert (all reverted spots are covered by the new ignores; remaining diffs are pre-existing version-drift noise already present on this branch). Full test suite: 1618 examples, 0 failures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LYQc4tRAkDEfp6vZsvHaXr

The comment block above require_all_unique_types_match_expected? was a stale, hand-maintained tally of @sg-ignore reasons and counts. Recomputed from a full grep over lib/**/*.rb: 745 total (was ~373), reflecting both growth in the underlying campaign (e.g. "Need to add nil check here" 281 -> 465) and the new castwide#1245-deferred entries from this PR's split (29 nil-check, 13 downcast, 6 return-value, 3 dead-code-removal). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LYQc4tRAkDEfp6vZsvHaXr

PR castwide#1201 disabled tuple/literal element-type inference wholesale to fix specious-inference reports (castwide#1196). That's the root cause of a broad swath of downstream nil-check/downcast/overload-resolution gaps across the codebase, not just tuple indexing. Open PR castwide#1223 restores the capability properly (with real reassignment tracking) rather than leaving it off. Determined the exact set empirically: test-merged castwide#1223's branch onto this one and diffed `solargraph typecheck --level strong` output before/after (line numbers stripped to avoid false positives from line-count shifts). Every one of the 79 lines flagged "Unneeded @sg-ignore comment" in that diff had its comment rewritten to `# @sg-ignore https://github.com/castwide/solargraph/pull/1223`, replacing whatever specific reason (or blank comment) was there before - including one of this branch's own castwide#1245-deferred entries (pin/block.rb), which turns out to be downstream of the same root cause. Updated the @sg-ignore count doc in TypeChecker::Rules to add this as a third bucket and adjust the other two accordingly. Verified: full test suite (1618 examples, 0 failures) and `solargraph typecheck --level strong` both unchanged from before this commit (comment-only diff, confirmed via normalized before/after output comparison). Rubocop offenses on touched files identical before and after (36, all pre-existing). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LYQc4tRAkDEfp6vZsvHaXr

Collapse the four different castwide#1245-deferred reason strings (nil-check, downcast, return-value, dead-code-removal) down to one consistent `# @sg-ignore https://github.com/castwide/solargraph/pull/1245`, matching the castwide#1223 reference style and the repo's existing convention of pointing an ignore straight at the PR that resolves it rather than re-describing the reason inline. Reverted the count doc in TypeChecker::Rules to the original flat two-bucket format (no prose commentary) and regenerated the counts using the actual ~/bin/solargraph-errors-group tool per the documented recipe in ~/Dropbox/Shared/solargraph.md, rather than an ad-hoc filter. The castwide#1223 and castwide#1245 buckets are now single flat count lines in "pending code fixes," not broken out by what they used to be. Verified: full test suite (1618 examples, 0 failures), rubocop clean, and `solargraph typecheck --level strong` stable at 72 problems (unchanged from before this commit). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LYQc4tRAkDEfp6vZsvHaXr

The 28 "flow sensitive typing should support case/when" / "flow based typing needs to understand case when class pattern" ignores in rbs_translator.rb and rbs_map/conversions.rb all describe the same gap: the type checker doesn't narrow a case/when subject's type inside each branch. Filed and confirmed as castwide#1241 - rewrote all 28 to point there instead of restating the reason inline, matching the castwide#1223/castwide#1245 convention. Checked for issue coverage on the other "flow sensitive typing could handle" categories too (attrs, redefinition, ||= on lvars, .class == .class, boolish support, etc.) - no clear existing issue found for those via search, so left as-is. Also checked "Need to handle duck-typed method calls on union types": issues castwide#453/castwide#511 looked like a match at first glance but describe a different mechanism (YARD `@return [#call]` duck-type tags, not union-type method resolution) so left unlinked rather than mis-attribute it. Regenerated the count doc using solargraph-errors-group per the documented recipe. Verified via a clean stash/restore comparison (not just before/after diffing, since consecutive typecheck runs have shown transient non-determinism this session) that this comment-only change introduces zero new problems: full test suite 1618 examples/0 failures, rubocop clean, typecheck stable at 72. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LYQc4tRAkDEfp6vZsvHaXr

@todo

Three of the largest remaining "flow sensitive typing could handle" categories described coherent, reproducible gaps with no existing tracking issue (searched castwide/solargraph issues first, no match): - "flow sensitive typing needs to handle attrs" (30): a nil-guard on an attr_reader-style call doesn't narrow a later repeated call to the same accessor, since each call is treated as independent rather than as if it were a local variable. Filed as castwide#1249. - "flow sensitive typing should be able to handle redefinition" (20): reassigning a variable to a value of a different (non-literal) type doesn't update its tracked type - distinct from castwide#1196/castwide#1223, which cover literal-value tracking through reassignment specifically for array/tuple indexing. Filed as castwide#1250. - "flow sensitive typing needs to narrow down type with an if is_a? check" (12): narrower-scoped than castwide#1241 (case/when) - covers is_a? checks combined with && and elsif branches whose body doesn't see the narrowing established by its own condition. Filed as castwide#1251. Rewrote all matching @sg-ignore comments to point at the new issues, matching the castwide#1223/castwide#1245/castwide#1241 convention. Left the 4 sg-ignore notes inside the disabled block in source/chain/literal.rb untouched (not live directives) and the standalone @todo in shell.rb (different tag, outside this doc's scope). Regenerated the count doc via solargraph-errors-group. Verified: full test suite (1618 examples, 0 failures), rubocop clean on all touched files, and solargraph typecheck --level strong stable at 72 (checked against a pre-edit baseline captured via stash, given transient non-determinism observed between consecutive runs this session). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LYQc4tRAkDEfp6vZsvHaXr

CI's strong-typecheck job flagged one problem this PR introduced: reverting the found_versions extraction (deferred to castwide#1245) earlier this session dropped the @sg-ignore Need a downcast here comment that covered it, since CI's fresh gem install resolves Gem::Version differently than my local (stale) gem cache did. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LYQc4tRAkDEfp6vZsvHaXr

apiology · 2026-08-03T00:53:15Z

      def rooted_name type_name
        name = type_name.to_s
-        RBS_TO_CLASS.fetch(name, name)
+        RBS_TO_CLASS[name] || name


Let's fix the signature rather than avoid the function

apiology · 2026-08-11T16:11:54Z

+              target_type = target.type
              Solargraph.assert_or_log(:opasgn_unknown_target,
-                                       "Unexpected op_asgn target type: #{target.type}")
+                                       "Unexpected op_asgn target type: #{target_type}")


This shouldn't be needed - would rather revert it and add an sg-ignore describing why it doesn't work

apiology · 2026-08-11T16:12:17Z

-                                       NodeChainer.chain(n.children[1], @filename, n)])
+            or_lhs = NodeChainer.chain(n.children[0], @filename)
+            or_rhs = NodeChainer.chain(n.children[1], @filename, n)
+            result.push Chain::Or.new([or_lhs, or_rhs])


Likewise - why was this needed?

apiology · 2026-08-11T16:15:05Z

-          ComplexType::UniqueType.new(base, [], params.reject(&:undefined?), rooted: type_name.absolute?,
-                                                                             parameters_type: :list)
-        end
+        RBS_TO_CLASS[ns] || ns


.fetch should have worked - prefer properly tagged sg-ignore

apiology · 2026-08-11T16:15:53Z

        def splatted_hash? node
-          Parser.is_ast_node?(node.children[0]) && node.children[0].type == :kwsplat
+          child = node.children[0]
+          !!(child.is_a?(::Parser::AST::Node) && child.type == :kwsplat)


Let's sg-ignore this and attribute it to the lack of boolish

apiology · 2026-08-11T16:20:14Z


      def prepare_rename?
-        client_capabilities['rename'] && client_capabilities['rename']['prepareSupport']
+        !!(client_capabilities['rename'] && client_capabilities['rename']['prepareSupport'])


Drop the !!s added in this PR and mark them as sg-ignore on the boolish issue

apiology · 2026-08-11T19:31:29Z

          raise InvalidRubocopVersionError,
                "could not find '#{e.name}' (#{e.requirement}) - " \
-                "did find: [#{specs.map { |s| s.version.version }.join(', ')}]"
+                "did find: [#{found_versions}]"


Shouldn't be needed, please revert and add an sg-ignore if need be

apiology mentioned this pull request Aug 2, 2026

Fix and re-enable strong-level typechecking in CI #1240

Open

2 tasks

apiology changed the title ~~Extract behavior changes and refactors from strong-typecheck cleanup~~ Fix nil-safety, return-value, and dead-code bugs across parser/type-checker/RBS layers Aug 2, 2026

apiology commented Aug 3, 2026

View reviewed changes

apiology commented Aug 11, 2026

View reviewed changes

Navigation Menu

Uh oh!

Fix nil-safety, return-value, and dead-code bugs across parser/type-checker/RBS layers - #1245

Fix nil-safety, return-value, and dead-code bugs across parser/type-checker/RBS layers#1245
apiology wants to merge 1 commit into
castwide:masterfrom
apiology:extract-1240-code-changes

apiology commented Aug 2, 2026 •

edited

Loading

Uh oh!

apiology Aug 3, 2026

Uh oh!

apiology Aug 11, 2026

Uh oh!

apiology Aug 11, 2026

Uh oh!

apiology Aug 11, 2026

Uh oh!

apiology Aug 11, 2026

Uh oh!

apiology Aug 11, 2026

Uh oh!

apiology Aug 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

apiology commented Aug 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

apiology Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

apiology Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

apiology Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

apiology Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

apiology Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

apiology Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

apiology Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

apiology commented Aug 2, 2026 •

edited

Loading