feat(migrations): require constant defaults, resolve complex ones in the body - #2784
Open
krlmlr wants to merge 7 commits into
Open
feat(migrations): require constant defaults, resolve complex ones in the body#2784krlmlr wants to merge 7 commits into
krlmlr wants to merge 7 commits into
Conversation
… body
The 23 non-constant defaults across the 22 migrated functions become
constant: `V(graph)`/`E(graph)` selectors, as_adjacency_matrix()'s
`igraph_opt("sparsematrices")`, and max_bipartite_match()'s
`.Machine$double.eps` are now declared as `NULL` in the signature and
resolved in the body once all arguments are available.
Behavior notes:
- Passing the selector NULL explicitly now selects the full vertex or
edge set, same as omitting the argument.
Previously an explicit NULL was coerced to an empty selection --
an accident of as_igraph_vs(), never documented or tested.
- This fixes legacy positional recovery of diversity(),
which died with a spurious "supplied more than once" error because
re-evaluating the V(graph) default never compares identical().
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016M32izVHZPfxAqemAe4BrX
Defaults in migrated (`new`) signatures must be constant expressions: literals, NULL/TRUE/FALSE/NA*/Inf/NaN, c()/list() of constants, a unary sign, or the deprecated() sentinel. The generator stops with an error on anything else -- option lookups, V(graph), cross-references, RNG draws -- and there is no escape hatch: complex defaults are declared as NULL and resolved in the body. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016M32izVHZPfxAqemAe4BrX
krlmlr
force-pushed
the
claude/migrations-const-defaults
branch
from
July 26, 2026 21:39
30194e8 to
915b5f2
Compare
10 tasks
Contributor
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 915b5f2 is merged into main:
|
Extends the constant-defaults treatment from the migrated functions to every exported hand-written function: 106 defaults across 67 functions in 23 files (games, layout, attributes, print, structural-properties, make, embedding, operators, plot, and 14 more) are now declared as `NULL` and resolved in the body after all arguments are available, plus the two `plot_dendrogram()` S3 methods for coherence with their generic. Registry entries of the 32 affected migrated functions carry the same `NULL` defaults, so the generated recovery blocks stay constant. Deliberate exceptions: - `layout_with_gem(temp.min = 1 / 10)` and `plot.igraph(mark.shape = 1 / 2)` become the literal constants `0.1` and `0.5`. - `layout_as_tree()`'s `root = numeric()` and `rootlevel = numeric()` stay: `is_constant_default()` now accepts zero-argument typed-empty constructors as literals. - `print.igraph(max.lines = )` and the `normalize()` limits resolve via `missing()` instead of `is.null()`: NULL is a legal value there (print all lines; skip normalization along an axis) and keeps its meaning. - `sample_motifs(cut.prob = NULL)` needs no body resolution: the C layer treats NULL as "no cuts", exactly like the old `rep(0, size)`, matching its `motifs()`/`count_motifs()` siblings. Behavior notes: - Passing NULL explicitly now means "use the default" for all treated arguments: selectors pick the full vertex/edge set (previously an accidental empty selection), option-backed arguments fall back to the igraph option (previously an empty combination spec or an error), and computed defaults are recomputed (previously usually an error). - `layout_with_drl()`'s random default seed is now drawn inside the function, after `ensure_igraph()`; the RNG stream consumption of defaulted calls is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016M32izVHZPfxAqemAe4BrX
`c()` evaluates to NULL, so the five empty-HRG fields that `fit_hrg()` builds when no starting model is given (`left`, `right`, `prob`, `edges`, `vertices` -- formerly `hrg()`'s signature defaults) declared NULL values that meant "empty sequence". NULL is now the resolve-in-body sentinel, so empty sequences are spelled `numeric()` instead (all five fields are `as.numeric()`-coerced on the spot), and tests pass `integer()` where an empty id selection is meant. `is_constant_default()` recognizes typed empties as literals, pinned together with the behavior in test-constant-defaults.R: an explicit typed empty keeps meaning "nothing selected", while passing NULL to mean "empty" is no longer supported for treated arguments -- a deliberate breaking change (`max_degree(g, v = NULL)` now means all vertices). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016M32izVHZPfxAqemAe4BrX
Contributor
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 4783887 is merged into main:
|
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.
Reworked per review direction: no grandfather mechanism.
Two commits:
1.
refactor:move the 23 non-constant defaults into the bodiesAll non-constant defaults across the 22 migrated functions on
mainbecome constant
NULLin the signature,resolved in the body once all arguments are available:
V(graph)/E(graph)selectors (19 arguments):alpha_centrality(),betweenness(),closeness(),diversity(),edge_betweenness(),harmonic_centrality(),page_rank(),power_centrality(),strength(),all_simple_paths(),all_shortest_paths(),constraint(),degree(),distances()(
vandto),ego(),ego_size(),knn(),make_ego_graph(),shortest_paths(),which_mutual();as_adjacency_matrix(sparse = igraph_opt("sparsematrices"));max_bipartite_match(eps = .Machine$double.eps).Body pattern, inserted right after the ARG_HANDLE block
(after
ensure_igraph()where present):Behavior notes for review:
NULLexplicitly now means the full vertex/edgeset, same as omitting the argument.
Previously an explicit
NULL(including a computedc()that came outempty) was coerced to an empty selection — an undocumented, untested
accident of
as_igraph_vs(). The new semantics are pinned intest-constant-defaults.R; if you'd rather keep the accidentalempty-selection meaning, say so and I'll switch the resolution to a
missing()check instead.NULLmeaning"no vertex" for optional single-vertex parameters
(
random_spanning_tree(vid = NULL)). Plural selectors saying"
NULL= all" vs. singular saying "NULL= none" is a distinctionworth an explicit line in the docs policy.
main: legacy positional recovery ofdiversity()died with a spurious "supplied more than once" errorbecause re-evaluating its
V(graph)default never comparesidentical(). With a constant default the recovery machinery needs nofurther changes — the defect class fix(migrations): detect supplied tail args with
missing()#2783 addressed disappears at theroot, per "avoid nonconst defaults at all".
2.
feat:the generator enforces the rule — no escape hatchis_constant_default()(recursive AST check): constant = literals,NULL/TRUE/FALSE/NA*/Inf/NaN,c()/list()of constants,typed empty vectors (
integer(),numeric(), ...),unary sign, parentheses, the
deprecated()sentinel.Everything else errors at generation time —
option lookups,
V(graph), cross-references (bins * 2), RNG draws,rep(),.Machine$…, bare symbols includingT/F.A leftover
nonconst_defaultsfield errors with its own message.Documented in
tools/migrations/README.md(and the matching rule prose is on #2757).
Round 2: the whole package
After merging
main(which migrated the games/layout/make/… topicsbehind the ellipsis), the same treatment now covers every exported,
non-deprecated, hand-written function — the remaining 106 non-constant
defaults across 67 functions, migrated or not, plus the two
plot_dendrogram()S3 methods for coherence with their generic.The registry
newliterals of the 32 affected migrated functions carrythe same
NULLdefaults, so the regenerated recovery blocks stayconstant.
type.dist,pref.matrix,types,pref,agebins,p— cross-references, incl. thesample_*/spec-wrapper pairsvcount()/edge_density()-derived constants,drlseed + options,normalize()limitsindex = V(graph)/E(graph)accessors, getters and settersprint.igraph()'s sixigraph_opt()-backed argumentsbfs()/dfs()rho = parent.frame(),eids/vselectorsmake_graph()n/dirand the directed/undirected wrappers (missing()→is.null())cvec(referencesweights),options = arpack_defaults()compose()/disjoint_union()graph.attr.comb,reverse_edges(eids = )mark.col/mark.border(resolve after themark.groupscoercion),mark.shape = 1/2→0.5vselectors,contract()/plot_dendrogram()options,add_shape()clip/plothits_scores(),plot_hierarchy(),as_undirected(),local_efficiency(),graphlet_proj(),sample_motifs(),indent_print(),similarity(),simplify(),stochastic_matrix(),count_triangles()Special cases, handled deliberately: cross-referencing defaults resolve
in signature order (
areabeforerepulserad/cellsize,typesbefore
pref,agebinsbeforepref);layout_with_drl()'s randomdefault seed is now drawn inside the function with unchanged RNG stream
consumption for defaulted calls;
options = arpack_defaults()resolvesin the body;
layout_with_gem(temp.min = 1/10)andplot.igraph(mark.shape = 1/2)became the literals0.1/0.5insteadof
NULL;sample_motifs(cut.prob = NULL)needs no resolution becausethe C layer already treats
NULLas "no cuts" (matchingmotifs());and two places keep
NULLas a legal value and therefore resolve viamissing()instead ofis.null():print.igraph(max.lines = NULL)still prints everything, and
normalize(ymin = NULL)still disablesnormalization along that axis (as in
norm_coords()). Theexplicit-NULL flip described above now also applies to the
option-backed combination specs (
edge.attr.comb = NULLetc. used tomean an empty combination spec, dropping attributes).
c()defaults were NULL in disguisec()evaluates toNULL. The five empty-HRG fields thatfit_hrg()builds when no starting model is given (formerly
hrg()'s signaturedefaults
left = c(),right = c(), …) therefore declared NULL valuesthat historically meant "empty sequence". Under the new doctrine NULL
is the resolve-in-body sentinel, so NULL-as-empty and NULL-as-default
cannot coexist: empty-sequence defaults must be spelled as typed
empty vectors (
numeric()here — the fields areas.numeric()-coercedon the spot;
layout_as_tree()'sroot = numeric()was alreadyspelled right and the checker now accepts it as a literal). Tests pass
integer()where an empty id selection is meant(
max_degree(g, v = integer())is 0). PassingNULLexplicitly tomean "empty" is no longer supported for treated arguments — a
deliberate breaking change:
max_degree(g, v = NULL)now means allvertices, pinned in
test-constant-defaults.R.Remaining worklist (not this PR)
The exported hand-written remainder is zero: scanning every
function definition against the classifier finds no non-constant
default outside
R/aaa-*.R. What is left is the Stimulus-generated_impllayer — 103 defaults across 78 internal wrappers — and handlingthose in the generator (constant-ness check there, or dropping defaults
from
_implsignatures entirely) is still under discussion.Validation: generator idempotent (generate →
air→ generate is abyte-identical fixed point); full suite FAIL 0 · PASS 9250 (the only
error is the known sandbox network fetch in
test-foreign.R);gate-on
IGRAPH_LIFECYCLE_ERRORS=truemigration/constant-defaultsfilter FAIL 0 · PASS 151; 85 Rd files regenerated via
devtools::document().