[DO NOT MERGE] Rewrite PinCache as an instance-based engine, with wiring and CLI update - #97
Draft
apiology wants to merge 22 commits into
Draft
[DO NOT MERGE] Rewrite PinCache as an instance-based engine, with wiring and CLI update#97apiology wants to merge 22 commits into
apiology wants to merge 22 commits into
Conversation
Introduce Solargraph::PinCache, replacing the old class-method-based PinCache module with an instance-based engine that owns YARD and RBS collection caching, plus combining them into a single cached pin set per gem. Yardoc, GemPins, and RbsMap are updated to support it: * Yardoc splits doc-building (build_docs/build_pins) out of its old do-everything cache method, so PinCache can drive the build and caching steps separately. * GemPins drops build_yard_pins (now owned by PinCache) and adds combine_method_pins_by_path for deduping method pins by path. * RbsMap falls back to StdlibMap resolution when a gemspec isn't found in the RBS collection. Also fixes a real bug in RbsMap::Conversions surfaced while extracting this: two pairs of duplicate method definitions (parts_of_function, build_type) where an old implementation was left in place, shadowed and made unreachable by a newer one added elsewhere in the file. The dead code referenced two helper methods (other_type_to_type, method_type_to_type) that don't exist anywhere in lib/, so it would have raised NoMethodError had it ever been called - removing it drops this file's strong-typecheck problem count from 32 to 21 (all pre-existing, unrelated to this change). Extracted from castwide#1006 (Improve pin caching) as the foundational piece of that PR: the new caching engine and its direct collaborators, without yet wiring it into DocMap/Workspace/ApiMap or the CLI (those follow in stacked PRs on top of this one). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replace DocMap's ad hoc gem-caching logic with delegation to PinCache (introduced in the prior stacked PR), simplifying DocMap substantially. Workspace gains a pin_cache accessor plus cache_gem/uncache_gem/ cache_all_for_workspace! entry points that drive PinCache for a given workspace's gemspecs. ApiMap follows the renamed DocMap API (cache_all! -> cache_doc_map_gems!, uncached_gemspecs.any? -> any_uncached?) and dedupes resolved method aliases via GemPins.combine_method_pins_by_path. Library exposes pin_cache (delegating to workspace), uses it to check whether a gem's cache build is already in progress, and fixes a subprocess chdir bug in its background gem-caching thread. Extracted from castwide#1006 (Improve pin caching) as the second piece of that PR, stacked on top of the PinCache engine PR. This depends on PinCache existing; the CLI updates that depend on this wiring follow in a further stacked PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reimplement `solargraph cache`, `uncache`, and `gems` as thin wrappers
over the Workspace#cache_gem/uncache_gem/cache_all_for_workspace!
entry points added in the prior stacked PR, removing the CLI's own
duplicated build/cache logic.
Note: 2 specs in this PR ("with unbundled environments #cache
succeeds" / "#gems succeeds") will fail until
castwide#1225 merges - they exercise
Workspace::Gemspecs#find_gem in an environment with no discoverable
Gemfile, which currently raises Bundler::GemfileNotFound instead of
falling back gracefully. Verified locally that applying castwide#1225's fix
makes both pass with no other changes needed here.
Extracted from castwide#1006 (Improve pin caching) as the
final piece of that PR, stacked on top of the DocMap/Workspace wiring
PR.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The YAML/Psych stdlib resolution spec timed out at 240.33s against a 240s limit in CI - a genuine near-miss (0.14% over), not contention from running multiple PRs' CI concurrently (each job gets its own runner, so concurrent jobs affect queue time, not execution time). Widen that limit and the other 120s limits proportionally to give real headroom against normal run-to-run variance on GitHub Actions' shared runners, rather than re-running and hoping. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Earlier, while this PR's PinCache-core piece was still a standalone branch (before combining the engine, wiring, and CLI update into one PR), this spec was pointed at DocMap#cache_all! since that was the only name that existed on master at the time. Now that this PR also includes the DocMap wiring that renames cache_all! to cache_doc_map_gems!, the spec needs to follow that rename too - CI caught the drift (undefined method 'cache_all!' for an instance of Solargraph::DocMap). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Both pre-existing on master, unrelated to any currently open PR: Pin::Method#== (super && other.node == node) is from castwide#930 (2025-05-11) and never compared signatures. Pin::Parameter#type_arity_decl (arity_decl + return_type.items.count.to_s) is from castwide#1177 (2026-05-12), the same commit that added the spec/pin/method_spec.rb "combines signatures by type" test this fix makes pass. Both bugs are dormant on plain master: GemPins.combine_method_pins_by_path, the only caller that exercises this combining logic, was itself removed by castwide#1195 ("Limit pin combination to doc maps"), so this fix has no observable effect and no test to point to on this base until that function and its call site are restored. See PR description for context on where that currently stands. Traced from a CI-only failure on an unrelated integration-testing branch, where a different, in-progress PR stack (apiology/solargraph pin-caching-3/4) happens to re-add GemPins.combine_method_pins_by_path and its caller, waking up both of these bugs: Integer#+ inferred a return type of "Integer, BigDecimal" instead of "Integer" for `x = 0; x += 1; x`, because Pin::Method#== treated two RBS declarations of Integer#+ with different signatures (core Ruby's and the bigdecimal gem's reopening) as equal - both have nil location and identical rdoc-derived comments - so GemPins.combine_method_pins' skip-if-already-identical shortcut fired and one declaration was silently dropped instead of merged. Separately, type_arity_decl grouped signatures for merging by how many types are in each parameter's union rather than the types themselves, so distinct single-type overloads (Integer, Float, Rational, Complex, BigDecimal) bucketed together and had their return types incorrectly unioned. Fixed by comparing actual type tags in type_arity_decl and by including signatures in Pin::Method#==.
Gem::Specification.find_by_path already resolves relative to a gem's
own require_paths, so passing "lib/#{require}.rb" instead of the bare
require path always missed. This silently dropped any gem whose
conventional require path differs from its RubyGems package name
(e.g. activesupport/active_support), along with its transitive
dependencies, with no error or warning.
Reported in castwide#1252 (comment)
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015yvznkiHNc5iXyycEj8tmR
Porting PR 1311's fallback_pins mechanism from the pre-1252 DocMap#deserialize_combined_pin_cache onto this branch's PinCache#deserialize_combined_pin_cache, which 1252 rewrote without carrying it forward. Known issue: since deserialize_combined_pin_cache now returns non-nil via the fallback, the caller's uncached_gemspecs queue never gets populated for a gem that hits this path, so no real combined cache ever gets built for it. Still being investigated.
deserialize_combined_pin_cache can now return RbsMap#fallback_pins before the real combined cache exists (see the prior commit). This call site inferred "is this gem cached" from "did I get pins back", which was true before that fallback existed but stopped being true once a fallback answer could also be non-nil. Left as-is, a gem that hits the fallback path would never get queued into uncached_gemspecs, so no real combined cache (with YARD pins merged in) would ever get built for it. Check pin_cache.cached? separately instead, only when pins came back at all - when nothing came back, it's unconditionally uncached already and a second check is redundant.
Retag the sg-ignore in ApiMap#clip with the matching rules.rb entry (flow sensitive typing needs to handle attrs) rather than the generic nil-check placeholder; confirmed unneeded and dropped entirely once removed and re-checked. Revert the 7 time_limit_seconds: RSpec metadata additions across 5 spec files, per review request to split them into their own PR.
undercover flagged lib/solargraph/yardoc.rb:18-41 as uncovered against castwide/solargraph:master. Lines 26-27 (the missing-gem_dir branch) were genuinely untested: the existing "bails quietly" spec stubbed File.exist? but build_docs checks Dir.exist?, so the branch it claimed to cover never actually ran. Stub Dir.exist? instead, and assert Open3.capture2e is never called so the test fails if the early return stops firing.
Workspace#gemfile? had no coverage for either branch. The cache CLI command had no coverage for the core-caching branch, which calls PinCache.cache_core.
undercover flagged the names.each loop in Shell#gems (66.67%, lib/solargraph/shell.rb:181 on origin/master) for missing coverage of both rescue clauses: no spec triggered Gem::MissingSpecError or Gem::Requirement::BadRequirementError from find_gem. Add specs stubbing workspace.find_gem to raise each rescued error, split out of the combined cover-shell-gaps branch (the other half, covering Shell#scan, belongs on pin-cli-ux instead).
…nto pin-caching-3-pincache-core # Conflicts: # spec/shell_spec.rb
Brings PR 1252 (Rewrite PinCache as an instance-based engine) up to date with master, so its own CI can pass again. Two failures on the stale branch: 1. spec/yard_map/mapper_spec.rb called the old Yardoc.load!(gemspec) API. Master added this spec (castwide#1279) after this branch forked, exercising a namespace-conflict fix in yard_map/mapper.rb that this branch didn't have. This branch's own Yardoc.load! now takes a path string, not a Gem::Specification, so the merged tree hit: TypeError: no implicit conversion of Gem::Specification into String The auto-merge carried master's version of the test through with no conflict, so the break landed silently. Fixed by adapting the spec to build the yardoc into a temp path first (build_docs) and loading that path, matching the pattern yardoc_spec.rb already uses. The merge itself supplies the missing namespace-conflict fix (Mapper#namespace_with_bug_fix), so the adapted spec now passes. 2. .github/workflows/plugins.yml's run_solargraph_rspec_specs job cloned a stale branch of lekemula/solargraph-rspec. Already fixed on master by castwide#1291 ("Use the original repo"), merged 2026-08-30. No local conflict; this branch never touched the file, so the merge carries master's fix through untouched. No other file needed manual resolution. Every other changed file auto-merged without conflict markers, verified file-by-file against each side's own commits touching it. Verified after merge: full spec suite 1680 examples/0 failures/67 pending; solargraph typecheck --level strong 527 problems in 89 of 250 files, with every problem not already present on this branch's old tip confirmed pre-existing on plain master (531 problems standalone) rather than merge-introduced; rubocop 48 offenses vs 51 on this branch's old tip, the improvement coming entirely from master's own rewrite of source/chain/call.rb.
Both class-level methods were entirely untested: uncache_by_prefix's glob-delete-and-log loop (including that it skips directories and leaves non-matching files alone), and exist?'s file-vs-missing check. Split from the 2026-08-04 integration branch's bundled undercover coverage commit 4aab8b2.
GemPins.combine guards its loop body with
next yard_pin unless rbs_pin && yard_pin.is_a?(Pin::Method)
so rbs_pin is truthy on every line below it. The `unless rbs_pin`
block that followed could therefore never run - neither its
logger.debug call nor its `next yard_pin`.
undercover over a full 1684-example run reported
lib/solargraph/gem_pins.rb:60:62 at 0.0% coverage, 0 hits on both
executable lines. Nothing else moves: 1684 examples, 0 failures,
67 pending before and after, and `solargraph typecheck --level
strong` holds at 527 problems in 89 of 250 files.
extract_method_type_return_type only ever truth-tests implicit_nil, and its sole caller parts_of_function already documents it as Boolean. The YARD tag here declared it Object and sat below @return, so the signature read as taking an arbitrary value. Declare it Boolean, put the tag back in parameter order, and pass it by name at the one call site so the meaning is visible there.
resolve_require built the lib-relative filename at the top of the method, twenty lines above the manual spec.files scan that is the only thing reading it. Gem::Specification.find_by_path takes a requirable path rather than a filename, so nothing between the two points needs the value.
…nto pin-caching-3-pincache-core
type_arity_decl fed unrooted tags into the signature grouping key, so two same-named types in different namespaces shared a bucket and their signatures combined. Rooted tags cannot collide that way. The cost is splitting a bucket that mixed rooted and unrooted spellings of one type, which leaves signatures separate rather than merging them wrongly. The type_arity example still showed the type-count form it replaced, so update it alongside.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Claude: This PR was opened by Claude Code on behalf of @apiology. It carries over the body of castwide#1252 unchanged; that PR is being closed in favour of this one.
Rewrites
PinCachefrom a class-method-based module into an instance-based gem-caching engine, and updates every consumer to match.Why
Fixes a stale-type bug: after adding, updating, or removing an
rbs_collection.yamlentry for a gem, hover/completion/type-checking could keep showing the previous RBS source's results, with no indication anything was stale short of a manualsolargraph cache --rebuild.How
The old cache key conflated a gem's possible RBS sources (bundled types,
rbs_collection, local override, unresolved) into one entry. Replaced with explicitCACHE_KEY_GEM_EXPORT/_UNRESOLVED/_STDLIB/_LOCALclassification plus a content hash of the collection's per-gem lock data.PinCachebecomes an instance scoped to that config, since the key logic now needs to know which config produced it.What's in it
PinCache: instantiable class takingrbs_collection_path/rbs_collection_config_path/directory/yard_plugins;Yardoc,GemPins,RbsMapupdated to match.DocMap,ApiMap,Workspace,Librarydelegate to it; CLIcache/uncache/gemsare thin wrappers.Gemspecs#in_this_bundle?no longer raises with no Gemfile (#1225).An unrelated
RbsMap::Conversionsdead-code fix, in a file this PR already touches, is split out to #41.Testing:
bundle exec rspec— 1595 examples, 0 failures, 64 pending.