Make cmdstan_version_compare() refuse a missing version

R/path.R

@@ -195,15 +195,12 @@ cmdstan_version_for_comparison <- function(version) {
   sub("-rc[0-9]+$", "", version)
 }
 
-# Scalar comparison of versions numbers. Returns -1, 0, or 1.
-# Empty strings are used when no native or WSL install was found during path discovery.
+# Scalar comparison of version numbers. Returns -1, 0, or 1. Both
+# arguments must be versions, so a caller with a possibly missing one
+# checks that itself first.
 cmdstan_version_compare <- function(version, other) {
-  if (length(version) != 1 || is.na(version) || !nzchar(version)) {
-    return(-1L)
-  }
-  if (length(other) != 1 || is.na(other) || !nzchar(other)) {
-    return(1L)
-  }
+  checkmate::assert_string(version, min.chars = 1)
+  checkmate::assert_string(other, min.chars = 1)
   utils::compareVersion(
     cmdstan_version_for_comparison(version),
     cmdstan_version_for_comparison(other)
@@ -319,11 +316,14 @@ cmdstan_default_path <- function(dir = NULL) {
     if (!nzchar(latest_cmdstan) && !nzchar(latest_wsl_cmdstan)) {
       return(NULL)
     }
-    if (cmdstan_version_compare(latest_wsl_cmdstan, latest_cmdstan) >= 0) {
-      return(file.path(wsl_installs_path, latest_wsl_cmdstan))
-    } else {
+    if (!nzchar(latest_wsl_cmdstan)) {
       return(file.path(installs_path, latest_cmdstan))
     }
+    if (!nzchar(latest_cmdstan) ||
+        cmdstan_version_compare(latest_wsl_cmdstan, latest_cmdstan) >= 0) {
+      return(file.path(wsl_installs_path, latest_wsl_cmdstan))
+    }
+    return(file.path(installs_path, latest_cmdstan))
   }
   NULL
 }

R/zzz.R

@@ -35,11 +35,14 @@ startup_messages <- function() {
     )
   }
   if (!skip_version_check) {
-    latest_version <- try(suppressWarnings(latest_released_version(retries = 0)), silent = TRUE)
-    current_version <- try(cmdstan_version(), silent = TRUE)
-    if (!inherits(latest_version, "try-error")
-        && !inherits(current_version, "try-error")
-        && cmdstan_version_compare(latest_version, current_version) > 0) {
+    newer <- try(
+      cmdstan_version_compare(
+        suppressWarnings(latest_released_version(retries = 0)),
+        cmdstan_version()
+      ) > 0,
+      silent = TRUE
+    )
+    if (isTRUE(newer)) {
       packageStartupMessage(
         "\nA newer version of CmdStan is available. See ?install_cmdstan() to install it.",
         "\nTo disable this check set option or environment variable cmdstanr_no_ver_check=TRUE."

dev-notes/compilation-state.md

@@ -3835,22 +3835,22 @@ reintroducing `NA` for unknown in the record reintroduces the collapse. §8 uses
 `NA` in the public result for the opposite reason, and serialization is the whole
 of the difference: what never reaches a file keeps its R type.
 
-`cmdstan_version_compare()` is a third instance, in a different costume: it returns
-`-1` for a version that is missing, `NA`, or empty (`R/path.R:162-164`), so an
-absent version compares as older than everything and every `<` gate fires. That
-fallback is correct where it is used, in install-path code where "no CmdStan"
-should lose every comparison. It is wrong for a model.
-
-The guard is also narrower than "unusable", which matters because it removes the
-temptation to lean on it: a malformed non-empty string never reaches the `-1` at
-all. Both `".."` and `"garbage"` error inside `utils::compareVersion()`, with
-`missing value where TRUE/FALSE needed`; `"garbage"` emits `NAs introduced by
-coercion` first. So the bad-input behaviour is an error, sometimes with a warning
-in front of it, and never the `-1`. Filed as #1260, separately from this design:
-the comparison should not answer a question it was not asked, but fixing it is
-defence in depth rather than what closes this. That is why §7 makes "an adopted
-executable always yields a valid version" a checked invariant rather than an
-observation, and why a model without an executable cannot reach a gate (§8).
+`cmdstan_version_compare()` was a third instance, in a different costume: it
+returned `-1` for a version that was missing, `NA`, or empty, so an absent
+version compared as older than everything and every `<` gate fired. That
+fallback suited the install-path code, where "no CmdStan" should lose every
+comparison, and it was wrong for a model. #1260 removed it: both arguments
+must be non-empty strings, and the two callers that can meet a missing
+version, `cmdstan_default_path()` and the startup check for a newer release,
+handle that case before comparing. A malformed non-empty string never reached
+the `-1` in the first place: both `".."` and `"garbage"` error inside
+`utils::compareVersion()`, with `missing value where TRUE/FALSE needed`;
+`"garbage"` emits `NAs introduced by coercion` first. So the bad-input
+behaviour is an error, sometimes with a warning in front of it. Fixing the
+comparison is defence in depth rather than what closes this: that is why §7
+makes "an adopted executable always yields a valid version" a checked
+invariant rather than an observation, and why a model without an executable
+cannot reach a gate (§8).
 
 <!-- contract -->
 

tests/testthat/test-path.R

@@ -256,6 +256,7 @@ test_that("CmdStan version helpers handle invalid inputs", {
   expect_identical(cmdstan_min_version(), "2.35.0")
   expect_false(is_supported_cmdstan_version(NULL))
   expect_false(is_supported_cmdstan_version("not-a-version"))
+  expect_error(cmdstan_version_compare("", "2.35.0"))
 })
 
 test_that("CmdStan version helpers use numeric ordering", {