Fix gem rewrite deleting a ;-joined declaration (#826)

crates/socket-patch-cli/tests/e2e_redirect_gem_build.rs

@@ -440,6 +440,13 @@ enum Driver {
     ScanVexHeredocDeclaration,
     /// A double-quoted interpolation can itself contain a heredoc opener.
     ScanVexInterpolatedHeredocDeclaration,
+    /// A second declaration joined to the gem's line by `;` (#826): the
+    /// line rewrite would delete it. Same contract as
+    /// [`Driver::ScanVexDuplicateDeclaration`].
+    ScanVexSemicolonJoinedDeclaration,
+    /// [`Driver::ScanVex`] on a declaration ending in a bare `;` and a
+    /// comment (#826): a complete declaration, so it is redirected.
+    ScanVexTrailingSemicolonDeclaration,
     /// [`Driver::ScanVexDualBoot`] with `BUNDLE_GEMFILE=Gemfile` exported to
     /// socket-patch too (#507): bundler's local app config outranks the
     /// environment, so bundler still loads `Gemfile.next` and the run must
@@ -471,6 +478,12 @@ impl Driver {
             Driver::ScanVexDualBootEnvGemfile => {
                 "scan --mode hosted (config Gemfile.next, env BUNDLE_GEMFILE=Gemfile)"
             }
+            Driver::ScanVexSemicolonJoinedDeclaration => {
+                "scan --mode hosted (two `;`-joined gem declarations)"
+            }
+            Driver::ScanVexTrailingSemicolonDeclaration => {
+                "scan --mode hosted (gem line ending in `;`)"
+            }
             Driver::ScanVexCustomGitSource => "scan --mode hosted (gem from a custom git_source)",
         }
     }
@@ -809,6 +822,14 @@ async fn redirect_scanned_project(
             "source \"{}/upstream\"\n\ngem \"{DEP}\", require: \"#{{<<~REQUIRE_PATH}}\".chomp\n  vuln_gem\nREQUIRE_PATH\n",
             server.uri()
         ),
+        Driver::ScanVexSemicolonJoinedDeclaration => format!(
+            "source \"{}/upstream\"\n\ngem \"{DEP}\", \"{DEP_VERSION}\"; gem \"{TRANSITIVE}\", \"1.0.0\"\n",
+            server.uri()
+        ),
+        Driver::ScanVexTrailingSemicolonDeclaration => format!(
+            "source \"{}/upstream\"\n\ngem \"{DEP}\"; # the vulnerable one\n",
+            server.uri()
+        ),
         _ => format!("source \"{}/upstream\"\n\ngem \"{DEP}\"\n", server.uri()),
     };
     std::fs::write(proj.join(gemfile_name), gemfile_body).unwrap();
