From 4b477fa59e2c488e305005ad31e9c80b792acdb1 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 19 Sep 2024 15:02:10 +0900 Subject: [PATCH] fileset: pass diagnostics receiver around, add printing function CLI tests will be added later. --- cli/src/cli_util.rs | 13 +++++++--- cli/src/command_error.rs | 18 +++++++++++++ cli/src/commands/debug/fileset.rs | 6 ++++- cli/src/commands/fix.rs | 37 +++++++++++++++------------ cli/src/commit_templater.rs | 4 ++- lib/src/fileset.rs | 42 +++++++++++++++++++------------ lib/src/fileset_parser.rs | 5 ++++ lib/src/revset.rs | 4 ++- 8 files changed, 91 insertions(+), 38 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index cdb8e62aa..f8bdf3362 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -58,6 +58,7 @@ use jj_lib::commit::Commit; use jj_lib::dag_walk; use jj_lib::file_util; use jj_lib::fileset; +use jj_lib::fileset::FilesetDiagnostics; use jj_lib::fileset::FilesetExpression; use jj_lib::git; use jj_lib::git_backend::GitBackend; @@ -131,6 +132,7 @@ use crate::command_error::config_error_with_message; use crate::command_error::handle_command_result; use crate::command_error::internal_error; use crate::command_error::internal_error_with_message; +use crate::command_error::print_parse_diagnostics; use crate::command_error::user_error; use crate::command_error::user_error_with_hint; use crate::command_error::user_error_with_message; @@ -1082,25 +1084,30 @@ impl WorkspaceCommandHelper { /// Parses the given fileset expressions and concatenates them all. pub fn parse_union_filesets( &self, - _ui: &Ui, + ui: &Ui, file_args: &[String], // TODO: introduce FileArg newtype? ) -> Result { + let mut diagnostics = FilesetDiagnostics::new(); let expressions: Vec<_> = file_args .iter() - .map(|arg| fileset::parse_maybe_bare(arg, self.path_converter())) + .map(|arg| fileset::parse_maybe_bare(&mut diagnostics, arg, self.path_converter())) .try_collect()?; + print_parse_diagnostics(ui, "In fileset expression", &diagnostics)?; Ok(FilesetExpression::union_all(expressions)) } - pub fn auto_tracking_matcher(&self, _ui: &Ui) -> Result, CommandError> { + pub fn auto_tracking_matcher(&self, ui: &Ui) -> Result, CommandError> { + let mut diagnostics = FilesetDiagnostics::new(); let pattern = self.settings().config().get_string("snapshot.auto-track")?; let expression = fileset::parse( + &mut diagnostics, &pattern, &RepoPathUiConverter::Fs { cwd: "".into(), base: "".into(), }, )?; + print_parse_diagnostics(ui, "In `snapshot.auto-track`", &diagnostics)?; Ok(expression.to_matcher()) } diff --git a/cli/src/command_error.rs b/cli/src/command_error.rs index 9bd78b396..489ad8db9 100644 --- a/cli/src/command_error.rs +++ b/cli/src/command_error.rs @@ -22,6 +22,7 @@ use std::sync::Arc; use itertools::Itertools as _; use jj_lib::backend::BackendError; +use jj_lib::dsl_util::Diagnostics; use jj_lib::fileset::FilePatternParseError; use jj_lib::fileset::FilesetParseError; use jj_lib::fileset::FilesetParseErrorKind; @@ -835,3 +836,20 @@ fn handle_clap_error(ui: &mut Ui, err: &clap::Error, hints: &[ErrorHint]) -> io: print_error_hints(ui, hints)?; Ok(ExitCode::from(2)) } + +/// Prints diagnostic messages emitted during parsing. +pub fn print_parse_diagnostics( + ui: &Ui, + context_message: &str, + diagnostics: &Diagnostics, +) -> io::Result<()> { + for diag in diagnostics { + writeln!(ui.warning_default(), "{context_message}")?; + for err in iter::successors(Some(diag as &dyn error::Error), |err| err.source()) { + writeln!(ui.stderr(), "{err}")?; + } + // If we add support for multiple error diagnostics, we might have to do + // find_source_parse_error_hint() and print it here. + } + Ok(()) +} diff --git a/cli/src/commands/debug/fileset.rs b/cli/src/commands/debug/fileset.rs index 2d0468a94..de4154375 100644 --- a/cli/src/commands/debug/fileset.rs +++ b/cli/src/commands/debug/fileset.rs @@ -16,8 +16,10 @@ use std::fmt::Debug; use std::io::Write as _; use jj_lib::fileset; +use jj_lib::fileset::FilesetDiagnostics; use crate::cli_util::CommandHelper; +use crate::command_error::print_parse_diagnostics; use crate::command_error::CommandError; use crate::ui::Ui; @@ -36,7 +38,9 @@ pub fn cmd_debug_fileset( let workspace_command = command.workspace_helper(ui)?; let path_converter = workspace_command.path_converter(); - let expression = fileset::parse_maybe_bare(&args.path, path_converter)?; + let mut diagnostics = FilesetDiagnostics::new(); + let expression = fileset::parse_maybe_bare(&mut diagnostics, &args.path, path_converter)?; + print_parse_diagnostics(ui, "In fileset expression", &diagnostics)?; writeln!(ui.stdout(), "-- Parsed:")?; writeln!(ui.stdout(), "{expression:#?}")?; writeln!(ui.stdout())?; diff --git a/cli/src/commands/fix.rs b/cli/src/commands/fix.rs index b098e602e..f4c751fae 100644 --- a/cli/src/commands/fix.rs +++ b/cli/src/commands/fix.rs @@ -25,6 +25,7 @@ use jj_lib::backend::CommitId; use jj_lib::backend::FileId; use jj_lib::backend::TreeValue; use jj_lib::fileset; +use jj_lib::fileset::FilesetDiagnostics; use jj_lib::fileset::FilesetExpression; use jj_lib::matchers::EverythingMatcher; use jj_lib::matchers::Matcher; @@ -46,6 +47,7 @@ use tracing::instrument; use crate::cli_util::CommandHelper; use crate::cli_util::RevisionArg; use crate::command_error::config_error; +use crate::command_error::print_parse_diagnostics; use crate::command_error::CommandError; use crate::config::to_toml_value; use crate::config::CommandNameAndArgs; @@ -473,25 +475,28 @@ fn get_tools_config(ui: &mut Ui, config: &config::Config) -> Result = tools_table .into_iter() .sorted_by(|a, b| a.0.cmp(&b.0)) - .map(|(_name, value)| -> Result { + .map(|(name, value)| -> Result { + let mut diagnostics = FilesetDiagnostics::new(); let tool: RawToolConfig = value.try_deserialize()?; + let expression = FilesetExpression::union_all( + tool.patterns + .iter() + .map(|arg| { + fileset::parse( + &mut diagnostics, + arg, + &RepoPathUiConverter::Fs { + cwd: "".into(), + base: "".into(), + }, + ) + }) + .try_collect()?, + ); + print_parse_diagnostics(ui, &format!("In `fix.tools.{name}`"), &diagnostics)?; Ok(ToolConfig { command: tool.command, - matcher: FilesetExpression::union_all( - tool.patterns - .iter() - .map(|arg| { - fileset::parse( - arg, - &RepoPathUiConverter::Fs { - cwd: "".into(), - base: "".into(), - }, - ) - }) - .try_collect()?, - ) - .to_matcher(), + matcher: expression.to_matcher(), }) }) .try_collect()?; diff --git a/cli/src/commit_templater.rs b/cli/src/commit_templater.rs index 2ca37d495..56e01783c 100644 --- a/cli/src/commit_templater.rs +++ b/cli/src/commit_templater.rs @@ -28,6 +28,7 @@ use jj_lib::copies::CopiesTreeDiffEntry; use jj_lib::copies::CopyRecords; use jj_lib::extensions_map::ExtensionsMap; use jj_lib::fileset; +use jj_lib::fileset::FilesetDiagnostics; use jj_lib::fileset::FilesetExpression; use jj_lib::git; use jj_lib::hex_util::to_reverse_hex; @@ -767,7 +768,8 @@ fn expect_fileset_literal( path_converter: &RepoPathUiConverter, ) -> Result { template_parser::expect_string_literal_with(node, |text, span| { - fileset::parse(text, path_converter).map_err(|err| { + let mut inner_diagnostics = FilesetDiagnostics::new(); // TODO + fileset::parse(&mut inner_diagnostics, text, path_converter).map_err(|err| { TemplateParseError::expression("In fileset expression", span).with_source(err) }) }) diff --git a/lib/src/fileset.rs b/lib/src/fileset.rs index 78c64a0c3..a7136b037 100644 --- a/lib/src/fileset.rs +++ b/lib/src/fileset.rs @@ -28,6 +28,7 @@ use crate::fileset_parser; use crate::fileset_parser::BinaryOp; use crate::fileset_parser::ExpressionKind; use crate::fileset_parser::ExpressionNode; +pub use crate::fileset_parser::FilesetDiagnostics; pub use crate::fileset_parser::FilesetParseError; pub use crate::fileset_parser::FilesetParseErrorKind; pub use crate::fileset_parser::FilesetParseResult; @@ -378,18 +379,21 @@ fn union_all_matchers(matchers: &mut [Option>]) -> Box FilesetParseResult; +type FilesetFunction = fn( + &mut FilesetDiagnostics, + &RepoPathUiConverter, + &FunctionCallNode, +) -> FilesetParseResult; 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, FilesetFunction> = HashMap::new(); - map.insert("none", |_path_converter, function| { + map.insert("none", |_diagnostics, _path_converter, function| { function.expect_no_arguments()?; Ok(FilesetExpression::none()) }); - map.insert("all", |_path_converter, function| { + map.insert("all", |_diagnostics, _path_converter, function| { function.expect_no_arguments()?; Ok(FilesetExpression::all()) }); @@ -397,11 +401,12 @@ static BUILTIN_FUNCTION_MAP: Lazy> = Lazy }); fn resolve_function( + diagnostics: &mut FilesetDiagnostics, path_converter: &RepoPathUiConverter, function: &FunctionCallNode, ) -> FilesetParseResult { if let Some(func) = BUILTIN_FUNCTION_MAP.get(function.name) { - func(path_converter, function) + func(diagnostics, path_converter, function) } else { Err(FilesetParseError::new( FilesetParseErrorKind::NoSuchFunction { @@ -414,6 +419,7 @@ fn resolve_function( } fn resolve_expression( + diagnostics: &mut FilesetDiagnostics, path_converter: &RepoPathUiConverter, node: &ExpressionNode, ) -> FilesetParseResult { @@ -436,14 +442,14 @@ fn resolve_expression( Ok(FilesetExpression::pattern(pattern)) } ExpressionKind::Unary(op, arg_node) => { - let arg = resolve_expression(path_converter, arg_node)?; + let arg = resolve_expression(diagnostics, path_converter, arg_node)?; match op { UnaryOp::Negate => Ok(FilesetExpression::all().difference(arg)), } } ExpressionKind::Binary(op, lhs_node, rhs_node) => { - let lhs = resolve_expression(path_converter, lhs_node)?; - let rhs = resolve_expression(path_converter, rhs_node)?; + let lhs = resolve_expression(diagnostics, path_converter, lhs_node)?; + let rhs = resolve_expression(diagnostics, path_converter, rhs_node)?; match op { BinaryOp::Intersection => Ok(lhs.intersection(rhs)), BinaryOp::Difference => Ok(lhs.difference(rhs)), @@ -452,22 +458,25 @@ fn resolve_expression( ExpressionKind::UnionAll(nodes) => { let expressions = nodes .iter() - .map(|node| resolve_expression(path_converter, node)) + .map(|node| resolve_expression(diagnostics, path_converter, node)) .try_collect()?; Ok(FilesetExpression::union_all(expressions)) } - ExpressionKind::FunctionCall(function) => resolve_function(path_converter, function), + ExpressionKind::FunctionCall(function) => { + resolve_function(diagnostics, path_converter, function) + } } } /// Parses text into `FilesetExpression` without bare string fallback. pub fn parse( + diagnostics: &mut FilesetDiagnostics, text: &str, path_converter: &RepoPathUiConverter, ) -> FilesetParseResult { let node = fileset_parser::parse_program(text)?; // TODO: add basic tree substitution pass to eliminate redundant expressions - resolve_expression(path_converter, &node) + resolve_expression(diagnostics, path_converter, &node) } /// Parses text into `FilesetExpression` with bare string fallback. @@ -475,12 +484,13 @@ pub fn parse( /// If the text can't be parsed as a fileset expression, and if it doesn't /// contain any operator-like characters, it will be parsed as a file path. pub fn parse_maybe_bare( + diagnostics: &mut FilesetDiagnostics, text: &str, path_converter: &RepoPathUiConverter, ) -> FilesetParseResult { let node = fileset_parser::parse_program_or_bare_string(text)?; // TODO: add basic tree substitution pass to eliminate redundant expressions - resolve_expression(path_converter, &node) + resolve_expression(diagnostics, path_converter, &node) } #[cfg(test)] @@ -515,7 +525,7 @@ mod tests { cwd: PathBuf::from("/ws/cur"), base: PathBuf::from("/ws"), }; - let parse = |text| parse_maybe_bare(text, &path_converter); + let parse = |text| parse_maybe_bare(&mut FilesetDiagnostics::new(), text, &path_converter); // cwd-relative patterns assert_eq!( @@ -567,7 +577,7 @@ mod tests { cwd: PathBuf::from("/ws/cur*"), base: PathBuf::from("/ws"), }; - let parse = |text| parse_maybe_bare(text, &path_converter); + let parse = |text| parse_maybe_bare(&mut FilesetDiagnostics::new(), text, &path_converter); let glob_expr = |dir: &str, pattern: &str| { FilesetExpression::pattern(FilePattern::FileGlob { dir: repo_path_buf(dir), @@ -652,7 +662,7 @@ mod tests { cwd: PathBuf::from("/ws/cur"), base: PathBuf::from("/ws"), }; - let parse = |text| parse_maybe_bare(text, &path_converter); + let parse = |text| parse_maybe_bare(&mut FilesetDiagnostics::new(), text, &path_converter); assert_eq!(parse("all()").unwrap(), FilesetExpression::all()); assert_eq!(parse("none()").unwrap(), FilesetExpression::none()); @@ -680,7 +690,7 @@ mod tests { cwd: PathBuf::from("/ws/cur"), base: PathBuf::from("/ws"), }; - let parse = |text| parse_maybe_bare(text, &path_converter); + let parse = |text| parse_maybe_bare(&mut FilesetDiagnostics::new(), text, &path_converter); insta::assert_debug_snapshot!(parse("~x").unwrap(), @r###" Difference( diff --git a/lib/src/fileset_parser.rs b/lib/src/fileset_parser.rs index ad62eea33..211be4ce0 100644 --- a/lib/src/fileset_parser.rs +++ b/lib/src/fileset_parser.rs @@ -27,6 +27,7 @@ use pest_derive::Parser; use thiserror::Error; use crate::dsl_util; +use crate::dsl_util::Diagnostics; use crate::dsl_util::InvalidArguments; use crate::dsl_util::StringLiteralParser; @@ -74,6 +75,10 @@ impl Rule { } } +/// Manages diagnostic messages emitted during fileset parsing and name +/// resolution. +pub type FilesetDiagnostics = Diagnostics; + /// Result of fileset parsing and name resolution. pub type FilesetParseResult = Result; diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 790082139..9abff3486 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -36,6 +36,7 @@ use crate::dsl_util; use crate::dsl_util::collect_similar; use crate::dsl_util::AliasExpandError as _; use crate::fileset; +use crate::fileset::FilesetDiagnostics; use crate::fileset::FilesetExpression; use crate::graph::GraphEdge; use crate::hex_util::to_forward_hex; @@ -804,7 +805,8 @@ pub fn expect_fileset_expression( // 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| { - fileset::parse(node.span.as_str(), path_converter).map_err(|err| { + 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) }) })