From 6beecfa0449505d879d84449c952da1d1cb0ce88 Mon Sep 17 00:00:00 2001 From: kek kek kek Date: Wed, 18 Oct 2023 09:47:11 -0700 Subject: [PATCH] chore: unify of expression formatting in parentheses (#3138) --- tooling/nargo_fmt/src/lib.rs | 2 + tooling/nargo_fmt/src/utils.rs | 177 +++++++++++++++++++ tooling/nargo_fmt/src/visitor.rs | 18 +- tooling/nargo_fmt/src/visitor/expr.rs | 201 +++++++++++++++------- tooling/nargo_fmt/tests/expected/call.nr | 22 +++ tooling/nargo_fmt/tests/expected/expr.nr | 11 ++ tooling/nargo_fmt/tests/expected/tuple.nr | 47 +++++ tooling/nargo_fmt/tests/input/call.nr | 32 ++++ tooling/nargo_fmt/tests/input/expr.nr | 11 ++ tooling/nargo_fmt/tests/input/tuple.nr | 48 ++++++ 10 files changed, 502 insertions(+), 67 deletions(-) create mode 100644 tooling/nargo_fmt/src/utils.rs create mode 100644 tooling/nargo_fmt/tests/expected/tuple.nr create mode 100644 tooling/nargo_fmt/tests/input/tuple.nr diff --git a/tooling/nargo_fmt/src/lib.rs b/tooling/nargo_fmt/src/lib.rs index 9bc148ae304..887e71be735 100644 --- a/tooling/nargo_fmt/src/lib.rs +++ b/tooling/nargo_fmt/src/lib.rs @@ -20,7 +20,9 @@ /// in both placement and content during the formatting process. mod config; pub mod errors; +#[macro_use] mod visitor; +mod utils; use noirc_frontend::ParsedModule; use visitor::FmtVisitor; diff --git a/tooling/nargo_fmt/src/utils.rs b/tooling/nargo_fmt/src/utils.rs new file mode 100644 index 00000000000..05a79485093 --- /dev/null +++ b/tooling/nargo_fmt/src/utils.rs @@ -0,0 +1,177 @@ +use crate::visitor::FmtVisitor; +use noirc_frontend::hir::resolution::errors::Span; +use noirc_frontend::lexer::Lexer; +use noirc_frontend::token::Token; +use noirc_frontend::Expression; + +pub(crate) fn recover_comment_removed(original: &str, new: String) -> String { + if changed_comment_content(original, &new) { + original.to_string() + } else { + new + } +} + +pub(crate) fn changed_comment_content(original: &str, new: &str) -> bool { + comments(original).ne(comments(new)) +} + +pub(crate) fn comments(source: &str) -> impl Iterator + '_ { + Lexer::new(source).skip_comments(false).flatten().filter_map(|spanned| { + if let Token::LineComment(content) | Token::BlockComment(content) = spanned.into_token() { + Some(content) + } else { + None + } + }) +} + +#[derive(Debug)] +pub(crate) struct Expr { + pub(crate) leading: String, + pub(crate) expr: String, + pub(crate) trailing: String, + pub(crate) different_line: bool, +} + +pub(crate) struct Exprs<'me> { + pub(crate) visitor: &'me FmtVisitor<'me>, + pub(crate) elements: std::iter::Peekable>, + pub(crate) last_position: u32, + pub(crate) end_position: u32, +} + +impl<'me> Exprs<'me> { + pub(crate) fn new( + visitor: &'me FmtVisitor<'me>, + span: Span, + elements: Vec, + ) -> Self { + Self { + visitor, + last_position: span.start() + 1, /*(*/ + end_position: span.end() - 1, /*)*/ + elements: elements.into_iter().peekable(), + } + } +} + +impl Iterator for Exprs<'_> { + type Item = Expr; + + fn next(&mut self) -> Option { + let element = self.elements.next()?; + let element_span = element.span; + + let start = self.last_position; + let end = element_span.start(); + + let is_last = self.elements.peek().is_none(); + let next_start = self.elements.peek().map_or(self.end_position, |expr| expr.span.start()); + + let (leading, newlines) = self.leading(start, end); + let expr = self.visitor.format_expr(element); + let trailing = self.trailing(element_span.end(), next_start, is_last); + + Expr { leading, expr, trailing, different_line: newlines }.into() + } +} + +impl<'me> Exprs<'me> { + pub(crate) fn leading(&mut self, start: u32, end: u32) -> (String, bool) { + let mut newlines = false; + + let leading = slice!(self.visitor, start, end); + let leading_trimmed = slice!(self.visitor, start, end).trim(); + + let starts_with_block_comment = leading_trimmed.starts_with("/*"); + let ends_with_block_comment = leading_trimmed.ends_with("*/"); + let starts_with_single_line_comment = leading_trimmed.starts_with("//"); + + if ends_with_block_comment { + let comment_end = leading_trimmed.rfind(|c| c == '/').unwrap(); + + if leading[comment_end..].contains('\n') { + newlines = true; + } + } else if starts_with_single_line_comment || starts_with_block_comment { + newlines = true; + }; + + (leading_trimmed.to_string(), newlines) + } + + pub(crate) fn trailing(&mut self, start: u32, end: u32, is_last: bool) -> String { + let slice = slice!(self.visitor, start, end); + let comment_end = find_comment_end(slice, is_last); + let trailing = slice[..comment_end].trim_matches(',').trim(); + self.last_position = start + (comment_end as u32); + trailing.to_string() + } +} + +pub(crate) trait FindToken { + fn find_token(&self, token: Token) -> Option; + fn find_token_with(&self, f: impl Fn(&Token) -> bool) -> Option; +} + +impl FindToken for str { + fn find_token(&self, token: Token) -> Option { + Lexer::new(self) + .flatten() + .find_map(|it| (it.token() == &token).then(|| it.to_span().start())) + } + + fn find_token_with(&self, f: impl Fn(&Token) -> bool) -> Option { + Lexer::new(self) + .skip_comments(false) + .flatten() + .into_iter() + .find_map(|spanned| f(spanned.token()).then(|| spanned.to_span().end())) + } +} + +pub(crate) fn find_comment_end(slice: &str, is_last: bool) -> usize { + fn find_comment_end(slice: &str) -> usize { + slice + .find_token_with(|token| { + matches!(token, Token::LineComment(_) | Token::BlockComment(_)) + }) + .map(|index| index as usize) + .unwrap_or(slice.len()) + } + + if is_last { + return slice.len(); + } + + let mut block_open_index = slice.find("/*"); + if let Some(index) = block_open_index { + match slice.find('/') { + Some(slash) if slash < index => block_open_index = None, + _ if slice[..index].ends_with('/') => block_open_index = None, + _ => (), + } + } + + let newline_index = slice.find('\n'); + if let Some(separator_index) = slice.find_token(Token::Comma).map(|index| index as usize) { + match (block_open_index, newline_index) { + (Some(block), None) if block > separator_index => separator_index + 1, + (Some(block), None) => { + let slice = &slice[block..]; + std::cmp::max(find_comment_end(slice) + block, separator_index + 1) + } + (Some(block), Some(newline)) if block < newline => { + let slice = &slice[block..]; + std::cmp::max(find_comment_end(slice) + block, separator_index + 1) + } + (_, Some(newline)) if newline > separator_index => newline + 1, + _ => slice.len(), + } + } else if let Some(newline_index) = newline_index { + newline_index + 1 + } else { + 0 + } +} diff --git a/tooling/nargo_fmt/src/visitor.rs b/tooling/nargo_fmt/src/visitor.rs index b0b897c4cd7..e2a20053a3f 100644 --- a/tooling/nargo_fmt/src/visitor.rs +++ b/tooling/nargo_fmt/src/visitor.rs @@ -1,7 +1,7 @@ /// A macro to create a slice from a given data source, helping to avoid borrow checker errors. #[macro_export] macro_rules! slice { - ($this:ident, $start:expr, $end:expr) => { + ($this:expr, $start:expr, $end:expr) => { &$this.source[$start as usize..$end as usize] }; } @@ -17,8 +17,8 @@ use crate::config::Config; pub(crate) struct FmtVisitor<'me> { config: &'me Config, buffer: String, - source: &'me str, - block_indent: Indent, + pub(crate) source: &'me str, + indent: Indent, last_position: u32, } @@ -29,17 +29,17 @@ impl<'me> FmtVisitor<'me> { config, source, last_position: 0, - block_indent: Indent { block_indent: 0 }, + indent: Indent { block_indent: 0 }, } } pub(crate) fn fork(&self) -> Self { Self { - config: self.config, buffer: String::new(), + config: self.config, source: self.source, - block_indent: self.block_indent, last_position: self.last_position, + indent: self.indent, } } @@ -48,9 +48,9 @@ impl<'me> FmtVisitor<'me> { } fn with_indent(&mut self, f: impl FnOnce(&mut Self) -> T) -> T { - self.block_indent.block_indent(self.config); + self.indent.block_indent(self.config); let ret = f(self); - self.block_indent.block_unindent(self.config); + self.indent.block_unindent(self.config); ret } @@ -82,7 +82,7 @@ impl<'me> FmtVisitor<'me> { } if should_indent { - let indent = this.block_indent.to_string(); + let indent = this.indent.to_string(); this.push_str(&indent); } }); diff --git a/tooling/nargo_fmt/src/visitor/expr.rs b/tooling/nargo_fmt/src/visitor/expr.rs index c2d3b4f6632..5ed6c4bd3df 100644 --- a/tooling/nargo_fmt/src/visitor/expr.rs +++ b/tooling/nargo_fmt/src/visitor/expr.rs @@ -1,22 +1,26 @@ +use std::ops::Range; + use noirc_frontend::{ - hir::resolution::errors::Span, lexer::Lexer, token::Token, ArrayLiteral, BlockExpression, - Expression, ExpressionKind, Literal, Statement, UnaryOp, + hir::resolution::errors::Span, token::Token, ArrayLiteral, BlockExpression, Expression, + ExpressionKind, Literal, Statement, UnaryOp, }; -use super::FmtVisitor; +use super::{FmtVisitor, Indent}; +use crate::utils::{self, Expr, FindToken}; impl FmtVisitor<'_> { pub(crate) fn visit_expr(&mut self, expr: Expression) { let span = expr.span; let rewrite = self.format_expr(expr); - let rewrite = recover_comment_removed(slice!(self, span.start(), span.end()), rewrite); + let rewrite = + utils::recover_comment_removed(slice!(self, span.start(), span.end()), rewrite); self.push_rewrite(rewrite, span); self.last_position = span.end(); } - fn format_expr(&self, Expression { kind, mut span }: Expression) -> String { + pub(crate) fn format_expr(&self, Expression { kind, mut span }: Expression) -> String { match kind { ExpressionKind::Block(block) => { let mut visitor = self.fork(); @@ -51,33 +55,23 @@ impl FmtVisitor<'_> { ) } ExpressionKind::Call(call_expr) => { - let formatted_func = self.format_expr(*call_expr.func); - let formatted_args = call_expr - .arguments - .into_iter() - .map(|arg| self.format_expr(arg)) - .collect::>() - .join(", "); - format!("{}({})", formatted_func, formatted_args) + let span = call_expr.func.span.end()..span.end(); + let span = normalized_parenthesized_span(slice!(self, span.start, span.end), span); + + let callee = self.format_expr(*call_expr.func); + let args = format_parens(self.fork(), false, call_expr.arguments, span); + + format!("{callee}{args}") } ExpressionKind::MethodCall(method_call_expr) => { - let formatted_object = self.format_expr(method_call_expr.object).trim().to_string(); - let formatted_args = method_call_expr - .arguments - .iter() - .map(|arg| { - let arg_str = self.format_expr(arg.clone()).trim().to_string(); - if arg_str.contains('(') { - return arg_str - .replace(" ,", ",") - .replace("( ", "(") - .replace(" )", ")"); - } - arg_str - }) - .collect::>() - .join(", "); - format!("{}.{}({})", formatted_object, method_call_expr.method_name, formatted_args) + let span = method_call_expr.method_name.span().end()..span.end(); + let span = normalized_parenthesized_span(slice!(self, span.start, span.end), span); + + let object = self.format_expr(method_call_expr.object); + let method = method_call_expr.method_name.to_string(); + let args = format_parens(self.fork(), false, method_call_expr.arguments, span); + + format!("{object}.{method}{args}") } ExpressionKind::MemberAccess(member_access_expr) => { let lhs_str = self.format_expr(member_access_expr.lhs); @@ -89,6 +83,9 @@ impl FmtVisitor<'_> { let formatted_index = self.format_expr(index_expr.index); format!("{}[{}]", formatted_collection, formatted_index) } + ExpressionKind::Tuple(exprs) => { + format_parens(self.fork(), exprs.len() == 1, exprs, span) + } ExpressionKind::Literal(literal) => match literal { Literal::Integer(_) => slice!(self, span.start(), span.end()).to_string(), Literal::Array(ArrayLiteral::Repeated { repeated_element, length }) => { @@ -135,9 +132,9 @@ impl FmtVisitor<'_> { } else { let mut visitor = self.fork(); - let indent = visitor.block_indent.to_string_with_newline(); - visitor.block_indent.block_indent(self.config); - let nested_indent = visitor.block_indent.to_string_with_newline(); + let indent = visitor.indent.to_string_with_newline(); + visitor.indent.block_indent(self.config); + let nested_indent = visitor.indent.to_string_with_newline(); let sub_expr = visitor.format_expr(*sub_expr); @@ -195,7 +192,7 @@ impl FmtVisitor<'_> { self.push_str("\n"); if should_indent { - self.push_str(&self.block_indent.to_string()); + self.push_str(&self.indent.to_string()); } self.push_str("}"); } @@ -215,11 +212,10 @@ impl FmtVisitor<'_> { let block_str = if comment_str.is_empty() { "{}".to_string() } else { - self.block_indent.block_indent(self.config); - let open_indent = self.block_indent.to_string(); - self.block_indent.block_unindent(self.config); - let close_indent = - if should_indent { self.block_indent.to_string() } else { String::new() }; + self.indent.block_indent(self.config); + let open_indent = self.indent.to_string(); + self.indent.block_unindent(self.config); + let close_indent = if should_indent { self.indent.to_string() } else { String::new() }; let ret = format!("{{\n{open_indent}{comment_str}\n{close_indent}}}"); ret @@ -229,29 +225,118 @@ impl FmtVisitor<'_> { } } -fn recover_comment_removed(original: &str, new: String) -> String { - if changed_comment_content(original, &new) { - original.to_string() +fn format_parens( + mut visitor: FmtVisitor, + trailing_comma: bool, + exprs: Vec, + span: Span, +) -> String { + visitor.indent.block_indent(visitor.config); + + let nested_indent = visitor.indent; + let exprs: Vec<_> = utils::Exprs::new(&visitor, span, exprs).collect(); + let (exprs, force_one_line) = format_exprs(trailing_comma, exprs, nested_indent); + + visitor.indent.block_unindent(visitor.config); + + wrap_exprs(exprs, nested_indent, visitor.indent, force_one_line) +} + +fn format_exprs(trailing_comma: bool, exprs: Vec, indent: Indent) -> (String, bool) { + let mut result = String::new(); + + let mut force_one_line = true; + let indent_str = indent.to_string(); + + let tactic = Tactic::of(&exprs); + let mut exprs = exprs.into_iter().enumerate().peekable(); + + while let Some((index, expr)) = exprs.next() { + let is_first = index == 0; + let separate = exprs.peek().is_some() || trailing_comma; + + match tactic { + Tactic::Vertical if !is_first && !expr.expr.is_empty() && !result.is_empty() => { + result.push('\n'); + result.push_str(&indent_str); + } + Tactic::Horizontal if !is_first => { + result.push(' '); + } + _ => {} + } + + result.push_str(&expr.leading); + + if expr.different_line { + force_one_line = false; + result.push('\n'); + result.push_str(&indent_str); + } else if !expr.leading.is_empty() { + result.push(' '); + } + + result.push_str(&expr.expr); + + if tactic == Tactic::Horizontal { + result.push_str(&expr.trailing); + } + + if separate && expr.trailing.find_token(Token::Comma).is_none() { + result.push(','); + } + + if tactic == Tactic::Vertical { + if !expr.different_line { + result.push(' '); + } + result.push_str(&expr.trailing); + } + } + + (result, force_one_line) +} + +fn wrap_exprs( + exprs: String, + nested_indent: Indent, + indent: Indent, + force_one_line: bool, +) -> String { + if force_one_line && !exprs.contains('\n') { + format!("({exprs})") } else { - new + let nested_indent_str = "\n".to_string() + &nested_indent.to_string(); + let indent_str = "\n".to_string() + &indent.to_string(); + + format!("({nested_indent_str}{exprs}{indent_str})") } } -fn changed_comment_content(original: &str, new: &str) -> bool { - comments(original) != comments(new) +#[derive(PartialEq, Eq)] +enum Tactic { + Vertical, + Horizontal, } -fn comments(source: &str) -> Vec { - Lexer::new(source) - .skip_comments(false) - .flatten() - .filter_map(|spanned| { - if let Token::LineComment(content) | Token::BlockComment(content) = spanned.into_token() - { - Some(content) - } else { - None - } - }) - .collect() +impl Tactic { + fn of(exprs: &[Expr]) -> Self { + if exprs.iter().any(|item| { + has_single_line_comment(&item.leading) || has_single_line_comment(&item.trailing) + }) { + Tactic::Vertical + } else { + Tactic::Horizontal + } + } +} + +fn has_single_line_comment(slice: &str) -> bool { + slice.trim_start().starts_with("//") +} + +fn normalized_parenthesized_span(slice: &str, mut span: Range) -> Span { + let offset = slice.find_token(Token::LeftParen).expect("parenthesized expression"); + span.start += offset; + span.into() } diff --git a/tooling/nargo_fmt/tests/expected/call.nr b/tooling/nargo_fmt/tests/expected/call.nr index 105a69acedc..eeb2e386bc8 100644 --- a/tooling/nargo_fmt/tests/expected/call.nr +++ b/tooling/nargo_fmt/tests/expected/call.nr @@ -1,3 +1,25 @@ fn foo() { my_function(10, some_value, another_func(20, 30)); + + outer_function( + some_function(), // Original inner function call + another_function() // Original inner function call + ); + + outer_function( + some_function(), // Original inner function call + another_function() // Original inner function call + ); + + my_function( + // Comment + some_value, + /* Multiline + Comment */ + another_func(20, 30) + ); + + my_function(some_function(10, "arg1", another_function()), another_func(20, some_function(), 30)); + + outer_function(some_function(), another_function(some_function(), some_value)); } diff --git a/tooling/nargo_fmt/tests/expected/expr.nr b/tooling/nargo_fmt/tests/expected/expr.nr index 6d4fedba4b3..c4fd1633bdb 100644 --- a/tooling/nargo_fmt/tests/expected/expr.nr +++ b/tooling/nargo_fmt/tests/expected/expr.nr @@ -79,3 +79,14 @@ fn parenthesized() { fn parenthesized() { (i as u8) + (j as u8) + (k as u8) + x + y + z } + +fn parenthesized() { + value + (/*test*/x as Field/*test*/) +} + +fn parenthesized() { + value + ( + // line + x as Field + ) +} diff --git a/tooling/nargo_fmt/tests/expected/tuple.nr b/tooling/nargo_fmt/tests/expected/tuple.nr new file mode 100644 index 00000000000..71f67c9f14f --- /dev/null +++ b/tooling/nargo_fmt/tests/expected/tuple.nr @@ -0,0 +1,47 @@ +fn main() { + (1,); + ( + // hello + 1, + ); + (/*hello*/ 1,); + (1/*hello*/,); + (1,); + (/*test*/ 1,); + (/*a*/ 1/*b*/,); + (/*a*/ 1/*b*/, /*c*/ 2/*d*/, /*c*/ 2/*d*/); + (/*a*/ 1/*b*/, /*c*/ 2/*d*/, /*c*/ 2/*d*/, /*e*/ 3/*f*/); + + (1/*1*/, 2/* 2*/); + + (1/*test*/,); + + ( + // + 1, + ); + + ( + // 1 + 1, + // 2, + 2 + ); + + (/*1*/ 1, /*2*/ 2); + +// FIXME: + ( + ( + ( + //2 + 1, + ), + ), + ); + ( + /*a*/ + 1/*b*/, +/*c*/ 2/*d*/, /*c*/ 2/*d*/, /*e*/ 3/*f*/ + ); +} diff --git a/tooling/nargo_fmt/tests/input/call.nr b/tooling/nargo_fmt/tests/input/call.nr index 93d2597a05c..f76157e83ca 100644 --- a/tooling/nargo_fmt/tests/input/call.nr +++ b/tooling/nargo_fmt/tests/input/call.nr @@ -1,3 +1,35 @@ fn foo() { my_function( 10,some_value,another_func( 20 , 30) ); + + outer_function(some_function(), // Original inner function call + another_function(), // Original inner function call + ); + + outer_function( + + + some_function(), // Original inner function call + another_function(), // Original inner function call + ); + + my_function( + // Comment + some_value, + /* Multiline + Comment */ + another_func( 20, 30 ) + ); + + my_function( + some_function( 10, "arg1", another_function() ), + another_func (20, some_function() , 30 ) + ); + + outer_function( + some_function(), + + + another_function( + some_function(), some_value) + ); } diff --git a/tooling/nargo_fmt/tests/input/expr.nr b/tooling/nargo_fmt/tests/input/expr.nr index 25602c640d3..0baef877169 100644 --- a/tooling/nargo_fmt/tests/input/expr.nr +++ b/tooling/nargo_fmt/tests/input/expr.nr @@ -88,3 +88,14 @@ fn parenthesized() { fn parenthesized() { ( i as u8 ) + ( j as u8 ) + ( k as u8 ) + x + y + z } + +fn parenthesized() { + value + ( /*test*/x as Field /*test*/ ) +} + +fn parenthesized() { + value + ( +// line + x as Field + ) +} diff --git a/tooling/nargo_fmt/tests/input/tuple.nr b/tooling/nargo_fmt/tests/input/tuple.nr new file mode 100644 index 00000000000..da3b6ed597b --- /dev/null +++ b/tooling/nargo_fmt/tests/input/tuple.nr @@ -0,0 +1,48 @@ +fn main() { +(1,); +( +// hello +1,); +(/*hello*/1,); +(1,/*hello*/); + ( 1, ); +( /*test*/1, ); +( /*a*/1/*b*/, ); +( /*a*/1/*b*/, /*c*/2/*d*/, /*c*/2/*d*/ ); +( /*a*/1/*b*/, /*c*/2/*d*/, /*c*/2/*d*/, /*e*/3/*f*/ ); + +( 1 /*1*/ , 2 /* 2*/ ); + + + + +( 1, /*test*/ ); + + ( +// +1, +); + +( +// 1 +1, +// 2, +2, +); + +(/*1*/1, /*2*/2); + +// FIXME: +((( +//2 +1,),),); +( + /*a*/ + 1 + /*b*/, +/*c*/ +2/*d*/, +/*c*/2/*d*/, +/*e*/3/*f*/ +); +}