Another issue I noticed while working on #1235. I asked Claude to write up the report below:
$cpp_options() can list the same option twice, under two spellings, once the
model has merged what the executable reports about itself.
Reproduction
The ordinary threading workflow is enough:
mod$compile(cpp_options = list(stan_threads = TRUE), force_recompile = TRUE)
names(mod$cpp_options())
#> [1] "stan_threads" "STAN_THREADS"
model_compile_info() upper-cases the keys it parses out of <exe> info, so the
merge adds STAN_THREADS beside the stan_threads that came from the request.
Why it is cosmetic
Every consumer resolves it correctly:
cpp_option_value() matches case-insensitively and deliberately takes the last
match, with a comment saying why.
parsed_cpp_options() reduces to last-wins, so only one assignment reaches
Make.
built_cpp_options_ holds only what was passed on the command line and never
the merged copy, so the mismatch detection in the no-op path never sees the
duplicate.
Nothing wrong is compiled and nothing wrong is reported by
assert_valid_threads(). It is what a user sees when they print
$cpp_options().
The trap in the obvious fix
Deduplicating case-insensitively in merge_exe_info_cpp_options() looks like a
two-line change. It is, but the direction matters and is easy to get backwards.
The merge appends the metadata key after the request key, so cpp_option_value()'s
last-wins gives the executable precedence over the request. That is the right
precedence for a field whose job is to describe the artifact on disk rather than
what was asked for. A dedupe that keeps the first occurrence — the natural way to
write it — silently inverts that.
It would also be invisible in tests: the merge only writes options the binary
reports as enabled, so in practice the two entries almost always carry the same
value, and a test would have to construct disagreeing values on purpose to catch
the inversion.
Anything touching this should also check exe_info_reflects_cpp_options() and the
inherited / explicit name filtering in the no-op path, both of which key off
the names in these lists.
Scope
Pre-existing: on master the same duplicate arises through initialize() and the
no-op path. #1235 added the merge after a successful compile as well, so it now
also appears after $compile(force_recompile = TRUE) on an existing object, where
it previously did not — and #1235's NEWS entry draws attention to $cpp_options()
reporting more than it used to.
Possibly best folded into #1238 rather than fixed on its own: once the build
record is read from a sidecar instead of parsed back out of <exe> info, the
upper-casing that creates the second spelling goes away, and the dedupe question
changes shape.
Another issue I noticed while working on #1235. I asked Claude to write up the report below:
$cpp_options()can list the same option twice, under two spellings, once themodel has merged what the executable reports about itself.
Reproduction
The ordinary threading workflow is enough:
model_compile_info()upper-cases the keys it parses out of<exe> info, so themerge adds
STAN_THREADSbeside thestan_threadsthat came from the request.Why it is cosmetic
Every consumer resolves it correctly:
cpp_option_value()matches case-insensitively and deliberately takes the lastmatch, with a comment saying why.
parsed_cpp_options()reduces to last-wins, so only one assignment reachesMake.
built_cpp_options_holds only what was passed on the command line and neverthe merged copy, so the mismatch detection in the no-op path never sees the
duplicate.
Nothing wrong is compiled and nothing wrong is reported by
assert_valid_threads(). It is what a user sees when they print$cpp_options().The trap in the obvious fix
Deduplicating case-insensitively in
merge_exe_info_cpp_options()looks like atwo-line change. It is, but the direction matters and is easy to get backwards.
The merge appends the metadata key after the request key, so
cpp_option_value()'slast-wins gives the executable precedence over the request. That is the right
precedence for a field whose job is to describe the artifact on disk rather than
what was asked for. A dedupe that keeps the first occurrence — the natural way to
write it — silently inverts that.
It would also be invisible in tests: the merge only writes options the binary
reports as enabled, so in practice the two entries almost always carry the same
value, and a test would have to construct disagreeing values on purpose to catch
the inversion.
Anything touching this should also check
exe_info_reflects_cpp_options()and theinherited/explicitname filtering in the no-op path, both of which key offthe names in these lists.
Scope
Pre-existing: on
masterthe same duplicate arises throughinitialize()and theno-op path. #1235 added the merge after a successful compile as well, so it now
also appears after
$compile(force_recompile = TRUE)on an existing object, whereit previously did not — and #1235's NEWS entry draws attention to
$cpp_options()reporting more than it used to.
Possibly best folded into #1238 rather than fixed on its own: once the build
record is read from a sidecar instead of parsed back out of
<exe> info, theupper-casing that creates the second spelling goes away, and the dedupe question
changes shape.