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: type formatting #3618

Merged
merged 7 commits into from Nov 29, 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: 4 additions & 0 deletions compiler/noirc_errors/src/position.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ impl Span {
Span::inclusive(start, start)
}

pub fn empty(position: u32) -> Span {
Span::from(position..position)
}

#[must_use]
pub fn merge(self, other: Span) -> Span {
Span(self.0.merge(other.0))
Expand Down
3 changes: 3 additions & 0 deletions compiler/noirc_frontend/src/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ pub enum UnresolvedTypeData {
FormatString(UnresolvedTypeExpression, Box<UnresolvedType>),
Unit,

Parenthesized(Box<UnresolvedType>),

/// A Named UnresolvedType can be a struct type or a type variable
Named(Path, Vec<UnresolvedType>),

Expand Down Expand Up @@ -152,6 +154,7 @@ impl std::fmt::Display for UnresolvedTypeData {
Unit => write!(f, "()"),
Error => write!(f, "error"),
Unspecified => write!(f, "unspecified"),
Parenthesized(typ) => write!(f, "({typ})"),
}
}
}
Expand Down
32 changes: 18 additions & 14 deletions compiler/noirc_frontend/src/ast/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,11 @@ impl From<Ident> for Expression {
fn from(i: Ident) -> Expression {
Expression {
span: i.0.span(),
kind: ExpressionKind::Variable(Path { segments: vec![i], kind: PathKind::Plain }),
kind: ExpressionKind::Variable(Path {
span: i.span(),
segments: vec![i],
kind: PathKind::Plain,
}),
}
}
}
Expand Down Expand Up @@ -311,6 +315,7 @@ impl UseTree {
pub struct Path {
pub segments: Vec<Ident>,
pub kind: PathKind,
pub span: Span,
}

impl Path {
Expand All @@ -330,18 +335,11 @@ impl Path {
}

pub fn from_ident(name: Ident) -> Path {
Path { segments: vec![name], kind: PathKind::Plain }
Path { span: name.span(), segments: vec![name], kind: PathKind::Plain }
jfecher marked this conversation as resolved.
Show resolved Hide resolved
}

pub fn span(&self) -> Span {
let mut segments = self.segments.iter();
let first_segment = segments.next().expect("ice : cannot have an empty path");
let mut span = first_segment.0.span();

for segment in segments {
span = span.merge(segment.0.span());
}
span
self.span
}

pub fn last_segment(&self) -> Ident {
Expand Down Expand Up @@ -545,8 +543,11 @@ impl ForRange {

// array.len()
let segments = vec![array_ident];
let array_ident =
ExpressionKind::Variable(Path { segments, kind: PathKind::Plain });
let array_ident = ExpressionKind::Variable(Path {
segments,
kind: PathKind::Plain,
span: array_span,
});

let end_range = ExpressionKind::MethodCall(Box::new(MethodCallExpression {
object: Expression::new(array_ident.clone(), array_span),
Expand All @@ -561,8 +562,11 @@ impl ForRange {

// array[i]
let segments = vec![Ident::new(index_name, array_span)];
let index_ident =
ExpressionKind::Variable(Path { segments, kind: PathKind::Plain });
let index_ident = ExpressionKind::Variable(Path {
segments,
kind: PathKind::Plain,
span: array_span,
});

let loop_element = ExpressionKind::Index(Box::new(IndexExpression {
collection: Expression::new(array_ident, array_span),
Expand Down
8 changes: 6 additions & 2 deletions compiler/noirc_frontend/src/hir/resolution/import.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use iter_extended::partition_results;
use noirc_errors::CustomDiagnostic;
use noirc_errors::{CustomDiagnostic, Span};

use crate::graph::CrateId;
use std::collections::BTreeMap;
Expand Down Expand Up @@ -202,7 +202,11 @@ fn resolve_external_dep(
// Create an import directive for the dependency crate
let path_without_crate_name = &path[1..]; // XXX: This will panic if the path is of the form `use dep::std` Ideal algorithm will not distinguish between crate and module

let path = Path { segments: path_without_crate_name.to_vec(), kind: PathKind::Plain };
let path = Path {
segments: path_without_crate_name.to_vec(),
kind: PathKind::Plain,
span: Span::default(),
};
let dep_directive =
ImportDirective { module_id: dep_module.local_id, path, alias: directive.alias.clone() };

Expand Down
2 changes: 2 additions & 0 deletions compiler/noirc_frontend/src/hir/resolution/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@

/// True if the current module is a contract.
/// This is usually determined by self.path_resolver.module_id(), but it can
/// be overriden for impls. Impls are an odd case since the methods within resolve

Check warning on line 98 in compiler/noirc_frontend/src/hir/resolution/resolver.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (overriden)
/// as if they're in the parent module, but should be placed in a child module.
/// Since they should be within a child module, in_contract is manually set to false
/// for these so we can still resolve them in the parent module without them being in a contract.
Expand Down Expand Up @@ -441,6 +441,7 @@
MutableReference(element) => {
Type::MutableReference(Box::new(self.resolve_type_inner(*element, new_variables)))
}
Parenthesized(typ) => self.resolve_type_inner(*typ, new_variables),
}
}

Expand Down Expand Up @@ -1787,6 +1788,7 @@
self.verify_type_valid_for_program_input(element);
}
}
UnresolvedTypeData::Parenthesized(typ) => self.verify_type_valid_for_program_input(typ),
}
}

Expand Down
27 changes: 20 additions & 7 deletions compiler/noirc_frontend/src/parser/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -726,21 +726,21 @@ fn token_kind(token_kind: TokenKind) -> impl NoirParser<Token> {

fn path() -> impl NoirParser<Path> {
let idents = || ident().separated_by(just(Token::DoubleColon)).at_least(1);
let make_path = |kind| move |segments| Path { segments, kind };
let make_path = |kind| move |segments, span| Path { segments, kind, span };

let prefix = |key| keyword(key).ignore_then(just(Token::DoubleColon));
let path_kind = |key, kind| prefix(key).ignore_then(idents()).map(make_path(kind));
let path_kind = |key, kind| prefix(key).ignore_then(idents()).map_with_span(make_path(kind));

choice((
path_kind(Keyword::Crate, PathKind::Crate),
path_kind(Keyword::Dep, PathKind::Dep),
idents().map(make_path(PathKind::Plain)),
idents().map_with_span(make_path(PathKind::Plain)),
))
}

fn empty_path() -> impl NoirParser<Path> {
let make_path = |kind| move |_| Path { segments: Vec::new(), kind };
let path_kind = |key, kind| keyword(key).map(make_path(kind));
let make_path = |kind| move |_, span| Path { segments: Vec::new(), kind, span };
let path_kind = |key, kind| keyword(key).map_with_span(make_path(kind));

choice((path_kind(Keyword::Crate, PathKind::Crate), path_kind(Keyword::Dep, PathKind::Dep)))
}
Expand Down Expand Up @@ -1015,13 +1015,24 @@ fn parse_type_inner(
named_type(recursive_type_parser.clone()),
named_trait(recursive_type_parser.clone()),
array_type(recursive_type_parser.clone()),
recursive_type_parser.clone().delimited_by(just(Token::LeftParen), just(Token::RightParen)),
parenthesized_type(recursive_type_parser.clone()),
tuple_type(recursive_type_parser.clone()),
function_type(recursive_type_parser.clone()),
mutable_reference_type(recursive_type_parser),
))
}

fn parenthesized_type(
recursive_type_parser: impl NoirParser<UnresolvedType>,
) -> impl NoirParser<UnresolvedType> {
recursive_type_parser
.delimited_by(just(Token::LeftParen), just(Token::RightParen))
.map_with_span(|typ, span| UnresolvedType {
typ: UnresolvedTypeData::Parenthesized(Box::new(typ)),
span: span.into(),
})
}

fn optional_visibility() -> impl NoirParser<Visibility> {
keyword(Keyword::Pub).or_not().map(|opt| match opt {
Some(_) => Visibility::Public,
Expand Down Expand Up @@ -1177,7 +1188,9 @@ where
.ignore_then(type_parser.clone())
.then_ignore(just(Token::RightBracket))
.or_not()
.map_with_span(|t, span| t.unwrap_or_else(|| UnresolvedTypeData::Unit.with_span(span)));
.map_with_span(|t, span| {
t.unwrap_or_else(|| UnresolvedTypeData::Unit.with_span(Span::empty(span.end())))
});

keyword(Keyword::Fn)
.ignore_then(env)
Expand Down
5 changes: 3 additions & 2 deletions tooling/nargo_fmt/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ fn generate_formatter_tests(test_file: &mut File, test_data_dir: &Path) {
.collect::<Vec<_>>()
.join("\n");

let output_source_path = outputs_dir.join(file_name);
let output_source = std::fs::read_to_string(output_source_path).unwrap();
let output_source_path = outputs_dir.join(file_name).display().to_string();
let output_source = std::fs::read_to_string(output_source_path.clone()).unwrap();

write!(
test_file,
Expand All @@ -63,6 +63,7 @@ fn format_{test_name}() {{
let config = nargo_fmt::Config::of("{config}").unwrap();
let fmt_text = nargo_fmt::format(&input, parsed_module, &config);

std::fs::write("{output_source_path}", fmt_text.clone());

similar_asserts::assert_eq!(fmt_text, expected_output);
}}
Expand Down
2 changes: 2 additions & 0 deletions tooling/nargo_fmt/src/rewrite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ mod array;
mod expr;
mod infix;
mod parenthesized;
mod typ;

pub(crate) use array::rewrite as array;
pub(crate) use expr::{rewrite as expr, rewrite_sub_expr as sub_expr};
pub(crate) use infix::rewrite as infix;
pub(crate) use parenthesized::rewrite as parenthesized;
pub(crate) use typ::rewrite as typ;
70 changes: 70 additions & 0 deletions tooling/nargo_fmt/src/rewrite/typ.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
use noirc_frontend::{UnresolvedType, UnresolvedTypeData};

use crate::{
utils::span_is_empty,
visitor::{FmtVisitor, Shape},
};

pub(crate) fn rewrite(visitor: &FmtVisitor, _shape: Shape, typ: UnresolvedType) -> String {
match typ.typ {
UnresolvedTypeData::Array(length, element) => {
let typ = rewrite(visitor, _shape, *element);
if let Some(length) = length {
let length = visitor.slice(length.span());
format!("[{typ}; {length}]")
} else {
format!("[{typ}]")
}
}
UnresolvedTypeData::Parenthesized(typ) => {
let typ = rewrite(visitor, _shape, *typ);
format!("({typ})")
}
UnresolvedTypeData::MutableReference(typ) => {
let typ = rewrite(visitor, _shape, *typ);
format!("&mut {typ}")
}
UnresolvedTypeData::Tuple(mut types) => {
if types.len() == 1 {
let typ = types.pop().unwrap();
let typ = rewrite(visitor, _shape, typ);

format!("({typ},)")
} else {
let types: Vec<_> =
types.into_iter().map(|typ| rewrite(visitor, _shape, typ)).collect();
let types = types.join(", ");
format!("({types})")
}
}
UnresolvedTypeData::Function(args, return_type, env) => {
let env = if span_is_empty(env.span.unwrap()) {
"".into()
} else {
let ty = rewrite(visitor, _shape, *env);
format!("[{ty}]")
};

let args = args
.into_iter()
.map(|arg| rewrite(visitor, _shape, arg))
.collect::<Vec<_>>()
.join(", ");

let return_type = rewrite(visitor, _shape, *return_type);

format!("fn{env}({args}) -> {return_type}")
}
UnresolvedTypeData::Unspecified => todo!(),
UnresolvedTypeData::FieldElement
| UnresolvedTypeData::Integer(_, _)
| UnresolvedTypeData::Bool
| UnresolvedTypeData::Named(_, _)
| UnresolvedTypeData::Unit
| UnresolvedTypeData::Expression(_)
| UnresolvedTypeData::String(_)
| UnresolvedTypeData::FormatString(_, _)
| UnresolvedTypeData::TraitAsType(_, _) => visitor.slice(typ.span.unwrap()).into(),
UnresolvedTypeData::Error => unreachable!(),
}
}
8 changes: 6 additions & 2 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 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 @@ -250,13 +250,13 @@
self.span
}

fn format(self, visitor: &FmtVisitor, _shape: Shape) -> String {
fn format(self, visitor: &FmtVisitor, shape: Shape) -> String {
let visibility = match self.visibility {
Visibility::Public => "pub ",
Visibility::Private => "",
};
let pattern = visitor.slice(self.pattern.span());
let ty = visitor.slice(self.typ.span.unwrap());
let ty = rewrite::typ(visitor, shape, self.typ);

format!("{pattern}: {visibility}{ty}")
}
Expand All @@ -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 All @@ -295,3 +295,7 @@
offset + s.chars().count()
}
}

pub(crate) fn span_is_empty(span: Span) -> bool {
span.start() == span.end()
}
4 changes: 3 additions & 1 deletion tooling/nargo_fmt/src/visitor/item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use noirc_frontend::{
};

use crate::{
rewrite,
utils::{last_line_contains_single_line_comment, last_line_used_width, FindToken},
visitor::expr::{format_seq, NewlineMode},
};
Expand Down Expand Up @@ -122,7 +123,8 @@ impl super::FmtVisitor<'_> {
result.push_str("pub ");
}

result.push_str(self.slice(span));
let typ = rewrite::typ(self, self.shape(), func.return_type());
result.push_str(&typ);

let slice = self.slice(span.end()..func_span.start());
if !slice.trim().is_empty() {
Expand Down
18 changes: 17 additions & 1 deletion tooling/nargo_fmt/tests/expected/fn.nr
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,21 @@ fn apply_binary_field_op<N>(
registers: &mut Registers<N>
) -> bool {}

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

fn ret_normal_lambda1() -> ((fn() -> Field)) {}

fn ret_normal_lambda1() -> fn() -> Field {}

fn ret_closure1() -> fn[(Field,)]() -> Field {}

fn ret_closure2() -> fn[(Field, Field)]() -> Field {}

fn ret_closure3() -> fn[(u32, u64)]() -> u64 {}

fn make_counter() -> fn[(&mut Field,)]() -> Field {}

fn get_some<Env>(generator: fn[Env]() -> Field) -> [Field; 5] {}

fn main(
message: [u8; 10],
Expand All @@ -45,3 +59,5 @@ fn main(
pub_key_y: Field,
signature: [u8; 64]
) {}

pub fn from_baz(x: [Field; crate::foo::MAGIC_NUMBER]) {}
16 changes: 16 additions & 0 deletions tooling/nargo_fmt/tests/input/fn.nr
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,22 @@ fn apply_binary_field_op<N>(lhs: RegisterIndex, rhs: RegisterIndex, result: Regi

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

fn ret_normal_lambda1() -> ((fn() -> Field)) {}

fn ret_normal_lambda1() -> fn() -> Field {}

fn ret_closure1() -> fn[(Field,)]() -> Field {}

fn ret_closure2() -> fn[(Field,Field)]() -> Field {}

fn ret_closure3() -> fn[(u32,u64)]() -> u64 {}

fn make_counter() -> fn[(&mut Field,)]() -> Field {}

fn get_some<Env>(generator: fn[Env]() -> Field) -> [Field;5] {}

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

pub fn from_baz(x: [Field; crate::foo::MAGIC_NUMBER]) {}
Loading