Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: update code formatting to prevent parameter line wrapping #3588

Merged
merged 5 commits into from Nov 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions tooling/nargo_fmt/src/rewrite/array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use noirc_frontend::{hir::resolution::errors::Span, token::Token, Expression};

use crate::{
utils::{Expr, FindToken},
visitor::FmtVisitor,
visitor::{expr::NewlineMode, FmtVisitor},
};

pub(crate) fn rewrite(mut visitor: FmtVisitor, array: Vec<Expression>, array_span: Span) -> String {
Expand Down Expand Up @@ -80,6 +80,6 @@ pub(crate) fn rewrite(mut visitor: FmtVisitor, array: Vec<Expression>, array_spa
items_str.trim().into(),
nested_indent,
visitor.shape(),
true,
NewlineMode::IfContainsNewLineAndWidth,
)
}
17 changes: 13 additions & 4 deletions tooling/nargo_fmt/src/rewrite/expr.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use noirc_frontend::{token::Token, ArrayLiteral, Expression, ExpressionKind, Literal, UnaryOp};

use crate::visitor::{
expr::{format_brackets, format_parens},
expr::{format_brackets, format_parens, NewlineMode},
ExpressionType, FmtVisitor, Indent, Shape,
};

Expand Down Expand Up @@ -60,6 +60,7 @@ pub(crate) fn rewrite(
call_expr.arguments,
args_span,
true,
NewlineMode::IfContainsNewLineAndWidth,
);

format!("{callee}{args}")
Expand All @@ -80,6 +81,7 @@ pub(crate) fn rewrite(
method_call_expr.arguments,
args_span,
true,
NewlineMode::IfContainsNewLineAndWidth,
);

format!("{object}.{method}{args}")
Expand All @@ -97,9 +99,16 @@ pub(crate) fn rewrite(

format!("{collection}{index}")
}
ExpressionKind::Tuple(exprs) => {
format_parens(None, visitor.fork(), shape, exprs.len() == 1, exprs, span, false)
}
ExpressionKind::Tuple(exprs) => format_parens(
None,
visitor.fork(),
shape,
exprs.len() == 1,
exprs,
span,
true,
NewlineMode::Normal,
),
ExpressionKind::Literal(literal) => match literal {
Literal::Integer(_) | Literal::Bool(_) | Literal::Str(_) | Literal::FmtStr(_) => {
visitor.slice(span).to_string()
Expand Down
8 changes: 4 additions & 4 deletions tooling/nargo_fmt/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@
let starts_with_single_line_comment = leading_trimmed.starts_with("//");

if ends_with_block_comment {
let comment_end = leading_trimmed.rfind(|c| c == '/').unwrap();

Check warning on line 100 in tooling/nargo_fmt/src/utils.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (rfind)

if leading[comment_end..].contains('\n') {
different_line = true;
Expand All @@ -120,19 +120,19 @@

pub(crate) trait FindToken {
fn find_token(&self, token: Token) -> Option<Span>;
fn find_token_with(&self, f: impl Fn(&Token) -> bool) -> Option<u32>;
fn find_token_with(&self, f: impl Fn(&Token) -> bool) -> Option<Span>;
}

impl FindToken for str {
fn find_token(&self, token: Token) -> Option<Span> {
Lexer::new(self).flatten().find_map(|it| (it.token() == &token).then(|| it.to_span()))
}

fn find_token_with(&self, f: impl Fn(&Token) -> bool) -> Option<u32> {
fn find_token_with(&self, f: impl Fn(&Token) -> bool) -> Option<Span> {
Lexer::new(self)
.skip_comments(false)
.flatten()
.find_map(|spanned| f(spanned.token()).then(|| spanned.to_span().end()))
.find_map(|spanned| f(spanned.token()).then(|| spanned.to_span()))
}
}

Expand All @@ -142,7 +142,7 @@
.find_token_with(|token| {
matches!(token, Token::LineComment(_, _) | Token::BlockComment(_, _))
})
.map(|index| index as usize)
.map(|index| index.end() as usize)
.unwrap_or(slice.len())
}

Expand Down Expand Up @@ -198,7 +198,7 @@
}

pub(crate) fn count_newlines(slice: &str) -> usize {
bytecount::count(slice.as_bytes(), b'\n')

Check warning on line 201 in tooling/nargo_fmt/src/utils.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (bytecount)
}

pub(crate) trait Item {
Expand Down Expand Up @@ -277,7 +277,7 @@
}

pub(crate) fn last_line_width(s: &str) -> usize {
s.rsplit('\n').next().unwrap_or("").chars().count()

Check warning on line 280 in tooling/nargo_fmt/src/utils.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (rsplit)
}

pub(crate) fn is_single_line(s: &str) -> bool {
Expand Down
52 changes: 41 additions & 11 deletions tooling/nargo_fmt/src/visitor/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
false,
exprs,
nested_indent,
true,
);

visitor.indent.block_unindent(visitor.config);
Expand Down Expand Up @@ -133,7 +134,7 @@
if let Some(first_stmt) = block.first() {
let slice = self.slice(self.last_position..first_stmt.span.start());
let len =
slice.chars().take_while(|ch| ch.is_whitespace()).collect::<String>().rfind('\n');

Check warning on line 137 in tooling/nargo_fmt/src/visitor/expr.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (rfind)
self.last_position += len.unwrap_or(0) as u32;
}
}
Expand Down Expand Up @@ -186,6 +187,7 @@
}
}

