Found during the design review for #1238. I asked Claude to write up the report below.
$check_syntax() and $format() silently inherit C++/stanc options that were
supplied for compilation, and $format() inherits the wrong ones.
What happens
Both read the constructor-options field directly:
$check_syntax(), R/model.R:1117 — if (length(stanc_options) == 0 && !is.null(private$precompile_stanc_options_))
$format(), R/model.R:1252 — stanc_options <- as.list(private$precompile_stanc_options_)
$check_syntax() at least falls back only when the caller supplied nothing.
$format() has no length check at all: it overwrites whatever the caller
passed with the constructor's options, unconditionally.
And because precompile_stanc_options_ is cleared by the first successful
compile (R/model.R:945), both methods see constructor options only, never ones
passed to $compile(), and see nothing at all once the model has been built.
So the current behaviour is not just leaky, it is inconsistent with itself
depending on whether a compile has happened yet.
Why it matters more after #1248
#1248 makes cpp_options and stanc_options persistent build configuration and
retires the precompile_* fields. The tempting migration is to repoint these two
reads at the new persistent field.
That would be the wrong fix. It would make build configuration permanently
leak into unrelated stanc operations — $format() would start reformatting under
whatever --O1 or optimisation flags were set for the last build.
Proposed contract
$check_syntax() and $format() do not inherit persistent build stanc
options. They use their own arguments.
- They do need the shared requirements that make the source parseable at all:
include paths, and --allow-undefined when a user header is configured. Those
are source configuration, not build configuration.
pedantic stays an invocation-only convenience argument on each method.
Scope
Independent of #1238 — needs no build provenance. Behaviour change, so worth
doing in the 1.0 window alongside #1248 rather than after, since #1248 removes
the field these two currently read and something has to be decided at that point
regardless.
The $format() missing length check looks like a plain bug and is arguably
fixable on its own today.
Found during the design review for #1238. I asked Claude to write up the report below.
$check_syntax()and$format()silently inherit C++/stanc options that weresupplied for compilation, and
$format()inherits the wrong ones.What happens
Both read the constructor-options field directly:
$check_syntax(),R/model.R:1117—if (length(stanc_options) == 0 && !is.null(private$precompile_stanc_options_))$format(),R/model.R:1252—stanc_options <- as.list(private$precompile_stanc_options_)$check_syntax()at least falls back only when the caller supplied nothing.$format()has no length check at all: it overwrites whatever the callerpassed with the constructor's options, unconditionally.
And because
precompile_stanc_options_is cleared by the first successfulcompile (
R/model.R:945), both methods see constructor options only, never onespassed to
$compile(), and see nothing at all once the model has been built.So the current behaviour is not just leaky, it is inconsistent with itself
depending on whether a compile has happened yet.
Why it matters more after #1248
#1248 makes
cpp_optionsandstanc_optionspersistent build configuration andretires the
precompile_*fields. The tempting migration is to repoint these tworeads at the new persistent field.
That would be the wrong fix. It would make build configuration permanently
leak into unrelated stanc operations —
$format()would start reformatting underwhatever
--O1or optimisation flags were set for the last build.Proposed contract
$check_syntax()and$format()do not inherit persistent build stancoptions. They use their own arguments.
include paths, and
--allow-undefinedwhen a user header is configured. Thoseare source configuration, not build configuration.
pedanticstays an invocation-only convenience argument on each method.Scope
Independent of #1238 — needs no build provenance. Behaviour change, so worth
doing in the 1.0 window alongside #1248 rather than after, since #1248 removes
the field these two currently read and something has to be decided at that point
regardless.
The
$format()missing length check looks like a plain bug and is arguablyfixable on its own today.