From 1de7fc2e5eff9bbd03bf2b354d823f59255aa93b Mon Sep 17 00:00:00 2001 From: jgabry Date: Wed, 23 Sep 2026 10:09:49 -0600 Subject: [PATCH 1/2] Make cmdstan_version_compare() refuse a missing version It returned -1 for an empty or NA first argument and 1 for an empty second one, standing in for "no install found" during path discovery. That is the same -1 an older version gets, so a caller could not tell the two apart. Both arguments must now be non-empty strings, since utils::compareVersion() itself treats "" as older. cmdstan_default_path() handles a missing native or WSL install before comparing, and the startup check wraps the whole comparison in try(). Closes #1260. --- R/path.R | 22 +++++++++++----------- R/zzz.R | 13 ++++++++----- tests/testthat/test-path.R | 1 + 3 files changed, 20 insertions(+), 16 deletions(-) diff --git a/R/path.R b/R/path.R index e08e7822b..84bb1c2c0 100644 --- a/R/path.R +++ b/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 } diff --git a/R/zzz.R b/R/zzz.R index ba5c44c77..decc6cec6 100644 --- a/R/zzz.R +++ b/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." diff --git a/tests/testthat/test-path.R b/tests/testthat/test-path.R index 14b25dad6..bae7c3f95 100644 --- a/tests/testthat/test-path.R +++ b/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", { From 9925293f5ae2a21e05aa533aa6dcf615c88cf2ae Mon Sep 17 00:00:00 2001 From: jgabry Date: Wed, 23 Sep 2026 10:21:26 -0600 Subject: [PATCH 2/2] Update the design notes for the stricter version comparison The notes still described cmdstan_version_compare() as returning -1 for a missing version and the install-path code as relying on that. Both are false since the comparison started refusing a missing version, so the passage now records the old behavior as the removed instance of the tri-state collapse and says where the callers handle a missing version instead. The contract file did not change because the passage sits outside the contract markers. Part of #1260. --- dev-notes/compilation-state.md | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/dev-notes/compilation-state.md b/dev-notes/compilation-state.md index a417d24cb..a03a741be 100644 --- a/dev-notes/compilation-state.md +++ b/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).