Add EnvironmentLoader for ruby-rbs crate - #3050
Open
dak2 wants to merge 25 commits into
Open
Conversation
Mirrors RBS::EnvironmentLoader: resolves core, library (with manifest.yaml dependency expansion), and explicit-directory sources in Ruby's load order, then parses each .rbs file once into an Environment. ruby#2954
soutaro
reviewed
Jul 30, 2026
soutaro
reviewed
Jul 30, 2026
soutaro
reviewed
Jul 30, 2026
The library client passes resolved gem paths directly; the resolver indirection was unnecessary. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Optional builder setters let core and stdlib be skipped by silent omission instead of explicit choice. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
version alone can't recover where a library's signatures came from; path can. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fixes rejection of YAML that Ruby's YAML.safe_load accepts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Without this, the natural default configuration (core_root + stdlib_root,
no extra wiring) always failed with UnknownLibrary("stringio"), since the
loading repository stayed empty unlike Ruby's auto-registered stdlib.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
An invalid version string was indistinguishable from a missing gem at the lookup boundary, so every caller had to guard against it separately. Parsing once in the loader and carrying the result alongside its Library makes that mismatch unrepresentable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
collect() propagated every io::Error, so one EACCES subdirectory failed the entire load. Ruby's Dir.glob silently skips such directories, so only skip PermissionDenied and NotFound to match it, while still surfacing genuine I/O failures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
line_ranges, line_count, line, pos_to_loc, loc_to_pos, and last_position had no callers outside their own tests, and their byte-offset semantics diverge from Ruby's character-offset RBS::Buffer anyway. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Merging upstream/master reintroduced this arm earlier in the match (added independently on master), leaving the branch's copy at the end unreachable. With RUSTFLAGS=-D warnings this failed cargo test and cargo clippy in CI. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Steep-style callers need to add parsed sources to an Environment without going through EnvironmentLoader's directory scan. Parsing always uses the environment's own Interners, so a source added this way can never carry ids from a different environment. load() now calls add_rbs_file too, so parsing lives in one place. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Ruby now resolves gem name/version to a directory before calling Rust, so add_library just takes the path. Drop the loader's own resolution path with it: dependency expansion, the stdlib/loading repositories, and the version-related LoadError variants. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Unused now that add_library takes an already-resolved path instead of a name/version pair. Recoverable from git history if Rust ever needs its own version comparison again. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This reverts commit fcf2f0f.
Ruby's RBS::Buffer exists because RBS::Location is lazy: it holds a reference to the buffer and slices out text on demand for #source and pos_to_loc. The Rust port mirrored that shape, but the architecture underneath it isn't lazy — AstConverter eagerly interns or copies every semantically relevant string (names, comments, annotations, literals) at conversion time, and LocationRange is a plain 4x u32 offset Copy type with no reference back into the source. There is no deferred slicing step left that would ever need the buffer, so nothing in the crate calls Buffer::content() after parse_one() returns — it's a write-only store. Keeping it anyway means holding the full text of every loaded file for the Environment's lifetime for no benefit: ~4.3MB across core + stdlib alone, more with gems, growing for as long as the Environment lives. Source.buffer: Buffer is replaced with Source.path: PathBuf, keeping just enough to identify the file for error reporting. parse_one() no longer moves `content` into a Buffer, so the explicit drop(signature) that used to be needed to end its borrow first is gone too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dak2
force-pushed
the
environment-loader-ruby-rbs-crate
branch
from
August 14, 2026 00:28
3ffad9d to
eddc09a
Compare
The "skip this entry" match was written out three times, differing only in whether it returned or continued. Collapse it into one `skippable` adapter returning `io::Result<Option<T>>`, so the policy for which IO errors Ruby swallows lives in one place. The entry name was also derived twice per entry: from `entry.file_name()` for the leading-`.` test, then again from `path.file_name()` for the leading-`_` test. Derive it once from `path`, which drops the `OsString` that `DirEntry::file_name` allocates for every entry walked, matched or not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
each_dir built a Vec of owned pairs before the load loop started, cloning every registered path twice: once into the SourceKind and once as the directory to walk. The kinds still have to be built, but the directory to walk can be borrowed from the loader, which outlives the load call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing calls it. It forwards to StringInterner::merge and TypeNameInterner::merge, which are already covered by tests next to their own definitions, so its test asserted nothing new either. The parallel load loop parse_one is shaped for would need it back, but re-adding a two-line delegation then is cheaper than carrying an untested-by-use API until that lands. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Default delegated to a new() that listed the fields by hand, so adding a field meant remembering to update new(). Reverse the delegation and derive Default, leaving the generated code as the only place that knows the fields. This is also the shape StringInterner and Interners already use. TypeNameInterner keeps its hand-written Default because it has real initialisation to do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bool it returns is only accepted by file_finder::each_file, which is pub(crate), so a caller outside the crate could obtain the answer but had nowhere to pass it. The variants themselves stay public, so no information is withdrawn from consumers of Source.kind. Also record on Library why it carries the resolved path, since dropping version for path was a deliberate choice and the field reads like duplication next to each_dir without it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing pinned down the return value: every assertion against it was answerable from env.sources(), and one of them compared the two lengths against each other. Add load_reports_only_what_this_call_added, which loads twice into one Environment and checks the second call reports only its own file — the case env.sources() cannot answer, and the reason the return value is not just a projection of the environment. Say as much on load itself, and note on LoadedFile that it corresponds to an entry of what RBS::EnvironmentLoader#load returns, at file rather than declaration granularity. The remaining tests reuse the tree helper this file already had instead of hand-rolling tempdir plus write, and from_loader_is_the_primary_entry_point no longer reads stdlib to assert that something was loaded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Remove Buffer. In details, 2b3df5a |
dak2
marked this pull request as ready for review
August 14, 2026 01:37
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
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.
Summary
Mirrors RBS::EnvironmentLoader: resolves core, library, and explicit-directory sources in Ruby's load order, then parses each .rbs file once into an Environment.
#2954
Architecture differences from
RBS::EnvironmentLoaderEnvironmentstate instead of a process-global one.gem_sig_path/Repository, and expandsmanifest.yamldependenciesadd_library(name, path)takes an already-resolved directory; noRepository, no manifest expansion, no implicitstringioadd(path:/library:/version:)with keyword modesadd_library/add_dir, the modes separated by typelibsis aSet, to stop dependency expansion from recursingVec; with no expansion, first-wins onseen_filesis enoughTypeNameobjectsEnvironmentowns theInterners, so loaded names stay resolvable for its lifetimePathname.glob("**/*.rbs"), sorted as/-joined stringsread_dir, sorted by platform path; symlinked directories are not followed, and onlyPermissionDenied/NotFoundare skippedUsage
Register the directories on the loader, then load them into an
Environment:loadcan also fill an existingEnvironment, and returns what it read, in theorder it read it:
Notes
LoadErrorisIo { path, .. }orParse { path, message }; the sources read before the failure are already in theEnvironment.Environmentowns the interners, soenv.interners()is what resolves and displays the interned names held by the loaded declarations.Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com