// TODO: fixme
#[allow(clippy::too_many_arguments)]
pub(crate) fn format_seq<T: Item>(
shape: Shape,
Expand All @@ -196,17 +198,18 @@
exprs: Vec<T>,
span: Span,
tactic: Tactic,
soft_newline: bool,
mode: NewlineMode,
reduce: bool,
) -> String {
let mut nested_indent = shape;
let shape = shape;

nested_indent.indent.block_indent(visitor.config);

let exprs: Vec<_> = utils::Exprs::new(&visitor, nested_indent, span, exprs).collect();
let exprs = format_exprs(visitor.config, tactic, trailing_comma, exprs, nested_indent);
let exprs = format_exprs(visitor.config, tactic, trailing_comma, exprs, nested_indent, reduce);

wrap_exprs(prefix, suffix, exprs, nested_indent, shape, soft_newline)
wrap_exprs(prefix, suffix, exprs, nested_indent, shape, mode)
}

pub(crate) fn format_brackets(
Expand All @@ -225,21 +228,25 @@
exprs,
span,
Tactic::LimitedHorizontalVertical(array_width),
NewlineMode::Normal,
false,
)
}

// TODO: fixme
#[allow(clippy::too_many_arguments)]
pub(crate) fn format_parens(
max_width: Option<usize>,
visitor: FmtVisitor,
shape: Shape,
trailing_comma: bool,
exprs: Vec<Expression>,
span: Span,
soft_newline: bool,
reduce: bool,
mode: NewlineMode,
) -> String {
let tactic = max_width.map(Tactic::LimitedHorizontalVertical).unwrap_or(Tactic::Horizontal);
format_seq(shape, "(", ")", visitor, trailing_comma, exprs, span, tactic, soft_newline)
format_seq(shape, "(", ")", visitor, trailing_comma, exprs, span, tactic, mode, reduce)
}

fn format_exprs(
Expand All @@ -248,11 +255,12 @@
trailing_comma: bool,
exprs: Vec<Expr>,
shape: Shape,
reduce: bool,
) -> String {
let mut result = String::new();
let indent_str = shape.indent.to_string();

let tactic = tactic.definitive(&exprs, config.short_array_element_width_threshold);
let tactic = tactic.definitive(&exprs, config.short_array_element_width_threshold, reduce);
let mut exprs = exprs.into_iter().enumerate().peekable();
let mut line_len = 0;
let mut prev_expr_trailing_comment = false;
Expand Down Expand Up @@ -325,16 +333,32 @@
result
}

#[derive(PartialEq, Eq)]
pub(crate) enum NewlineMode {
IfContainsNewLine,
IfContainsNewLineAndWidth,
Normal,
}

