fix(checker): track literal character conditions

[sims1253]

value <- "hello"; if (value) 1L was silent even though R rejects the condition. Track decoded character literals through ordinary assignments and identifier aliases, then apply the existing condition check to their values.

Keep these facts in the scope journal so branch rollback restores the right value. Writes, loops, branch merges, and deferred execution discard them. Calls with unknown effects also prevent later assignments from claiming a literal value, because a call can install an active binding. Tests cover accepted strings, aliases, backticks, chained assignment, sibling branches, closures, superassignment, and custom binding behavior.

Closes #425. Depends on #442 for the condition family's documented warning severity. Known NA constants remain tracked in #354, as described in #425.

Validation: workspace tests, Clippy with warnings denied, formatting, and the full R oracle pass. The oracle confirms accepted and rebound strings and the exact error for the rejected alias. The fixed 472-package corpus retains all 3,106 baseline diagnostics with no additions or removals; the regression is covered by focused fixtures.

The method-effect follow-up also passes a fresh workspace build, full R oracle, and all eight release performance tests. Its fixed 472-package comparison retains all 3,106 baseline diagnostics, with zero additions and zero removals.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 9e2f1ab0-1651-4e82-88c5-4dbf6625afa1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[pullfrog]

ℹ️ No blocking issues — one edge-case observation inline.

Reviewed changes

• Known-string tracking: new literal_values.rs module adds Scope::known_strings + literal_values_unknown with journal undo (Undo::KnownString/KnownStrings, Mark.literal_values_unknown), so d <- "hello"; if (d) reports RY001 through ordinary assignments and identifier aliases (condition_string_literal now resolves Expr::Ident via the scope).
• Invalidation points: calls with unknown effects, complex-target writes, binop/S3 dispatch, branch merges (statements, expressions, pipes, cloned-scope), loop entry/exit, closures, and per-name Scope::insert all discard the facts — soundness holds on every shape I could reach, including short-circuit clone/merge paths, sibling branches, and parenthesized-RHS assignments (which fail ordinary_assignment and wipe the map).
• Tests/oracle: 5 focused test fns including an exact-count + span-pinned sibling-branch leak test and an oracle fixture pinning R's exact "argument is not interpretable as logical" error; reverting infer/mod.rs to base fails the 3 positive tests, so they genuinely pin the fix.

Validated: 769 lib tests pass, clippy -D warnings clean, fmt --check clean. CHANGELOG entry matches the sibling ### Fixed style and cites #425.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using openai-compatible/glm-5.3 ｜ 𝕏}

[pullfrog]

ℹ️ No blocking issues — the one open thread from the prior review remains the only outstanding item.

Reviewed changes

• Guard short-circuit for unrelated assignments: both known_string computation sites in infer/mod.rs now call the cheap condition_string_literal lookup first and only run the expensive guards (ordinary_assignment source scan plus two operator_rebound binding scans) when a literal candidate exists, with the literal_values_unknown/effects_unknown check hoisted into a .filter. Behavior-preserving — set_known_string's internal guard still re-checks the flags after try_assign_value — and the flag-blocked and plain shapes behave identically to the prior commit under CLI probing.

Re-validated on the new head: 769 lib tests pass, clippy -D warnings clean, fmt --check clean. The informational active-binding thread from the prior review (S3-operator/replacement-function route) is unchanged by this commit and remains open for the author to weigh; nothing else new.

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using openai-compatible/glm-5.3 ｜ 𝕏}

[pullfrog]

ℹ️ No critical issues — the flagged routes are fixed; one residual invalidation route inline.

Reviewed changes

• Method-dispatch invalidation: new Scope::invalidate_literal_values_for_dispatch runs at infer_binop entry (both operands), after operand inference for unary operators, and after base inference for index extraction — invalidating literal facts whenever an operand's class is unknown or non-empty or its mode is opaque/union, so S3 method dispatch (+.flag, [.foo, $.bar, unary -.foo) can no longer leave a later plain assignment tracked. The skip case (provably empty class, concrete mode) matches R, where internal generics only dispatch on objects with a class attribute.
• Masked operators and replacement writes: the Expr::BinOp arm now invalidates literal values when has_explicit_operator_mask reports a shadowing binding (the same predicate that gates infer_custom_operator), and try_assign_value's complex-target arm sets the literal flag alongside the ops flag before any replacement-function route runs.
• Short-circuit merge propagation: merge_condition_assignments now propagates literal_values_unknown from the cloned &&/|| RHS scope back to the real scope, replacing the previous reliance on unconditional invalidation inside try_s3_binop_dispatch.
• Tests/oracle: new method_and_replacement_effects_block_later_literal_claims covers three active-binding-via-method shapes (S3 with class(d) <-, replacement `activate<-`, pure base::structure classing) plus a plain-numeric control that keeps the warning, and literal_conditions_after_methods.R joins the oracle fixtures (auto-discovered via read_dir).

Validated: 770 lib tests pass, clippy --all-targets -D warnings clean, fmt --check clean; reverting this commit's three source files to 72118c5 fails the new test, so it genuinely pins the fix; CLI probes confirm the originally flagged S3 route and the replacement route are now silent while the plain control still warns. The prior review's active-binding thread is addressed and resolved.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using openai-compatible/glm-5.3 ｜ 𝕏}

[pullfrog]

✅ No new issues found.

Reviewed changes

