Keep model state consistent with the executable, and stop dropping compile-time inputs

NEWS.md

@@ -23,13 +23,89 @@ as of CmdStanR 1.0.0; use the lowercase `cmdstanr_no_ver_check` forms instead.
 `canonicalize`. The values were shell-quoted for Make and the same quoted
 strings were also passed to `stanc` directly, which rejected them. (#1227)
 * `$compile()` now enables `allow-undefined` for user headers supplied through
-`cpp_options`, not just through the `user_header` argument. (#1227)
+`cpp_options`, not just through the `user_header` argument. `$check_syntax()`
+and `$format()` also now correctly enable `allow-undefined` for models that use
+a user header. (#1227, #1234)
 * `stanc` failures during `$compile()` are now reported immediately, with the
 `stanc` error message. Previously they surfaced several steps later. (#1227)
 * Errors for include paths that do not exist now report the resolved absolute
 path. (#1227)
 * Numeric `stanc_options` values such as `list("max-line-length" = 78)` are no
 longer dropped. (#1233)
+* `$compile()` now refreshes `$code()` and `$variables()` after a successful
+compilation. (#1228)
+* `$compile()` now discards standalone functions exposed from an earlier
+version of the Stan program. They must be exposed again with
+`$expose_functions()` after a recompilation. (#1228)
+* `$compile()` now reuses the include paths and the user header of the previous
+compilation when they are not supplied again. Recompiling a model that uses
+`#include` directives or a user header through the same object previously
+failed because those inputs were dropped. (#1234)
+* `$compile()` now recompiles when `include_paths` change. Previously the model
+went on using the executable built against the old paths while `$variables()`
+and `$include_paths()` described the new ones, so data and initial values were
+validated against a program that was not running. (#1235)
+* A `user_header` supplied to `cmdstan_model()` is now used by a later
+`$compile()`. Previously it was only honored when the model was compiled
+immediately. (#1234)
+* `$compile()` now accepts `user_header = NULL` to compile without a user
+header. Previously a header, once supplied, could not be removed. (#1235)
+* `$compile()` now recompiles when the user header changes. Previously a
+different header was ignored if the executable was otherwise up to date. (#1235)
+* `$compile()` now reduces duplicate `USER_HEADER`/`user_header` entries in
+`cpp_options` to the one actually used, so `$cpp_options()` no longer reports
+the ignored spelling after a successful compilation. (#1235)
+* A `$compile()` call that finds the executable up to date no longer erases
+`$cpp_options()`. (#1235)
+* `$expose_functions()` now works after a `$compile()` call that found the
+executable up to date. (#1235)
+* A failed compilation no longer moves `$exe_file()` or replaces the generated
+C++ used by `$hpp_file()` and `fit$init_model_methods()`. Previously a failure
+at the C++ stage left the old executable paired with model methods generated
+from the new program. (#1235)
+* `$compile()` now warns when `cpp_options` are supplied but the existing
+executable is up to date, so nothing is rebuilt and the options are not applied.
+The check is best effort. For an executable the model object compiled itself it
+compares the options passed to `Make` against those requested, and treats
+anything the binary reports but was never passed as inherited from `make/local`
+and so unchanged by a rebuild. For one adopted from an earlier session only the
+few `STAN_*` flags the binary reports can be checked, and anything else passes
+unremarked. It can also warn when nothing would in fact change: an option
+inherited from `make/local` that the binary does not report looks like a request
+the executable lacks, and one that was both passed explicitly and set in
+`make/local` looks like something a rebuild would drop when it would be
+inherited again. Use `force_recompile = TRUE` when a supplied option has to take
+effect. (#1235)
+* `$cpp_options()` now also reports options the executable was built with that
+were never passed to `$compile()`, such as those inherited from `make/local`,
+when the binary reports them. `$sample()` and friends previously refused
+`threads_per_chain` for an executable that did have threading. (#1019, #1235)
+* `$cpp_options()` no longer reports options the executable was not built with.
+Previously a request that did not rebuild the model was recorded as though it
+had, so `$sample()` could fail with "the model executable was built with
+threading enabled" for a binary that had no threading. (#1019, #1235)
+* `$format(overwrite_file = TRUE)` now refreshes `$variables()` along with
+`$code()`, which previously kept describing the program as it was before
+formatting. (#1235)
+* `$compile()` now errors if the newly compiled executable cannot be installed,
+restoring the previous executable. Previously the replacement was unchecked, so
+a failure could silently leave the model with no executable at all. (#1235)
+* `$compile()` now errors instead of installing an executable over a directory.
+An executable path that names a directory, which `$exe_file()` and
+`cmdstan_model(exe_file = )` both accept without checking, previously had that
+directory renamed aside as though it were the old executable and a file put in
+its place. (#1235)
+* `$compile()` now checks that it can record the compiled model before replacing
+the executable, so a failure at that point can no longer leave a new executable
+on disk that the model object knows nothing about. (#1235)
+* A duplicated `USER_HEADER` or `user_header` entry in `cpp_options` now selects
+the last one, matching what `Make` does with repeated assignments. Previously
+the first was compiled with and the rest were left in `cpp_options`. (#1235)
+* A `USER_HEADER` or `user_header` entry in `cpp_options` set to `NULL` now
+clears a previously configured user header instead of being ignored. It stands
+for an explicit `USER_HEADER=`, which `Make` takes as clearing anything set
+before it, so the model previously compiled with no header while continuing to
+report the old one. (#1235)
 * CmdStanModel methods now correctly handle `#include` directories with spaces
 in their paths. (#820)
 * `$include_paths()` now returns absolute paths, and relative include paths are
@@ -111,6 +187,7 @@ are recompiled lazily if needed. (#1158)
     - `stepsize` (`step_size`)
 
 
+
 # cmdstanr 0.9.0
 
 ## General Improvements/Changes

R/cpp_opts.R

@@ -73,6 +73,59 @@ model_compile_info <- function(exe_file, version) {
   info
 }
 
+# Merge build options reported by the executable. Ignore STAN_VERSION and false
+# flags (passing FLAG=FALSE back to CmdStan can enable the flag).
+merge_exe_info_cpp_options <- function(cpp_options, exe_info) {
+  for (option_name in names(exe_info)) {
+    value <- exe_info[[option_name]]
+    if (tolower(option_name) != "stan_version" &&
+        (!is.logical(value) || isTRUE(value))) {
+      cpp_options[[option_name]] <- value
+    }
+  }
+  cpp_options
+}
+
+# Normalize the flags sent to make. Assignment names are case-insensitive and
+# the last value wins. Nonassignments keep their order. Headers are handled
+# separately.
+parsed_cpp_options <- function(cpp_options) {
+  assignments <- list()
+  opaque <- character()
+  for (flag in cpp_options_to_compile_flags(cpp_options)) {
+    if (!grepl("^[A-Za-z_][A-Za-z0-9_]*=", flag)) {
+      opaque <- c(opaque, flag)
+      next
+    }
+    option_name <- tolower(sub("=.*$", "", flag))
+    if (option_name %in% c("user_header", "stan_version")) {
+      next
+    }
+    assignments[[option_name]] <- sub("^[^=]*=", "", flag)
+  }
+  list(assignments = assignments, opaque = opaque)
+}
+
+normalized_cpp_options <- function(cpp_options) {
+  parsed <- parsed_cpp_options(cpp_options)
+  reduced <- character()
+  if (length(parsed$assignments) > 0) {
+    reduced <- paste0(
+      names(parsed$assignments), "=",
+      unlist(parsed$assignments, use.names = FALSE)
+    )
+  }
+  c(sort(reduced), parsed$opaque)
+}
+
+# Omitted recorded options count as changes because cpp_options are one-shot.
+cpp_options_disagree <- function(requested, recorded) {
+  !identical(
+    normalized_cpp_options(requested),
+    normalized_cpp_options(recorded)
+  )
+}
+
 # convert to compile flags --------------------
 # from list(flag1=TRUE, flag2=FALSE) to "FLAG1=TRUE\nFLAG2=FALSE"
 cpp_options_to_compile_flags <- function(cpp_options) {
@@ -128,6 +181,78 @@ validate_cpp_options <- function(cpp_options) {
   cpp_options
 }
 
+# user headers ---------------------------------------------------------
+# Resolve one header and remove both header spellings from cpp_options.
+# Precedence is explicit user_header (including NULL), USER_HEADER,
+# user_header, then previous. `supplied` distinguishes NULL from omission.
+# `cpp_options_supplied` limits conflict warnings to this call.
+resolve_user_header <- function(user_header,
+                                supplied,
+                                cpp_options,
+                                cpp_options_supplied = TRUE,
+                                previous = NULL) {
+  # Use positions so duplicate options follow make's last-value-wins behavior.
+  upper_at <- which(names(cpp_options) == "USER_HEADER")
+  lower_at <- which(names(cpp_options) == "user_header")
+  last_of <- function(positions) {
+    if (length(positions) == 0) {
+      NULL
+    } else {
+      cpp_options[[positions[[length(positions)]]]]
+    }
+  }
+  # NULL is still present here because it emits an empty USER_HEADER= assignment.
+  has_upper <- length(upper_at) > 0
+  has_lower <- length(lower_at) > 0
+  from_upper <- last_of(upper_at)
+  from_lower <- last_of(lower_at)
+  conflict <- NULL
+  spelling <- "USER_HEADER"
+
+  if (supplied) {
+    if (cpp_options_supplied && (has_upper || has_lower)) {
+      conflict <- "argument"
+    }
+    header <- user_header
+  } else if (has_upper) {
+    if (has_lower) {
+      conflict <- "cpp_options"
+    }
+    header <- from_upper
+  } else if (has_lower) {
+    header <- from_lower
+    spelling <- "user_header"
+  } else {
+    header <- previous
+  }
+
+  # Validate the value now and check file existence when compiling.
+  if (!is.null(header)) {
+    checkmate::assert_string(header, .var.name = "user_header")
+  }
+  # Guarded because x[-integer(0)] is empty.
+  header_at <- c(upper_at, lower_at)
+  if (length(header_at) > 0) {
+    cpp_options <- cpp_options[-header_at]
+  }
+
+  list(
+    user_header = header,
+    spelling = spelling,
+    cpp_options = cpp_options,
+    conflict = conflict
+  )
+}
+
+warn_user_header_conflict <- function(conflict) {
+  if (identical(conflict, "argument")) {
+    warning("User header specified both via user_header argument and via cpp_options arguments")
+  } else if (identical(conflict, "cpp_options")) {
+    warning('User header specified both via cpp_options[["USER_HEADER"]] and cpp_options[["user_header"]].', call. = FALSE)
+  }
+  invisible(NULL)
+}
+
 # check specific options for validity ---------------------------------
 cpp_option_value <- function(cpp_options, option) {
   # CmdStanR input and executable metadata can use different casing. Prefer
@@ -206,11 +331,17 @@ exe_info_reflects_cpp_options <- function(exe_info, cpp_options) {
   }
   if (is.null(cpp_options)) return(TRUE)
 
-  cpp_options <- exe_info_style_cpp_options(cpp_options)[tolower(names(cpp_options))]
-  overlap <- names(cpp_options)[names(cpp_options) %in% names(exe_info)]
+  # Compare only options reported by the executable. Other options are unknown.
+  # Parse the emitted flags so duplicates and unnamed assignments match make.
+  assignments <- parsed_cpp_options(cpp_options)$assignments
+  reported <- intersect(names(assignments), tolower(names(exe_info)))
 
-  if (length(overlap) == 0) TRUE else all.equal(
-    exe_info[overlap],
-    cpp_options[overlap]
-  )
+  for (option_name in reported) {
+    # CmdStan treats any nonempty make value as enabled.
+    requested <- nzchar(assignments[[option_name]])
+    if (requested != isTRUE(cpp_option_value(exe_info, option_name))) {
+      return(FALSE)
+    }
+  }
+  TRUE
 }