pub(crate) fn wrap_exprs(
prefix: &str,
suffix: &str,
exprs: String,
nested_shape: Shape,
shape: Shape,
soft_newline: bool,
newline_mode: NewlineMode,
) -> String {
let mut force_one_line = first_line_width(&exprs) <= shape.width;
if soft_newline && force_one_line {
let mut force_one_line = if newline_mode == NewlineMode::IfContainsNewLine {
true
} else {
first_line_width(&exprs) <= shape.width
};

if matches!(
newline_mode,
NewlineMode::IfContainsNewLine | NewlineMode::IfContainsNewLineAndWidth
) && force_one_line
{
force_one_line = !exprs.contains('\n');
}

Expand Down Expand Up @@ -373,7 +397,8 @@
fn definitive(
self,
exprs: &[Expr],
short_array_element_width_threshold: usize,
short_width_threshold: usize,
reduce: bool,
) -> DefinitiveTactic {
let tactic = || {
let has_single_line_comment = exprs.iter().any(|item| {
Expand Down Expand Up @@ -407,7 +432,12 @@
}
};

tactic().reduce(exprs, short_array_element_width_threshold)
let definitive_tactic = tactic();
if reduce {
definitive_tactic.reduce(exprs, short_width_threshold)
} else {
definitive_tactic
}
}
}

Expand Down
18 changes: 13 additions & 5 deletions tooling/nargo_fmt/src/visitor/item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use noirc_frontend::{

use crate::{
utils::{last_line_contains_single_line_comment, last_line_used_width, FindToken},
visitor::expr::format_seq,
visitor::expr::{format_seq, NewlineMode},
};

use super::{
Expand Down Expand Up @@ -54,6 +54,7 @@ impl super::FmtVisitor<'_> {
generics,
span,
HorizontalVertical,
NewlineMode::IfContainsNewLine,
false,
);

Expand All @@ -63,12 +64,18 @@ impl super::FmtVisitor<'_> {
let parameters = if parameters.is_empty() {
self.slice(params_span).into()
} else {
let fn_start = result.find_token(Token::Keyword(Keyword::Fn)).unwrap().start();
let slice = self.slice(fn_start..result.len() as u32);
let fn_start = result
.find_token_with(|token| {
matches!(token, Token::Keyword(Keyword::Fn | Keyword::Unconstrained))
})
.unwrap()
.start();

let slice = self.slice(fn_start..result.len() as u32);
let indent = self.indent;
let used_width = last_line_used_width(slice, indent.width());
let one_line_budget = self.budget(used_width + return_type.len());
let overhead = if return_type.is_empty() { 2 } else { 3 }; // 2 = `()`, 3 = `() `
let one_line_budget = self.budget(used_width + return_type.len() + overhead);
let shape = Shape { width: one_line_budget, indent };

let tactic = LimitedHorizontalVertical(one_line_budget);
Expand All @@ -82,7 +89,8 @@ impl super::FmtVisitor<'_> {
parameters,
params_span.into(),
tactic,
true,
NewlineMode::IfContainsNewLine,
false,
)
};

Expand Down
11 changes: 9 additions & 2 deletions tooling/nargo_fmt/src/visitor/stmt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use noirc_frontend::{

use crate::{rewrite, visitor::expr::wrap_exprs};

use super::ExpressionType;
use super::{expr::NewlineMode, ExpressionType};

impl super::FmtVisitor<'_> {
pub(crate) fn visit_stmts(&mut self, stmts: Vec<Statement>) {
Expand Down Expand Up @@ -68,7 +68,14 @@ impl super::FmtVisitor<'_> {
}
};

let args = wrap_exprs("(", ")", args, nested_shape, shape, true);
let args = wrap_exprs(
"(",
")",
args,
nested_shape,
shape,
NewlineMode::IfContainsNewLineAndWidth,
);
let constrain = format!("{callee}{args};");

self.push_rewrite(constrain, span);
Expand Down
8 changes: 8 additions & 0 deletions tooling/nargo_fmt/tests/expected/fn.nr
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,11 @@ fn apply_binary_field_op<N>(
) -> bool {}

fn main() -> distinct pub [Field;2] {}

fn main(
message: [u8; 10],
message_field: Field,
pub_key_x: Field,
pub_key_y: Field,
signature: [u8; 64]
) {}
4 changes: 4 additions & 0 deletions tooling/nargo_fmt/tests/input/fn.nr
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,7 @@ fn main(tape: [Field; TAPE_LEN], initial_registers: [Field; REGISTER_COUNT], ini
fn apply_binary_field_op<N>(lhs: RegisterIndex, rhs: RegisterIndex, result: RegisterIndex, op: u8, registers: &mut Registers<N>) -> bool {}

fn main() -> distinct pub [Field;2] {}

fn main(
message: [u8; 10], message_field: Field, pub_key_x: Field, pub_key_y: Field, signature: [u8; 64]
) {}
Loading