[nextest-filtering] reject unknown binary IDs and binary names

sunshowers · sunshowers · commit ccffb23acbdc · 2025-02-15T20:01:28.000-08:00
We would previously accept binary IDs and names that were not present in the
graph. That is definitely not right -- we should reject any that weren't known.

This may cause some minor breakage, but hopefully that will alert folks about
mistakes rather than be disruptive.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -61,7 +61,7 @@ fs-err = "3.1.0"
 future-queue = "0.4.0"
 futures = "0.3.31"
 globset = "0.4.15"
-guppy = "0.17.14"
+guppy = "0.17.16"
 hex = "0.4.3"
 home = "0.5.11"
 http = "1.2.0"
@@ -157,6 +157,7 @@ nextest-workspace-hack = { path = "workspace-hack" }
 
 # Uncomment for testing.
 # cargo_metadata = { path = "../cargo_metadata" }
+# guppy = { path = "../../guppy/guppy" }
 # future-queue = { path = "../future-queue" }
 # target-spec = { path = "../guppy/target-spec" }
 # target-spec-miette = { path = "../guppy/target-spec-miette" }
diff --git a/fixtures/nextest-tests/Cargo.toml b/fixtures/nextest-tests/Cargo.toml
@@ -24,7 +24,10 @@ test = true
 
 [[example]]
 name = "other"
-test = true
+# Note that setting test = false does not mean that tests are disabled. It just
+# means that `cargo nextest run` won't run tests by default. It will run tests
+# when `cargo nextest run --all-targets` is used.
+test = false
 
 # Make this crate its own workspace.
 [workspace]
@@ -34,7 +37,7 @@ members = [
     "derive",
     "dylib-test",
     "with-build-script",
-    "proc-macro-test"
+    "proc-macro-test",
 ]
 
 [dependencies]
diff --git a/integration-tests/tests/integration/main.rs b/integration-tests/tests/integration/main.rs
@@ -164,6 +164,38 @@ fn test_list_binaries_only() {
         .output();
 
     check_list_binaries_output(&output.stdout);
+
+    // Check error messages for unknown binary IDs.
+    let output = CargoNextestCli::for_test()
+        .args([
+            "--manifest-path",
+            p.manifest_path().as_str(),
+            "list",
+            "--cargo-quiet",
+            "--workspace",
+            "--all-targets",
+            "--message-format",
+            "json",
+            "--list-type",
+            "binaries-only",
+            // Doesn't exist.
+            "-E",
+            "binary(unknown) & binary_id(unknown) & binary(=unknown) | binary_id(=unknown)",
+            // Does exist.
+            "-E",
+            "binary_id(with-build-script) | binary_id(=with-build-script) | \
+             binary(nextest-tests) | binary(=nextest-tests)",
+            // First one doesn't exist, second one does.
+            "-E",
+            "binary_id(nextest-tests::does_not_exist) | binary_id(=nextest-tests::basic)",
+            // First one exists, second one doesn't.
+            "-E",
+            "binary_id(nextest-tests::example/*) | binary_id(dylib-test::example/*)",
+        ])
+        .unchecked(true)
+        .output();
+
+    insta::assert_snapshot!(output.stderr_as_str());
 }
 
 #[test]
