From 8b1760ca5df926f89031721293283c721776e3a0 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 19 Sep 2024 15:02:10 +0900 Subject: [PATCH] revset: pass diagnostics receiver around Stacking at AliasExpanded node looks wonky. If we migrate error handling to Diagnostics API, it might make sense to remove AliasExpanded node and add node.aliases: vec![(id, span), ..] field instead. Some closure arguments are inlined in order to help type inference. --- cli/examples/custom-commit-templater/main.rs | 2 + cli/src/cli_util.rs | 40 +++- cli/src/commands/debug/revset.rs | 6 +- cli/src/commit_templater.rs | 12 +- cli/src/revset_util.rs | 4 +- lib/src/revset.rs | 227 +++++++++++-------- lib/src/revset_parser.rs | 52 +++-- lib/tests/test_revset.rs | 10 +- 8 files changed, 223 insertions(+), 130 deletions(-) diff --git a/cli/examples/custom-commit-templater/main.rs b/cli/examples/custom-commit-templater/main.rs index ec13862d8..03a410b88 100644 --- a/cli/examples/custom-commit-templater/main.rs +++ b/cli/examples/custom-commit-templater/main.rs @@ -31,6 +31,7 @@ use jj_lib::object_id::ObjectId; use jj_lib::repo::Repo; use jj_lib::revset::FunctionCallNode; use jj_lib::revset::PartialSymbolResolver; +use jj_lib::revset::RevsetDiagnostics; use jj_lib::revset::RevsetExpression; use jj_lib::revset::RevsetFilterExtension; use jj_lib::revset::RevsetFilterPredicate; @@ -185,6 +186,7 @@ impl RevsetFilterExtension for EvenDigitsFilter { } fn even_digits( + _diagnostics: &mut RevsetDiagnostics, function: &FunctionCallNode, _context: &RevsetParseContext, ) -> Result, RevsetParseError> { diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index f8bdf3362..acd083078 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -93,6 +93,7 @@ use jj_lib::repo_path::RepoPathUiConverter; use jj_lib::repo_path::UiPathParseError; use jj_lib::revset; use jj_lib::revset::RevsetAliasesMap; +use jj_lib::revset::RevsetDiagnostics; use jj_lib::revset::RevsetExpression; use jj_lib::revset::RevsetExtensions; use jj_lib::revset::RevsetFilterPredicate; @@ -682,15 +683,21 @@ impl WorkspaceCommandEnvironment { fn load_immutable_heads_expression( &self, - _ui: &Ui, + ui: &Ui, ) -> Result, CommandError> { - revset_util::parse_immutable_heads_expression(&self.revset_parse_context()) - .map_err(|e| config_error_with_message("Invalid `revset-aliases.immutable_heads()`", e)) + let mut diagnostics = RevsetDiagnostics::new(); + let expression = revset_util::parse_immutable_heads_expression( + &mut diagnostics, + &self.revset_parse_context(), + ) + .map_err(|e| config_error_with_message("Invalid `revset-aliases.immutable_heads()`", e))?; + print_parse_diagnostics(ui, "In `revset-aliases.immutable_heads()`", &diagnostics)?; + Ok(expression) } fn load_short_prefixes_expression( &self, - _ui: &Ui, + ui: &Ui, ) -> Result>, CommandError> { let revset_string = self .settings() @@ -700,10 +707,14 @@ impl WorkspaceCommandEnvironment { if revset_string.is_empty() { Ok(None) } else { - let (expression, modifier) = - revset::parse_with_modifier(&revset_string, &self.revset_parse_context()).map_err( - |err| config_error_with_message("Invalid `revsets.short-prefixes`", err), - )?; + let mut diagnostics = RevsetDiagnostics::new(); + let (expression, modifier) = revset::parse_with_modifier( + &mut diagnostics, + &revset_string, + &self.revset_parse_context(), + ) + .map_err(|err| config_error_with_message("Invalid `revsets.short-prefixes`", err))?; + print_parse_diagnostics(ui, "In `revsets.short-prefixes`", &diagnostics)?; let (None | Some(RevsetModifier::All)) = modifier; Ok(Some(revset::optimize(expression))) } @@ -1316,26 +1327,31 @@ impl WorkspaceCommandHelper { fn parse_revset_with_modifier( &self, - _ui: &Ui, + ui: &Ui, revision_arg: &RevisionArg, ) -> Result<(RevsetExpressionEvaluator<'_>, Option), CommandError> { + let mut diagnostics = RevsetDiagnostics::new(); let context = self.revset_parse_context(); - let (expression, modifier) = revset::parse_with_modifier(revision_arg.as_ref(), &context)?; + let (expression, modifier) = + revset::parse_with_modifier(&mut diagnostics, revision_arg.as_ref(), &context)?; + print_parse_diagnostics(ui, "In revset expression", &diagnostics)?; Ok((self.attach_revset_evaluator(expression), modifier)) } /// Parses the given revset expressions and concatenates them all. pub fn parse_union_revsets( &self, - _ui: &Ui, + ui: &Ui, revision_args: &[RevisionArg], ) -> Result, CommandError> { + let mut diagnostics = RevsetDiagnostics::new(); let context = self.revset_parse_context(); let expressions: Vec<_> = revision_args .iter() - .map(|arg| revset::parse_with_modifier(arg.as_ref(), &context)) + .map(|arg| revset::parse_with_modifier(&mut diagnostics, arg.as_ref(), &context)) .map_ok(|(expression, None | Some(RevsetModifier::All))| expression) .try_collect()?; + print_parse_diagnostics(ui, "In revset expression", &diagnostics)?; let expression = RevsetExpression::union_all(&expressions); Ok(self.attach_revset_evaluator(expression)) } diff --git a/cli/src/commands/debug/revset.rs b/cli/src/commands/debug/revset.rs index 9c1a44017..f6ee8142e 100644 --- a/cli/src/commands/debug/revset.rs +++ b/cli/src/commands/debug/revset.rs @@ -17,8 +17,10 @@ use std::io::Write as _; use jj_lib::object_id::ObjectId; use jj_lib::revset; +use jj_lib::revset::RevsetDiagnostics; use crate::cli_util::CommandHelper; +use crate::command_error::print_parse_diagnostics; use crate::command_error::CommandError; use crate::revset_util; use crate::ui::Ui; @@ -38,7 +40,9 @@ pub fn cmd_debug_revset( let workspace_ctx = workspace_command.revset_parse_context(); let repo = workspace_command.repo().as_ref(); - let expression = revset::parse(&args.revision, &workspace_ctx)?; + let mut diagnostics = RevsetDiagnostics::new(); + let expression = revset::parse(&mut diagnostics, &args.revision, &workspace_ctx)?; + print_parse_diagnostics(ui, "In revset expression", &diagnostics)?; writeln!(ui.stdout(), "-- Parsed:")?; writeln!(ui.stdout(), "{expression:#?}")?; writeln!(ui.stdout())?; diff --git a/cli/src/commit_templater.rs b/cli/src/commit_templater.rs index 56e01783c..99896a0e9 100644 --- a/cli/src/commit_templater.rs +++ b/cli/src/commit_templater.rs @@ -43,6 +43,7 @@ use jj_lib::repo::Repo; use jj_lib::repo_path::RepoPathUiConverter; use jj_lib::revset; use jj_lib::revset::Revset; +use jj_lib::revset::RevsetDiagnostics; use jj_lib::revset::RevsetExpression; use jj_lib::revset::RevsetModifier; use jj_lib::revset::RevsetParseContext; @@ -799,10 +800,13 @@ fn evaluate_user_revset<'repo>( span: pest::Span<'_>, revset: &str, ) -> Result, TemplateParseError> { - let (expression, modifier) = - revset::parse_with_modifier(revset, &language.revset_parse_context).map_err(|err| { - TemplateParseError::expression("In revset expression", span).with_source(err) - })?; + let mut inner_diagnostics = RevsetDiagnostics::new(); // TODO + let (expression, modifier) = revset::parse_with_modifier( + &mut inner_diagnostics, + revset, + &language.revset_parse_context, + ) + .map_err(|err| TemplateParseError::expression("In revset expression", span).with_source(err))?; let (None | Some(RevsetModifier::All)) = modifier; evaluate_revset_expression(language, span, expression) diff --git a/cli/src/revset_util.rs b/cli/src/revset_util.rs index fed2d3858..cdfe140e8 100644 --- a/cli/src/revset_util.rs +++ b/cli/src/revset_util.rs @@ -28,6 +28,7 @@ use jj_lib::revset::DefaultSymbolResolver; use jj_lib::revset::Revset; use jj_lib::revset::RevsetAliasesMap; use jj_lib::revset::RevsetCommitRef; +use jj_lib::revset::RevsetDiagnostics; use jj_lib::revset::RevsetEvaluationError; use jj_lib::revset::RevsetExpression; use jj_lib::revset::RevsetExtensions; @@ -201,13 +202,14 @@ pub fn default_symbol_resolver<'a>( /// Parses user-configured expression defining the heads of the immutable set. /// Includes the root commit. pub fn parse_immutable_heads_expression( + diagnostics: &mut RevsetDiagnostics, context: &RevsetParseContext, ) -> Result, RevsetParseError> { let (_, _, immutable_heads_str) = context .aliases_map() .get_function(USER_IMMUTABLE_HEADS, 0) .unwrap(); - let heads = revset::parse(immutable_heads_str, context)?; + let heads = revset::parse(diagnostics, immutable_heads_str, context)?; Ok(heads.union(&RevsetExpression::root())) } diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 9abff3486..a1e2081f8 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -54,6 +54,7 @@ pub use crate::revset_parser::ExpressionKind; pub use crate::revset_parser::ExpressionNode; pub use crate::revset_parser::FunctionCallNode; pub use crate::revset_parser::RevsetAliasesMap; +pub use crate::revset_parser::RevsetDiagnostics; pub use crate::revset_parser::RevsetParseError; pub use crate::revset_parser::RevsetParseErrorKind; pub use crate::revset_parser::UnaryOp; @@ -565,104 +566,113 @@ impl ResolvedExpression { } } -pub type RevsetFunction = - fn(&FunctionCallNode, &RevsetParseContext) -> Result, RevsetParseError>; +pub type RevsetFunction = fn( + &mut RevsetDiagnostics, + &FunctionCallNode, + &RevsetParseContext, +) -> Result, RevsetParseError>; static BUILTIN_FUNCTION_MAP: Lazy> = Lazy::new(|| { // Not using maplit::hashmap!{} or custom declarative macro here because // code completion inside macro is quite restricted. let mut map: HashMap<&'static str, RevsetFunction> = HashMap::new(); - map.insert("parents", |function, context| { + map.insert("parents", |diagnostics, function, context| { let [arg] = function.expect_exact_arguments()?; - let expression = lower_expression(arg, context)?; + let expression = lower_expression(diagnostics, arg, context)?; Ok(expression.parents()) }); - map.insert("children", |function, context| { + map.insert("children", |diagnostics, function, context| { let [arg] = function.expect_exact_arguments()?; - let expression = lower_expression(arg, context)?; + let expression = lower_expression(diagnostics, arg, context)?; Ok(expression.children()) }); - map.insert("ancestors", |function, context| { + map.insert("ancestors", |diagnostics, function, context| { let ([heads_arg], [depth_opt_arg]) = function.expect_arguments()?; - let heads = lower_expression(heads_arg, context)?; + let heads = lower_expression(diagnostics, heads_arg, context)?; let generation = if let Some(depth_arg) = depth_opt_arg { - let depth = expect_literal("integer", depth_arg)?; + let depth = expect_literal(diagnostics, "integer", depth_arg)?; 0..depth } else { GENERATION_RANGE_FULL }; Ok(heads.ancestors_range(generation)) }); - map.insert("descendants", |function, context| { + map.insert("descendants", |diagnostics, function, context| { let ([roots_arg], [depth_opt_arg]) = function.expect_arguments()?; - let roots = lower_expression(roots_arg, context)?; + let roots = lower_expression(diagnostics, roots_arg, context)?; let generation = if let Some(depth_arg) = depth_opt_arg { - let depth = expect_literal("integer", depth_arg)?; + let depth = expect_literal(diagnostics, "integer", depth_arg)?; 0..depth } else { GENERATION_RANGE_FULL }; Ok(roots.descendants_range(generation)) }); - map.insert("connected", |function, context| { + map.insert("connected", |diagnostics, function, context| { let [arg] = function.expect_exact_arguments()?; - let candidates = lower_expression(arg, context)?; + let candidates = lower_expression(diagnostics, arg, context)?; Ok(candidates.connected()) }); - map.insert("reachable", |function, context| { + map.insert("reachable", |diagnostics, function, context| { let [source_arg, domain_arg] = function.expect_exact_arguments()?; - let sources = lower_expression(source_arg, context)?; - let domain = lower_expression(domain_arg, context)?; + let sources = lower_expression(diagnostics, source_arg, context)?; + let domain = lower_expression(diagnostics, domain_arg, context)?; Ok(sources.reachable(&domain)) }); - map.insert("none", |function, _context| { + map.insert("none", |_diagnostics, function, _context| { function.expect_no_arguments()?; Ok(RevsetExpression::none()) }); - map.insert("all", |function, _context| { + map.insert("all", |_diagnostics, function, _context| { function.expect_no_arguments()?; Ok(RevsetExpression::all()) }); - map.insert("working_copies", |function, _context| { + map.insert("working_copies", |_diagnostics, function, _context| { function.expect_no_arguments()?; Ok(RevsetExpression::working_copies()) }); - map.insert("heads", |function, context| { + map.insert("heads", |diagnostics, function, context| { let [arg] = function.expect_exact_arguments()?; - let candidates = lower_expression(arg, context)?; + let candidates = lower_expression(diagnostics, arg, context)?; Ok(candidates.heads()) }); - map.insert("roots", |function, context| { + map.insert("roots", |diagnostics, function, context| { let [arg] = function.expect_exact_arguments()?; - let candidates = lower_expression(arg, context)?; + let candidates = lower_expression(diagnostics, arg, context)?; Ok(candidates.roots()) }); - map.insert("visible_heads", |function, _context| { + map.insert("visible_heads", |_diagnostics, function, _context| { function.expect_no_arguments()?; Ok(RevsetExpression::visible_heads()) }); - map.insert("root", |function, _context| { + map.insert("root", |_diagnostics, function, _context| { function.expect_no_arguments()?; Ok(RevsetExpression::root()) }); - map.insert("bookmarks", |function, _context| { + map.insert("bookmarks", |diagnostics, function, _context| { let ([], [opt_arg]) = function.expect_arguments()?; let pattern = if let Some(arg) = opt_arg { - expect_string_pattern(arg)? + expect_string_pattern(diagnostics, arg)? } else { StringPattern::everything() }; Ok(RevsetExpression::bookmarks(pattern)) }); - map.insert("remote_bookmarks", |function, _context| { - parse_remote_bookmarks_arguments(function, None) - }); - map.insert("tracked_remote_bookmarks", |function, _context| { - parse_remote_bookmarks_arguments(function, Some(RemoteRefState::Tracking)) - }); - map.insert("untracked_remote_bookmarks", |function, _context| { - parse_remote_bookmarks_arguments(function, Some(RemoteRefState::New)) + map.insert("remote_bookmarks", |diagnostics, function, _context| { + parse_remote_bookmarks_arguments(diagnostics, function, None) }); + map.insert( + "tracked_remote_bookmarks", + |diagnostics, function, _context| { + parse_remote_bookmarks_arguments(diagnostics, function, Some(RemoteRefState::Tracking)) + }, + ); + map.insert( + "untracked_remote_bookmarks", + |diagnostics, function, _context| { + parse_remote_bookmarks_arguments(diagnostics, function, Some(RemoteRefState::New)) + }, + ); // TODO: Remove in jj 0.28+ map.insert("branches", map["bookmarks"]); @@ -673,56 +683,56 @@ static BUILTIN_FUNCTION_MAP: Lazy> = Lazy: map["untracked_remote_bookmarks"], ); - map.insert("tags", |function, _context| { + map.insert("tags", |_diagnostics, function, _context| { function.expect_no_arguments()?; Ok(RevsetExpression::tags()) }); - map.insert("git_refs", |function, _context| { + map.insert("git_refs", |_diagnostics, function, _context| { function.expect_no_arguments()?; Ok(RevsetExpression::git_refs()) }); - map.insert("git_head", |function, _context| { + map.insert("git_head", |_diagnostics, function, _context| { function.expect_no_arguments()?; Ok(RevsetExpression::git_head()) }); - map.insert("latest", |function, context| { + map.insert("latest", |diagnostics, function, context| { let ([candidates_arg], [count_opt_arg]) = function.expect_arguments()?; - let candidates = lower_expression(candidates_arg, context)?; + let candidates = lower_expression(diagnostics, candidates_arg, context)?; let count = if let Some(count_arg) = count_opt_arg { - expect_literal("integer", count_arg)? + expect_literal(diagnostics, "integer", count_arg)? } else { 1 }; Ok(candidates.latest(count)) }); - map.insert("merges", |function, _context| { + map.insert("merges", |_diagnostics, function, _context| { function.expect_no_arguments()?; Ok(RevsetExpression::filter( RevsetFilterPredicate::ParentCount(2..u32::MAX), )) }); - map.insert("description", |function, _context| { + map.insert("description", |diagnostics, function, _context| { let [arg] = function.expect_exact_arguments()?; - let pattern = expect_string_pattern(arg)?; + let pattern = expect_string_pattern(diagnostics, arg)?; Ok(RevsetExpression::filter( RevsetFilterPredicate::Description(pattern), )) }); - map.insert("author", |function, _context| { + map.insert("author", |diagnostics, function, _context| { let [arg] = function.expect_exact_arguments()?; - let pattern = expect_string_pattern(arg)?; + let pattern = expect_string_pattern(diagnostics, arg)?; Ok(RevsetExpression::filter(RevsetFilterPredicate::Author( pattern, ))) }); - map.insert("author_date", |function, context| { + map.insert("author_date", |diagnostics, function, context| { let [arg] = function.expect_exact_arguments()?; - let pattern = expect_date_pattern(arg, context.date_pattern_context())?; + let pattern = expect_date_pattern(diagnostics, arg, context.date_pattern_context())?; Ok(RevsetExpression::filter(RevsetFilterPredicate::AuthorDate( pattern, ))) }); - map.insert("mine", |function, context| { + map.insert("mine", |_diagnostics, function, context| { function.expect_no_arguments()?; // Email address domains are inherently case‐insensitive, and the local‐parts // are generally (although not universally) treated as case‐insensitive too, so @@ -731,25 +741,25 @@ static BUILTIN_FUNCTION_MAP: Lazy> = Lazy: StringPattern::exact_i(&context.user_email), ))) }); - map.insert("committer", |function, _context| { + map.insert("committer", |diagnostics, function, _context| { let [arg] = function.expect_exact_arguments()?; - let pattern = expect_string_pattern(arg)?; + let pattern = expect_string_pattern(diagnostics, arg)?; Ok(RevsetExpression::filter(RevsetFilterPredicate::Committer( pattern, ))) }); - map.insert("committer_date", |function, context| { + map.insert("committer_date", |diagnostics, function, context| { let [arg] = function.expect_exact_arguments()?; - let pattern = expect_date_pattern(arg, context.date_pattern_context())?; + let pattern = expect_date_pattern(diagnostics, arg, context.date_pattern_context())?; Ok(RevsetExpression::filter( RevsetFilterPredicate::CommitterDate(pattern), )) }); - map.insert("empty", |function, _context| { + map.insert("empty", |_diagnostics, function, _context| { function.expect_no_arguments()?; Ok(RevsetExpression::is_empty()) }); - map.insert("file", |function, context| { + map.insert("file", |diagnostics, function, context| { let ctx = context.workspace.as_ref().ok_or_else(|| { RevsetParseError::with_span( RevsetParseErrorKind::FsPathWithoutWorkspace, @@ -759,14 +769,14 @@ static BUILTIN_FUNCTION_MAP: Lazy> = Lazy: // TODO: emit deprecation warning if multiple arguments are passed let ([arg], args) = function.expect_some_arguments()?; let file_expressions = itertools::chain([arg], args) - .map(|arg| expect_fileset_expression(arg, ctx.path_converter)) + .map(|arg| expect_fileset_expression(diagnostics, arg, ctx.path_converter)) .try_collect()?; let expr = FilesetExpression::union_all(file_expressions); Ok(RevsetExpression::filter(RevsetFilterPredicate::File(expr))) }); - map.insert("diff_contains", |function, context| { + map.insert("diff_contains", |diagnostics, function, context| { let ([text_arg], [files_opt_arg]) = function.expect_arguments()?; - let text = expect_string_pattern(text_arg)?; + let text = expect_string_pattern(diagnostics, text_arg)?; let files = if let Some(files_arg) = files_opt_arg { let ctx = context.workspace.as_ref().ok_or_else(|| { RevsetParseError::with_span( @@ -774,7 +784,7 @@ static BUILTIN_FUNCTION_MAP: Lazy> = Lazy: files_arg.span, ) })?; - expect_fileset_expression(files_arg, ctx.path_converter)? + expect_fileset_expression(diagnostics, files_arg, ctx.path_converter)? } else { // TODO: defaults to CLI path arguments? // https://github.com/martinvonz/jj/issues/2933#issuecomment-1925870731 @@ -784,13 +794,13 @@ static BUILTIN_FUNCTION_MAP: Lazy> = Lazy: RevsetFilterPredicate::DiffContains { text, files }, )) }); - map.insert("conflict", |function, _context| { + map.insert("conflict", |_diagnostics, function, _context| { function.expect_no_arguments()?; Ok(RevsetExpression::filter(RevsetFilterPredicate::HasConflict)) }); - map.insert("present", |function, context| { + map.insert("present", |diagnostics, function, context| { let [arg] = function.expect_exact_arguments()?; - let expression = lower_expression(arg, context)?; + let expression = lower_expression(diagnostics, arg, context)?; Ok(Rc::new(RevsetExpression::Present(expression))) }); map @@ -798,55 +808,73 @@ static BUILTIN_FUNCTION_MAP: Lazy> = Lazy: /// Parses the given `node` as a fileset expression. pub fn expect_fileset_expression( + diagnostics: &mut RevsetDiagnostics, node: &ExpressionNode, path_converter: &RepoPathUiConverter, ) -> Result { // Alias handling is a bit tricky. The outermost expression `alias` is // substituted, but inner expressions `x & alias` aren't. If this seemed // weird, we can either transform AST or turn off revset aliases completely. - revset_parser::expect_expression_with(node, |node| { - let mut inner_diagnostics = FilesetDiagnostics::new(); // TODO - fileset::parse(&mut inner_diagnostics, node.span.as_str(), path_converter).map_err(|err| { - RevsetParseError::expression("In fileset expression", node.span).with_source(err) - }) + revset_parser::expect_expression_with(diagnostics, node, |diagnostics, node| { + let mut inner_diagnostics = FilesetDiagnostics::new(); + let expression = fileset::parse(&mut inner_diagnostics, node.span.as_str(), path_converter) + .map_err(|err| { + RevsetParseError::expression("In fileset expression", node.span).with_source(err) + })?; + diagnostics.extend_with(inner_diagnostics, |diag| { + RevsetParseError::expression("In fileset expression", node.span).with_source(diag) + }); + Ok(expression) }) } -pub fn expect_string_pattern(node: &ExpressionNode) -> Result { - let parse_pattern = |value: &str, kind: Option<&str>| match kind { - Some(kind) => StringPattern::from_str_kind(value, kind), - None => Ok(StringPattern::Substring(value.to_owned())), - }; - revset_parser::expect_pattern_with("string pattern", node, parse_pattern) +pub fn expect_string_pattern( + diagnostics: &mut RevsetDiagnostics, + node: &ExpressionNode, +) -> Result { + revset_parser::expect_pattern_with( + diagnostics, + "string pattern", + node, + |_diagnostics, value, kind| match kind { + Some(kind) => StringPattern::from_str_kind(value, kind), + None => Ok(StringPattern::Substring(value.to_owned())), + }, + ) } pub fn expect_date_pattern( + diagnostics: &mut RevsetDiagnostics, node: &ExpressionNode, context: &DatePatternContext, ) -> Result { - let parse_pattern = - |value: &str, kind: Option<&str>| -> Result<_, Box> { + revset_parser::expect_pattern_with( + diagnostics, + "date pattern", + node, + |_diagnostics, value, kind| -> Result<_, Box> { match kind { None => Err("Date pattern must specify 'after' or 'before'".into()), Some(kind) => Ok(context.parse_relative(value, kind)?), } - }; - revset_parser::expect_pattern_with("date pattern", node, parse_pattern) + }, + ) } fn parse_remote_bookmarks_arguments( + diagnostics: &mut RevsetDiagnostics, function: &FunctionCallNode, remote_ref_state: Option, ) -> 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 { - expect_string_pattern(bookmark_arg)? + expect_string_pattern(diagnostics, bookmark_arg)? } else { StringPattern::everything() }; let remote_pattern = if let Some(remote_arg) = remote_opt_arg { - expect_string_pattern(remote_arg)? + expect_string_pattern(diagnostics, remote_arg)? } else { StringPattern::everything() }; @@ -859,12 +887,13 @@ fn parse_remote_bookmarks_arguments( /// Resolves function call by using the given function map. fn lower_function_call( + diagnostics: &mut RevsetDiagnostics, function: &FunctionCallNode, context: &RevsetParseContext, ) -> Result, RevsetParseError> { let function_map = &context.extensions.function_map; if let Some(func) = function_map.get(function.name) { - func(function, context) + func(diagnostics, function, context) } else { Err(RevsetParseError::with_span( RevsetParseErrorKind::NoSuchFunction { @@ -879,6 +908,7 @@ fn lower_function_call( /// Transforms the given AST `node` into expression that describes DAG /// operation. Function calls will be resolved at this stage. pub fn lower_expression( + diagnostics: &mut RevsetDiagnostics, node: &ExpressionNode, context: &RevsetParseContext, ) -> Result, RevsetParseError> { @@ -914,7 +944,7 @@ pub fn lower_expression( Ok(RevsetExpression::root().range(&RevsetExpression::visible_heads())) } ExpressionKind::Unary(op, arg_node) => { - let arg = lower_expression(arg_node, context)?; + let arg = lower_expression(diagnostics, arg_node, context)?; match op { UnaryOp::Negate => Ok(arg.negated()), UnaryOp::DagRangePre => Ok(arg.ancestors()), @@ -926,8 +956,8 @@ pub fn lower_expression( } } ExpressionKind::Binary(op, lhs_node, rhs_node) => { - let lhs = lower_expression(lhs_node, context)?; - let rhs = lower_expression(rhs_node, context)?; + let lhs = lower_expression(diagnostics, lhs_node, context)?; + let rhs = lower_expression(diagnostics, rhs_node, context)?; match op { BinaryOp::Intersection => Ok(lhs.intersection(&rhs)), BinaryOp::Difference => Ok(lhs.minus(&rhs)), @@ -938,11 +968,13 @@ pub fn lower_expression( ExpressionKind::UnionAll(nodes) => { let expressions: Vec<_> = nodes .iter() - .map(|node| lower_expression(node, context)) + .map(|node| lower_expression(diagnostics, node, context)) .try_collect()?; Ok(RevsetExpression::union_all(&expressions)) } - ExpressionKind::FunctionCall(function) => lower_function_call(function, context), + ExpressionKind::FunctionCall(function) => { + lower_function_call(diagnostics, function, context) + } ExpressionKind::Modifier(modifier) => { let name = modifier.name; Err(RevsetParseError::expression( @@ -951,31 +983,40 @@ pub fn lower_expression( )) } ExpressionKind::AliasExpanded(id, subst) => { - lower_expression(subst, context).map_err(|e| e.within_alias_expansion(*id, node.span)) + let mut inner_diagnostics = RevsetDiagnostics::new(); + let expression = lower_expression(&mut inner_diagnostics, subst, context) + .map_err(|e| e.within_alias_expansion(*id, node.span))?; + diagnostics.extend_with(inner_diagnostics, |diag| { + diag.within_alias_expansion(*id, node.span) + }); + Ok(expression) } } } pub fn parse( + diagnostics: &mut RevsetDiagnostics, revset_str: &str, context: &RevsetParseContext, ) -> Result, RevsetParseError> { let node = revset_parser::parse_program(revset_str)?; let node = dsl_util::expand_aliases(node, context.aliases_map)?; - lower_expression(&node, context) + lower_expression(diagnostics, &node, context) .map_err(|err| err.extend_function_candidates(context.aliases_map.function_names())) } pub fn parse_with_modifier( + diagnostics: &mut RevsetDiagnostics, revset_str: &str, context: &RevsetParseContext, ) -> 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( + diagnostics, &node, - |node| lower_expression(node, context), - |name, span| match name { + |diagnostics, node| lower_expression(diagnostics, node, context), + |_diagnostics, name, span| match name { "all" => Ok(RevsetModifier::All), _ => Err(RevsetParseError::with_span( RevsetParseErrorKind::NoSuchModifier(name.to_owned()), @@ -2178,7 +2219,7 @@ mod tests { &extensions, None, ); - super::parse(revset_str, &context) + super::parse(&mut RevsetDiagnostics::new(), revset_str, &context) } fn parse_with_aliases_and_workspace( @@ -2207,7 +2248,7 @@ mod tests { &extensions, Some(workspace_ctx), ); - super::parse(revset_str, &context) + super::parse(&mut RevsetDiagnostics::new(), revset_str, &context) } fn parse_with_modifier( @@ -2232,7 +2273,7 @@ mod tests { &extensions, None, ); - super::parse_with_modifier(revset_str, &context) + super::parse_with_modifier(&mut RevsetDiagnostics::new(), revset_str, &context) } fn insta_settings() -> insta::Settings { diff --git a/lib/src/revset_parser.rs b/lib/src/revset_parser.rs index 7acee3b28..ea3ecea45 100644 --- a/lib/src/revset_parser.rs +++ b/lib/src/revset_parser.rs @@ -39,6 +39,7 @@ use crate::dsl_util::AliasExpandError; use crate::dsl_util::AliasExpandableExpression; use crate::dsl_util::AliasId; use crate::dsl_util::AliasesMap; +use crate::dsl_util::Diagnostics; use crate::dsl_util::ExpressionFolder; use crate::dsl_util::FoldableExpression; use crate::dsl_util::InvalidArguments; @@ -126,6 +127,10 @@ impl Rule { } } +/// Manages diagnostic messages emitted during revset parsing and function-call +/// resolution. +pub type RevsetDiagnostics = Diagnostics; + #[derive(Debug, Error)] #[error("{pest_error}")] pub struct RevsetParseError { @@ -764,32 +769,41 @@ impl AliasDefinitionParser for RevsetAliasParser { /// Applies the given functions to the top-level expression body node with an /// optional modifier. Alias expansion nodes are unwrapped accordingly. pub(super) fn expect_program_with( + diagnostics: &mut RevsetDiagnostics, node: &ExpressionNode, - parse_body: impl FnOnce(&ExpressionNode) -> Result, - parse_modifier: impl FnOnce(&str, pest::Span<'_>) -> Result, + parse_body: impl FnOnce(&mut RevsetDiagnostics, &ExpressionNode) -> Result, + parse_modifier: impl FnOnce( + &mut RevsetDiagnostics, + &str, + pest::Span<'_>, + ) -> Result, ) -> Result<(B, Option), RevsetParseError> { - expect_expression_with(node, |node| match &node.kind { + expect_expression_with(diagnostics, node, |diagnostics, node| match &node.kind { ExpressionKind::Modifier(modifier) => { - let parsed_modifier = parse_modifier(modifier.name, modifier.name_span)?; - Ok((parse_body(&modifier.body)?, Some(parsed_modifier))) + let parsed_modifier = parse_modifier(diagnostics, modifier.name, modifier.name_span)?; + let parsed_body = parse_body(diagnostics, &modifier.body)?; + Ok((parsed_body, Some(parsed_modifier))) } - _ => Ok((parse_body(node)?, None)), + _ => Ok((parse_body(diagnostics, node)?, None)), }) } pub(super) fn expect_pattern_with>>( + diagnostics: &mut RevsetDiagnostics, type_name: &str, node: &ExpressionNode, - parse_pattern: impl FnOnce(&str, Option<&str>) -> Result, + parse_pattern: impl FnOnce(&mut RevsetDiagnostics, &str, Option<&str>) -> Result, ) -> Result { let wrap_error = |err: E| { RevsetParseError::expression(format!("Invalid {type_name}"), node.span).with_source(err) }; - expect_expression_with(node, |node| match &node.kind { - ExpressionKind::Identifier(name) => parse_pattern(name, None).map_err(wrap_error), - ExpressionKind::String(name) => parse_pattern(name, None).map_err(wrap_error), + expect_expression_with(diagnostics, node, |diagnostics, node| match &node.kind { + ExpressionKind::Identifier(name) => { + parse_pattern(diagnostics, name, None).map_err(wrap_error) + } + ExpressionKind::String(name) => parse_pattern(diagnostics, name, None).map_err(wrap_error), ExpressionKind::StringPattern { kind, value } => { - parse_pattern(value, Some(kind)).map_err(wrap_error) + parse_pattern(diagnostics, value, Some(kind)).map_err(wrap_error) } _ => Err(RevsetParseError::expression( format!("Expected expression of {type_name}"), @@ -799,6 +813,7 @@ pub(super) fn expect_pattern_with } pub fn expect_literal( + diagnostics: &mut RevsetDiagnostics, type_name: &str, node: &ExpressionNode, ) -> Result { @@ -808,7 +823,7 @@ pub fn expect_literal( node.span, ) }; - expect_expression_with(node, |node| match &node.kind { + expect_expression_with(diagnostics, node, |_diagnostics, node| match &node.kind { ExpressionKind::Identifier(name) => name.parse().map_err(|_| make_error()), ExpressionKind::String(name) => name.parse().map_err(|_| make_error()), _ => Err(make_error()), @@ -818,13 +833,20 @@ pub fn expect_literal( /// Applies the give function to the innermost `node` by unwrapping alias /// expansion nodes. pub(super) fn expect_expression_with( + diagnostics: &mut RevsetDiagnostics, node: &ExpressionNode, - f: impl FnOnce(&ExpressionNode) -> Result, + f: impl FnOnce(&mut RevsetDiagnostics, &ExpressionNode) -> Result, ) -> Result { if let ExpressionKind::AliasExpanded(id, subst) = &node.kind { - expect_expression_with(subst, f).map_err(|e| e.within_alias_expansion(*id, node.span)) + let mut inner_diagnostics = RevsetDiagnostics::new(); + let expression = expect_expression_with(&mut inner_diagnostics, subst, f) + .map_err(|e| e.within_alias_expansion(*id, node.span))?; + diagnostics.extend_with(inner_diagnostics, |diag| { + diag.within_alias_expansion(*id, node.span) + }); + Ok(expression) } else { - f(node) + f(diagnostics, node) } } diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs index 2467f1782..cddd39ecc 100644 --- a/lib/tests/test_revset.rs +++ b/lib/tests/test_revset.rs @@ -43,6 +43,7 @@ use jj_lib::revset::FailingSymbolResolver; use jj_lib::revset::ResolvedExpression; use jj_lib::revset::Revset; use jj_lib::revset::RevsetAliasesMap; +use jj_lib::revset::RevsetDiagnostics; use jj_lib::revset::RevsetExpression; use jj_lib::revset::RevsetExtensions; use jj_lib::revset::RevsetFilterPredicate; @@ -70,7 +71,7 @@ fn resolve_symbol_with_extensions( let now = chrono::Local::now(); let context = RevsetParseContext::new(&aliases_map, String::new(), now.into(), extensions, None); - let expression = parse(symbol, &context).unwrap(); + let expression = parse(&mut RevsetDiagnostics::new(), symbol, &context).unwrap(); assert_matches!(*expression, RevsetExpression::CommitRef(_)); let symbol_resolver = DefaultSymbolResolver::new(repo, extensions.symbol_resolvers()); match expression.resolve_user_expression(repo, &symbol_resolver)? { @@ -211,7 +212,8 @@ fn test_resolve_symbol_commit_id() { None, ); assert_matches!( - optimize(parse("present(04)", &context).unwrap()).resolve_user_expression(repo.as_ref(), &symbol_resolver), + optimize(parse(&mut RevsetDiagnostics::new(), "present(04)", &context).unwrap()) + .resolve_user_expression(repo.as_ref(), &symbol_resolver), Err(RevsetResolutionError::AmbiguousCommitIdPrefix(s)) if s == "04" ); assert_eq!( @@ -880,7 +882,7 @@ fn resolve_commit_ids(repo: &dyn Repo, revset_str: &str) -> Vec { &revset_extensions, None, ); - let expression = optimize(parse(revset_str, &context).unwrap()); + let expression = optimize(parse(&mut RevsetDiagnostics::new(), revset_str, &context).unwrap()); let symbol_resolver = DefaultSymbolResolver::new(repo, revset_extensions.symbol_resolvers()); let expression = expression .resolve_user_expression(repo, &symbol_resolver) @@ -912,7 +914,7 @@ fn resolve_commit_ids_in_workspace( &extensions, Some(workspace_ctx), ); - let expression = optimize(parse(revset_str, &context).unwrap()); + let expression = optimize(parse(&mut RevsetDiagnostics::new(), revset_str, &context).unwrap()); let symbol_resolver = DefaultSymbolResolver::new(repo, &([] as [&Box; 0])); let expression = expression