• Promise-forcing invalidation: infer_identifier's parameter/unbound-name arm (mod.rs:1716-1719) now calls invalidate_literal_values() alongside invalidate_ops_environment(), closing the last active-binding route — f <- function(x) { x; e <- "hello"; if (e) 1L } no longer warns when a caller's forced argument can install an active binding. The invalidation is order-sensitive as it should be: an assignment made before the parameter read keeps its fact (f <- function(x) { e <- "hello"; if (e) 1L; x } still warns — probe-verified), and reading an unbound name wipes prior facts, which matches the established "d <- 'hello'; mutate(); if (d)" model (a runtime-resolved name may be an active binding; a genuinely unbound one errors in R, leaving the rest dead and RY010 fired).
• Tests/oracle: new forced_promises_block_later_literal_claims covers parameter-read, unbound-read, and the oracle fixture — falsifiable (fails with infer/mod.rs reverted to 372dd19), and literal_conditions_after_promises.R pins R's acceptance of the default-promise active-binding shape via # oracle: must-pass. The sibling-branch test now binds flag <- TRUE first, so it no longer relies on reading an unknown binding being effect-free.

Validated on the new head: 771 lib tests pass, clippy -p ry-checker --all-targets -- -D warnings clean, fmt --check clean. The open thread from the prior review is addressed and resolved.

  ^{｜ View workflow run ｜ Using openai-compatible/glm-5.3 ｜ 𝕏}

[sims1253]

This PR extends the RY001 character-condition rule from direct string literals to bindings with known values: ordinary <-/= assignments of string literals (or of identifiers already holding a known string — so aliases, backticked names, and chained assignment work) are recorded in a new per-scope known_strings map, and condition_string_literal consults it when classifying if/while/if-expression conditions. The facts participate in the scope journal with LIFO undo, and are deliberately discarded on any unknown call, promise forcing, possible S3 dispatch, non-identifier write, loop entry/exit, branch merge, and deferred execution — sound reasoning, since a call can install an active binding whose setter swallows the later assignment (I verified with Rscript: with makeActiveBinding("e", ...), e <- "hello" succeeds silently and if (e) passes on the getter's value).

Findings

• [minor] crates/ry-checker/src/infer/mod.rs:87-93 — if (d <- "hello") 1L and while (d <- "hello") still pass silently, though R errors with argument is not interpretable as logical (verified with Rscript). condition_string_literal matches Expr::String/Expr::Ident but not an expression-position assignment, and the resulting character-scalar type is classified coercible. Since the assign arm (crates/ry-checker/src/infer/mod.rs:1992-2005) already records the fact for a later if (d) while inferring the condition, recursing into the RHS of an Assign binop here would close the gap nearly for free. (flag && (d <- "hello") is the same family via the cloned-scope path in crates/ry-checker/src/infer/binop.rs:664-676, which merges only binding types back.)

• [minor] crates/ry-checker/src/infer/mod.rs:766-767 and crates/ry-checker/src/infer/cloned_scope_reference.rs:61-67 — every if merge clears all known strings, including names neither branch touches: in d <- "hello"; if (flag) x <- 1L; if (d) 1L the second condition stays silent. The journal already records per-name Undo::KnownString entries and per-name assignment deltas (the capture loop skips them at crates/ry-checker/src/scope_journal.rs:447-448), so retaining facts for names with no assignment or fact change in either branch looks feasible. This errs conservative and matches the unchanged 472-package corpus — raising it as a precision opportunity, not a defect.

• [nit] crates/ry-checker/src/infer/mod.rs:1935-1940 — &format!("{symbol}") allocates per BinOp while literal facts are live, and for arithmetic/comparison ops infer_custom_operator (crates/ry-checker/src/infer/custom_operator.rs:69-80) immediately re-runs the identical has_explicit_operator_mask lookup with static &strs. A static quoted-symbol match (as there) plus reusing that result would avoid both the allocation and the duplicate lookup. The !scope.literal_values_unknown guard keeps the cost bounded in practice since the first unknown call makes it sticky.

• [question] crates/ry-checker/src/infer/mod.rs:1995 — <<- is excluded from tracking (*op == BinOpKind::Assign only) and additionally invalidates everything via the ordinary_assignment failure path. That looks correct (superassign writes an outer frame and <<- can be masked), but the PR description lists only calls/writes/loops/merges — is the blanket invalidation on every <<- intended as part of this change's contract?

• [nit] crates/ry-checker/src/lib.rs:313-314 — the two new Scope fields (literal_values_unknown, known_strings) are the only undocumented fields in the struct; the sticky-flag vs. cleared-map distinction is explained in the PR description but deserves a doc comment where the neighboring fields all have one.

Overall

The invalidation lattice is the hard part of this change and it checks out: journal undo for the new entries is LIFO-correct and pairs with the Mark flag restore (crates/ry-checker/src/scope_journal.rs:461-468 and :524), the dispatch gate in crates/ry-checker/src/literal_values.rs:11-18 only skips provably classless operands (S3 Ops dispatch requires a class attribute; environments/S4 are Mode::Opaque, so indexing them invalidates), and the accepted-string boundaries match R behavior I verified with Rscript (if ("NA") and whitespace-padded spellings error, the eight exact spellings pass, NA_character_ correctly deferred to #354). Test coverage — aliases, backticks, chaining, sibling-branch isolation, closures, superassignment, custom <-/+/replacement masks, forced promises, and oracle fixtures asserting exact error messages — is thorough.

This review was generated with AI assistance.