Commit the executable path before the optional exposure work

Removing the early self$exe_file(exe) left compile_standalone's call to
expose_stan_functions() ahead of the assignment in the tail, so a
failure there installed the executable and then returned an object that
could not find it. A later $compile() would find that executable up to
date, take the adoption branch because exe_file_ was still empty, and
set existing_exe, after which $expose_functions() refused permanently.
Both optional exposures now run after every field describing the
installed executable is committed.

The cpp_options mismatch warning moves after the no-op branch records
cpp_options_ and exe_file_, for the reason the leftover-backup warning
is raised last: under options(warn = 2) it is an error, and raising it
earlier unwound with the object half-updated.

That warning, and the decision to record the requested options at all,
now key off whether options are available rather than whether they
arrived with this call. Options held from cmdstan_model(compile = FALSE)
are equally the caller's intent, and were being discarded; the
supplied-ness flag remains for the narrower question of whether a header
conflict occurred within a single call.

unlink() glob-expands by default, unlike the file.remove() it replaced,
so a model directory containing [, ], * or ? matched nothing and
reported success while a full copy of the previous executable stayed on
disk. Both call sites pass expand = FALSE.

Author: jgabry

NEWS.md

@@ -13,7 +13,7 @@ variables are, instead of erroring. (#1225)
 * Supplying a factor for a variable not declared as `int` is now an error. (#1225)
 * Factors are now accepted for length-1 `int` arrays (e.g. `array[1] int x`),
 which previously errored. (#1225)
-* The `CMDSTANR_NO_VER_CHECK` R option and environment variable are deprecated
+* The `CMDSTANR_NO_VER_CHECK` R option and environment variable are deprecated 
 as of CmdStanR 1.0.0; use the lowercase `cmdstanr_no_ver_check` forms instead.
 * `$compile()` now works with named `stanc_options` values such as
 `canonicalize`. The values were shell-quoted for Make and the same quoted
@@ -68,17 +68,17 @@ resolved when the model object is created or `$compile()` is called rather than
 on each `stanc` call. Previously a model created from a relative path could
 resolve `#include` directives against the wrong directory if the working
 directory changed. (#1229)
-* `$cpp_options()` no longer includes a `STAN_VERSION` entry read from the model
+* `$cpp_options()` no longer includes a `STAN_VERSION` entry read from the model 
 executable's metadata. It was never a C++ option; use `$cmdstan_version()` instead. (#1215)
-* CmdStanModel methods now use executable metadata regardless of the
-capitalization of C++ option names. Any executable reporting threading enabled
+* CmdStanModel methods now use executable metadata regardless of the 
+capitalization of C++ option names. Any executable reporting threading enabled 
 requires the corresponding `threads` or `threads_per_chain` argument. (#765, #1100)
-* Pathfinder fits used as initial values now use uniform weights when CmdStan
+* Pathfinder fits used as initial values now use uniform weights when CmdStan 
 already PSIS-resampled their draws, avoiding a second application of importance weights. (#1206)
-* Pathfinder fits used as initial values now correctly treat draws with different
-initialization parameter values as distinct even when their log weights are equal,
+* Pathfinder fits used as initial values now correctly treat draws with different 
+initialization parameter values as distinct even when their log weights are equal, 
 and collapse duplicate resampled draws while retaining their selection frequency. (#1207)
-* `pathfinder()` now passes separately supplied initial values to every path
+* `pathfinder()` now passes separately supplied initial values to every path 
 instead of using only the first path's initial values. (#1206)
 * `pathfinder()` now respects `save_single_paths = TRUE` instead of always
 passing `0` to CmdStan.
@@ -87,10 +87,10 @@ to be consistent with other methods.
 * The `num_paths` documentation for `pathfinder()` now notes that running
 multiple paths in parallel requires compiling with
 `cpp_options = list(stan_threads = TRUE)` and setting `threads`. (#896)
-* The `save_latent_dynamics` argument is now limited to `$sample()`,
-`$sample_mpi()`, and `$variational()`, matching the CmdStan algorithms
+* The `save_latent_dynamics` argument is now limited to `$sample()`, 
+`$sample_mpi()`, and `$variational()`, matching the CmdStan algorithms 
 that support diagnostic CSV output.
-* Informative error when exposing functions using names that are reserved
+* Informative error when exposing functions using names that are reserved 
 keywords (@VisruthSK, #1154)
 * `save_cmdstan_config` and `save_metric` default to `FALSE` but can be
 set to `TRUE` for an entire R session via new global options. (#1159)

R/model.R

@@ -618,6 +618,10 @@ compile <- function(quiet = TRUE,
   if (length(cpp_options) == 0 && !is.null(private$precompile_cpp_options_)) {
     cpp_options <- private$precompile_cpp_options_
   }
+  # Distinct from cpp_options_supplied, which is only about whether this call
+  # carried a conflicting header: options held from cmdstan_model(compile =
+  # FALSE) did not arrive with this call but are still the caller's intent.
+  cpp_options_available <- length(cpp_options) > 0
   if (length(stanc_options) == 0 && !is.null(private$precompile_stanc_options_)) {
     stanc_options <- private$precompile_stanc_options_
   }
@@ -726,13 +730,13 @@ compile <- function(quiet = TRUE,
     # stan_threads makes assert_valid_threads() run a threaded executable
     # single-threaded.
     recorded_cpp_options <-
-      if (cpp_options_supplied) cpp_options else private$cpp_options_
+      if (cpp_options_available) cpp_options else private$cpp_options_
 
     # Asking the executable about itself. Best effort, because
     # model_compile_info() runs it and errors outright rather than returning a
     # status when the file is not runnable.
     exe_info <- NULL
-    if (cpp_options_supplied || length(private$exe_file_) == 0) {
+    if (cpp_options_available || length(private$exe_file_) == 0) {
       exe_info <- tryCatch(
         model_compile_info(exe, self$cmdstan_version()),
         error = function(e) NULL
@@ -745,20 +749,15 @@ compile <- function(quiet = TRUE,
     # STAN_THREADS, runs single-threaded. Rebuilding on a mismatch is the real
     # fix and is still outstanding (see the skipped tests in
     # test-model-recompile-logic.R); until then, say so.
-    if (cpp_options_supplied && length(exe_info) > 0) {
+    options_mismatch <- FALSE
+    if (cpp_options_available && length(exe_info) > 0) {
       # model_compile_info() reports upper-case names while
       # exe_info_reflects_cpp_options() compares lower-case ones, so without
       # aligning them the comparison finds no overlap and always agrees.
       reported <- exe_info
       names(reported) <- tolower(names(reported))
-      if (!isTRUE(exe_info_reflects_cpp_options(reported, cpp_options))) {
-        warning(
-          "The existing executable was not built with the requested ",
-          "'cpp_options' and was not rebuilt, so they will have no effect. ",
-          "Use 'force_recompile = TRUE' to rebuild the model.",
-          call. = FALSE
-        )
-      }
+      options_mismatch <-
+        !isTRUE(exe_info_reflects_cpp_options(reported, cpp_options))
     }
 
     if (length(private$exe_file_) == 0) {
@@ -775,6 +774,17 @@ compile <- function(quiet = TRUE,
     }
     private$cpp_options_ <- recorded_cpp_options
     private$exe_file_ <- exe
+    # Warned about only once the state above is recorded: under
+    # options(warn = 2) this is an error, and raising it earlier would unwind
+    # with the object half-updated.
+    if (options_mismatch) {
+      warning(
+        "The existing executable was not built with the requested ",
+        "'cpp_options' and was not rebuilt, so they will have no effect. ",
+        "Use 'force_recompile = TRUE' to rebuild the model.",
+        call. = FALSE
+      )
+    }
     return(invisible(self))
   } else {
     if (rlang::is_interactive()) {
@@ -918,10 +928,6 @@ compile <- function(quiet = TRUE,
     private$precompile_cpp_options_ <- NULL
     private$precompile_stanc_options_ <- NULL
     private$precompile_include_paths_ <- NULL
-
-    if (compile_standalone) {
-      expose_stan_functions(self$functions, verbose = !quiet)
-    }
   } # End - if(!dry_run)
 
   # Both are exceptions to the rule that state describing the compiled artifact
@@ -932,6 +938,13 @@ compile <- function(quiet = TRUE,
   private$exe_file_ <- exe
 
   if (!dry_run) {
+    # Both exposures are optional and fallible, and both run only once every
+    # field describing the installed executable has been committed -- including
+    # exe_file_ above. Failing here must not leave the object unable to find an
+    # executable that is sitting on disk.
+    if (compile_standalone) {
+      expose_stan_functions(self$functions, verbose = !quiet)
+    }
     if (compile_model_methods) {
       expose_model_methods(env = private$model_methods_env_, verbose = !quiet)
     }

R/utils.R

@@ -307,7 +307,7 @@ install_executable <- function(from, to) {
   # left rather than implying it is gone and sending the user looking for a file
   # that is still there.
   discard_candidate <- function() {
-    if (unlink(candidate) == 0L) {
+    if (unlink(candidate, expand = FALSE) == 0L) {
       ""
     } else {
       paste0(" The staged copy has been left at '", candidate, "'.")
@@ -376,7 +376,7 @@ install_executable <- function(from, to) {
     )
   }
 
-  if (!is.null(backup) && unlink(backup) != 0L) {
+  if (!is.null(backup) && unlink(backup, expand = FALSE) != 0L) {
     return(backup)
   }
   NULL