@@ -930,7 +951,9 @@ async fn redirect_scanned_project(
         | Driver::ScanVexConditionalDeclaration
         | Driver::ScanVexScopedConstantModifier
         | Driver::ScanVexHeredocDeclaration
-        | Driver::ScanVexInterpolatedHeredocDeclaration => vec![
+        | Driver::ScanVexInterpolatedHeredocDeclaration
+        | Driver::ScanVexSemicolonJoinedDeclaration
+        | Driver::ScanVexTrailingSemicolonDeclaration => vec![
             "scan",
             "--mode",
             "hosted",
@@ -992,7 +1015,8 @@ async fn redirect_scanned_project(
         | Driver::ScanVexConditionalDeclaration
         | Driver::ScanVexScopedConstantModifier
         | Driver::ScanVexHeredocDeclaration
-        | Driver::ScanVexInterpolatedHeredocDeclaration => {
+        | Driver::ScanVexInterpolatedHeredocDeclaration
+        | Driver::ScanVexSemicolonJoinedDeclaration => {
             Some("redirect_gem_unrecognized_declaration")
         }
         _ => None,
@@ -1073,7 +1097,7 @@ async fn redirect_scanned_project(
         );
     }
     match driver {
-        Driver::ScanVex => {
+        Driver::ScanVex | Driver::ScanVexTrailingSemicolonDeclaration => {
             assert_eq!(env["vex"]["statements"], 1, "vex block: {env}");
             assert_eq!(
                 env["vex"]["verified"], false,
@@ -1089,7 +1113,8 @@ async fn redirect_scanned_project(
         | Driver::ScanVexConditionalDeclaration
         | Driver::ScanVexScopedConstantModifier
         | Driver::ScanVexHeredocDeclaration
-        | Driver::ScanVexInterpolatedHeredocDeclaration => {
+        | Driver::ScanVexInterpolatedHeredocDeclaration
+        | Driver::ScanVexSemicolonJoinedDeclaration => {
             unreachable!("asserted and returned above")
         }
         Driver::GetUuid => {
@@ -1877,6 +1902,53 @@ async fn gem_hosted_multi_line_declaration_is_refused_and_still_installs() {
     assert!(fx.is_none(), "the multi-line driver asserts in place");
 }
 
+/// #826: `gem "x"; gem "y"` must not be rewritten. The line rewrite
+/// deleted `gem "y"`, so the next frozen install failed.
+#[tokio::test(flavor = "multi_thread")]
+#[ignore = "host capstone: shells out to a real ruby/gem/bundler (>= 1.17); \
+            run with a pinned toolchain via --ignored"]
+async fn gem_hosted_semicolon_joined_declarations_are_refused_and_still_install() {
+    let fx = redirect_scanned_project(
+        "semicolon-joined",
+        Spelling::Gemfile,
+        false,
+        true,
+        None,
+        Driver::ScanVexSemicolonJoinedDeclaration,
+    )
+    .await;
+    assert!(fx.is_none(), "the `;`-joined driver asserts in place");
+}
+
+/// #826: `gem "x"; # c` is a complete declaration. Since #637 it was
+/// refused as continuing on the next line; it must redirect, and a fresh
+/// checkout must install the patched bytes.
+#[tokio::test(flavor = "multi_thread")]
+#[ignore = "host capstone: shells out to a real ruby/gem/bundler (>= 1.17); \
+            run with a pinned toolchain via --ignored"]
+async fn gem_hosted_trailing_semicolon_declaration_redirects_and_installs() {
+    let Some(fx) = redirect_scanned_project(
+        "trailing-semicolon",
+        Spelling::Gemfile,
+        false,
+        true,
+        None,
+        Driver::ScanVexTrailingSemicolonDeclaration,
+    )
+    .await
+    else {
+        return;
+    };
+    let (fresh, install) = fresh_checkout_bundle_install(&fx);
+    assert!(
+        install.status.success(),
+        "fresh-checkout `bundle install` must succeed from the patch registry.\nstdout:\n{}\nstderr:\n{}",
+        String::from_utf8_lossy(&install.stdout),
+        String::from_utf8_lossy(&install.stderr),
+    );
+    assert_patched_install(&fx, &fresh);
+}
+
 /// #340: a `gem` declaration with an `if` modifier must not be rewritten
 /// (the rewrite dropped the condition and declared the gem unconditionally).
 #[tokio::test(flavor = "multi_thread")]

crates/socket-patch-core/src/crawlers/gradle_cache.rs

@@ -70,8 +70,7 @@ pub fn hash_eq(dir_name: &str, sha1_hex: &str) -> bool {
 /// Whether `bytes` are the pristine download Gradle stored in the hash
 /// directory `dir_name` (their sha1 names it).
 pub fn pristine(dir_name: &str, bytes: &[u8]) -> bool {
-    use sha1::{Digest, Sha1};
-    hash_eq(dir_name, &hex::encode(Sha1::digest(bytes)))
+    hash_eq(dir_name, &crate::utils::digest::sha1_hex_of(bytes))
 }
 
 /// Whether `path` is a version directory of a `files-2.1` tree
@@ -432,8 +431,6 @@ impl DerivedIndex {
     /// The [`DerivedCopies`] of the jar `jar_leaf` whose pristine bytes
     /// hash to `pristine_sha1`.
     pub fn query(&self, jar_leaf: &str, pristine_sha1: &str) -> DerivedCopies {
-        use sha1::{Digest, Sha1};
-
         let instrumented = format!("instrumented-{jar_leaf}");
         let mut out = DerivedCopies {
             incomplete: self.incomplete,
@@ -460,7 +457,9 @@ impl DerivedIndex {
                 out.stale.push(path.clone());
             } else if name == jar_leaf || name == instrumented {
                 match crate::utils::fs::read_regular_to_bytes_sync(path) {
-                    Ok(bytes) if hash_eq(&hex::encode(Sha1::digest(&bytes)), pristine_sha1) => {
+                    Ok(bytes)
+                        if hash_eq(&crate::utils::digest::sha1_hex_of(&bytes), pristine_sha1) =>
+                    {
                         out.stale.push(path.clone())
                     }
                     Ok(_) => out.unknown.push(path.clone()),

crates/socket-patch-core/src/formats/gem/gemfile.rs

@@ -243,6 +243,7 @@ pub(crate) struct SourceOption {
 /// it may carry one. Positional arguments (`*V`, constants, method calls)
 /// are version constraints and never do.
 pub(crate) fn source_option(tail: &str) -> Option<SourceOption> {
+    let tail = &without_statement_end(tail);
     let Some(args) = args(tail) else {
         return Some(SourceOption {
             key: tail.trim().to_string(),
@@ -273,6 +274,7 @@ pub(crate) fn source_option(tail: &str) -> Option<SourceOption> {
 /// `path:`. Empty when the line carries none; bails to empty on an
 /// unparseable tail (unbalanced quote or bracket).
 pub(crate) fn trailing_options(tail: &str) -> String {
+    let tail = &without_statement_end(tail);
     let Some(args) = args(tail) else {
         return String::new();
     };
@@ -284,6 +286,43 @@ pub(crate) fn trailing_options(tail: &str) -> String {
         .unwrap_or_default()
 }
 
+/// `opts` minus a top-level `;` statement terminator (and any extra `;`s),
+/// keeping a trailing `#` comment. Both rewriters first refuse a tail where
+/// another statement follows the `;` (`gem_line_tail_blocks_edit`), so only
+/// `;`s, whitespace and a comment can follow it here (#826).
+fn without_statement_end(opts: &str) -> String {
+    let mut quote: Option<char> = None;
+    let mut depth: i64 = 0;
+    let mut chars = opts.char_indices();
+    while let Some((i, c)) = chars.next() {
+        if let Some(q) = quote {
+            if c == '\\' {
+                chars.next();
+            } else if c == q {
+                quote = None;
+            }
+            continue;
+        }
+        match c {
+            '#' => break,
+            '"' | '\'' => quote = Some(c),
+            '(' | '[' | '{' => depth += 1,
+            ')' | ']' | '}' => depth -= 1,
+            ';' if depth == 0 => {
+                let code = opts[..i].trim_end();
+                let rest = opts[i..].trim_start_matches(|c: char| c == ';' || c.is_whitespace());
+                return if rest.is_empty() {
+                    code.to_string()
+                } else {
+                    format!("{code} {rest}")
+                };
+            }
+            _ => {}
+        }
+    }
+    opts.to_string()
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -397,4 +436,30 @@ mod tests {
             "require: \"a,b\", group: [:x, :y]"
         );
     }
+
+    /// #826: a bare `;` ending the statement is not part of the options
+    /// (it would otherwise read as a positional `"7.0";` and be kept).
+    #[test]
+    fn trailing_options_drop_the_statement_terminator() {
+        // Nor is it an unreadable tail, which would fail closed as a
+        // source-selecting option.
+        for tail in [";", "; # c", ", \"0.8.1\";", ", require: false; # c"] {
+            assert_eq!(key(tail), None, "{tail:?}");
+        }
+        assert_eq!(key(", git: \"x\";"), Some("git:".into()));
+        for (tail, opts) in [
+            (", \"0.8.1\";", ""),
+            (", \"0.8.1\"; # c", ""),
+            (", require: false;", "require: false"),
+            (
+                ", require: false ;; # lazy; ok",
+                "require: false # lazy; ok",
+            ),
+            (", require: \"a;b\";", "require: \"a;b\""),
+            (", require: \"a;b\"", "require: \"a;b\""),
+            (", require: false # x;", "require: false # x;"),
+        ] {
+            assert_eq!(trailing_options(tail), opts, "{tail:?}");
+        }
+    }
 }

crates/socket-patch-core/src/patch/jvm_jar.rs

@@ -25,8 +25,6 @@
 use std::collections::HashMap;
 use std::path::{Path, PathBuf};
 
-use sha1::Digest as _;
-
 use crate::crawlers::gradle_cache;
 use crate::hash::git_sha256::compute_git_sha256_from_bytes;
 use crate::manifest::schema::PatchFileInfo;
@@ -353,12 +351,11 @@ fn unpatched_members(
 }
 
 fn sha256_hex(bytes: &[u8]) -> String {
-    use sha2::Digest as _;
-    hex::encode(sha2::Sha256::digest(bytes))
+    crate::utils::digest::sha256_hex_of(bytes)
 }
 
 fn sha1_hex(bytes: &[u8]) -> String {
-    hex::encode(sha1::Sha1::digest(bytes))
+    crate::utils::digest::sha1_hex_of(bytes)
 }
 
 /// `<socket_dir>/jvm-originals/<sha256>.jar`.