docs: record scalar guard limits and remaining prerequisites - #449
Conversation
|
Warning Review limit reachedNext included review available in 4 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
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. Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestions inline.
Reviewed changes
- New
docs/scalar-guards.mddocumenting RY032's parameter-pattern heuristic and its current boundaries: the package-mode false positive (length(x) == 1L && x == 1Lwith a barelength), the assertion-continuation false positive, the return-membership and alias/loop misses, plus the S3-dispatch argument for why qualifying the callee name alone cannot prove scalar length. Records the decision to keep #372/#351 open and cut them from the current milestone per #392's "requires a new analysis architecture → record the boundary" rule. - Trailing note in
docs/rules.mdpointing RY032 readers at the new page.
Every behavioral claim was verified: I reproduced all four boundary rows plus both prose coverage claims empirically against a built binary (ry check on standalone, package-mode (DESCRIPTION), base::length-qualified, assertion-before-use, return-membership, and alias/loop probes — outcomes match the doc and the PR's probe table exactly), confirmed the lookup narrative against resolves_to_base's documented order in crates/ry-checker/src/resolve.rs, and the prerequisites prose against the recorded scope dispositions on #372/#351/#392. The readme_rule_table guard passes — the added note sits below the parsed table, same shape as the existing RY003 paragraph. The documentation is accurate for this head; only the scope-wording point below is worth a touch before merge.
Verification detail
markdown
# Empirical probe outcomes (built from bf74949)
- f <- function(x) is.null(x) || is.na(x) -> RY032 fires (return-position coverage claim)
- f <- function(x) length(x) == 1L && x == 1L -> silent (guard recognized, standalone)
- same in a DESCRIPTION package dir -> RY032 fires (package-mode FP, row 1)
- stopifnot(...) then if (is.null(b) || b == 1) -> RY032 fires on the later use (row 2)
- f <- function(type) length(type) > 1 || !type %in% modes -> silent (row 3 miss)
- alias + loop reassignment then && comparison -> silent (row 4 miss)
- parameter-form base::length S3 disguise -> silent (matches probe table "qualified-s3-length | none")
Gate: cargo test -p ry-checker --test readme_rule_table -> 2 passed.
Mechanism: short_circuit_parameter_vector (crates/ry-checker/src/infer/binop.rs)
is purely syntactic over scope.is_parameter; resolves_to_base steps 1/6/8
(crates/ry-checker/src/resolve.rs) produce the package/qualified/import split.
ℹ️ Nitpicks
docs/rules.md:52is 83 characters where the file's prose wraps at roughly 70–78 (convention only, no CI gate); reflowing so the link starts its own line would match the neighboring paragraphs.- Precedent exists for a
**Documentation**changelog bullet when a user-facing doc page lands (CHANGELOG.md:177); an[Unreleased]bullet for the new page would be consistent, though optional for a docs-only change.
Mode: Review (initial) · Files reviewed: 2 · Commits reviewed: 1 · Base: main (92c822a) · Head: docs/scalar-guard-boundaries (bf74949) · Prior pullfrog review: none
openai-compatible/glm-5.3 ｜ 𝕏
sims1253
left a comment
sims1253
left a comment
There was a problem hiding this comment.
This PR adds a new user-facing page, docs/scalar-guards.md, that documents what the RY032 parameter-pattern heuristic covers and where it can over- or under-warn (package lookup for bare length, assertion continuation past stopifnot, empty-input membership returned to if, alias/loop reassignment), illustrates with an S3 length example why a qualified name is not a scalar proof, states the remaining prerequisites for #372/#351, and cross-links the page from the RY032 note in docs/rules.md.
Findings
- [minor]
docs/scalar-guards.md:47— "outside this milestone's implementation scope under the milestone's rule" is unresolvable from the page itself: the milestone is neither named nor linked, and the sentence will read stale once it closes. An evergreen wording ("deferred until #372/#351 land") plus an optional milestone link would keep the reference page accurate over time. - [nit]
docs/scalar-guards.md:20— "A barelengthmay come from the package or an import" names only own-definition and NAMESPACE-import masking. The actual gate,Checker::resolves_to_base(crates/ry-checker/src/resolve.rs:469), also refuses to prove base resolution whenever any package is attached (library()/require()seedbare_loaded) or the search path is unknown, so the warning can also appear in a file that merely loads an unrelated package. One extra clause would make the stated cause match the mechanism. - [nit]
docs/scalar-guards.md:20— the boundary is documented but not the demonstrated escape hatch:base::length(x) == 1LandimportFrom(base, length)are proven to suppress the package-context warning (crates/ry-checker/src/tests/type_inference.rs,parameter_guards_respect_scalar_membership_and_exact_length). Mentioning the qualified or explicitly-imported form as the current workaround would make this row actionable for users hit by the false positive. - [nit]
docs/scalar-guards.md:1— the page lacks the nav header line ([Getting started](../README.md) · [Usage](usage.md) · [Configuration](configuration.md)) that the other user-facing reference pages (rules.md,types.md,facts.md,usage.md,configuration.md) carry.
Overall
Everything I could check against the code and against R 4.6.1 is accurate: all four table rows match short_circuit_parameter_vector and the narrowing rules in crates/ry-checker/src/infer/binop.rs / infer/narrow.rs, the intro example and the S3 length example error in R exactly as described (is.null(x) || is.na(x) with a length-2 vector gives 'length = 2' in coercion to 'logical(1)'; FALSE || logical(0) is NA, so "missing-value result for an empty input" is precise), and the scalar-guards.md link plus both issue links resolve. With the milestone wording addressed this is ready.
This review was generated with AI assistance.
1 participant
The next-release screening reproduces #351 and #372 on main
92c822awith R 4.6.1. Keep both issues open and cut their implementation from this milestone under its explicit rule for work that requires broader analysis changes. Document the current user-visible boundaries and link them from RY032.The four #372 toggles still differ. Local/formal masks and S3 dispatch show why relaxing package lookup is insufficient; even qualified
base::lengthcan hide a vector behind an S3 method. #351 still has assertion-continuation and alias/loop gaps. Some non-if expressions are already covered, but the reported return-membership case is not.The runtime checks include valid NULL/scalar/default cases, vector-error controls, and a guarded assertion that rejects invalid input before use. Installed library discovery was disabled (
RY_NO_INSTALLED_LIBRARIES=1); there were no typeshed overrides. Each probe used its own package or standalone directory. This is a 15-case semantic audit, not a new corpus reduction claim.The documented prerequisites are callee/class provenance and effect invalidation (#372), followed by length facts across assertions, aliases, and loop joins (#351). No diagnostics are suppressed and neither issue is closed by this documentation PR.
Validation: every runtime outcome above was asserted by Rscript; the checker JSON was captured against the same sources. Documentation links and diff whitespace checked. Baseline binary SHA-256:
27d28fd7045a1c009b33226279cd88570064d4de57a2ec4fc9b9c88833c9aeae.