Replace Snapshot::_unsafe_create with create_for_testing. (pantsb…

…uild#18995)

When run in `MODE=debug`, (most) tests using `Snapshot::_unsafe_create`
were failing a debug assertion for `Digest` equality.

`Snapshot`s can now calculate their own `Digest`s without a `Store`, so
replace `_unsafe_create` with `create_for_testing`, which calculates the
`Digest` for the `Snapshot`.

src/python/pants/backend/project_info/peek_test.py

@@ -15,14 +15,14 @@
 from pants.base.specs import RawSpecs, RecursiveGlobSpec
 from pants.core.target_types import ArchiveTarget, FilesGeneratorTarget, FileTarget, GenericTarget
 from pants.engine.addresses import Address
-from pants.engine.fs import Digest, Snapshot
+from pants.engine.fs import Snapshot
 from pants.engine.internals.dep_rules import DependencyRuleAction, DependencyRuleApplication
 from pants.engine.rules import QueryRule
 from pants.testutil.rule_runner import RuleRunner
 
 
 def _snapshot(fingerprint: str, files: tuple[str, ...]) -> Snapshot:
-    return Snapshot._unsafe_create(Digest(fingerprint.ljust(64, "0"), 1), files, ())
+    return Snapshot.create_for_testing(files, ())
 
 
 @pytest.mark.parametrize(
@@ -74,7 +74,7 @@ def _snapshot(fingerprint: str, files: tuple[str, ...]) -> Snapshot:
                       "bar.txt",
                       "foo.txt"
                     ],
-                    "sources_fingerprint": "2000000000000000000000000000000000000000000000000000000000000000",
+                    "sources_fingerprint": "d3dd0a1f72aaa1fb2623e7024d3ea460b798f6324805cfad5c2b751e2dfb756b",
                     "sources_raw": [
                       "*.txt"
                     ]
@@ -111,7 +111,7 @@ def _snapshot(fingerprint: str, files: tuple[str, ...]) -> Snapshot:
                     "sources": [
                       "foo.txt"
                     ],
-                    "sources_fingerprint": "1000000000000000000000000000000000000000000000000000000000000000",
+                    "sources_fingerprint": "b5e73bb1d7a3f8c2e7f8c43f38ab4d198e3512f082c670706df89f5abe319edf",
                     "sources_raw": [
                       "foo.txt"
                     ],
@@ -158,7 +158,7 @@ def _snapshot(fingerprint: str, files: tuple[str, ...]) -> Snapshot:
                     "target_type": "files",
                     "dependencies": [],
                     "sources": [],
-                    "sources_fingerprint": "0000000000000000000000000000000000000000000000000000000000000000",
+                    "sources_fingerprint": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
                     "sources_raw": [
                       "*.txt"
                     ],
@@ -239,7 +239,7 @@ def _snapshot(fingerprint: str, files: tuple[str, ...]) -> Snapshot:
                     "sources": [
                       "foo/a.txt"
                     ],
-                    "sources_fingerprint": "0000000000000000000000000000000000000000000000000000000000000000",
+                    "sources_fingerprint": "72ceef751c940b5797530e298f4d9f66daf3c51f7d075bfb802295ffb01d5de3",
                     "sources_raw": [
                       "*.txt"
                     ]

src/python/pants/core/goals/fix_test.py

@@ -28,7 +28,6 @@
 from pants.core.util_rules import source_files
 from pants.core.util_rules.partitions import PartitionerType
 from pants.engine.fs import (
-    EMPTY_DIGEST,
     EMPTY_SNAPSHOT,
     CreateDigest,
     Digest,
@@ -410,12 +409,8 @@ def test_no_targets() -> None:
 
 
 def test_message_lists_added_files() -> None:
-    input_snapshot = Snapshot._unsafe_create(
-        Digest("a" * 64, 1000), ["f.ext", "dir/f.ext"], ["dir"]
-    )
-    output_snapshot = Snapshot._unsafe_create(
-        Digest("b" * 64, 1000), ["f.ext", "added.ext", "dir/f.ext"], ["dir"]
-    )
+    input_snapshot = Snapshot.create_for_testing(["f.ext", "dir/f.ext"], ["dir"])
+    output_snapshot = Snapshot.create_for_testing(["f.ext", "added.ext", "dir/f.ext"], ["dir"])
     result = FixResult(
         input=input_snapshot,
         output=output_snapshot,
@@ -427,12 +422,8 @@ def test_message_lists_added_files() -> None:
 
 
 def test_message_lists_removed_files() -> None:
-    input_snapshot = Snapshot._unsafe_create(
-        Digest("a" * 64, 1000), ["f.ext", "removed.ext", "dir/f.ext"], ["dir"]
-    )
-    output_snapshot = Snapshot._unsafe_create(
-        Digest("b" * 64, 1000), ["f.ext", "dir/f.ext"], ["dir"]
-    )
+    input_snapshot = Snapshot.create_for_testing(["f.ext", "removed.ext", "dir/f.ext"], ["dir"])
+    output_snapshot = Snapshot.create_for_testing(["f.ext", "dir/f.ext"], ["dir"])
     result = FixResult(
         input=input_snapshot,
         output=output_snapshot,
@@ -444,14 +435,8 @@ def test_message_lists_removed_files() -> None:
 
 
 def test_message_lists_files() -> None:
-    # _unsafe_create() cannot be used to simulate changed files,
-    # so just make sure added and removed work together.
-    input_snapshot = Snapshot._unsafe_create(
-        Digest("a" * 64, 1000), ["f.ext", "removed.ext", "dir/f.ext"], ["dir"]
-    )
-    output_snapshot = Snapshot._unsafe_create(
-        Digest("b" * 64, 1000), ["f.ext", "added.ext", "dir/f.ext"], ["dir"]
-    )
+    input_snapshot = Snapshot.create_for_testing(["f.ext", "removed.ext", "dir/f.ext"], ["dir"])
+    output_snapshot = Snapshot.create_for_testing(["f.ext", "added.ext", "dir/f.ext"], ["dir"])
     result = FixResult(
         input=input_snapshot,
         output=output_snapshot,
@@ -530,8 +515,7 @@ class FixKitchenRequest(FixTargetsRequest):
 
 def test_streaming_output_changed(caplog) -> None:
     caplog.set_level(logging.DEBUG)
-    changed_digest = Digest(EMPTY_DIGEST.fingerprint, 2)
-    changed_snapshot = Snapshot._unsafe_create(changed_digest, [], [])
+    changed_snapshot = Snapshot.create_for_testing(["other_file.txt"], [])
     result = FixResult(
         input=EMPTY_SNAPSHOT,
         output=changed_snapshot,
@@ -540,7 +524,7 @@ def test_streaming_output_changed(caplog) -> None:
         tool_name="fixer",
     )
     assert result.level() == LogLevel.WARN
-    assert result.message() == "fixer made changes."
+    assert result.message() == "fixer made changes.\n  other_file.txt"
     assert ["Output from fixer\nstdout\nstderr"] == [
         rec.message for rec in caplog.records if rec.levelno == logging.DEBUG
     ]

src/python/pants/engine/fs_test.py

@@ -15,7 +15,7 @@
 from http.server import BaseHTTPRequestHandler
 from io import BytesIO
 from pathlib import Path
-from typing import Callable, Dict, Iterable, List, Optional, Set, Union
+from typing import Callable, Dict, Iterable, Optional, Set, Union
 
 import pytest
 
@@ -1399,49 +1399,20 @@ def test_digest_is_not_file_digest() -> None:
 
 
 def test_snapshot_properties() -> None:
-    digest = Digest("691638f4d58abaa8cfdc9af2e00682f13f07f96ad1d177f146216a7341ca4982", 154)
-    snapshot = Snapshot._unsafe_create(digest, ["f.ext", "dir/f.ext"], ["dir"])
-    assert snapshot.digest == digest
+    snapshot = Snapshot.create_for_testing(["f.ext", "dir/f.ext"], ["dir"])
+    assert snapshot.digest is not None
     assert snapshot.files == ("dir/f.ext", "f.ext")
     assert snapshot.dirs == ("dir",)
 
 
-def test_snapshot_hash() -> None:
-    def assert_hash(
-        expected: int,
-        *,
-        digest_char: str = "a",
-        files: Optional[List[str]] = None,
-        dirs: Optional[List[str]] = None,
-    ) -> None:
-        digest = Digest(digest_char * 64, 1000)
-        snapshot = Snapshot._unsafe_create(digest, files or ["f.ext", "dir/f.ext"], dirs or ["dir"])
-        assert hash(snapshot) == expected
-
-    # The digest's fingerprint is used for the hash, so all other properties are irrelevant.
-    assert_hash(-6148914691236517206)
-    assert_hash(-6148914691236517206, files=["f.ext"])
-    assert_hash(-6148914691236517206, dirs=["foo"])
-    assert_hash(-6148914691236517206, dirs=["foo"])
-    assert_hash(-4919131752989213765, digest_char="b")
-
-
-def test_snapshot_equality() -> None:
-    # Only the digest is used for equality.
-    snapshot = Snapshot._unsafe_create(Digest("a" * 64, 1000), ["f.ext", "dir/f.ext"], ["dir"])
-    assert snapshot == Snapshot._unsafe_create(
-        Digest("a" * 64, 1000), ["f.ext", "dir/f.ext"], ["dir"]
-    )
-    assert snapshot == Snapshot._unsafe_create(
-        Digest("a" * 64, 1000), ["f.ext", "dir/f.ext"], ["foo"]
-    )
-    assert snapshot == Snapshot._unsafe_create(Digest("a" * 64, 1000), ["f.ext"], ["dir"])
-    assert snapshot != Snapshot._unsafe_create(Digest("a" * 64, 0), ["f.ext", "dir/f.ext"], ["dir"])
-    assert snapshot != Snapshot._unsafe_create(
-        Digest("b" * 64, 1000), ["f.ext", "dir/f.ext"], ["dir"]
-    )
-    with pytest.raises(TypeError):
-        snapshot < snapshot  # type: ignore[operator]
+def test_snapshot_hash_and_eq() -> None:
+    one = Snapshot.create_for_testing(["f.ext"], ["dir"])
+    two = Snapshot.create_for_testing(["f.ext"], ["dir"])
+    assert hash(one) == hash(two)
+    assert one == two
+    three = Snapshot.create_for_testing(["f.ext"], [])
+    assert hash(two) != hash(three)
+    assert two != three
 
 
 @pytest.mark.parametrize(

src/python/pants/engine/internals/native_engine.pyi

@@ -87,9 +87,7 @@ class Snapshot:
     """
 
     @classmethod
-    def _unsafe_create(
-        cls, digest: Digest, files: Sequence[str], dirs: Sequence[str]
-    ) -> Snapshot: ...
+    def create_for_testing(cls, files: Sequence[str], dirs: Sequence[str]) -> Snapshot: ...
     @property
     def digest(self) -> Digest: ...
     @property

src/rust/engine/fs/store/src/snapshot.rs

@@ -177,14 +177,8 @@ impl Snapshot {
     }
   }
 
-  /// # Safety
-  ///
-  /// This should only be used for testing, as this will always create an invalid Snapshot.
-  pub unsafe fn create_for_testing_ffi(
-    digest: Digest,
-    files: Vec<String>,
-    dirs: Vec<String>,
-  ) -> Result<Self, String> {
+  /// Creates a snapshot containing empty Files for testing purposes.
+  pub fn create_for_testing(files: Vec<String>, dirs: Vec<String>) -> Result<Self, String> {
     // NB: All files receive the EMPTY_DIGEST.
     let file_digests = files
       .iter()
@@ -216,8 +210,7 @@ impl Snapshot {
       &file_digests,
     )?;
     Ok(Self {
-      // NB: The DigestTrie's computed digest is ignored in favor of the given Digest.
-      digest,
+      digest: tree.compute_root_digest(),
       tree,
     })
   }

src/rust/engine/src/externs/fs.rs

@@ -157,15 +157,10 @@ pub struct PySnapshot(pub Snapshot);
 #[pymethods]
 impl PySnapshot {
   #[classmethod]
-  fn _unsafe_create(
-    _cls: &PyType,
-    py_digest: PyDigest,
-    files: Vec<String>,
-    dirs: Vec<String>,
-  ) -> PyResult<Self> {
-    let snapshot =
-      unsafe { Snapshot::create_for_testing_ffi(py_digest.0.as_digest(), files, dirs) };
-    Ok(Self(snapshot.map_err(PyException::new_err)?))
+  fn create_for_testing(_cls: &PyType, files: Vec<String>, dirs: Vec<String>) -> PyResult<Self> {
+    Ok(Self(
+      Snapshot::create_for_testing(files, dirs).map_err(PyException::new_err)?,
+    ))
   }
 
   fn __hash__(&self) -> u64 {