Feature/plan implementation#1
Merged
Merged
Conversation
added 30 commits
July 3, 2026 01:30
PLAN Phase 0. Adds corpus fixtures and harness extensions that pin the
current buggy behavior so the rewrite is verifiable. These tests were
expected to FAIL until Phase 1; they turn green once the showstopper
fixes land.
- Function-body fixtures: err_fnbody_{arith,unbound,nested_if} (RY040/
RY010) and ok_fnbody_sequential (no-diag, pins the Stmt::If scope-clone
bug). PLAN finding 1.
- Parse-error marker '# expect-parse-error' in the corpus harness
(Expectation::ParseError), asserting RY000 is emitted. Fixture
err_parse_error.R. PLAN findings 2/3.
- Parser round-trip tests (ry-core/tests/parser_correctness.rs) for
'x <<- 1' (must be SuperAssign), '2 ** 3' (must be Pow),
'n <- 1e5L; m <- n + 1' (no statement drop), and top-level
'{ a <- 1; b <- 2 }' (both statements preserved). PLAN finding 6.
- Performance regression test (tests/perf.rs, #[ignore]): 20k-line file
must check in under 2s. Budget met only after Phase 1.1. PLAN item 0.4.
- Oracle harness skeleton (tests/oracle.rs, #[ignore]): runs Rscript on
testdata/oracle/ fixtures and cross-checks ry's error diagnostics.
Skips cleanly when Rscript is absent. 10 seed fixtures covering
<<-, **, NULL + char, arithmetic on char, $ on atomic, non-function
call, plus must-pass cases. PLAN items 0.5/7.
PLAN Phase 1 (the non-lib.rs portions). Small, surgical fixes inside
the existing architecture. Verified: cargo test --workspace green;
20k-line perf test 0.79s (was 47s); all five appendix showstopper
probes behave.
- parser: fix O(n^2) column computation. RParser::span now uses
tree-sitter's start_position().column (byte column) directly; the old
char_col rescanned the whole file from byte 0 per node. Span::col is
now byte-indexed (documented); a per-line byte_col_to_char_col helper
is staged for future renderers. PLAN 1.1.
- parser: surface parse errors. RParser::parse collects ERROR/MISSING
tree-sitter nodes into a new SourceFile::parse_errors field; the
checker emits them as RY000. PLAN 1.2 / findings 2,3.
- parser bug batch: '<<-' matched in both try_lower_assign and
lower_binary (tree-sitter emits '<<-', not '<<'), routed to
SuperAssign; '**' mapped to Pow (was Mul); integer literals that
fail i64 parse (1e5L, 0x10L) fall back to Double then Unknown
instead of dropping the whole statement via '?'-propagation;
top-level '{...}' braced expressions splice all children into the
statement list (was: only the last survived). PLAN 1.4 / finding 6.
- types: invalid_condition no longer treats Mode::Opaque as invalid.
Required so 'if (untyped_param)' in a function body is not a false
RY001 once Phase 1.3 starts walking function bodies.
- rules: register RY000 (syntax-error), keeping codes lexicographic.
- typeshed: load_base_cached() returns &'static Typeshed via OnceLock;
the 61KB base_r.json is now parsed once per process instead of per
Checker::new. PLAN 1.5.
PLAN Phases 1.2/1.3 (lib.rs portions) and Phase 2. cargo test --workspace green; cargo clippy clean; determinism test passes. Phase 1 (lib.rs): - Emit RY000 parse errors from SourceFile::parse_errors in check(), check_with_scope(), and emit_diagnostics() so broken input no longer checks 'clean' (plumbs through Checker/Project/LSP). PLAN 1.2. - Stmt::Assign walks named function bodies for diagnostics when the value is an Expr::Function (mirrors Stmt::FunctionDef). Without this, almost all real R code went unchecked. PLAN 1.3 / finding 1. - Stmt::If clones the branch scope ONCE before the loop instead of per statement, so sequential bindings inside a branch resolve (tmp <- 1; out <- tmp + 1). PLAN 1.3 / ok_fnbody_sequential.R. Phase 2 (single inference engine): - Add a 'discarding: bool' field on Checker; emit() short-circuits when discarding is true. This is the unification seam (an enum sink was sketched in the plan; the flag is simpler and avoids &mut self vs &self borrow conflicts). - Replace infer_pure_at_depth with infer_discarding, a thin delegator that sets discarding=true, calls the single diagnostic infer, and restores the flag. Pass-2 fixpoint refinement and closure-signature building now run the FULL infer (Ident resolution ladder, all Call cases, narrowing, pipes, tidyeval, NSE verbs, S3 dispatch) with emission suppressed -- so return-type inference is now strictly more precise than the old pure subset, with no separate logic to keep in sync. - Delete the pure twins: infer_binop_pure, infer_c_pure, apply_sig_pure (~120 lines). Their two external callers (ho_do_call, callback_return_type's typeshed arm) route through apply_sig. - apply_sig: when args is empty (caller has types but no Arg slice, as the higher-order callback path does), fall back to positional arg_types so arg0/arg1/... resolve correctly. Fixes the sapply(c(...), sqrt) simplification regression (ok_ho_typeshed_callback.R). - Rename build_function_signature_pure -> build_function_signature; update stale _pure / 'mirrors pass 3' comments. - Propagate &mut self through collect_returns_and_simulate_at_depth, try_infer_return_call_at_depth, build_function_signature, trailing_return_type, function_value_from_literal, callback_literal_return, callback_return_type, infer_higher_order_call, infer_ho_result, and the ho_* helpers. - Add diagnostics_are_deterministic_across_runs test (recursion, mutual refs, function-body errors, higher-order callbacks, closure factory). PLAN Phase 2 acceptance. Deferred to a follow-up (reviewer-recommended, non-blocking): the walk_stmt fusion of check_stmt and collect_returns_and_simulate (biggest remaining line-removal lever; also forces reconciling the pass-2/pass-3 scope model) and Rc<[Stmt]> body storage (pure perf). lib.rs: 6844 -> 6623 lines.
PLAN Phase 3 step 1 (the type-representation conversion; no behavior change). cargo test --workspace green; cargo clippy clean. Replaces the three Box::leak-based global interners (intern_class_name, intern_column_schema, intern_function_signature) with Arc-shared storage owned by each RType value. The unbounded memory growth those global OnceLock<Mutex<...>> tables caused in LSP sessions is gone: the Arcs are released when the owning Checker (and its Scope/FnTable/ReturnSlots) is dropped. Design: Arc-based rather than ID-based (PLAN offered both). The decisive reason: Project::check shuffles RType across Checker instances (with_tables/into_tables/FnTable::clone), which assumes RType is self-contained. An ID-based Ty(u32) would require either a shared &TypeInterner (re-introducing interior locking) or ID remapping across checker boundaries. Arc has none of that, and field accesses (t.mode, t.length) stay unchanged across the ~6000-line checker -- the blast radius is purely mechanical (Clone, .cloned(), snapshot-before- consume) rather than a per-read rewrite. Changes: - RType: drop Copy (now Clone); columns: Option<Arc<ColumnSchema>>, fn_sig: Option<Arc<FunctionSignature>>. RType::UNKNOWN (const) -> RType::unknown() (fn, since Arc::new isn't const). with_columns / with_fn_sig take Arc. - ClassVector: drop Copy; names: [Option<Arc<str>>; 4]. single(&str), from_slice(&[&str]), with_class(&[&str]) replace the &'static str APIs. first() returns Option<Arc<str>>. - Delete intern_class_name / intern_column_schema / intern_function_signature and KNOWN_CLASSES (9 leaked literals). Call sites now use Arc::new(...) / Arc::from(...) / ClassVector constructors directly. - element() takes &self (it only reads; lets callers read x_type after). - Snapshot lt.mode/rt.mode into locals in infer_binop before the consuming arith/compare calls. - Checker: import Arc; .copied() -> .cloned() on RType containers; *t -> t.clone(); if-let-Some(schema) borrows via &bt.columns / ref schema so the Arc isn't moved out of a borrowed RType. - scope_with_columns / emit_undefined_column / homogeneous_list_element_type take &ColumnSchema / &Arc<ColumnSchema> instead of &'static. - build_function_signature returns Option<Arc<FunctionSignature>>. - Tests: delete the 6 interner-pointer-identity tests (intern_class_name_is_idempotent, intern_column_schema_*, intern_function_signature_*); update the rest to Arc::new / from_slice / RType::unknown() / .as_deref() on first(). grep -rn 'Box::leak' crates/ : empty. grep OnceLock/static Mutex in types.rs: empty. All 167+ checker/corpus/lsp/cli tests still green.
PLAN Phase 3 item 3. cargo test --workspace green. - Mode::arith_result: move the Character/List/Function rejections BEFORE the Null arm. Previously (Null, x) ran first, so 'NULL + "a"' typed as Character instead of erroring. R errors at runtime with 'non-numeric argument to binary operator'. PLAN finding 7. - RType::arith: when exactly one operand is Mode::Null, override the result length to Zero (R returns numeric(0)/integer(0) for 'NULL + 1'), preserving the correct 'NULL + 1 -> numeric(0)' semantics alongside the now-erroring 'NULL + "a"'. - Length::binary: collapse the duplicate Known/Known branches (the divides/does-not-divide cases both returned Known(a.max(b))). New tests: arith_null_plus_character_errors, arith_null_plus_int_is_zero_length. Manual probe: NULL + "a" now reports RY040 (was: silent / mistyped).
PLAN Phase 3 item 2. cargo test --workspace green; cargo clippy clean. R never coerces at a control-flow merge -- `if (p) 1L else "a"` is honestly both integer and character, not silently character. The old `join` used the coercion ladder (`coerce_rank` max), which lost information and (combined with the arith table) caused false diagnostics. Phase 3 replaces it with an honest bounded union type. - Mode::Union (no payload; Mode stays Copy + Eq + Hash). Members live on a new RType::members: Option<Arc<[RType]>> field (capped at MAX_UNION_MEMBERS = 4; beyond the cap, join collapses to unknown). with_members constructor added. - join: equal -> self; opaque -> unknown; otherwise union_of(a,b) (flatten/dedup/cap). No more coerce_rank promotion. - arith/compare: distribute over union members via the new `distribute` helper; error ONLY if every member-pair errors. The old atomic bodies moved to private arith_atomic/compare_atomic. So union[integer,character] + 1 stays quiet (integer ok), while union[list,function] + 1 fires RY040 (all invalid). PLAN's two fixtures. - invalid_condition: true only if ALL union members invalid (one valid branch -> stay quiet). - Display: union renders as union[m1, m2, ...]. - Mode::arith_result/compare_result/coerce_rank gain a Union arm (sentinel; unions are handled above the mode layer). Fixtures: repurposed err_if_expr_join.R and err_switch_mixed.R -- they previously relied on the coercion-ladder join (1L/"hello" joined to character so x+1 errored). Now they use all-invalid unions (list+function) so RY040 still fires under the new semantics. Added ok_union_if_arith.R (quiet some-member-ok case) and err_union_all_invalid.R (all-invalid case). New unit tests: join_of_incompatible_modes_is_a_union, join_equal_modes_returns_self, arith_distributes_over_union_quiet_when_some_member_ok, arith_errors_when_all_union_members_invalid.
PLAN Phase 3 item 5. cargo test --workspace green; cargo clippy clean. The NaFlag carried 'may contain NA' on every RType, but no diagnostic or inference branch ever consumed it -- it was decorative, threaded through ~60 constructor call sites and Display (as a '?NA' suffix) for no behavioral effect. Remove it to shrink the type and stop paying the plumbing tax. - Delete NaFlag; drop the `na` field from RType. - RType::new(mode, length, na) -> RType::new(mode, length); scalar(mode, na) -> scalar(mode). ~60 call sites updated across ry-core/ry-checker/ry-lsp/ry-cli. - infer_c: drop the any_na accumulation (was only ever fed to the removed field). - Display: drop the '?NA' suffix. - The typeshed JSON struct still parses its `na` field (kept for forward-compat with the data file) but no longer reads it. - Drop the re-export of NaFlag from ry-core::lib.
PLAN Phase 3 item 4. cargo test --workspace green; cargo clippy clean. The narrowing logic misused coerce_rank as a subtype lattice: a predicate like is.numeric(x) returned Mode::Double, and apply_narrowing rewrote the binding whenever the existing mode's rank was <= the predicate's. So is.numeric on a KNOWN Integer rewrote it to Double -- losing type information and (in principle) mis-typing downstream ops. New rule (PLAN): a predicate narrows only when the existing type is opaque (untyped), or a union that already contains the predicate's mode. A known type is never rewritten. Incompatible known modes are left untouched. - predicate_mode -> predicate_target, returning a full RType. Group predicates produce unions: is.numeric -> union[integer, double] (NOT plain Double). This is what lets the new rule distinguish 'is.numeric' (a group) from 'is.double' (a single mode). - Narrowing::Positive now carries a target: RType instead of a bare Mode. - extract_type_narrowing: only is.null is modeled as a Negative (negation) narrowing; other predicates stay Positive. Previously !is.integer(x) etc. produced a spurious Negative that the consumer never handled correctly. - apply_narrowing Positive arm: opaque -> install the target; union containing the predicate's mode -> keep; known-equal -> keep; known-incompatible -> leave untouched. No coerce_rank. Probes: is.numeric on a known integer stays integer (was: double); is.numeric on an opaque narrows to union[integer,double]. Both clean.
…irst-param heuristic PLAN Phase 3 item 6. cargo test --workspace green; cargo clippy clean. The S3 method-name splitter misregistered ordinary dotted functions as S3 methods. matched generic + class because the S3_GENERICS table included short/common words (t, c, format, is.na, length, names, dim, [, [[, $, rep, rev, sort, unique, head, tail, subset, transform, within, merge) and the multi-segment as.*/model.matrix generics. The implementation also used a [0u8; 64] scratch buffer to avoid one format! allocation -- premature and error-prone. - Trim S3_GENERICS to 16 unambiguous base-R dispatch generics (print, summary, plot, predict, fitted, residuals, coef, vcov, logLik, AIC, BIC, update, deviance, anova, str, terms). - Add S3_DENYLIST of well-known dotted-but-not-method names (t.test, all.equal, file.path, Sys.time/Date, as.data.frame, read/write.csv, data.frame, ...). Checked first; never split. - Replace the [0u8; 64] buffer with format! (runs once per top-level assignment; negligible cost). - First-param heuristic at the registration call site: only register as an S3 method when the function's first parameter is named `x` (the conventional S3 dispatch-object name). The function is still recorded as a plain function either way, so name resolution is unaffected. New fixture ok_no_s3_collision.R pins that t.test is NOT misregistered. Also bind `p` in the two union fixtures (ok_union_if_arith / err_union_all_invalid) so they don't spuriously fire RY010 on the unbound condition variable.
PLAN Phase 4 item 2. cargo test --workspace green.
The string-literal lowering just stripped the first and last byte,
which silently dropped escape sequences ('a\nb' became the 4-char
literal a\nb instead of a+newline+b) and mishandled raw strings.
Correct escape processing matters because column-name matching
(df$"my col", list("a b" = 1)) and # ry: directive parsing depend
on the literal value.
- unquote_r_string: dispatches between ordinary quoted strings
(escapes processed) and raw strings r"(...)" / R"[...]" /
r"-(...)-" (content taken literally, delimiters stripped).
- process_r_escapes: handles ", \, \n, \r, \t, \b, \f, \v, \0, \a,
\', line continuations, \uXXXX, \UXXXXXXXX, and \xXX. Unknown
escapes are kept verbatim (R warns but retains the backslash).
- UTF-8-safe advancement via utf8_char_len (no unicode crate).
New tests: string_escape_sequences_are_processed, string_unicode_escapes,
string_single_quotes_work, raw_strings_skip_escape_processing,
string_with_embedded_quote_directive_is_not_a_suppression.
…ops statement
PLAN Phase 4 items 1 and 3. cargo test --workspace green; cargo clippy clean.
Item 3 -- suppression comments become lexical. The old suppression
parser scanned source lines for '#' via line.find('#'), which
mistakenly matched a '#' INSIDE a string literal: `x <- "# noqa"`
was treated as a suppression directive and silently swallowed real
diagnostics. has_file_suppression also used substring .contains(),
so prose like '# see ry: ignore-file' triggered a file-wide suppress.
- SourceFile gains a comments: Vec<Comment> field, populated by the
parser from tree-sitter comment nodes (the ONLY lexically-real
comments). Comment carries line, col, and body (text after '#').
col distinguishes standalone (col 0, defers to next line) from
trailing (col > 0, applies to own line).
- New lexical APIs: parse_suppressions_from_comments,
has_file_suppression_from_comments, filter_suppressed_with_comments.
has_file_suppression_from_comments uses starts_with (not contains).
The legacy &str APIs remain for tests/back-compat.
- Route the corpus harness, CLI, and LSP through the comment-based
filter (each captures file.comments during parse).
Item 1 -- float literal parse failure no longer drops the statement.
float used .ok()? which propagated None up via '?' and deleted the
enclosing statement; now yields Expr::Unknown(span) so the statement
survives. (The integer arm was already fixed in Phase 1.)
Acceptance fixture ok_string_containing_hash_noqa.R: a string value
containing '# noqa' followed by a real arithmetic diagnostic on the
same line still reports the diagnostic. Verified via CLI and corpus.
PLAN Phase 4 item 4. cargo test --workspace green. - infer_pipe dispatch: %<>% (PipeAssign) now rebinds the LHS identifier in scope to the pipe's result type, matching R/magrittr semantics (x %<>% f() is x <- x %>% f()). Previously it shared the result type with %>% but never updated the binding, so downstream uses of x kept its pre-pipe type. - Delete BinOpKind::NotIn and BinOpKind::PipeBind: neither was ever produced by the parser (no %notin% / %>_% grammar mapping), so they were dead enum variants that forced exhaustive matches to carry dead arms. Removed from the AST, the compare-op match, and op_symbol. New fixture ok_pipe_assign_rebinds.R pins the rebinding.
PLAN Phase 5 items 2 and 5. cargo test --workspace green;
cargo clippy --workspace --all-targets -- -D warnings clean;
cargo fmt --all -- --check clean.
- Add .github/workflows/ci.yml: builds, tests, runs clippy with
-D warnings, and checks rustfmt on every push/PR. RUSTFLAGS=-D
warnings so the build itself also denies warnings.
- The --all-targets clippy gate surfaced test code that the Phase 3
NaFlag removal and Copy->Clone conversion had broken but that
cargo test --workspace had silently skipped (the test binary failed
to compile, producing no 'test result:' line, so the earlier
grep-based check missed it). Fixed:
* types.rs tests: strip the removed na arg from RType::new/scalar;
clone before consume (i.join(d) then d.join(i)) now that RType
is Clone-not-Copy.
* lib.rs tests: clone Option<Arc<_>> before .expect()/.unwrap() on
&RType references; drop the obsolete assert!(...na.0) checks.
* if_expr_mismatched_branches_join: repurposed to the all-invalid
union case (was the coercion-ladder character join that unions
replaced).
- arith_result: move the Opaque arm BEFORE the Character/List/Function
rejections so opaque + character stays quiet (opaque could be
numeric). The Phase 3 reorder had opaque after the rejections,
causing a false RY040 on depth-capped / unknown operands
(closure_depth_cap_falls_back_to_opaque caught it).
- parser: try_unwrap_raw_string now strips the leading quote after the
r/R prefix (the token text includes the quotes).
- oracle.rs doc comment: indent the markdown list items so clippy's
doc-markdown lint passes.
PLAN Phase 6 item 1 (position handling). cargo test --workspace green; cargo clippy --workspace --all-targets -- -D warnings clean. The LSP spec defines Position.character as a UTF-16 code-unit offset. ry's byte_offset_to_position and position_to_byte_offset counted Rust chars (Unicode scalar values), which silently mis-counted astral-plane characters (emoji, rare CJK) and would have broken go-to-def / hover / rename / squiggle positions on lines containing them. - byte_offset_to_position: count UTF-16 code units via utf16_len (1 for BMP, 2 for astral). ASCII is unchanged. - position_to_byte_offset: new (text, line, utf16_col) -> Option<usize> that inverts the above; position_to_byte_offset_pos wraps it for callers holding a Position. The old char-counting (text, Position) variant is deleted. - span_to_range: derive BOTH endpoints from byte_offset_to_position (was: start from span.col byte column, end from byte_offset_to_position -- inconsistent on non-ASCII). - diagnostic_to_lsp_with_source (the production diagnostic-publish path) already maps span.start/end through byte_offset_to_position; kept the byte-col diagnostic_to_lsp as a test/no-source fallback. New tests: utf16_position_roundtrip_on_non_ascii (BMP), utf16_position_counts_astral_as_two_units (astral). Deferred: AST-based node_at_position lookup (replaces the byte-scanning find_identifier_at_position family) and the ry-lsp/lib.rs module split (item 3) -- both are large mechanical refactors with no behavior change.
…le.sh
PLAN Phase 7 items 1 and 2. Standard suite green; oracle suite (#[ignore]'d)
37/40 passing with 3 triaged known-limitations.
Item 1 -- expand testdata/oracle/ from 10 to 40 fixtures spanning:
- coercion ladder (int+double, c() mixed)
- ':' semantics (-1:3, fractional endpoints, descending)
- recycling (short-vector arith -- R warns but succeeds)
- [[ vs [ vs $ on lists and vectors
- S3-ish dispatch (no explicit classed value, but $ on lists)
- NSE verbs (with())
- HOFs (lapply, sapply)
- closures + <<- state (counter factory, global assign)
- switch (match, no-match default)
- tryCatch (caught error)
- control flow (for/while/repeat)
- misc (matrix %*%, paste collapse, rep times/each, seq_len,
named args out of order, function defaults, NULL in list)
Item 2 -- scripts/oracle.sh: runs the ignored oracle suite when Rscript
is available, exits 0 if not (informational, not a CI gate).
The 3 oracle failures are the suite doing its job -- surfacing real ry
gaps as triaged known-limitations in the fixture comments:
- call_non_function.R: ry misses '42()' (calling a non-function).
- dollar_missing.R: ry false-positives RY060 on list$missing (R
returns NULL; RY060 should be scoped to data frames).
- lapply_use.R: ry false-positives RY040 on an opaque-parameter
callback body.
Deferred: item 3 (vendor a small CRAN package + insta snapshot) requires
fetching external MIT-licensed source and explicit license handling;
better done as a follow-up with the package chosen deliberately.
…r fixpoint Deferred Phase 2 item 5. cargo test --workspace green; clippy clean. refine_fn_return walked each user function's body once per fixpoint iteration, cloning the whole Vec<Stmt> each time (O(functions x body size) allocation per iteration). Store the body as Arc<[Stmt]> in UserFn so the per-iteration 'clone' is a refcount bump. The body is immutable after record_fn, so sharing is safe. Arc (not Rc) so FnTable stays Send -- the LSP moves it across async tasks. record_fn wraps the incoming Vec<Stmt> via Arc::from; the walk sites deref via .iter() / &body_clone[..].
…into walk_stmt Deferred Phase 2 item 3 (the highest-risk item). cargo test --workspace green; cargo clippy --workspace --all-targets -- -D warnings clean. Merges the two parallel statement walkers into one unified walk_stmt(s, scope, returns: Option<&mut Vec<RType>>) that handles BOTH diagnostic emission (gated by self.discarding) AND return-type collection (when returns is Some). The diagnostic and pure paths no longer duplicate per-statement logic (if-condition RY001/RY002 checks, narrowing, for-loop element binding, function-body walking). - check_stmt is now a thin wrapper: walk_stmt(s, scope, None). - refine_fn_return and build_function_signature call walk_stmt with returns=Some in discarding mode. - run_fixpoint sets discarding=true around the fixpoint loop (the unified walker calls self.infer directly, so the flag must be set by the caller rather than via the old infer_discarding wrapper). - build_function_signature and callback_literal_return force discarding=true for their return-type walks so they never emit (their diagnostics come from check_stmt's function-body arm / walk_callback_for_diagnostics in pass 3). This fixed a double-emit regression on err_ho_callback_unbound.R (RY010 x2). - Deleted collect_returns_and_simulate_at_depth and try_infer_return_call_at_depth (folded into walk_stmt's Stmt::Expr return-call detection). lib.rs: 6623 -> 6523 lines (-100; the duplication is gone).
Deferred Phase 6 item 1 (second half). cargo test --workspace green; cargo clippy --workspace --all-targets -- -D warnings clean. The LSP handlers (hover, goto_definition, references, rename, prepare_rename, document_highlight) used a byte-scanning find_identifier_at_position that only inspected a single source line and ASCII identifier characters -- it mis-handled identifiers split across cursor placements and could not see into nested syntactic positions reliably. Replace with find_ident_at_offset(file, byte_offset): an AST walk that visits every Stmt and Expr (including nested function bodies and control-flow branches) and returns the SMALLEST Expr::Ident whose Span contains the offset (so f(g(x)) resolves to x when the cursor is on x). Ties break by span length (shortest wins). Same numeric/keyword filter as the old scanner (is_numeric_or_keyword) so rename/hover/go-to-def still ignore reserved words and literals. - All six LSP handlers reordered to parse-then-find (the finder needs the SourceFile), using position_to_byte_offset_pos to convert the LSP Position to a byte offset first. - The references handler reuses the already-parsed current document rather than re-parsing it inside the per-doc loop. - find_ident_in_stmt / find_ident_in_expr: recursive visitors covering Assign, Expr, If, For (loop var via stmt span), While, FunctionDef, Return, BinOp, UnaryOp, Call, Index, nested If-expr, Function-literal. - The old byte-scanner is kept (#[allow(dead_code)]) as a tested ASCII fallback; its unit tests document the legacy behavior.
Deferred Phase 6 item 3 (partial -- the lowest-risk slice of the module split). cargo test --workspace green; clippy clean. Pulls the self-contained, Backend-independent position/range conversion helpers out of the 5.3k-line lib.rs into a sibling util.rs: byte_offset_to_position, utf16_len, position_to_byte_offset, position_to_byte_offset_pos, span_to_range. These are pure functions reused across every LSP handler and have no dependency on the Backend/State, so they extract cleanly with no visibility gymnastics. lib.rs shrinks ~85 lines and the position layer is now independently testable. The full multi-module split (handlers.rs, features/ for hover/goto/ references/rename/highlight/symbols/folding/code-action, backend.rs) is a large purely-mechanical reorg of ~60 functions with high regression risk for zero behavioral benefit; this slice captures the highest-value, lowest-risk extraction. The remaining split can proceed incrementally on top of this without bisection concerns.
…ssertion-free baseline PLAN Phase 7 item 3. cargo test --workspace green; clippy clean. Replaces the assertion-free real_world.rs 'baseline report' printers (which only println'd a diagnostic distribution and never failed) with insta snapshot tests that ASSERT the exact diagnostic list per snippet. A snapshot diff is the false-positive alarm the old printers pretended to be: if the diagnostics shift, review, update deliberately, record why. - cargo add insta --dev -p ry-checker (yaml feature). - 9 snippets (base_mean_default, tidyverse_pipe, mtcars_dataset, pipe_subset_nse, s3_dispatch, na_propagation, coercion_ladder, loop_with_accumulator, vectorized_op) each get an assert_yaml_snapshot pinning the sorted (code, message) list. - Each snippet's triage is in a comment: all are currently (clean) -- no false positives on these real-world patterns, which is the goal. - Snapshots committed under tests/snapshots/*.snap; run `cargo insta review` to accept intentional changes. The vendored-CRAN-package variant of this item (fetching an external MIT-licensed source) is left for an explicit follow-up: these snippets already model real CRAN/base-R patterns and the snapshot mechanism is in place, so vendoring a package becomes 'add a fixture + snapshot' on top of this without new infrastructure.
…snapshot, perf/oracle/config tests, README, color scaffold, plan round 3) Captures the in-progress working-tree state referenced by PLAN.md round 3 as 'round 2 fully implemented': modularized LSP, glue vendor snapshot with triage, honest CLI flags, parser/types/typeshed fixes, expanded oracle corpus, and perf/config fixtures. Baseline gate green (test/clippy/fmt).
- LICENSE-MIT and LICENSE-APACHE at repo root (copyright 'ry contributors'). - description on every crate Cargo.toml; keywords/categories/readme on ry-cli. - scripts/audit_typeshed.R: base-R-only (no network, no jsonlite) audit that walks every name in base_r.json and asserts exists() in a vanilla R session, with an S3-method fallback for <generic>.<class> names. Wired into the CI oracle job. - Removed fabricated base_r.json entries 'identical_to' (real 'identical' already present) and 'colsum' (real 'colSums'/'rowsum' already present). Gate green: cargo test/clippy -D warnings/fmt --check; audit exits 0.
The dominant glue vendor false positive: `if (!inherits(x, "foo"))` types as logical<len=Unknown> and warned. RY002 now fires ONLY when the condition length is Known(n) with n > 1; Length::Unknown and One stay quiet. Length::Zero still routes to RY001 via invalid_condition. Applied to both emission sites (Stmt::If and infer_if_expr); message now names the concrete length. rules.rs summary updated. Corpus fixture ok_if_unknown_length_condition.R pins the patterns from the glue snapshot (no-diag). The existing ry002_if_length.R still pins RY002 firing for c(TRUE, FALSE) (Known(2)). Glue vendor snapshot: 6 RY002 'length Unknown' FPs drop (expected; the snapshot is re-pinned at the end of Phase 1).
…ing to parent
The color.R:123 RY070 false positive: a value guarded by
`if (is.null(x)) ... else x(out)` typed as NULL in the else branch and
fired RY070. Two problems fixed:
1. extract_type_narrowing misrouted the NON-negated `is.null(x)` case to
Narrowing::Negative (then=non-null, else=null) -- backwards. It now
falls through to Positive { target: NULL } so then=null, else=non-null,
matching the negated `!is.null(x)` form (which stays Negative).
2. New narrow_away_from_null helper: pure NULL -> opaque; a union with a
NULL member rebuilds minus NULL (collapsing to the single remaining
member or opaque). Applied to the Positive else-branch and the
Negative then-branch.
3. Branch-locality fix: narrowing refinements no longer leak into the
enclosing scope via merge_branch_bindings. apply_narrowing now returns
the set of narrowed names; merge_branch_bindings skips them so a
known-NULL parent stays NULL after the `if` (a post-if call correctly
fires RY070). Previously the refinement degraded the parent to opaque
and masked later errors.
Corpus fixtures: ok_is_null_guard_narrowing.R (param/local-var/negated,
no-diag) and err_is_null_narrowing_no_leak.R (post-if call, expect RY070).
The color.R:123 glue diagnostic is gone (snapshot re-pinned at end of Phase 1).
The glue.R:187/319 RY010 false positive: `.Call(glue_, ...)` treated the C entry-point symbol as a variable read. These FFI primitives take a native routine entry-point as their FIRST argument -- conventionally a bare identifier (or backtick name), not a variable reference. infer_call now special-cases the six FFI primitives: skip RY010 on a bare-identifier first arg, infer the remaining args normally, return opaque (the return type depends on the native routine). New is_ffi_primitive helper. Corpus fixture ok_ffi_call_first_arg.R pins .Call/.Fortran/.External. The glue.R glue_/trim_ diagnostics are gone (snapshot re-pinned at end of Phase 1).
….); R's function/value call separation Added to base_r.json: lengths (integer, len unknown), delayedAssign (NULL), inherits / requireNamespace / isTRUE / isFALSE (logical len 1), nzchar (logical len arg0), xor (logical), Negate (function), Recall (opaque), on.exit (NULL), match.arg (character len 1). All verified by scripts/audit_typeshed.R. These silence the glue utils.R:90 delayedAssign RY010 and the predicate-driven RY002s at the root. Also fixed R's function/value namespace separation at call sites: a CALL to `f(...)` searches the environment chain for a *function* named f and skips non-function bindings, so a local non-function shadowing a typeshed/FnTable function must NOT fire RY070. infer_call now checks whether a same-named function exists in the typeshed or FnTable before emitting RY070, and falls through if so. This fixes glue.R:191 `lengths(res)` after `lengths <- lengths(x)` (a true positive in the old conflated model, a false positive under R's real semantics). Corpus fixture ok_call_skips_nonfunction_shadow.R pins the call-skips- nonfunction-binding rule. Audit clean; corpus green.
…sion net Phase 1 acceptance: the glue vendor snapshot is now EMPTY -- all 12 of the original false positives resolved across 1.1-1.4. The triage comment is rewritten to record the resolution and assert the snapshot stays empty. Added a SECOND vendored package, purrr 1.2.2 (MIT), as the second regression net so the bar holds now that glue is clean. Refactored the vendor harness into a shared check_vendor helper; both packages snapshot independently. The purrr snapshot captures 26 diagnostics, all triaged in-comment as known-limitation false positives (none are true positives): - RY010 (22x): cross-package names (rlang/vctrs) and purrr's C-backed impls (map_impl, ...) not yet modeled -- Phase 2.2/2.3 territory. - RY001 (2x): the `if (length(x))` integer-truthiness idiom. - RY002 + RY032 (2x, same root cause): a real `%in%` length-modeling bug (result length follows the LHS, not the RHS) surfaced by the new net. Gate green: cargo test (incl. oracle with R)/clippy/fmt all pass.
…y/config
The single biggest semantic hole (PLAN 2.1): ry had no notion of
`library()`. The dplyr NSE verbs (filter/mutate/select/arrange/
group_by/summarise) were recognized by bare name unconditionally,
mis-typing `stats::filter` in code that never loads dplyr.
- Checker gains a `loaded: HashSet<String>`, populated from
`library(pkg)`/`require(pkg)` (bare-symbol arg) and
`requireNamespace("pkg")` (string-literal arg). Seeded via
`set_loaded`.
- Project unions loaded across files (a `library(dplyr)` in any file
makes dplyr verbs resolve everywhere, matching R's source() semantics)
via a new `collect_file_loaded` pre-scan in discarding mode, and
merges in `ry.toml`'s new `packages = [...] key.
- `infer_nse_call` now strips `pkg::`/`pkg:::` before verb lookup
(so `dplyr::filter(...)` resolves), and GATES the dplyr verbs on
dplyr/tidyverse being loaded OR the call being `dplyr::`-qualified.
Without that, a bare `filter(df, ...)` falls through to regular
resolution so RY010 still fires on genuinely unbound column refs. The
base-R NSE verbs (subset/with/within/transform) stay always-on.
- ry.toml gains a `packages = ["dplyr", ...] key for declaring
implicitly-loaded packages; threaded through the CLI into the Project.
Tests: 6 new gating unit tests (ungated fallthrough, dplyr::-qualified,
library/requireNamespace/tidyverse recording); 2 new e2e tests
(packages= enables NSE; absence gates it). Existing dplyr NSE unit tests
and corpus fixtures updated to prepend `library(dplyr)` now that the
verbs are gated. Oracle, both vendor snapshots, clippy, fmt all green.
PLAN 2.2. The typeshed was a single base file; the Bayesian/parallel packages the analyzer targets (purrr, mirai, brms, posterior, loo, bayesplot, cmdstanr) were absent, so every cross-package call was an unbound-name false positive. - Renamed data/base_r.json -> data/base.json (base + stats + utils; the three are always attached, splitting is optional). - New embedded package typesheds: dplyr.json, purrr.json, mirai.json, and bayes.json (a single multi-package file keyed `pkg.function` covering brms/posterior/loo/bayesplot/cmdstanr). - ry-typeshed gains load_package(name) (cached per-package via OnceLock) and is_known_package(name). For the multi-package bayes.json, the loader strips the `<pkg>.` prefix so each package view exposes bare function names. - Checker.resolve_typeshed_sig: a `pkg::fun`/`pkg:::fun` qualified call resolves against load_package(pkg); an unqualified call falls back from base to loaded packages (fixed priority order, since the loaded set is a HashSet). `stats::rnorm` (merged into base) resolves under the stripped name. has_function_anywhere mirrors this for the RY070 function/value separation. - An unbound identifier that names a loaded-package function now resolves to a function value (e.g. purrr's `map` used as a value). Tests: 5 new ry-typeshed tests (dplyr/purrr/bayes-prefix-strip/unknown/ is_known); a unit test pinning dplyr::filter vs stats::filter resolving differently (data.frame vs opaque); corpus fixture ok_package_qualified_resolution.R covering dplyr/purrr/mirai/brms/ posterior/loo/bayesplot + stats. Audit script path updated. Oracle, both vendor snapshots, clippy, fmt all green.
added 8 commits
July 5, 2026 00:29
…RY080 PLAN 2.3 -- the parallel-execution story. Workflow code reads `map(sims, in_parallel(function(s) fit_one(s)))` and must check identically to the sequential version. Extended HigherOrderFunc (renamed recognition to from_call(name, loaded)) with the purrr family, recognized ONLY when purrr is loaded or the call is `purrr::`-qualified (a bare `map` without purrr is not misread): - map/map_if/imap -> list of callback returns (lapply semantics, incl. column schema for element-type discovery). - map_lgl/int/dbl/chr -> typed vector of the target mode, len = .x; map_vec -> opaque. - map2/pmap -> list; keep/discard/compact -> same type as .x; reduce/accumulate -> callback return; walk -> invisible first arg. - in_parallel(.f) -> type-transparent wrapper (returns .f unchanged). The callback body is walked for diagnostics via the existing walk_callback_for_diagnostics machinery (extended with purrr callback positions .f@1, .p@1, etc.). New unwrap_in_parallel helper lets callback_return_type and the walker see through an in_parallel(.f) wrapper so the inner function is checked. RY080 (map-return-type-mismatch): a typed-map callback whose return mode is incompatible with the target (e.g. character into map_dbl) warns; numeric modes coerce harmlessly and stay silent. New rule + modes_compatible/mode_suffix helpers. mirai minimal signatures ship in mirai.json (Phase 2.2); corpus fixture ok_mirai_minimal.R pins daemons/mirai/collect_mirai. Tests: 5 purrr unit tests (callback walking, map_dbl typing, RY080, in_parallel transparency, ungated fallthrough); corpus fixtures ok_purrr_map_dbl/in_parallel/err_purrr_map_dbl_type_mismatch/ok_mirai; oracle fixtures purrr_map_use/purrr_in_parallel/mirai_daemons (CI oracle job now installs purrr+mirai). Gate green incl. oracle with R.
PLAN 2.5. Hand-writing JSON does not scale past base R. scripts/ gen_typeshed.R takes a package name, enumerates its exported functions via getNamespaceExports(), and emits a DRAFT typeshed JSON skeleton -- one entry per function with formals()-derived params and an opaque return type for a human to refine. Base-R only (no jsonlite dependency); hand-serialized for stable key ordering. Curated companion to audit_typeshed.R: the generator drafts, the auditor verifies. Phase 2.4 (Bayesian stack signatures for brms/posterior/loo/bayesplot/ cmdstanr) shipped in Phase 2.2's bayes.json; verified the demo workflow (brm -> as_draws_df -> summarise_draws -> loo -> ppc_dens_overlay) checks clean. CI: smoke-test the generator on stats (always installed) in the oracle job, alongside the audit step. Gate green incl. oracle with R.
…s.rs PLAN Phase 3.1 (partial). The first, lowest-coupling slice of the lib.rs split: Severity, Diagnostic, SeverityFilter, the entire inline- suppression machinery (Suppression + parse_suppressions[_from_comments] + has_file_suppression[_from_comments] + is_suppressed + filter_suppressed[_with_comments]), and apply_filter_to_diagnostics move to a new self-contained diagnostics.rs module. lib.rs re-exports them at the crate root for back-compat. No behavior change: test counts identical before/after, oracle with R green, clippy -D warnings and fmt clean. lib.rs drops from ~8010 to ~7580 lines. The deeper split (Scope/FnTable/Checker core into scope.rs/walk.rs/ infer/* -- the bulk of the remaining ~7580 lines) is a higher-risk mechanical surgery: nearly every helper is a method on Checker with shared private-field access, and Rust's cross-file impl blocks make that fragile to split under a tight gate. Deferring that to a dedicated focused pass rather than risk a half-broken tree here.
PLAN Phase 3.2. Both embarrassingly-parallel loops now run on rayon: - Project::check pass 3: per-file diagnostic emitters share the Arc-backed FnTable/ReturnSlots/loaded set read-only, so the emission loop is a par_iter. Results are re-sorted to input file order for deterministic output. - CLI run_check_once: the parse loop uses a rayon thread-local parser pool (tree-sitter parsers are not Send, so one RParser lives per rayon thread via thread_local!). Parsed files re-sort to input order. rayon added as a workspace dependency; threaded into ry-checker and ry-cli. perf.rs gets a before/after note; the 2s budgets are unchanged (parallelism is a multi-file/multi-core bonus, not a license to regress single-file latency). No behavior change: test counts identical, oracle green, clippy -D warnings, fmt clean.
…el); serial fallback
PLAN Phase 3.3 (dogfooding). The oracle suite spawned one Rscript
--vanilla per fixture serially (~8s wall, growing). Replaced with a
single driver, scripts/oracle_driver.R, that evaluates every fixture
through purrr::map(files, mirai::in_parallel(...)) -- the same pattern
the tool itself checks (Phase 2.3).
The driver reports errors structurally (one JSON object per fixture on
stdout: {file, errored, message}), retiring the locale-dependent
stderr-contains-"Error" heuristic of the serial path.
ISOLATION: mirai daemons persist across fixtures, so a fixture that
calls daemons() (nested-daemon error), uses <<- (shared global), or
library() (shared search path) cannot be isolated in a daemon. The
driver scans each fixture's source and routes side-effecting fixtures
to a fresh Rscript --vanilla subprocess (the serial path, fully
isolated). Side-effect-free fixtures (the majority) take the parallel
path.
oracle.rs prefers the driver and falls back to the serial per-fixture
path when purrr/mirai are absent or the driver exits nonzero (exit 3
signals deps missing). Oracle: 43/43 satisfied, 3.65s wall locally.
CI oracle job already installs purrr+mirai (Phase 2.3).
…ry rule alias PLAN Phase 4 (4 of 9 items, the high-value low-risk slice): - (2) `full` is now the default output format (was `concise`); the caret line underlines the WHOLE span (`^~~~~`) instead of a single `^`, via a new span_char_width helper. `concise` remains via --output-format/config. - (3) Human diagnostics now go to STDOUT (was stderr), so `ry check > log` captures them; the summary line and watch chrome stay on stderr (matches ruff/ty). Machine formats already used stdout. - (5) `ry check --statistics`: per-rule counts (sorted by count desc) printed to stderr after the run -- essential for corpus triage. - (7) `ry rule` is now an alias for `ry explain-rule` (matches `ruff rule`). config.rs DEFAULT_OUTPUT_FORMAT and the no-subcommand Check default move to "full"; e2e tests updated to assert on stdout for diagnostic content. Remaining Phase 4 items (real color, did-you-mean suggestions, notify-based watch, Rmd/Qmd, LSP workspace scan) are larger and deferred to a focused pass. Gate green; clippy/fmt clean.
Post-review hardening and documentation for the first tagged version. - fix %in% result length: logical of length(x), not the RHS length (kills the RY002/RY032 false positives the purrr vendor net caught) - suppress the RY001 coercion warning on the idiomatic numeric truthiness patterns if (length(x)) / if (nrow(x)) / if (ncol(x)) - typeshed: add identity and unclass; remove or correct 14 fabricated entries across the purrr, mirai, and bayes stubs; audit_typeshed.R now verifies package stubs against installed namespaces, not just base R - oracle driver: replace the unguarded parallelly dependency with base parallel::detectCores() so the parallel path works everywhere - docs: add ARCHITECTURE.md, CONTRIBUTING.md, ROADMAP.md; README.Rmd is now the README source, rendered with the real binary so every embedded output block is live; drop PLAN.md in favor of ROADMAP.md - bump workspace version to 0.1.0
The oracle job installed purrr/mirai with a bare install.packages(), which warns instead of erroring when installation fails. On CI the install failed silently: the parallel driver fell back to serial and purrr_in_parallel.R failed its must-pass tag because R had no purrr. - setup-r now uses public RSPM binaries (no compile-from-source) - package install goes through r-lib/actions/setup-r-dependencies, which fails the job on install errors, handles system requirements, and caches the library; an explicit requireNamespace guard follows, mirroring the existing Rscript-on-PATH guard - the typeshed audit moves after the install so purrr/mirai/dplyr stubs are audited in CI, not skipped as uninstalled - harness: fixtures that declare a CRAN package this machine lacks are now SKIPPED with a note instead of failed -- an environment gap is not a semantic ry-vs-R disagreement (CI installs everything, so a skip can never mask a regression there); skip count appears in the summary line
sims1253
force-pushed
the
feature/plan-implementation
branch
from
8c85831 to
741f808
Compare
added 2 commits
July 5, 2026 22:12
…le failures
Root cause of the second CI failure: purrr::in_parallel() requires
carrier::crate() at call time, but carrier is only a SUGGESTS of purrr,
so installing purrr did not bring it in. requireNamespace("purrr")
succeeded (hence 0 skips), yet both the parallel driver and the
purrr_in_parallel fixture errored the moment in_parallel() ran.
- CI installs carrier (and the import guard checks it)
- oracle_driver.R requires carrier in its exit-3 guard so a missing
package means a clean serial fallback, not a crash
- the harness now says WHY things fail: driver unavailability prints
the driver's exit code and stderr; a must-pass violation includes
R's actual error message (driver JSON message field, or the serial
path's captured stderr). Two CI rounds were spent guessing at
failures these lines would have named immediately.
Verified by simulating the broken environment locally (empty R_LIBS):
driver reports its reason, purrr/mirai fixtures skip loudly, suite
passes 40/40 with 3 skips; with packages present, 43/43 in parallel.
Strip every reference to the deleted PLAN.md and its phase numbering
from code comments, plus stale process narrative ('EXPECTED TO FAIL
until Phase X', 'fixed in Phase 1.1', 'the earlier form of this test').
Several comments stated things that have been false for two rounds
(function bodies 'currently NOT walked', the parser 'is O(n^2)',
parser regression tests 'EXPECTED TO FAIL against the current code').
Comments now describe the code as it is: regression-guard notes keep
the WHY (what bug they pin) without the project-management framing,
and the vendor triage blocks describe the current snapshots only.
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.
No description provided.