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

NEWS.md

@@ -19,13 +19,73 @@ 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)
 * CmdStanModel methods now correctly handle `#include` directories with spaces
 in their paths. (#820)
 * `$include_paths()` now returns absolute paths, and relative include paths are

R/cpp_opts.R

@@ -73,6 +73,77 @@ model_compile_info <- function(exe_file, version) {
   info
 }
 
+# Merge the options an executable reports about itself into the options already
+# recorded for it. STAN_VERSION describes the toolchain rather than a make
+# option, and a flag the executable reports as FALSE was never set at all, so
+# recording it would pass "FLAG=FALSE" to make, which CmdStan reads as enabling
+# 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
+}
+
+# The options a compilation would actually be run with, normalized so that a
+# request can be compared against what an executable was built with.
+#
+# Deliberately canonicalizes the output of cpp_options_to_compile_flags() rather
+# than reading the list itself: what make is handed is what decides whether two
+# builds differ, and any second reading of these lists drifts from the first.
+# Named and unnamed entries, duplicate names, vector values that expand into
+# several assignments and NULLs that expand into an empty NAME= are all already
+# resolved by the time the flags exist.
+#
+# Assignments are reduced last-wins, as a makefile does, and compared by
+# lower-cased name so that spelling is not a difference. Anything that is not an
+# assignment is opaque and keeps its order relative to the other opaque
+# arguments, though not its position among the assignments. Header entries are
+# dropped: header identity is tracked separately and forces a rebuild on its
+# own.
+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)
+}
+
+# Whether an executable built with `recorded` would differ from one built with
+# `requested`. Symmetric, because cpp_options are one-shot: a recompilation
+# carrying `requested` would drop anything `recorded` holds that it does not.
+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 +199,80 @@ validate_cpp_options <- function(cpp_options) {
   cpp_options
 }
 
+# user headers ---------------------------------------------------------
+# Decide which user header a compilation should use and reduce cpp_options to a
+# single, unambiguous source for it.
+#
+# Precedence:
+#   1. an explicit non-NULL `user_header` argument;
+#   2. an explicit `user_header = NULL`, which clears any header carried in
+#      cpp_options as well;
+#   3. only when the argument is omitted, cpp_options -- USER_HEADER ahead of
+#      user_header whichever order they appear in -- and then `previous`, the
+#      header the model already holds.
+#
+# `supplied` is what makes (2) expressible at all: `user_header = NULL` is also
+# the default, so the value alone cannot separate "cleared" from "not
+# mentioned". `cpp_options_supplied` separates a header passed in the same call,
+# a conflict worth warning about, from one inherited from an earlier call.
+#
+# Both spellings are always dropped from cpp_options; callers reinsert the
+# selected header under `spelling`, in whatever form they store. The header is
+# returned as supplied, neither made absolute nor WSL-safe, because callers
+# differ on which they need.
+resolve_user_header <- function(user_header,
+                                supplied,
+                                cpp_options,
+                                cpp_options_supplied = TRUE,
+                                previous = NULL) {
+  from_upper <- cpp_options[["USER_HEADER"]]
+  from_lower <- cpp_options[["user_header"]]
+  conflict <- NULL
+  spelling <- "USER_HEADER"
+
+  if (supplied) {
+    if (cpp_options_supplied && (!is.null(from_upper) || !is.null(from_lower))) {
+      conflict <- "argument"
+    }
+    header <- user_header
+  } else if (!is.null(from_upper)) {
+    if (!is.null(from_lower)) {
+      conflict <- "cpp_options"
+    }
+    header <- from_upper
+  } else if (!is.null(from_lower)) {
+    header <- from_lower
+    spelling <- "user_header"
+  } else {
+    header <- previous
+  }
+
+  # Shape is checked wherever a header is accepted; whether it exists is checked
+  # only when compiling, so that a header created between construction and
+  # $compile() still works.
+  if (!is.null(header)) {
+    checkmate::assert_string(header, .var.name = "user_header")
+  }
+  cpp_options[["USER_HEADER"]] <- NULL
+  cpp_options[["user_header"]] <- NULL
+
+  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 +351,21 @@ 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)]
+  # Only the assignments the binary can speak to. Anything else is left alone
+  # rather than reported as a mismatch: an adopted executable carries no record
+  # of what produced it, so an unreportable option is unverifiable, not wrong.
+  # Read through parsed_cpp_options() so that unnamed raw assignments, duplicate
+  # names and vector values mean here what they mean to 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 enables these whenever the make variable is non-empty, so an empty
+    # assignment is the only way to ask for one to be off.
+    requested <- nzchar(assignments[[option_name]])
+    if (requested != isTRUE(cpp_option_value(exe_info, option_name))) {
+      return(FALSE)
+    }
+  }
+  TRUE
 }