From e55d03a2ee611d08f09a38e01e0cb47c1d6cd315 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sun, 3 Nov 2024 16:37:21 +0900 Subject: [PATCH] revset: introduce type-safe user/resolved expression states This helps add library API that takes resolved revset expressions. For example, "jj absorb" will first compute annotation within a user-specified ancestor range such as "mutable()". Because the range expression may contain symbols, it should be resolved by caller. There are two ideas to check resolution state at compile time: a. add RevsetExpressionWrapper and guarantee inner tree consistency at public API boundary b. parameterize RevsetExpression variant types in a way that invalid variants can never be constructed (a) is nice if we want to combine "resolved" and "unresolved" expressions. The inner expression types are the same, so we can just calculate new state as Resolved & Unresolved = Unresolved. (b) is stricter as the compiler can guarantee invariants. This patch implements (b) because there are no existing callers who need to construct "resolved" expression and convert it to "user" expression. .evaluate_programmatic() now requires that the expression is resolved. --- cli/examples/custom-commit-templater/main.rs | 3 +- cli/src/cli_util.rs | 33 +- cli/src/commands/bench/revset.rs | 5 +- cli/src/commands/new.rs | 5 +- cli/src/commands/rebase.rs | 5 +- cli/src/commit_templater.rs | 8 +- cli/src/movement_util.rs | 7 +- cli/src/revset_util.rs | 17 +- lib/src/id_prefix.rs | 6 +- lib/src/revset.rs | 314 +++++++++++-------- 10 files changed, 231 insertions(+), 172 deletions(-) diff --git a/cli/examples/custom-commit-templater/main.rs b/cli/examples/custom-commit-templater/main.rs index 4b1c67c77..404e5ad71 100644 --- a/cli/examples/custom-commit-templater/main.rs +++ b/cli/examples/custom-commit-templater/main.rs @@ -39,6 +39,7 @@ use jj_lib::revset::RevsetParseContext; use jj_lib::revset::RevsetParseError; use jj_lib::revset::RevsetResolutionError; use jj_lib::revset::SymbolResolverExtension; +use jj_lib::revset::UserRevsetExpression; use once_cell::sync::OnceCell; struct HexCounter; @@ -191,7 +192,7 @@ fn even_digits( _diagnostics: &mut RevsetDiagnostics, function: &FunctionCallNode, _context: &RevsetParseContext, -) -> Result, RevsetParseError> { +) -> Result, RevsetParseError> { function.expect_no_arguments()?; Ok(RevsetExpression::filter(RevsetFilterPredicate::Extension( Rc::new(EvenDigitsFilter), diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 8342fa8fe..42e125c47 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -91,6 +91,7 @@ use jj_lib::repo_path::RepoPathBuf; use jj_lib::repo_path::RepoPathUiConverter; use jj_lib::repo_path::UiPathParseError; use jj_lib::revset; +use jj_lib::revset::ResolvedRevsetExpression; use jj_lib::revset::RevsetAliasesMap; use jj_lib::revset::RevsetDiagnostics; use jj_lib::revset::RevsetExpression; @@ -102,6 +103,7 @@ use jj_lib::revset::RevsetModifier; use jj_lib::revset::RevsetParseContext; use jj_lib::revset::RevsetWorkspaceContext; use jj_lib::revset::SymbolResolverExtension; +use jj_lib::revset::UserRevsetExpression; use jj_lib::rewrite::restore_tree; use jj_lib::settings::ConfigResultExt as _; use jj_lib::settings::UserSettings; @@ -604,8 +606,8 @@ pub struct WorkspaceCommandEnvironment { template_aliases_map: TemplateAliasesMap, path_converter: RepoPathUiConverter, workspace_id: WorkspaceId, - immutable_heads_expression: Rc, - short_prefixes_expression: Option>, + immutable_heads_expression: Rc, + short_prefixes_expression: Option>, } impl WorkspaceCommandEnvironment { @@ -676,21 +678,21 @@ impl WorkspaceCommandEnvironment { } /// User-configured expression defining the immutable set. - pub fn immutable_expression(&self) -> Rc { + pub fn immutable_expression(&self) -> Rc { // Negated ancestors expression `~::( | root())` is slightly // easier to optimize than negated union `~(:: | root())`. self.immutable_heads_expression.ancestors() } /// User-configured expression defining the heads of the immutable set. - pub fn immutable_heads_expression(&self) -> &Rc { + pub fn immutable_heads_expression(&self) -> &Rc { &self.immutable_heads_expression } fn load_immutable_heads_expression( &self, ui: &Ui, - ) -> Result, CommandError> { + ) -> Result, CommandError> { let mut diagnostics = RevsetDiagnostics::new(); let expression = revset_util::parse_immutable_heads_expression( &mut diagnostics, @@ -704,7 +706,7 @@ impl WorkspaceCommandEnvironment { fn load_short_prefixes_expression( &self, ui: &Ui, - ) -> Result>, CommandError> { + ) -> Result>, CommandError> { let revset_string = self .settings() .config() @@ -1373,7 +1375,7 @@ impl WorkspaceCommandHelper { pub fn attach_revset_evaluator( &self, - expression: Rc, + expression: Rc, ) -> RevsetExpressionEvaluator<'_> { RevsetExpressionEvaluator::new( self.repo().as_ref(), @@ -1829,14 +1831,15 @@ See https://martinvonz.github.io/jj/latest/working-copy/#stale-working-copy \ let removed_conflicts_expr = new_heads.range(&old_heads).intersection(&conflicts); let added_conflicts_expr = old_heads.range(&new_heads).intersection(&conflicts); - let get_commits = |expr: Rc| -> Result, CommandError> { - let commits = expr - .evaluate_programmatic(new_repo)? - .iter() - .commits(new_repo.store()) - .try_collect()?; - Ok(commits) - }; + let get_commits = + |expr: Rc| -> Result, CommandError> { + let commits = expr + .evaluate_programmatic(new_repo)? + .iter() + .commits(new_repo.store()) + .try_collect()?; + Ok(commits) + }; let removed_conflict_commits = get_commits(removed_conflicts_expr)?; let added_conflict_commits = get_commits(added_conflicts_expr)?; diff --git a/cli/src/commands/bench/revset.rs b/cli/src/commands/bench/revset.rs index 7dc91d52c..251198d3d 100644 --- a/cli/src/commands/bench/revset.rs +++ b/cli/src/commands/bench/revset.rs @@ -21,8 +21,8 @@ use criterion::BenchmarkGroup; use criterion::BenchmarkId; use jj_lib::revset; use jj_lib::revset::DefaultSymbolResolver; -use jj_lib::revset::RevsetExpression; use jj_lib::revset::SymbolResolverExtension; +use jj_lib::revset::UserRevsetExpression; use super::new_criterion; use super::CriterionArgs; @@ -87,7 +87,8 @@ fn bench_revset( .clone(), ); // Time both evaluation and iteration. - let routine = |workspace_command: &WorkspaceCommandHelper, expression: Rc| { + let routine = |workspace_command: &WorkspaceCommandHelper, + expression: Rc| { // Evaluate the expression without parsing/evaluating short-prefixes. let repo = workspace_command.repo().as_ref(); let symbol_resolver = diff --git a/cli/src/commands/new.rs b/cli/src/commands/new.rs index d08531b0e..c30a68afd 100644 --- a/cli/src/commands/new.rs +++ b/cli/src/commands/new.rs @@ -21,6 +21,7 @@ use jj_lib::backend::CommitId; use jj_lib::commit::CommitIteratorExt; use jj_lib::repo::ReadonlyRepo; use jj_lib::repo::Repo; +use jj_lib::revset::ResolvedRevsetExpression; use jj_lib::revset::RevsetExpression; use jj_lib::revset::RevsetIteratorExt; use jj_lib::rewrite::merge_commit_trees; @@ -234,8 +235,8 @@ pub(crate) fn cmd_new( /// parents of the new commit. fn ensure_no_commit_loop( repo: &ReadonlyRepo, - children_expression: &Rc, - parents_expression: &Rc, + children_expression: &Rc, + parents_expression: &Rc, ) -> Result<(), CommandError> { if let Some(commit_id) = children_expression .dag_range_to(parents_expression) diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index 5658ef21b..de6ba6514 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -25,6 +25,7 @@ use jj_lib::commit::CommitIteratorExt; use jj_lib::object_id::ObjectId; use jj_lib::repo::ReadonlyRepo; use jj_lib::repo::Repo; +use jj_lib::revset::ResolvedRevsetExpression; use jj_lib::revset::RevsetExpression; use jj_lib::revset::RevsetIteratorExt; use jj_lib::rewrite::move_commits; @@ -598,8 +599,8 @@ fn rebase_revisions_transaction( /// parents of rebased commits. fn ensure_no_commit_loop( repo: &ReadonlyRepo, - children_expression: &Rc, - parents_expression: &Rc, + children_expression: &Rc, + parents_expression: &Rc, ) -> Result<(), CommandError> { if let Some(commit_id) = children_expression .dag_range_to(parents_expression) diff --git a/cli/src/commit_templater.rs b/cli/src/commit_templater.rs index 60b36d31d..796b7f9e6 100644 --- a/cli/src/commit_templater.rs +++ b/cli/src/commit_templater.rs @@ -44,9 +44,9 @@ use jj_lib::revset; use jj_lib::revset::Revset; use jj_lib::revset::RevsetContainingFn; use jj_lib::revset::RevsetDiagnostics; -use jj_lib::revset::RevsetExpression; use jj_lib::revset::RevsetModifier; use jj_lib::revset::RevsetParseContext; +use jj_lib::revset::UserRevsetExpression; use jj_lib::store::Store; use once_cell::unsync::OnceCell; @@ -94,7 +94,7 @@ pub struct CommitTemplateLanguage<'repo> { // are contained in RevsetParseContext for example. revset_parse_context: RevsetParseContext<'repo>, id_prefix_context: &'repo IdPrefixContext, - immutable_expression: Rc, + immutable_expression: Rc, build_fn_table: CommitTemplateBuildFnTable<'repo>, keyword_cache: CommitKeywordCache<'repo>, cache_extensions: ExtensionsMap, @@ -109,7 +109,7 @@ impl<'repo> CommitTemplateLanguage<'repo> { workspace_id: &WorkspaceId, revset_parse_context: RevsetParseContext<'repo>, id_prefix_context: &'repo IdPrefixContext, - immutable_expression: Rc, + immutable_expression: Rc, extensions: &[impl AsRef], ) -> Self { let mut build_fn_table = CommitTemplateBuildFnTable::builtin(); @@ -845,7 +845,7 @@ fn expect_fileset_literal( fn evaluate_revset_expression<'repo>( language: &CommitTemplateLanguage<'repo>, span: pest::Span<'_>, - expression: Rc, + expression: Rc, ) -> Result, TemplateParseError> { let symbol_resolver = revset_util::default_symbol_resolver( language.repo, diff --git a/cli/src/movement_util.rs b/cli/src/movement_util.rs index 9d5913a80..6909c2b60 100644 --- a/cli/src/movement_util.rs +++ b/cli/src/movement_util.rs @@ -19,6 +19,7 @@ use itertools::Itertools; use jj_lib::backend::CommitId; use jj_lib::commit::Commit; use jj_lib::repo::Repo; +use jj_lib::revset::ResolvedRevsetExpression; use jj_lib::revset::RevsetExpression; use jj_lib::revset::RevsetFilterPredicate; use jj_lib::revset::RevsetIteratorExt; @@ -117,10 +118,10 @@ impl Direction { fn build_target_revset( &self, - working_revset: &Rc, - start_revset: &Rc, + working_revset: &Rc, + start_revset: &Rc, args: &MovementArgsInternal, - ) -> Result, CommandError> { + ) -> Result, CommandError> { let nth = match (self, args.should_edit) { (Direction::Next, true) => start_revset.descendants_at(args.offset), (Direction::Next, false) => start_revset diff --git a/cli/src/revset_util.rs b/cli/src/revset_util.rs index 21b8bd69d..5e6a1a106 100644 --- a/cli/src/revset_util.rs +++ b/cli/src/revset_util.rs @@ -36,6 +36,7 @@ use jj_lib::revset::RevsetParseContext; use jj_lib::revset::RevsetParseError; use jj_lib::revset::RevsetResolutionError; use jj_lib::revset::SymbolResolverExtension; +use jj_lib::revset::UserRevsetExpression; use jj_lib::settings::ConfigResultExt as _; use thiserror::Error; @@ -57,12 +58,12 @@ pub enum UserRevsetEvaluationError { Evaluation(RevsetEvaluationError), } -/// Wrapper around `RevsetExpression` to provide convenient methods. +/// Wrapper around `UserRevsetExpression` to provide convenient methods. pub struct RevsetExpressionEvaluator<'repo> { repo: &'repo dyn Repo, extensions: Arc, id_prefix_context: &'repo IdPrefixContext, - expression: Rc, + expression: Rc, } impl<'repo> RevsetExpressionEvaluator<'repo> { @@ -70,7 +71,7 @@ impl<'repo> RevsetExpressionEvaluator<'repo> { repo: &'repo dyn Repo, extensions: Arc, id_prefix_context: &'repo IdPrefixContext, - expression: Rc, + expression: Rc, ) -> Self { RevsetExpressionEvaluator { repo, @@ -81,12 +82,12 @@ impl<'repo> RevsetExpressionEvaluator<'repo> { } /// Returns the underlying expression. - pub fn expression(&self) -> &Rc { + pub fn expression(&self) -> &Rc { &self.expression } /// Intersects the underlying expression with the `other` expression. - pub fn intersect_with(&mut self, other: &Rc) { + pub fn intersect_with(&mut self, other: &Rc) { self.expression = self.expression.intersection(other); } @@ -183,7 +184,7 @@ pub fn load_revset_aliases( pub fn evaluate<'a>( repo: &'a dyn Repo, symbol_resolver: &DefaultSymbolResolver, - expression: Rc, + expression: Rc, ) -> Result, UserRevsetEvaluationError> { let resolved = revset::optimize(expression) .resolve_user_expression(repo, symbol_resolver) @@ -208,7 +209,7 @@ pub fn default_symbol_resolver<'a>( pub fn parse_immutable_heads_expression( diagnostics: &mut RevsetDiagnostics, context: &RevsetParseContext, -) -> Result, RevsetParseError> { +) -> Result, RevsetParseError> { let (_, _, immutable_heads_str) = context .aliases_map() .get_function(USER_IMMUTABLE_HEADS, 0) @@ -278,7 +279,7 @@ pub(super) fn evaluate_revset_to_single_commit<'a>( fn format_multiple_revisions_error( revision_str: &str, - expression: &RevsetExpression, + expression: &UserRevsetExpression, commits: &[Commit], elided: bool, template: &TemplateRenderer<'_, Commit>, diff --git a/lib/src/id_prefix.rs b/lib/src/id_prefix.rs index db42e4501..b41b8dd92 100644 --- a/lib/src/id_prefix.rs +++ b/lib/src/id_prefix.rs @@ -32,10 +32,10 @@ use crate::object_id::PrefixResolution; use crate::repo::Repo; use crate::revset::DefaultSymbolResolver; use crate::revset::RevsetEvaluationError; -use crate::revset::RevsetExpression; use crate::revset::RevsetExtensions; use crate::revset::RevsetResolutionError; use crate::revset::SymbolResolverExtension; +use crate::revset::UserRevsetExpression; #[derive(Debug, Error)] pub enum IdPrefixIndexLoadError { @@ -46,7 +46,7 @@ pub enum IdPrefixIndexLoadError { } struct DisambiguationData { - expression: Rc, + expression: Rc, indexes: OnceCell, } @@ -123,7 +123,7 @@ impl IdPrefixContext { } } - pub fn disambiguate_within(mut self, expression: Rc) -> Self { + pub fn disambiguate_within(mut self, expression: Rc) -> Self { self.disambiguation = Some(DisambiguationData { expression, indexes: OnceCell::new(), diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 24bc15083..9350e899f 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -177,14 +177,51 @@ pub enum RevsetFilterPredicate { Extension(Rc), } +mod private { + /// Defines [`RevsetExpression`] variants depending on resolution state. + pub trait ExpressionState { + type CommitRef: Clone; + type Operation: Clone; + } + + // Not constructible because these state types just define associated types. + #[derive(Debug)] + pub enum UserExpressionState {} + #[derive(Debug)] + pub enum ResolvedExpressionState {} +} + +use private::ExpressionState; +use private::ResolvedExpressionState; +use private::UserExpressionState; + +impl ExpressionState for UserExpressionState { + type CommitRef = RevsetCommitRef; + type Operation = String; +} + +impl ExpressionState for ResolvedExpressionState { + type CommitRef = Infallible; + type Operation = Infallible; +} + +/// [`RevsetExpression`] that may contain unresolved commit refs. +pub type UserRevsetExpression = RevsetExpression; +/// [`RevsetExpression`] that never contains unresolved commit refs. +pub type ResolvedRevsetExpression = RevsetExpression; + +/// Tree of revset expressions describing DAG operations. +/// +/// Use [`UserRevsetExpression`] or [`ResolvedRevsetExpression`] to construct +/// expression of that state. #[derive(Clone, Debug)] -pub enum RevsetExpression { +pub enum RevsetExpression { None, All, VisibleHeads, Root, Commits(Vec), - CommitRef(RevsetCommitRef), + CommitRef(St::CommitRef), Ancestors { heads: Rc, generation: Range, @@ -221,7 +258,7 @@ pub enum RevsetExpression { AsFilter(Rc), /// Resolves symbols and visibility at the specified operation. AtOperation { - operation: String, + operation: St::Operation, candidates: Rc, }, /// Resolves visibility within the specified repo state. @@ -238,8 +275,9 @@ pub enum RevsetExpression { Difference(Rc, Rc), } -// Leaf expression that never contains unresolved commit refs -impl RevsetExpression { +// Leaf expression that never contains unresolved commit refs, which can be +// either user or resolved expression +impl RevsetExpression { pub fn none() -> Rc { Rc::new(Self::None) } @@ -275,7 +313,7 @@ impl RevsetExpression { } // Leaf expression that represents unresolved commit refs -impl RevsetExpression { +impl> RevsetExpression { pub fn working_copy(workspace_id: WorkspaceId) -> Rc { Rc::new(Self::CommitRef(RevsetCommitRef::WorkingCopy(workspace_id))) } @@ -323,7 +361,7 @@ impl RevsetExpression { } // Compound expression -impl RevsetExpression { +impl RevsetExpression { pub fn latest(self: &Rc, count: usize) -> Rc { Rc::new(Self::Latest { candidates: self.clone(), @@ -480,7 +518,7 @@ impl RevsetExpression { } } -impl RevsetExpression { +impl> RevsetExpression { /// Returns symbol string if this expression is of that type. pub fn as_symbol(&self) -> Option<&str> { match self { @@ -488,27 +526,12 @@ impl RevsetExpression { _ => None, } } +} - /// Resolve a programmatically created revset expression. - /// - /// In particular, the expression must not contain any symbols (bookmarks, - /// tags, change/commit prefixes). Callers must not include - /// `RevsetExpression::symbol()` in the expression, and should instead - /// resolve symbols to `CommitId`s and pass them into - /// `RevsetExpression::commits()`. Similarly, the expression must not - /// contain any `RevsetExpression::remote_symbol()` or - /// `RevsetExpression::working_copy()`, unless they're known to be valid. - /// The expression must not contain `RevsetExpression::AtOperation` even if - /// it's known to be valid. It can fail at loading operation data. - pub fn resolve_programmatic(&self, repo: &dyn Repo) -> ResolvedExpression { - let symbol_resolver = FailingSymbolResolver; - resolve_symbols(repo, self, &symbol_resolver) - .map(|expression| resolve_visibility(repo, &expression)) - .unwrap() - } - +impl UserRevsetExpression { /// Resolve a user-provided expression. Symbols will be resolved using the /// provided `SymbolResolver`. + // TODO: return ResolvedRevsetExpression which can still be combined pub fn resolve_user_expression( &self, repo: &dyn Repo, @@ -517,14 +540,18 @@ impl RevsetExpression { resolve_symbols(repo, self, symbol_resolver) .map(|expression| resolve_visibility(repo, &expression)) } +} +impl ResolvedRevsetExpression { /// Resolve a programmatically created revset expression and evaluate it in /// the repo. + // TODO: rename to .evaluate(), and add .evaluate_unoptimized() for + // testing/debugging purpose? pub fn evaluate_programmatic<'index>( self: Rc, repo: &'index dyn Repo, ) -> Result, RevsetEvaluationError> { - optimize(self).resolve_programmatic(repo).evaluate(repo) + resolve_visibility(repo, &optimize(self)).evaluate(repo) } } @@ -548,6 +575,7 @@ pub enum ResolvedPredicateExpression { /// properties. /// /// Use `RevsetExpression` API to build a query programmatically. +// TODO: rename to BackendExpression? #[derive(Clone, Debug)] pub enum ResolvedExpression { Commits(Vec), @@ -603,7 +631,7 @@ pub type RevsetFunction = fn( &mut RevsetDiagnostics, &FunctionCallNode, &RevsetParseContext, -) -> Result, RevsetParseError>; +) -> Result, RevsetParseError>; static BUILTIN_FUNCTION_MAP: Lazy> = Lazy::new(|| { // Not using maplit::hashmap!{} or custom declarative macro here because @@ -973,7 +1001,7 @@ fn parse_remote_bookmarks_arguments( diagnostics: &mut RevsetDiagnostics, function: &FunctionCallNode, remote_ref_state: Option, -) -> Result, RevsetParseError> { +) -> Result, RevsetParseError> { let ([], [bookmark_opt_arg, remote_opt_arg]) = function.expect_named_arguments(&["", "remote"])?; let bookmark_pattern = if let Some(bookmark_arg) = bookmark_opt_arg { @@ -998,7 +1026,7 @@ fn lower_function_call( diagnostics: &mut RevsetDiagnostics, function: &FunctionCallNode, context: &RevsetParseContext, -) -> Result, RevsetParseError> { +) -> Result, RevsetParseError> { let function_map = &context.extensions.function_map; if let Some(func) = function_map.get(function.name) { func(diagnostics, function, context) @@ -1019,7 +1047,7 @@ pub fn lower_expression( diagnostics: &mut RevsetDiagnostics, node: &ExpressionNode, context: &RevsetParseContext, -) -> Result, RevsetParseError> { +) -> Result, RevsetParseError> { match &node.kind { ExpressionKind::Identifier(name) => Ok(RevsetExpression::symbol((*name).to_owned())), ExpressionKind::String(name) => Ok(RevsetExpression::symbol(name.to_owned())), @@ -1106,7 +1134,7 @@ pub fn parse( diagnostics: &mut RevsetDiagnostics, revset_str: &str, context: &RevsetParseContext, -) -> Result, RevsetParseError> { +) -> Result, RevsetParseError> { let node = revset_parser::parse_program(revset_str)?; let node = dsl_util::expand_aliases(node, context.aliases_map)?; lower_expression(diagnostics, &node, context) @@ -1117,7 +1145,7 @@ pub fn parse_with_modifier( diagnostics: &mut RevsetDiagnostics, revset_str: &str, context: &RevsetParseContext, -) -> Result<(Rc, Option), RevsetParseError> { +) -> Result<(Rc, Option), RevsetParseError> { let node = revset_parser::parse_program(revset_str)?; let node = dsl_util::expand_aliases(node, context.aliases_map)?; revset_parser::expect_program_with( @@ -1136,15 +1164,19 @@ pub fn parse_with_modifier( } /// `Some` for rewritten expression, or `None` to reuse the original expression. -type TransformedExpression = Option>; +type TransformedExpression = Option>>; /// Walks `expression` tree and applies `f` recursively from leaf nodes. -fn transform_expression_bottom_up( - expression: &Rc, - mut f: impl FnMut(&Rc) -> TransformedExpression, -) -> TransformedExpression { - try_transform_expression::(expression, |_| Ok(None), |expression| Ok(f(expression))) - .unwrap() +fn transform_expression_bottom_up( + expression: &Rc>, + mut f: impl FnMut(&Rc>) -> TransformedExpression, +) -> TransformedExpression { + try_transform_expression::( + expression, + |_| Ok(None), + |expression| Ok(f(expression)), + ) + .unwrap() } /// Walks `expression` tree and applies transformation recursively. @@ -1159,16 +1191,16 @@ fn transform_expression_bottom_up( /// If no nodes rewritten, this function returns `None`. /// `std::iter::successors()` could be used if the transformation needs to be /// applied repeatedly until converged. -fn try_transform_expression( - expression: &Rc, - mut pre: impl FnMut(&Rc) -> Result, - mut post: impl FnMut(&Rc) -> Result, -) -> Result { - fn transform_child_rec( - expression: &Rc, - pre: &mut impl FnMut(&Rc) -> Result, - post: &mut impl FnMut(&Rc) -> Result, - ) -> Result { +fn try_transform_expression( + expression: &Rc>, + mut pre: impl FnMut(&Rc>) -> Result, E>, + mut post: impl FnMut(&Rc>) -> Result, E>, +) -> Result, E> { + fn transform_child_rec( + expression: &Rc>, + pre: &mut impl FnMut(&Rc>) -> Result, E>, + post: &mut impl FnMut(&Rc>) -> Result, E>, + ) -> Result, E> { Ok(match expression.as_ref() { RevsetExpression::None => None, RevsetExpression::All => None, @@ -1274,11 +1306,11 @@ fn try_transform_expression( } #[allow(clippy::type_complexity)] - fn transform_rec_pair( - (expression1, expression2): (&Rc, &Rc), - pre: &mut impl FnMut(&Rc) -> Result, - post: &mut impl FnMut(&Rc) -> Result, - ) -> Result, Rc)>, E> { + fn transform_rec_pair( + (expression1, expression2): (&Rc>, &Rc>), + pre: &mut impl FnMut(&Rc>) -> Result, E>, + post: &mut impl FnMut(&Rc>) -> Result, E>, + ) -> Result>, Rc>)>, E> { match ( transform_rec(expression1, pre, post)?, transform_rec(expression2, pre, post)?, @@ -1292,11 +1324,11 @@ fn try_transform_expression( } } - fn transform_rec( - expression: &Rc, - pre: &mut impl FnMut(&Rc) -> Result, - post: &mut impl FnMut(&Rc) -> Result, - ) -> Result { + fn transform_rec( + expression: &Rc>, + pre: &mut impl FnMut(&Rc>) -> Result, E>, + post: &mut impl FnMut(&Rc>) -> Result, E>, + ) -> Result, E> { if let Some(new_expression) = pre(expression)? { return Ok(Some(new_expression)); } @@ -1314,41 +1346,42 @@ fn try_transform_expression( /// Visitor-like interface to transform [`RevsetExpression`] state recursively. /// /// This is similar to [`try_transform_expression()`], but is supposed to -/// transform the resolution state. -trait ExpressionStateFolder { +/// transform the resolution state from `InSt` to `OutSt`. +trait ExpressionStateFolder { type Error; /// Transforms the `expression`. By default, inner items are transformed /// recursively. fn fold_expression( &mut self, - expression: &RevsetExpression, - ) -> Result, Self::Error> { + expression: &RevsetExpression, + ) -> Result>, Self::Error> { fold_child_expression_state(self, expression) } /// Transforms commit ref such as symbol. fn fold_commit_ref( &mut self, - commit_ref: &RevsetCommitRef, - ) -> Result, Self::Error>; + commit_ref: &InSt::CommitRef, + ) -> Result>, Self::Error>; /// Transforms `at_operation(operation, candidates)` expression. - #[allow(clippy::ptr_arg)] // TODO: &String will be parameterized fn fold_at_operation( &mut self, - operation: &String, - candidates: &RevsetExpression, - ) -> Result, Self::Error>; + operation: &InSt::Operation, + candidates: &RevsetExpression, + ) -> Result>, Self::Error>; } /// Transforms inner items of the `expression` by using the `folder`. -fn fold_child_expression_state( +fn fold_child_expression_state( folder: &mut F, - expression: &RevsetExpression, -) -> Result, F::Error> + expression: &RevsetExpression, +) -> Result>, F::Error> where - F: ExpressionStateFolder + ?Sized, + InSt: ExpressionState, + OutSt: ExpressionState, + F: ExpressionStateFolder + ?Sized, { let expression: Rc<_> = match expression { RevsetExpression::None => RevsetExpression::None.into(), @@ -1466,22 +1499,25 @@ where /// help further optimization (e.g. combine `file(_)` matchers.) /// c. Wraps union of filter and set (e.g. `author(_) | heads()`), to /// ensure inner filter wouldn't need to evaluate all the input sets. -fn internalize_filter(expression: &Rc) -> TransformedExpression { - fn is_filter(expression: &RevsetExpression) -> bool { +fn internalize_filter( + expression: &Rc>, +) -> TransformedExpression { + fn is_filter(expression: &RevsetExpression) -> bool { matches!( expression, RevsetExpression::Filter(_) | RevsetExpression::AsFilter(_) ) } - fn is_filter_tree(expression: &RevsetExpression) -> bool { + fn is_filter_tree(expression: &RevsetExpression) -> bool { is_filter(expression) || as_filter_intersection(expression).is_some() } // Extracts 'c & f' from intersect_down()-ed node. - fn as_filter_intersection( - expression: &RevsetExpression, - ) -> Option<(&Rc, &Rc)> { + #[allow(clippy::type_complexity)] + fn as_filter_intersection( + expression: &RevsetExpression, + ) -> Option<(&Rc>, &Rc>)> { if let RevsetExpression::Intersection(expression1, expression2) = expression { is_filter(expression2).then_some((expression1, expression2)) } else { @@ -1493,10 +1529,10 @@ fn internalize_filter(expression: &Rc) -> TransformedExpressio // apply the whole bottom-up pass to new intersection node. Instead, just push // new 'c & (d & g)' down-left to '(c & d) & g' while either side is // an intersection of filter node. - fn intersect_down( - expression1: &Rc, - expression2: &Rc, - ) -> TransformedExpression { + fn intersect_down( + expression1: &Rc>, + expression2: &Rc>, + ) -> TransformedExpression { let recurse = |e1, e2| intersect_down(e1, e2).unwrap_or_else(|| e1.intersection(e2)); match (expression1.as_ref(), expression2.as_ref()) { // Don't reorder 'f1 & f2' @@ -1541,7 +1577,9 @@ fn internalize_filter(expression: &Rc) -> TransformedExpressio /// /// This does not rewrite 'x & none()' to 'none()' because 'x' may be an invalid /// symbol. -fn fold_redundant_expression(expression: &Rc) -> TransformedExpression { +fn fold_redundant_expression( + expression: &Rc>, +) -> TransformedExpression { transform_expression_bottom_up(expression, |expression| match expression.as_ref() { RevsetExpression::NotIn(outer) => match outer.as_ref() { RevsetExpression::NotIn(inner) => Some(inner.clone()), @@ -1558,10 +1596,10 @@ fn fold_redundant_expression(expression: &Rc) -> TransformedEx }) } -fn to_difference_range( - expression: &Rc, - complement: &Rc, -) -> TransformedExpression { +fn to_difference_range( + expression: &Rc>, + complement: &Rc>, +) -> TransformedExpression { match (expression.as_ref(), complement.as_ref()) { // ::heads & ~(::roots) -> roots..heads ( @@ -1597,11 +1635,13 @@ fn to_difference_range( /// Transforms negative intersection to difference. Redundant intersections like /// `all() & e` should have been removed. -fn fold_difference(expression: &Rc) -> TransformedExpression { - fn to_difference( - expression: &Rc, - complement: &Rc, - ) -> Rc { +fn fold_difference( + expression: &Rc>, +) -> TransformedExpression { + fn to_difference( + expression: &Rc>, + complement: &Rc>, + ) -> Rc> { to_difference_range(expression, complement).unwrap_or_else(|| expression.minus(complement)) } @@ -1627,7 +1667,9 @@ fn fold_difference(expression: &Rc) -> TransformedExpression { /// /// Since this rule inserts redundant `visible_heads()`, negative intersections /// should have been transformed. -fn fold_not_in_ancestors(expression: &Rc) -> TransformedExpression { +fn fold_not_in_ancestors( + expression: &Rc>, +) -> TransformedExpression { transform_expression_bottom_up(expression, |expression| match expression.as_ref() { RevsetExpression::NotIn(complement) if matches!(complement.as_ref(), RevsetExpression::Ancestors { .. }) => @@ -1644,7 +1686,9 @@ fn fold_not_in_ancestors(expression: &Rc) -> TransformedExpres /// /// For example, `all() ~ e` will become `all() & ~e`, which can be simplified /// further by `fold_redundant_expression()`. -fn unfold_difference(expression: &Rc) -> TransformedExpression { +fn unfold_difference( + expression: &Rc>, +) -> TransformedExpression { transform_expression_bottom_up(expression, |expression| match expression.as_ref() { // roots..heads -> ::heads & ~(::roots) RevsetExpression::Range { @@ -1667,7 +1711,9 @@ fn unfold_difference(expression: &Rc) -> TransformedExpression /// Transforms nested `ancestors()`/`parents()`/`descendants()`/`children()` /// like `h---`/`r+++`. -fn fold_generation(expression: &Rc) -> TransformedExpression { +fn fold_generation( + expression: &Rc>, +) -> TransformedExpression { fn add_generation(generation1: &Range, generation2: &Range) -> Range { // For any (g1, g2) in (generation1, generation2), g1 + g2. if generation1.is_empty() || generation2.is_empty() { @@ -1723,7 +1769,9 @@ fn fold_generation(expression: &Rc) -> TransformedExpression { /// Rewrites the given `expression` tree to reduce evaluation cost. Returns new /// tree. -pub fn optimize(expression: Rc) -> Rc { +pub fn optimize( + expression: Rc>, +) -> Rc> { let expression = unfold_difference(&expression).unwrap_or(expression); let expression = fold_redundant_expression(&expression).unwrap_or(expression); let expression = fold_generation(&expression).unwrap_or(expression); @@ -2141,13 +2189,15 @@ impl<'a> ExpressionSymbolResolver<'a> { } } -impl ExpressionStateFolder for ExpressionSymbolResolver<'_> { +impl ExpressionStateFolder + for ExpressionSymbolResolver<'_> +{ type Error = RevsetResolutionError; fn fold_expression( &mut self, - expression: &RevsetExpression, - ) -> Result, Self::Error> { + expression: &UserRevsetExpression, + ) -> Result, Self::Error> { match expression { // 'present(x)' opens new symbol resolution scope to map error to 'none()' RevsetExpression::Present(candidates) => { @@ -2170,7 +2220,7 @@ impl ExpressionStateFolder for ExpressionSymbolResolver<'_> { fn fold_commit_ref( &mut self, commit_ref: &RevsetCommitRef, - ) -> Result, Self::Error> { + ) -> Result, Self::Error> { let commit_ids = resolve_commit_ref(self.repo(), commit_ref, self.symbol_resolver)?; Ok(RevsetExpression::commits(commit_ids)) } @@ -2178,8 +2228,8 @@ impl ExpressionStateFolder for ExpressionSymbolResolver<'_> { fn fold_at_operation( &mut self, operation: &String, - candidates: &RevsetExpression, - ) -> Result, Self::Error> { + candidates: &UserRevsetExpression, + ) -> Result, Self::Error> { let repo = reload_repo_at_operation(self.repo(), operation)?; self.repo_stack.push(repo); let candidates = self.fold_expression(candidates)?; @@ -2194,9 +2244,9 @@ impl ExpressionStateFolder for ExpressionSymbolResolver<'_> { fn resolve_symbols( repo: &dyn Repo, - expression: &RevsetExpression, + expression: &UserRevsetExpression, symbol_resolver: &dyn SymbolResolver, -) -> Result, RevsetResolutionError> { +) -> Result, RevsetResolutionError> { let mut resolver = ExpressionSymbolResolver::new(repo, symbol_resolver); resolver.fold_expression(expression) } @@ -2210,7 +2260,10 @@ fn resolve_symbols( /// commit ids to make `all()` include hidden-but-specified commits. The /// return type `ResolvedExpression` is stricter than `RevsetExpression`, /// and isn't designed for such transformation. -fn resolve_visibility(repo: &dyn Repo, expression: &RevsetExpression) -> ResolvedExpression { +fn resolve_visibility( + repo: &dyn Repo, + expression: &ResolvedRevsetExpression, +) -> ResolvedExpression { let context = VisibilityResolutionContext { visible_heads: &repo.view().heads().iter().cloned().collect_vec(), root: repo.store().root_commit_id(), @@ -2226,7 +2279,7 @@ struct VisibilityResolutionContext<'a> { impl VisibilityResolutionContext<'_> { /// Resolves expression tree as set. - fn resolve(&self, expression: &RevsetExpression) -> ResolvedExpression { + fn resolve(&self, expression: &ResolvedRevsetExpression) -> ResolvedExpression { match expression { RevsetExpression::None => ResolvedExpression::Commits(vec![]), RevsetExpression::All => self.resolve_all(), @@ -2235,9 +2288,7 @@ impl VisibilityResolutionContext<'_> { RevsetExpression::Commits(commit_ids) => { ResolvedExpression::Commits(commit_ids.clone()) } - RevsetExpression::CommitRef(_) => { - panic!("Expression '{expression:?}' should have been resolved by caller"); - } + RevsetExpression::CommitRef(commit_ref) => match *commit_ref {}, RevsetExpression::Ancestors { heads, generation } => ResolvedExpression::Ancestors { heads: self.resolve(heads).into(), generation: generation.clone(), @@ -2283,9 +2334,7 @@ impl VisibilityResolutionContext<'_> { predicate: self.resolve_predicate(expression), } } - RevsetExpression::AtOperation { .. } => { - panic!("Expression '{expression:?}' should have been resolved by caller"); - } + RevsetExpression::AtOperation { operation, .. } => match *operation {}, RevsetExpression::WithinVisibility { candidates, visible_heads, @@ -2359,7 +2408,10 @@ impl VisibilityResolutionContext<'_> { /// /// For filter expression, this never inserts a hidden `all()` since a /// filter predicate doesn't need to produce revisions to walk. - fn resolve_predicate(&self, expression: &RevsetExpression) -> ResolvedPredicateExpression { + fn resolve_predicate( + &self, + expression: &ResolvedRevsetExpression, + ) -> ResolvedPredicateExpression { match expression { RevsetExpression::None | RevsetExpression::All @@ -2381,9 +2433,7 @@ impl VisibilityResolutionContext<'_> { ResolvedPredicateExpression::Filter(predicate.clone()) } RevsetExpression::AsFilter(candidates) => self.resolve_predicate(candidates), - RevsetExpression::AtOperation { .. } => { - panic!("Expression '{expression:?}' should have been resolved by caller"); - } + RevsetExpression::AtOperation { operation, .. } => match *operation {}, // Filters should be intersected with all() within the at-op repo. RevsetExpression::WithinVisibility { .. } => { ResolvedPredicateExpression::Set(self.resolve(expression).into()) @@ -2599,21 +2649,21 @@ mod tests { use super::*; - fn parse(revset_str: &str) -> Result, RevsetParseError> { + fn parse(revset_str: &str) -> Result, RevsetParseError> { parse_with_aliases(revset_str, [] as [(&str, &str); 0]) } fn parse_with_workspace( revset_str: &str, workspace_id: &WorkspaceId, - ) -> Result, RevsetParseError> { + ) -> Result, RevsetParseError> { parse_with_aliases_and_workspace(revset_str, [] as [(&str, &str); 0], workspace_id) } fn parse_with_aliases( revset_str: &str, aliases: impl IntoIterator, impl Into)>, - ) -> Result, RevsetParseError> { + ) -> Result, RevsetParseError> { let mut aliases_map = RevsetAliasesMap::new(); for (decl, defn) in aliases { aliases_map.insert(decl, defn).unwrap(); @@ -2633,7 +2683,7 @@ mod tests { revset_str: &str, aliases: impl IntoIterator, impl Into)>, workspace_id: &WorkspaceId, - ) -> Result, RevsetParseError> { + ) -> Result, RevsetParseError> { // Set up pseudo context to resolve `workspace_id@` and `file(path)` let path_converter = RepoPathUiConverter::Fs { cwd: PathBuf::from("/"), @@ -2660,14 +2710,14 @@ mod tests { fn parse_with_modifier( revset_str: &str, - ) -> Result<(Rc, Option), RevsetParseError> { + ) -> Result<(Rc, Option), RevsetParseError> { parse_with_aliases_and_modifier(revset_str, [] as [(&str, &str); 0]) } fn parse_with_aliases_and_modifier( revset_str: &str, aliases: impl IntoIterator, impl Into)>, - ) -> Result<(Rc, Option), RevsetParseError> { + ) -> Result<(Rc, Option), RevsetParseError> { let mut aliases_map = RevsetAliasesMap::new(); for (decl, defn) in aliases { aliases_map.insert(decl, defn).unwrap(); @@ -2704,10 +2754,10 @@ mod tests { fn test_revset_expression_building() { let settings = insta_settings(); let _guard = settings.bind_to_scope(); - let current_wc = RevsetExpression::working_copy(WorkspaceId::default()); - let foo_symbol = RevsetExpression::symbol("foo".to_string()); - let bar_symbol = RevsetExpression::symbol("bar".to_string()); - let baz_symbol = RevsetExpression::symbol("baz".to_string()); + let current_wc = UserRevsetExpression::working_copy(WorkspaceId::default()); + let foo_symbol = UserRevsetExpression::symbol("foo".to_string()); + let bar_symbol = UserRevsetExpression::symbol("bar".to_string()); + let baz_symbol = UserRevsetExpression::symbol("baz".to_string()); insta::assert_debug_snapshot!( current_wc, @@ -2779,7 +2829,7 @@ mod tests { ) "###); insta::assert_debug_snapshot!( - RevsetExpression::union_all(&[]), + UserRevsetExpression::union_all(&[]), @"None"); insta::assert_debug_snapshot!( RevsetExpression::union_all(&[current_wc.clone()]), @@ -2841,7 +2891,7 @@ mod tests { ) "###); insta::assert_debug_snapshot!( - RevsetExpression::coalesce(&[]), + UserRevsetExpression::coalesce(&[]), @"None"); insta::assert_debug_snapshot!( RevsetExpression::coalesce(&[current_wc.clone()]), @@ -3445,8 +3495,8 @@ mod tests { #[test] fn test_optimize_unchanged_subtree() { fn unwrap_union( - expression: &RevsetExpression, - ) -> (&Rc, &Rc) { + expression: &UserRevsetExpression, + ) -> (&Rc, &Rc) { match expression { RevsetExpression::Union(left, right) => (left, right), _ => panic!("unexpected expression: {expression:?}"),