fix(checker): bind data arguments before masked expressions - #447
fix(checker): bind data arguments before masked expressions#447sims1253 wants to merge 2 commits into
Conversation
|
Warning Review limit reachedNext included review available in 36 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 (7)
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 new issues found.
Reviewed changes — the full PR against main (all 7 files), plus verification: the complete ry-checker test suite, clippy --all-targets -D warnings, fmt --check, and six probe scenarios exercising pipes, declared-source formula APIs, and the missing-data fallback. The R oracle fixtures could not be executed in this environment (no Rscript); CI's oracle job covers them.
- First-formal data-source fallback in
data_mask_source_arg— when a signature declares nodata_mask_source, the source is the first formal unless it is...or itself masked/quoted, then resolved by full R argument matching, so nameddata =/.data =/x =actuals supply the mask from any position while quoting helpers (aes(),vars(),nesting()) keep an unknown mask. I audited the entire vendored population: every signature where this fallback fires has the data frame as its first formal (dplyr.data, tidyrdata, basex/_data/data, joinsx, recipesrecipe— the latter harmlessly yielding an unknown mask). - Eager data binding in
infer_argument_types— the bound data argument is inferred once in the caller scope before any masked expression, reused for DataMask/TidySelect scopes and the user-S3 dispatch probe; the removedleading_argument_is_maskedclassification is subsumed by the new rule, and the conditional formula-API behavior (extras evaluate in the caller when declareddatais absent) is preserved. - Schema path in
infer_schema_call— data now comes from the bound formal instead ofargs.first(); when the data formal is unbound the schema path bails to the general path, which avoids the old false RY010 of inferring a masked expression as data (verified:dplyr::mutate(new = some_unbound_column)is silent). Joins andExpressionValueresults now use formal-mapped types viamatch_args_to_params, fixing namedx =/y =join order andwith(expr = ..., data = ...)return values. - Tests and oracle fixtures — the four new tests assert exact codes, message contents, and result schemas/columns, so they fail if the binding regresses; the two oracle fixtures pin the R-side behavior of named data arguments.
The stub contract documented on FunctionSig::data_mask_source ("Absent means the traditional first-argument data mask") now matches the implementation exactly.
openai-compatible/glm-5.3 ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — commit 4c97468, the delta since the prior review at 3bc3cf9, with the full-PR diff re-read for context. Re-verified locally: all 767 ry-checker unit tests plus the integration suites pass, cargo clippy --all-targets -- -D warnings and fmt --check are clean, and I reverted the new guard to confirm the added test genuinely bites — without it, tidyselect::vars_rename(c('a', 'b'), B = .data$b) fails with RY061 ("$ operator is invalid for atomic vectors of mode character").
- Kept the
.datapronoun non-atomic indplyr_data_mask_scope— the pronoun now adopts the data-source type only when it has known columns and is list-like or a data frame; otherwise it staysRType::unknown(). This closes a false RY061 from the first-formal fallback binding a character vector of column names (tidyselectvars_rename/vars-style APIs), where.data$blegitimately names a column rather than$-accessing the atomic vector. TheList || data.framedisjunction mirrors the pre-existingschema_is_completepredicate in the same function, so known-schema frames keep their pronoun and unknown-column frames lose nothing (member access there was already conservative). - Test and oracle pin — the new unit test asserts no RY060/RY061 for the
vars_renamecall and was verified to fail without the guard; the addednamed_data_masks.Rlines pin R's character-mask semantics end to end (.data$bresolves to the column name, resultc(a = "a", B = "b")), withsuppressWarningsabsorbing version-dependent deprecation noise.
openai-compatible/glm-5.3 ｜ 𝕏
1 participant
Data-mask expressions now use the argument bound to the data formal, even when it appears later in the call. For example,
dplyr::mutate(new = .data$missing, .data = d)reports RY060 whendhas a known schema without that column.The schema and ordinary call paths share the source lookup. Data arguments are checked once in the caller scope. Tidy-select helpers such as
dplyr::vars(1L, .data$column)keep an unknown mask, and named arguments preserve result schemas andwith()return types.Validation: workspace tests, Clippy with warnings denied, formatting, and the full R oracle pass. Tests cover valid and missing columns, absent data bindings, named joins, and quoting helpers. The frozen 472-package comparison is unchanged: 3,106 diagnostics, none added or removed. The comparison caught a valid
vars_rename()call with a character vector of names; the follow-up keeps its.datapronoun opaque and adds an R oracle control.Closes #417.