@@ -321,7 +353,7 @@ fn test_run_no_tests() {
 
     let stderr = output.stderr_as_str();
     assert!(
-        stderr.contains("Starting 0 tests across 0 binaries (8 binaries skipped)"),
+        stderr.contains("Starting 0 tests across 0 binaries (7 binaries skipped)"),
         "stderr contains 'Starting' message: {output}"
     );
     assert!(
diff --git a/integration-tests/tests/integration/snapshots/integration__archive_includes.snap b/integration-tests/tests/integration/snapshots/integration__archive_includes.snap
@@ -1,6 +1,7 @@
 ---
 source: integration-tests/tests/integration/main.rs
 expression: output.stderr_as_str()
+snapshot_kind: text
 ---
    Archiving 17 binaries (including 2 non-test binaries), 2 build script output directories, 2 linked paths, 7 extra paths, and 1 standard library to <archive-file>
      Warning ignoring extra path `<target-dir>/excluded-dir` because it does not exist
diff --git a/integration-tests/tests/integration/snapshots/integration__archive_missing_includes.snap b/integration-tests/tests/integration/snapshots/integration__archive_missing_includes.snap
@@ -1,6 +1,7 @@
 ---
 source: integration-tests/tests/integration/main.rs
 expression: output.stderr_as_str()
+snapshot_kind: text
 ---
    Archiving 17 binaries (including 2 non-test binaries), 2 build script output directories, 2 linked paths, 1 extra path, and 1 standard library to <archive-file>
 error: error creating archive `<archive-file>`
diff --git a/integration-tests/tests/integration/snapshots/integration__archive_no_includes.snap b/integration-tests/tests/integration/snapshots/integration__archive_no_includes.snap
@@ -1,6 +1,7 @@
 ---
 source: integration-tests/tests/integration/main.rs
 expression: output.stderr_as_str()
+snapshot_kind: text
 ---
    Archiving 17 binaries (including 2 non-test binaries), 2 build script output directories, 2 linked paths, and 1 standard library to <archive-file>
      Warning linked path `<target-dir>/debug/build/<cdylib-link-hash>/does-not-exist` not found, requested by: cdylib-link v0.1.0
diff --git a/integration-tests/tests/integration/snapshots/integration__list_binaries_only.snap b/integration-tests/tests/integration/snapshots/integration__list_binaries_only.snap
@@ -0,0 +1,49 @@
+---
+source: integration-tests/tests/integration/main.rs
+expression: output.stderr_as_str()
+snapshot_kind: text
+---
+info: experimental features enabled: setup-scripts
+  error: operator didn't match any binary names
+   ╭────
+ 1 │ binary(unknown) & binary_id(unknown) & binary(=unknown) | binary_id(=unknown)
+   ·        ───┬───
+   ·           ╰── no binary names matched this
+   ╰────
+
+  error: operator didn't match any binary IDs
+   ╭────
+ 1 │ binary(unknown) & binary_id(unknown) & binary(=unknown) | binary_id(=unknown)
+   ·                             ───┬───
+   ·                                ╰── no binary IDs matched this
+   ╰────
+
+  error: operator didn't match any binary names
+   ╭────
+ 1 │ binary(unknown) & binary_id(unknown) & binary(=unknown) | binary_id(=unknown)
+   ·                                               ────┬───
+   ·                                                   ╰── no binary names matched this
+   ╰────
+
+  error: operator didn't match any binary IDs
+   ╭────
+ 1 │ binary(unknown) & binary_id(unknown) & binary(=unknown) | binary_id(=unknown)
+   ·                                                                     ────┬───
+   ·                                                                         ╰── no binary IDs matched this
+   ╰────
+
+  error: operator didn't match any binary IDs
+   ╭────
+ 1 │ binary_id(nextest-tests::does_not_exist) | binary_id(=nextest-tests::basic)
+   ·           ──────────────┬──────────────
+   ·                         ╰── no binary IDs matched this
+   ╰────
+
+  error: operator didn't match any binary IDs
+   ╭────
+ 1 │ binary_id(nextest-tests::example/*) | binary_id(dylib-test::example/*)
+   ·                                                 ──────────┬──────────
+   ·                                                           ╰── no binary IDs matched this
+   ╰────
+
+error: failed to parse filterset
diff --git a/nextest-filtering/Cargo.toml b/nextest-filtering/Cargo.toml
@@ -35,6 +35,7 @@ proptest = { workspace = true, optional = true }
 recursion.workspace = true
 regex-syntax.workspace = true
 regex.workspace = true
+smol_str.workspace = true
 test-strategy = { workspace = true, optional = true }
 thiserror.workspace = true
 winnow.workspace = true
diff --git a/nextest-filtering/src/compile.rs b/nextest-filtering/src/compile.rs
@@ -12,6 +12,7 @@ use guppy::{
 };
 use miette::SourceSpan;
 use recursion::CollapsibleExt;
+use smol_str::SmolStr;
 use std::collections::HashSet;
 
 pub(crate) fn compile(
@@ -22,9 +23,9 @@ pub(crate) fn compile(
     let mut errors = vec![];
     check_banned_predicates(expr, kind, &mut errors);
 
-    let workspace_packages = &cx.make_cache().workspace_packages;
+    let cx_cache = cx.make_cache();
     let mut cache = cx.graph().new_depends_cache();
-    let expr = compile_expr(expr, workspace_packages, &mut cache, &mut errors);
+    let expr = compile_expr(expr, cx_cache, &mut cache, &mut errors);
 
     if errors.is_empty() {
         Ok(expr)
@@ -110,29 +111,35 @@ fn rdependencies_packages(
 
 fn compile_set_def(
     set: &SetDef,
-    packages: &[PackageMetadata<'_>],
+    cx_cache: &ParseContextCache<'_>,
     cache: &mut DependsCache<'_>,
     errors: &mut Vec<ParseSingleError>,
 ) -> FiltersetLeaf {
     match set {
-        SetDef::Package(matcher, span) => FiltersetLeaf::Packages(expect_non_empty(
-            matching_packages(matcher, packages),
+        SetDef::Package(matcher, span) => FiltersetLeaf::Packages(expect_non_empty_packages(
+            matching_packages(matcher, &cx_cache.workspace_packages),
             *span,
             errors,
         )),
-        SetDef::Deps(matcher, span) => FiltersetLeaf::Packages(expect_non_empty(
-            dependencies_packages(matcher, packages, cache),
+        SetDef::Deps(matcher, span) => FiltersetLeaf::Packages(expect_non_empty_packages(
+            dependencies_packages(matcher, &cx_cache.workspace_packages, cache),
             *span,
             errors,
         )),
-        SetDef::Rdeps(matcher, span) => FiltersetLeaf::Packages(expect_non_empty(
-            rdependencies_packages(matcher, packages, cache),
+        SetDef::Rdeps(matcher, span) => FiltersetLeaf::Packages(expect_non_empty_packages(
+            rdependencies_packages(matcher, &cx_cache.workspace_packages, cache),
             *span,
             errors,
         )),
         SetDef::Kind(matcher, span) => FiltersetLeaf::Kind(matcher.clone(), *span),
-        SetDef::Binary(matcher, span) => FiltersetLeaf::Binary(matcher.clone(), *span),
-        SetDef::BinaryId(matcher, span) => FiltersetLeaf::BinaryId(matcher.clone(), *span),
+        SetDef::Binary(matcher, span) => FiltersetLeaf::Binary(
+            expect_non_empty_binary_names(matcher, &cx_cache.binary_names, *span, errors),
+            *span,
+        ),
+        SetDef::BinaryId(matcher, span) => FiltersetLeaf::BinaryId(
+            expect_non_empty_binary_ids(matcher, &cx_cache.binary_ids, *span, errors),
+            *span,
+        ),
         SetDef::Platform(platform, span) => FiltersetLeaf::Platform(*platform, *span),
         SetDef::Test(matcher, span) => FiltersetLeaf::Test(matcher.clone(), *span),
         SetDef::Default(_) => FiltersetLeaf::Default,
@@ -141,7 +148,7 @@ fn compile_set_def(
     }
 }
 
-fn expect_non_empty(
+fn expect_non_empty_packages(
     packages: HashSet<PackageId>,
     span: SourceSpan,
     errors: &mut Vec<ParseSingleError>,
@@ -152,16 +159,60 @@ fn expect_non_empty(
     packages
 }
 
+fn expect_non_empty_binary_names(
+    matcher: &NameMatcher,
+    all_binary_names: &HashSet<&str>,
+    span: SourceSpan,
+    errors: &mut Vec<ParseSingleError>,
+) -> NameMatcher {
+    let any_matches = match matcher {
+        NameMatcher::Equal { value, .. } => all_binary_names.contains(value.as_str()),
+        _ => {
+            // For anything more complex than equals, iterate over all the binary names.
+            all_binary_names
+                .iter()
+                .any(|binary_name| matcher.is_match(binary_name))
+        }
+    };
+
+    if !any_matches {
+        errors.push(ParseSingleError::NoBinaryNameMatch(span));
+    }
+    matcher.clone()
+}
+
+fn expect_non_empty_binary_ids(
+    matcher: &NameMatcher,
+    all_binary_ids: &HashSet<SmolStr>,
+    span: SourceSpan,
+    errors: &mut Vec<ParseSingleError>,
+) -> NameMatcher {
+    let any_matches = match matcher {
+        NameMatcher::Equal { value, .. } => all_binary_ids.contains(value.as_str()),
+        _ => {
+            // For anything more complex than equals, iterate over all the binary IDs.
+            all_binary_ids
+                .iter()
+                .any(|binary_id| matcher.is_match(binary_id))
+        }
+    };
+
+    if !any_matches {
+        errors.push(ParseSingleError::NoBinaryIdMatch(span));
+    }
+    matcher.clone()
+}
+
 fn compile_expr(
     expr: &ParsedExpr,
-    packages: &[PackageMetadata<'_>],
+    cx_cache: &ParseContextCache<'_>,
     cache: &mut DependsCache<'_>,
     errors: &mut Vec<ParseSingleError>,
 ) -> CompiledExpr {
     use crate::expression::ExprFrame::*;
 
     Wrapped(expr).collapse_frames(|layer: ExprFrame<&SetDef, CompiledExpr>| match layer {
-        Set(set) => CompiledExpr::Set(compile_set_def(set, packages, cache, errors)),
+        Set(set) => CompiledExpr::Set(compile_set_def(set, cx_cache, cache, errors)),
         Not(expr) => CompiledExpr::Not(Box::new(expr)),
         Union(expr_1, expr_2) => CompiledExpr::Union(Box::new(expr_1), Box::new(expr_2)),
         Intersection(expr_1, expr_2) => {
diff --git a/nextest-filtering/src/errors.rs b/nextest-filtering/src/errors.rs
@@ -118,6 +118,14 @@ pub enum ParseSingleError {
     #[error("operator didn't match any packages")]
     NoPackageMatch(#[label("no packages matched this")] SourceSpan),
 
+    /// This matcher didn't match any binary IDs.
+    #[error("operator didn't match any binary IDs")]
+    NoBinaryIdMatch(#[label("no binary IDs matched this")] SourceSpan),
+
+    /// This matcher didn't match any binary names.
+    #[error("operator didn't match any binary names")]
+    NoBinaryNameMatch(#[label("no binary names matched this")] SourceSpan),
+
     /// Expected "host" or "target" for a `platform()` predicate.
     #[error("invalid argument for platform")]
     InvalidPlatformArgument(#[label("expected \"target\" or \"host\"")] SourceSpan),
diff --git a/nextest-filtering/src/expression.rs b/nextest-filtering/src/expression.rs
diff --git a/nextest-filtering/tests/match.rs b/nextest-filtering/tests/match.rs
diff --git a/nextest-metadata/src/test_list.rs b/nextest-metadata/src/test_list.rs
diff --git a/workspace-hack/Cargo.toml b/workspace-hack/Cargo.toml
