Fix raise/fail nil guards, root-scoped is_a?, case/when, ||=, and namespace-scope narrowing in flow-sensitive typing - #1259
Conversation
…typing always_leaves_compound_statement? checked clause_node.type against :raise, but the parser gem never produces a :raise node type -- a raise call parses as a plain :send node, same as any other method call. As a result, a raise-based nil guard never narrowed the guarded variable type for the rest of the method, unlike an equivalent return-based guard, which uses the real :return node type. Fixes castwide#1254
type_name in FlowSensitiveTyping did not recognize the :cbase node that the parser gem emits for a leading :: on a constant reference (::Foo parses as s(:const, s(:cbase), :Foo)). Since :cbase is not a :const node, the recursive lookup fell through and type_name returned nil for any fully-qualified constant, silently disabling is_a?-based narrowing whenever the checked class was referenced with a leading :: -- including the guard-clause (&&) and elsif-branch shapes reported in the issue, which just happened to use fully-qualified names. Fixes castwide#1251
case/when had no flow-sensitive-typing support at all -- there was no NodeProcessor registered for :case nodes, so a case subject kept its full original (often union) static type inside every branch, even though each when clause has already established which member type it is. This meant methods only present on some members of the union needed an explanatory @sg-ignore in every branch, for what is normal, idiomatic Ruby type-dispatch code. Add a CaseNode processor that narrows the subject (a local or instance variable) to the union of the constant classes listed in each when clause, scoped to that branch body only. Multi-value when clauses (when A, B) narrow to a union; when clauses with a non-constant value (a splat, range, regexp, dynamic expression, etc.) are left unnarrowed rather than guessed at. Fixes castwide#1241
Add two pending specs demonstrating that a plain x ||= value assignment does not narrow x to eliminate nil for the rest of the method, unlike return/raise-based nil guards. The existing ||= to refine types using nil checks spec (nearby) only passes because its RHS contains a nested return-if-nil check, which narrows x for the rest of the enclosing method via the pre-existing return-if-nil mechanism -- independent of the ||= assignment itself. Verified by testing several plain-||= variants (local var reassigned to a class instance, keyword param reassigned to a literal, with and without a wrapping begin/end) with no nested nil check: all still report the pre-assignment nilable union type after the ||=, confirming there is no OR-union-aware narrowing for ||= on lvars at all. Referenced in lib/solargraph/type_checker/rules.rb todo census as "flow sensitive typing needs better handling of ||= on lvars" (6 occurrences) and matches concrete @sg-ignore markers in lib/solargraph/type_checker.rb, lib/solargraph/bench.rb, lib/solargraph/workspace/gemspecs.rb, lib/solargraph/complex_type/unique_type.rb, and lib/solargraph/api_map/constants.rb. No fix included -- reproduction only.
x ||= value only actually assigns when x is falsy -- nil or false -- so if x was already truthy, it keeps whatever non-nil type it already had. Prior to this, OrasgnNode rewrote x ||= value as a plain x = value, which discarded x prior type entirely and typed it as just the RHS value type -- but this pin never actually shadowed the original declared type at lookup time, since plain reassignment pins get unioned together rather than overriding each other, a more general limitation shared with castwide#1250. The variable stayed nilable after the ||= no matter what. Instead of trying to build a new assignment pin, add FlowSensitiveTyping#process_or_asgn, which reuses the same downcast-pin machinery that already powers is_a?/nil? narrowing and is known to correctly override the base pin, unlike plain reassignment: it excludes nil from the pre-existing pin type, scoped to the rest of the enclosing closure. This is a conservative, scoped fix -- it does not attempt to union in the RHS value type when that type differs from the variable prior non-nil type, since that would need a proper union-typed downcast primitive and is really the same open question as castwide#1250 for the ||= case -- but it covers the overwhelmingly common lazy-init pattern, x ||= SomeDefault.new, where the default matches x declared non-nil type, which accounts for the concrete real-world @sg-ignore markers this was filed against. Only handles :lvasgn and :ivasgn left-hand sides; other assignment targets such as hash/array element writers or attr writers keep the old behavior. Fixes the reproduction added in the previous commit.
The four @sg-ignore comments added in the previous commit all copied the file existing "Need to add nil check here" phrase without verifying it fit. Checked each against the actual typecheck message with the ignore removed: - Range.from_node(or_asgn_node).start really was a missing nil check (Range.from_node can genuinely return nil) and is now fixed for real with an explicit guard instead of suppressed. - The other three (lhs.type, lhs.children[0].to_s, variable_name.empty?) are not about nil at all -- their error messages have no nil in them. Adding an actual nil check on lhs confirmed this: the errors were unchanged. The real cause is Parser::AST::Node#children being declared to return a bare Array, losing its element type, a pre-existing gap elsewhere in this same file. Relabeled to say that instead.
The presence-computation for or_asgn narrowing was suppressed rather than guarded, matching the pre-existing LvasgnNode identical pattern -- but that just means LvasgnNode has the same latent gap, not that suppressing here was the right call. region.closure.location is genuinely declared [Location, nil] (Pin::Base#location), so this was a real, live nil-deref risk if ever hit. Guard it explicitly and skip only the flow-sensitive narrowing step when it fires, rather than crashing or suppressing the check. No sg-ignore comments remain in this PR diff.
…gration branch 2026-08-04
|
Comment drafted by Claude (Anthropic), posted on behalf of @apiology while reviewing this branch. Found while running this branch against a real codebase: pre-existing malformed
|
|
Comment drafted by Claude (Anthropic), posted on behalf of @apiology while reviewing this branch. Separately: no error-handling boundary around per-call type inference in
|
…or boundary Two issues found while reviewing PR castwide#1259: - ComplexType::TypeMethods#generate_substring_from's Hash fallback branch unconditionally emitted the 2-parameter `<K, V>` notation, even when key_types/subtypes held more than one type (from a comma-separated union on either side of a Hash{} literal, or from generics substitution rebuilding a Hash-named type with parameters_type out of sync with its key_types/subtypes). Reparsing the resulting 3+-parameter string raised Solargraph::ComplexTypeError, and since ApiMap#get_method_stack reparses a receiver type's rooted_tag unguarded, this could crash method resolution for that receiver. Now falls back to the `{K => V}` notation, which reparses correctly regardless of how many types are on either side. - TypeChecker#call_problems ran chain.infer (and argument_problems_for) for every call node in a file with no rescue around it, so any exception raised while inferring a single call's type aborted TypeChecker#problems entirely, silently losing every diagnostic for the rest of the file. Each call site's inference is now wrapped in a rescue that logs the error and reports it as a Problem scoped to that call, so one bad call site degrades to one reported problem instead of killing the whole run. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H7Hb7H69hfyzgHJqFiEjgQ
#53 added a required closure parameter to FlowSensitiveTyping#initialize and updated every caller it knew about - but its branch is based on castwide#1258, not castwide#1259 (already merged into this integration branch separately), so it never saw case_node.rb (added by castwide#1259) or the already-existing call in orasgn_node.rb that castwide#1259 also touches. Both call sites already had region.closure in scope; added it as the 5th argument, matching every other already-updated caller (and_node.rb, if_node.rb, or_node.rb, while_node.rb). Verified: spec/parser/flow_sensitive_typing_spec.rb (65 examples), spec/parser (323 examples), and spec/source_map/clip_spec.rb all pass locally with 0 failures.
|
Claude: Tested this fix (58db074, "Fixes #1254") against apiology/checkoff's real # typed: true
# frozen_string_literal: true
class Example
# @param s [String, nil]
# @return [String]
def single_statement_raise(s)
if s.nil?
raise 'not found'
end
s
end
# @param s [String, nil]
# @return [String]
def multi_statement_raise(s)
if s.nil?
valid = %w[a b c]
raise "not found. Valid: #{valid.inspect}"
end
s
end
end
# solargraph typecheck --level strong reports only one error, on
# multi_statement_raise -- single_statement_raise (identical shape,
# minus one extra statement before the raise) narrows correctly:
#
# Declared return type ::String does not match inferred type
# ::String, nil for Example#multi_statement_raiseRoot cause: return false unless clause_node.type == :sendA multi-statement branch body parses as a |
|
Claude: Separately: # typed: true
# frozen_string_literal: true
class Example
# @param argv [Array<String>]
# @return [String]
def first_arg(argv)
argv[0] || raise('missing first argument')
end
end
# solargraph typecheck --level strong:
# Example#first_arg return type could not be inferred
|
always_leaves_compound_statement? only recognized a raise/fail call when it was the clause's sole statement (a :send node). A branch with more than one statement -- e.g. building an error message before raising -- parses as a :begin node, which fell through to the :send check and returned false, so the guard never narrowed the checked variable for the rest of the method. Move the check into ParserGem::NodeMethods so it can be shared with the chain-inference fix in the next commit, and recurse into a :begin clause's last child before applying the raise/fail shape check. Addresses a PR review comment on castwide#1259.
Chain::Or#resolve inferred the result of an 'or' expression as the
union of both sides' types. When the right-hand side is a call that
never returns control (raise/fail), its inferred type is 'undefined'
-- and ComplexType collapses any union containing an undefined item
down to just 'undefined', so argv[0] || raise('...') inferred as
undefined instead of argv[0]'s non-nil element type, and TypeChecker
reported the enclosing method's return type as uninferrable.
Since a raise/fail rhs never contributes a value, reaching code past
the 'or' expression implies the lhs was truthy, so the result type is
just the lhs type with nil excluded.
Addresses a PR review comment on castwide#1259.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NF1ZWsAo2LdLTQP1jvbmFi
Both were classified as tool-limitation:type-narrowing / issue #1254. Verified empirically that castwide/solargraph#1259 (already in our tracked branch) doesn't actually fix either shape: - sections.rb's raise-guard has an extra statement before the raise; #1259 only handles a branch that's exactly one raise/fail statement. - task_selectors.rb's `ARGV[n] || raise(...)` idiom goes through an entirely different code path (an :or node, not an :if node) that #1259 never touches at all. Reclassified to a dedicated subcategory pointing at the two follow-up comments filed on #1259, rather than the original #1254 issue, since that issue is (partially) fixed and these are what's left.
|
Comment drafted by Claude (Anthropic), posted on behalf of @apiology while reviewing this branch. Pushed fixes for both gaps identified above:
Both repro cases from the comments above now type-check clean, and the full suite is green (1633 examples, 0 failures, 65 pre-existing pending — matching this PR's original baseline). |
…only inference Pulls in two new upstream commits on top of the already-merged castwide#1259 base: - Recognize multi-statement raise/fail branches in flow-sensitive typing: always_leaves_compound_statement? now recurses into :begin nodes' last child, so a clause like `msg = "bad"; raise msg` is still recognized as unconditionally leaving, not just a single bare `raise`/`fail` send. - Infer only the lhs type for `x || raise(...)` and `x ||= raise(...)`: new logic in node_chainer.rb and Chain::Or#resolve treats a never-returning rhs the same way as the flow-sensitive-typing narrowing already does, so the combined type is just the lhs's non-nil type instead of a union with the (unreachable) rhs. Conflict in lib/solargraph/parser/flow_sensitive_typing.rb: the upstream commits move always_leaves_compound_statement? out of FlowSensitiveTyping and into the shared Solargraph::Parser::ParserGem::NodeMethods module (aliased as Solargraph::Parser::NodeMethods, already included by FlowSensitiveTyping), so it can be reused from node_chainer.rb, and extend it with :begin-node support. Removed the now-duplicate local copy in favor of the shared one; kept this branch's own :closure attr_reader addition (from #53, needed by every other FlowSensitiveTyping.new caller already updated on this branch). Also dropped two now-superfluous `@sg-ignore Need to add nil check here` comments in node_chainer.rb (above the always_leaves_compound_statement?(or_asgn_rhs_node) and always_leaves_compound_statement?(or_rhs_node) calls): on the PR's own source branch, NodeChainer.chain's node arguments are inferred as `Array, nil` throughout that branch (a broad, unrelated pre-existing mismatch visible across dozens of lines when typechecked standalone), so the @sg-ignore was suppressing a real mismatch there. On this integration branch those same node variables are already correctly typed `Parser::AST::Node, nil` (fixed by an earlier-merged PR), and always_leaves_compound_statement?'s own param is already declared nilable, so passing them needs no suppression - the local Solargraph typecheck hook correctly flagged both comments as unneeded. Verified: spec/parser/flow_sensitive_typing_spec.rb, spec/parser/node_methods_spec.rb, spec/source/chain/or_spec.rb, spec/parser/node_chainer_spec.rb (134 examples, 0 failures, 4 pending), and a broader safety net — spec/parser, spec/source, spec/source_map/clip_spec.rb (475 examples, 0 failures, 14 pending) — all passing locally.
Both were tagged issue-1254 but neither reproduces its actual mechanism (raise/return-based nil guard on a Hash not narrowed). PR castwide/solargraph#1259's fix commit is already an ancestor of our pinned fork revision, and strip-and-observe confirms both fail for unrelated reasons: - custom_field_param_converter.rb:71 -- Unresolved call to new on a Class<T>-typed variable, same root cause as test/unit/class_test.rb's create_object -- retagged generic-class-new-dispatch. - date_param_converter.rb:142 -- Array#[] stays nilable despite a preceding length == 1 guard -- a length-then-index-guard shape, distinct from any numbered issue on file -- retagged type-narrowing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D1KB8X6cDo6QtyYv1RwJzd
Re-enables `solargraph typecheck --level strong` as an enforced CI gate - removes `continue-on-error: true` from .github/workflows/typecheck.yml. Most of the diff is @sg-ignore comments documenting type gaps strong mode can't resolve on its own (flow-sensitive-typing limits, the nil-vs-NilClass representation mismatch, guard-then-fetch patterns that don't narrow, RBS overload/ type-alias gaps). A handful of real fixes are included (missing/wrong @return/@PARAM tags). This branch's own lib/ tree has diverged substantially from castwide#1240's target (39+ merged PRs' worth of independent work), so merging this required a full annotation sweep on top of the mechanical merge to actually make the newly-hard CI gate pass - see below. Conflicts (20 files) fell into two categories: 1. Genuine competing logic, where incoming's branch (based directly on castwide/master) predated work already merged into this branch. Kept this branch's side throughout: doc_map.rb's entire in-memory pin-cache architecture (superseded by the PinCache instance-based rewrite from castwide#1252, same pattern already identified during the castwide#1239 investigation earlier this session), rbs_translator.rb's compound-type-as-ComplexType-graph architecture (from castwide#1281, predates incoming's tag-string type_to_tag reintroduction), flow-sensitive-typing/node_chainer additions (rhs_never_returns tracking from castwide#1259), base_variable.rb's definite/narrowed_return_type naming (from castwide#1282), node_methods.rb's ENSURE handling (from castwide#1285), chain.rb/call.rb's receiver_path threading, and workspace.rb/pin_cache.rb duplicate method definitions incoming reintroduced that already exist elsewhere in this branch's own `class << self` blocks. 2. Pure annotation differences (add/adjust an @sg-ignore comment) where kept whichever side matched this branch's actual code structure. 2. Annotation sweep: after resolving conflicts, this branch's strong typecheck still reported 289 problems (down from 547 pre-merge, since castwide#1240's own annotations covered about half). 194 were "Unneeded @sg-ignore comment" (incoming's own ignore comments, correct on castwide#1240's target tree, landing on lines this branch's independent fixes already resolve) - removed mechanically by scanning upward from each flagged line for its comment. The remaining 95 were genuine new gaps on this branch's own code paths (mostly not exercised by castwide#1240's target tree at all) - added one @sg-ignore per flagged line, matching the established message-as-comment convention used throughout this codebase (@sg-ignore matches by string presence, not exact message, so one comment per line suffices even where a line has multiple flagged sub-expressions). Spot-checked the ones that looked most like real bugs rather than static-analysis gaps (BigDecimal-typed values in Integer-declared contexts, an Array#push type mismatch) against already-documented, already-tracked false-positive patterns in this codebase (the known BigDecimal-contamination artifact from earlier PR work, and a known is_a?-narrowing gap) - none were new bugs. Also fixed one new Style/Next rubocop offense the sweep introduced. Verified: full local `bundle exec rspec` (1826 examples, 0 failures, 51 pending - the only local-environment-dependent example, 'ignores undefined method calls from external sources', a pre-existing order-dependent kramdown-parser-gfm gem-cache flake already confirmed unrelated to this session's work, passed in this run), `solargraph typecheck --level strong` (0 problems, confirming the now-hard-gated CI job will pass), and `rubocop lib/` (13 offenses, matching this branch's pre-existing baseline exactly - none newly introduced by this merge).
The existing raise/||= specs went through TypeChecker#problems, which never actually invokes Chain::Or#resolve for this expression, so the rhs_never_returns branch (stripping nil from the lhs type) had no coverage. Added a clip.infer assertion, matching the pattern of the other specs in this file, which does exercise it.
A `return unless x.is_a?(Hash)`-style guard failed to narrow the type of `x` for the rest of a top-level script file, or for the rest of a class/module body, even though the identical guard narrows correctly inside a method or block. Solargraph reported Unresolved call to [] on Object, nil for the resulting code. FlowSensitiveTyping#process_if extends narrowing past a guard by reading enclosing_compound_statement_pin.node to find where the enclosing compound statement ends. NodeProcessor.process creates one root Pin::Namespace per file, and NamespaceNode creates one per class/module body, but neither passed node: to Pin::Namespace, so that pin's node was always nil and the narrowing was silently skipped. Pin::Method and Pin::Block already pass node: for exactly this reason, which is why narrowing worked inside them. Pass node: through at both sites, matching the existing Method/Block pattern. Add specs covering a guard at true top-level (no enclosing def) and inside a class body (no enclosing def), the two scopes this enables.
…il-guard-narrowing
Drop a duplicate node: rationale comment, shrink two oversized docstring/inline comments to budget, and replace two placeholder "Need to add nil check here" markers in the :or_asgn branch with a real nil guard plus the correctly-labeled sg-ignore it actually needs. Also relabel Chain::Or#equality_fields' sg-ignore: the copied "Not enough arguments to Module#protected" text named the wrong error - the real (and still current) finding is that its return type cannot be inferred.
Chain::Or#resolve's rhs_never_returns branch falls back to an undefined type when the lhs has no inferred type at all, which is distinct from the lhs inferring to ComplexType::UNDEFINED. No path through the parser can produce that: NodeChainer always gives Or two real Chain links, and Chain#infer never returns nil, so the guard had no coverage. Exercise it directly with an empty links array, which hits the same types.first.nil? check with real collaborators and no test doubles.
Two cached rerere resolutions for the castwide#1259 merge picked one side where both sides were needed, leaving three typecheck problems. node_chainer.rb: the ignore marker covering the left-hand-side call and the nil guard covering the right-hand side address different lines; the cached resolution dropped the marker in favour of the guard. Keep both, and drop the incoming marker for raise-if flow-sensitive typing, which this branch already narrows through. or.rb: the incoming ignore marker on equality_fields suppresses nothing here.
Index :or_asgn and :or children with fetch. Every form of ||= and || the parser builds has exactly two children, so the right-hand side is never nil: the raise guard was unreachable, and it did not narrow for the type checker either, which is why its line still needed an @sg-ignore. fetch asserts the same invariant and drops the guard, that suppression, and three "Need to add nil check here" markers on new code, one of which the type checker already called unneeded. On Chain::Or#equality_fields, replace "return type could not be inferred" with the catalogued rules.rb string. Both problems there have one cause, the literal array resolving to Chain::Array, so + does not resolve and the return type cannot be inferred. Drop the redefinition marker above without_nil, also reported as unneeded. Move the note about passing node: to Pin::Namespace and state what the parameter is for rather than what breaks without it. Trim always_leaves_compound_statement? to the four-line comment budget. Adjust the rules.rb counts by this delta. They were already stale -- lib holds 245 "Need to add nil check here" markers against 281 listed -- so the new numbers carry the same error, less what is removed here. Strong typecheck drops from 528 problems to 525 with none added. Specs unchanged: 1665 examples, 0 failures, 67 pending.
Fixes raise/fail-based nil guards, root-scoped (::-prefixed) is_a? narrowing, case/when subject narrowing, ||= on lvars/ivars, and is_a?/nil? guard narrowing at file/class-body scope. Conflict resolution: - node_chainer.rb: kept 1259's Array#fetch form for or_asgn children, which drops the two @sg-ignore comments HEAD needed for the equivalent []-based access. - source/chain/or.rb: dropped a literal-array @sg-ignore that 1259 carries from an older base; this branch already fixed that gap (rules.rb shows zero live sites for that reason) - confirmed by the typecheck run below. - type_checker/rules.rb: kept HEAD's counts. 1259's catalogue was computed against an older base and is missing this branch's already landed pull/1223, pull/1245, issues/1249-1251 tracking entirely; reconciling the catalogue with the post-merge count is a separate, explicit follow-up.
Fixes raise/fail-based nil guards, root-scoped (::-prefixed) is_a? narrowing, case/when subject narrowing, ||= on lvars/ivars, and is_a?/nil? guard narrowing at file/class-body scope. Conflict resolution: - node_chainer.rb: kept this branch's existing forms for the or_asgn and :or node-chaining branches (a raise-based nil guard plus one @sg-ignore for or_asgn, two @sg-ignores for :or) rather than 1259's Array#fetch refactor. fetch leaks an unresolved generic<T> type instead of Parser::AST::Node on this branch's Solargraph, confirmed by removing the ignores and re-running the self-hosted typecheck. - source/chain/or.rb: dropped a literal-array @sg-ignore that 1259 carries from an older base; this branch's self-hosted typecheck reports 0 problems on that line without it. - type_checker/rules.rb: kept this branch's counts. 1259's catalogue was computed against an older base and is missing this branch's already-landed pull/1223, pull/1245, and issues/1249-1251 tracking entirely.
Summary
Five related flow-sensitive-typing gaps, fixed together since they touch the same file and spec.
raise/fail-based nil guards (fixes #1254)
raise/failas a plain:sendnode, not a:raisenode type, soalways_leaves_compound_statement?never recognized a raise-based guard and it failed to narrow, unlike an equivalentreturn-based guard. Fix recognizes the:sendshape (no receiver, method name:raise/:fail) alongside the existing keyword-based node types.root-scoped (::-prefixed) constants in is_a? narrowing (fixes #1251)
type_nameinFlowSensitiveTypingdidn't recognize the:cbasenode the parser gem emits for a leading::on a constant (::Fooparses ass(:const, s(:cbase), :Foo)), so any fully-qualified class name silently disabledis_a?-based narrowing. Fix recognizes:cbaseand renders it as a leading::, recursing normally otherwise.case/when subject narrowing (fixes #1241)
:casenodes, so a case subject kept its full (often union) static type in every branch.CaseNodeprocessor andFlowSensitiveTyping#process_case, narrowing the subject to the union of eachwhenclause's constant classes, scoped to that branch body only.x ||= value on lvars/ivars
x ||= valueonly assigns whenxis falsy, so a prior non-nil value should survive -- butOrasgnNoderewrote it as a plainx = value, and plain-reassignment pins union together rather than override, so the narrowed type was lost.FlowSensitiveTyping#process_or_asgn, reusing the same downcast-pin machinery that already powersis_a?/nil?narrowing to excludenilfrom the prior type, scoped to the rest of the enclosing closure; no@sg-ignorecomments remain anywhere in this fix.lvasgn/ivasgntargets -- doesn't union in the RHS type when it differs from the variable's prior non-nil type.is_a?/nil? guard narrowing at file and class-body scope
FlowSensitiveTyping#process_ifextends narrowing past a guard viaenclosing_compound_statement_pin.node, but the rootPin::Namespace(NodeProcessor.process, one per file) and the per-class/modulePin::Namespace(NamespaceNode) never passednode:, so that pin's node was alwaysniland narrowing silently never applied there -- unlikePin::Method/Pin::Block, which already passnode:.node:through at both sites, matching the existing Method/Block pattern; the gap is specific to namespace-shaped compound statements (file root, class/module bodies), not "top-level" generally -- a block at the top level already narrowed correctly. Known follow-on, not fixed here:SclassNode(class << self) has the same missingnode:(unverified), andPin::Base#combine_with/Closure#combine_withdon't carrynode:forward across a reopened class merged from multiple files (doesn't affect this fix, since narrowing runs before combination).