[nextest-filtering] cache the list of workspace packages

Turn `ParseContext` into a cache store as shown here. In the future, it would
be nice to reuse a single `ParseContext` across many places.

Author: sunshowers

cargo-nextest/src/dispatch.rs

@@ -1548,15 +1548,12 @@ impl App {
     }
 
     fn build_filtering_expressions(&self) -> Result<Vec<Filterset>> {
-        let pcx = ParseContext {
-            graph: self.base.graph(),
-            kind: FiltersetKind::Test,
-        };
+        let pcx = ParseContext::new(self.base.graph());
         let (exprs, all_errors): (Vec<_>, Vec<_>) = self
             .build_filter
             .filterset
             .iter()
-            .map(|input| Filterset::parse(input.clone(), &pcx))
+            .map(|input| Filterset::parse(input.clone(), &pcx, FiltersetKind::Test))
             .partition_result();
 
         if !all_errors.is_empty() {

nextest-filtering/examples/parser.rs

@@ -53,11 +53,12 @@ fn main() {
     let args = Args::parse();
 
     let graph = load_graph(args.cargo_metadata);
-    let cx = nextest_filtering::ParseContext {
-        graph: &graph,
-        kind: nextest_filtering::FiltersetKind::Test,
-    };
-    match nextest_filtering::Filterset::parse(args.expr, &cx) {
+    let cx = nextest_filtering::ParseContext::new(&graph);
+    match nextest_filtering::Filterset::parse(
+        args.expr,
+        &cx,
+        nextest_filtering::FiltersetKind::Test,
+    ) {
         Ok(expr) => println!("{expr:?}"),
         Err(FiltersetParseErrors { input, errors, .. }) => {
             for error in errors {

nextest-filtering/src/compile.rs

@@ -17,17 +17,14 @@ use std::collections::HashSet;
 pub(crate) fn compile(
     expr: &ParsedExpr,
     cx: &ParseContext<'_>,
+    kind: FiltersetKind,
 ) -> Result<CompiledExpr, Vec<ParseSingleError>> {
     let mut errors = vec![];
-    check_banned_predicates(expr, cx.kind, &mut errors);
+    check_banned_predicates(expr, kind, &mut errors);
 
-    let in_workspace_packages: Vec<_> = cx
-        .graph
-        .resolve_workspace()
-        .packages(guppy::graph::DependencyDirection::Forward)
-        .collect();
-    let mut cache = cx.graph.new_depends_cache();
-    let expr = compile_expr(expr, &in_workspace_packages, &mut cache, &mut errors);
+    let workspace_packages = &cx.make_cache().workspace_packages;
+    let mut cache = cx.graph().new_depends_cache();
+    let expr = compile_expr(expr, workspace_packages, &mut cache, &mut errors);
 
     if errors.is_empty() {
         Ok(expr)

nextest-filtering/src/expression.rs

@@ -9,13 +9,13 @@ use crate::{
     },
 };
 use guppy::{
-    graph::{cargo::BuildPlatform, PackageGraph},
+    graph::{cargo::BuildPlatform, PackageGraph, PackageMetadata},
     PackageId,
 };
 use miette::SourceSpan;
 use nextest_metadata::{RustBinaryId, RustTestBinaryKind};
 use recursion::{Collapsible, CollapsibleExt, MappableFrame, PartiallyApplied};
-use std::{collections::HashSet, fmt};
+use std::{collections::HashSet, fmt, sync::OnceLock};
 
 /// Matcher for name
 ///
@@ -268,16 +268,51 @@ impl FiltersetLeaf {
 }
 
 /// Inputs to filterset parsing.
-#[derive(Copy, Clone, Debug)]
-pub struct ParseContext<'a> {
+#[derive(Debug)]
+pub struct ParseContext<'g> {
     /// The package graph.
-    pub graph: &'a PackageGraph,
+    graph: &'g PackageGraph,
 
-    /// What kind of expression this is.
-    ///
-    /// In some cases, expressions must restrict themselves to a subset of the full filtering
-    /// language. This is used to determine what subset of the language is allowed.
-    pub kind: FiltersetKind,
+    /// Cached data computed on first access.
+    cache: OnceLock<ParseContextCache<'g>>,
+}
+
+impl<'g> ParseContext<'g> {
+    /// Creates a new `ParseContext`.
+    #[inline]
+    pub fn new(graph: &'g PackageGraph) -> Self {
+        Self {
+            graph,
+            cache: OnceLock::new(),
+        }
+    }
+
+    /// Returns the package graph.
+    #[inline]
+    pub fn graph(&self) -> &'g PackageGraph {
+        self.graph
+    }
+
+    pub(crate) fn make_cache(&self) -> &ParseContextCache<'g> {
+        self.cache
+            .get_or_init(|| ParseContextCache::new(self.graph))
+    }
+}
+
+#[derive(Debug)]
+pub(crate) struct ParseContextCache<'g> {
+    pub(crate) workspace_packages: Vec<PackageMetadata<'g>>,
+}
+
+impl<'g> ParseContextCache<'g> {
+    fn new(graph: &'g PackageGraph) -> Self {
+        Self {
+            workspace_packages: graph
+                .resolve_workspace()
+                .packages(guppy::graph::DependencyDirection::Forward)
+                .collect(),
+        }
+    }
 }
 
 /// The kind of filterset being parsed.
@@ -311,7 +346,11 @@ pub struct EvalContext<'a> {
 
 impl Filterset {
     /// Parse a filterset.
-    pub fn parse(input: String, cx: &ParseContext<'_>) -> Result<Self, FiltersetParseErrors> {
+    pub fn parse(
+        input: String,
+        cx: &ParseContext<'_>,
+        kind: FiltersetKind,
+    ) -> Result<Self, FiltersetParseErrors> {
         let mut errors = Vec::new();
         match parse(new_span(&input, &mut errors)) {
             Ok(parsed_expr) => {
@@ -321,7 +360,7 @@ impl Filterset {
 
                 match parsed_expr {
                     ExprResult::Valid(parsed) => {
-                        let compiled = crate::compile::compile(&parsed, cx)
+                        let compiled = crate::compile::compile(&parsed, cx, kind)
                             .map_err(|errors| FiltersetParseErrors::new(input.clone(), errors))?;
                         Ok(Self {
                             input,

nextest-filtering/tests/match.rs

@@ -28,11 +28,8 @@ fn mk_pid(c: char) -> PackageId {
 }
 
 fn parse(input: &str, graph: &PackageGraph) -> Filterset {
-    let cx = ParseContext {
-        graph,
-        kind: FiltersetKind::Test,
-    };
-    let expr = Filterset::parse(input.to_owned(), &cx).unwrap();
+    let cx = ParseContext::new(graph);
+    let expr = Filterset::parse(input.to_owned(), &cx, FiltersetKind::Test).unwrap();
     eprintln!("expression: {expr:?}");
     expr
 }
@@ -393,21 +390,22 @@ fn test_expr_with_no_matching_packages() {
     }
 
     let graph = load_graph();
-    let cx = ParseContext {
-        graph: &graph,
-        kind: FiltersetKind::Test,
-    };
+    let cx = ParseContext::new(&graph);
 
-    let errors = Filterset::parse("deps(does-not-exist)".to_owned(), &cx).unwrap_err();
+    let errors =
+        Filterset::parse("deps(does-not-exist)".to_owned(), &cx, FiltersetKind::Test).unwrap_err();
     assert_error(&errors);
 
-    let errors = Filterset::parse("deps(=does-not-exist)".to_owned(), &cx).unwrap_err();
+    let errors =
+        Filterset::parse("deps(=does-not-exist)".to_owned(), &cx, FiltersetKind::Test).unwrap_err();
     assert_error(&errors);
 
-    let errors = Filterset::parse("deps(~does-not-exist)".to_owned(), &cx).unwrap_err();
+    let errors =
+        Filterset::parse("deps(~does-not-exist)".to_owned(), &cx, FiltersetKind::Test).unwrap_err();
     assert_error(&errors);
 
-    let errors = Filterset::parse("deps(/does-not/)".to_owned(), &cx).unwrap_err();
+    let errors =
+        Filterset::parse("deps(/does-not/)".to_owned(), &cx, FiltersetKind::Test).unwrap_err();
     assert_error(&errors);
 }
 

nextest-runner/src/config/overrides.rs

@@ -453,11 +453,8 @@ impl CompiledData<PreBuildPlatform> {
         errors: &mut Vec<ConfigCompileError>,
     ) -> Self {
         let profile_default_filter = profile_default_filter.and_then(|filter| {
-            let cx = ParseContext {
-                graph,
-                kind: FiltersetKind::DefaultFilter,
-            };
-            match Filterset::parse(filter.to_owned(), &cx) {
+            let cx = ParseContext::new(graph);
+            match Filterset::parse(filter.to_owned(), &cx, FiltersetKind::DefaultFilter) {
                 Ok(expr) => Some(CompiledDefaultFilter {
                     expr: expr.compiled,
                     profile: profile_name.to_owned(),
@@ -607,20 +604,19 @@ impl CompiledOverride<PreBuildPlatform> {
             });
             return None;
         }
-        let cx = ParseContext {
-            graph,
-            // In the future, based on the settings we may want to have restrictions on the kind
-            // here.
-            kind: FiltersetKind::Test,
-        };
+        let cx = ParseContext::new(graph);
+        // In the future, based on the settings we may want to have
+        // restrictions on the kind here.
+        let kind = FiltersetKind::Test;
 
         let host_spec = MaybeTargetSpec::new(source.platform.host.as_deref());
         let target_spec = MaybeTargetSpec::new(source.platform.target.as_deref());
         let filter = source.filter.as_ref().map_or(Ok(None), |filter| {
-            Some(Filterset::parse(filter.clone(), &cx)).transpose()
+            Some(Filterset::parse(filter.clone(), &cx, kind)).transpose()
         });
         let default_filter = source.default_filter.as_ref().map_or(Ok(None), |filter| {
-            Some(Filterset::parse(filter.clone(), &cx)).transpose()
+            // XXX: kind should be Filterset::DefaultFilter!
+            Some(Filterset::parse(filter.clone(), &cx, kind)).transpose()
         });
 
         match (host_spec, target_spec, filter, default_filter) {

nextest-runner/src/config/scripts.rs

@@ -279,14 +279,17 @@ impl CompiledProfileScripts<PreBuildPlatform> {
 
         let host_spec = MaybeTargetSpec::new(source.platform.host.as_deref());
         let target_spec = MaybeTargetSpec::new(source.platform.target.as_deref());
-        let cx = ParseContext {
-            graph,
-            // TODO: probably want to restrict the set of expressions here.
-            kind: FiltersetKind::Test,
-        };
+        let cx = ParseContext::new(graph);
 
         let filter_expr = source.filter.as_ref().map_or(Ok(None), |filter| {
-            Some(Filterset::parse(filter.clone(), &cx)).transpose()
+            // TODO: probably want to restrict the set of expressions here via
+            // the `kind` parameter.
+            Some(Filterset::parse(
+                filter.clone(),
+                &cx,
+                FiltersetKind::DefaultFilter,
+            ))
+            .transpose()
         });
 
         match (host_spec, target_spec, filter_expr) {

nextest-runner/src/list/test_list.rs

@@ -1175,17 +1175,16 @@ mod tests {
             benches::ignored_bench_foo: benchmark
         "};
 
-        let cx = ParseContext {
-            graph: &PACKAGE_GRAPH_FIXTURE,
-            kind: FiltersetKind::Test,
-        };
+        let cx = ParseContext::new(&PACKAGE_GRAPH_FIXTURE);
 
         let test_filter = TestFilterBuilder::new(
             RunIgnored::Default,
             None,
             TestFilterPatterns::default(),
             // Test against the platform() predicate because this is the most important one here.
-            vec![Filterset::parse("platform(target)".to_owned(), &cx).unwrap()],
+            vec![
+                Filterset::parse("platform(target)".to_owned(), &cx, FiltersetKind::Test).unwrap(),
+            ],
         )
         .unwrap();
         let fake_cwd: Utf8PathBuf = "/fake/cwd".into();

nextest-runner/tests/integration/basic.rs

@@ -238,11 +238,13 @@ fn test_run() -> Result<()> {
 fn test_run_ignored() -> Result<()> {
     set_env_vars();
 
-    let pcx = ParseContext {
-        graph: &PACKAGE_GRAPH,
-        kind: FiltersetKind::Test,
-    };
-    let expr = Filterset::parse("not test(test_slow_timeout)".to_owned(), &pcx).unwrap();
+    let pcx = ParseContext::new(&PACKAGE_GRAPH);
+    let expr = Filterset::parse(
+        "not test(test_slow_timeout)".to_owned(),
+        &pcx,
+        FiltersetKind::Test,
+    )
+    .unwrap();
 
     let test_filter = TestFilterBuilder::new(
         RunIgnored::Only,
@@ -336,13 +338,11 @@ fn test_run_ignored() -> Result<()> {
 fn test_filter_expr_with_string_filters() -> Result<()> {
     set_env_vars();
 
-    let pcx = ParseContext {
-        graph: &PACKAGE_GRAPH,
-        kind: FiltersetKind::Test,
-    };
+    let pcx = ParseContext::new(&PACKAGE_GRAPH);
     let expr = Filterset::parse(
         "test(test_multiply_two) | test(=tests::call_dylib_add_two)".to_owned(),
         &pcx,
+        FiltersetKind::Test,
     )
     .expect("filterset is valid");
 
@@ -410,13 +410,11 @@ fn test_filter_expr_with_string_filters() -> Result<()> {
 fn test_filter_expr_without_string_filters() -> Result<()> {
     set_env_vars();
 
-    let pcx = ParseContext {
-        graph: &PACKAGE_GRAPH,
-        kind: FiltersetKind::Test,
-    };
+    let pcx = ParseContext::new(&PACKAGE_GRAPH);
     let expr = Filterset::parse(
         "test(test_multiply_two) | test(=tests::call_dylib_add_two)".to_owned(),
         &pcx,
+        FiltersetKind::Test,
     )
     .expect("filterset is valid");
 
@@ -653,11 +651,13 @@ fn test_retries(retries: Option<RetryPolicy>) -> Result<()> {
 fn test_termination() -> Result<()> {
     set_env_vars();
 
-    let pcx = ParseContext {
-        graph: &PACKAGE_GRAPH,
-        kind: FiltersetKind::Test,
-    };
-    let expr = Filterset::parse("test(/^test_slow_timeout/)".to_owned(), &pcx).unwrap();
+    let pcx = ParseContext::new(&PACKAGE_GRAPH);
+    let expr = Filterset::parse(
+        "test(/^test_slow_timeout/)".to_owned(),
+        &pcx,
+        FiltersetKind::Test,
+    )
+    .unwrap();
     let test_filter = TestFilterBuilder::new(
         RunIgnored::Only,
         None,