Skip to content

Commit

Permalink
fix(fmt): Variable{Declaration,Definition} visit and fmt implementa…
Browse files Browse the repository at this point in the history 
…tions (foundry-rs#4785)
  • Loading branch information
@DaniPopes
DaniPopes authored Apr 20, 2023
1 parent 1447dda commit e99cf83
Show file tree
Hide file tree
Showing 7 changed files with 60 additions and 87 deletions.
9 changes: 5 additions & 4 deletions cli/src/cmd/forge/fmt.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,10 +154,11 @@ impl Cmd for FmtArgs {

solang_parser::parse(&output, 0).map_err(|diags| {
eyre::eyre!(
"Failed to construct valid Solidity code for {}. Leaving source unchanged.\nDebug info: {:?}",
input,
diags
)
"Failed to construct valid Solidity code for {}. Leaving source unchanged.\n\
Debug info: {:?}",
input,
diags,
)
})?;

if self.check || matches!(input, Input::Stdin(_)) {
Expand Down
7 changes: 1 addition & 6 deletions cli/src/cmd/forge/geiger/visitor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -260,17 +260,12 @@ impl Visitor for CheatcodeVisitor {
_: Loc,
declaration: &mut VariableDeclaration,
expr: &mut Option<Expression>,
_semicolon: bool,
) -> Result<(), Self::Error> {
declaration.visit(self)?;
expr.visit(self)
}

fn visit_var_declaration(
&mut self,
var: &mut VariableDeclaration,
_is_assignment: bool,
) -> Result<(), Self::Error> {
fn visit_var_declaration(&mut self, var: &mut VariableDeclaration) -> Result<(), Self::Error> {
var.ty.visit(self)
}

Expand Down
105 changes: 41 additions & 64 deletions fmt/src/formatter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,11 +157,18 @@ impl<'a, W: Write> Formatter<'a, W> {
}
}

/// Casts the current `W` writer as a `String` reference. Should only be used for debugging.
/// Casts the current writer `w` as a `String` reference. Should only be used for debugging.
#[allow(dead_code)]
unsafe fn buf_contents(&self) -> &String {
*(&self.buf.w as *const W as *const &mut String)
}

/// Casts the current `W` writer or the current temp buffer as a `String` reference.
/// Should only be used for debugging.
#[allow(dead_code)]
unsafe fn temp_buf_contents(&self) -> &String {
match &self.temp_bufs[..] {
[] => unsafe { *(&self.buf.w as *const W as *const &mut String) },
[] => self.buf_contents(),
[.., buf] => &buf.w,
}
}
Expand Down Expand Up @@ -987,7 +994,7 @@ impl<'a, W: Write> Formatter<'a, W> {
let (unwritten_whitespace_loc, unwritten_whitespace) =
unwritten_whitespace(last_byte_written, loc.start());
let ignore_whitespace = if self.inline_config.is_disabled(unwritten_whitespace_loc) {
trace!("Unwritten whitespace: {:?}", unwritten_whitespace);
trace!("Unwritten whitespace: {unwritten_whitespace:?}");
self.write_raw(unwritten_whitespace)?;
true
} else {
Expand Down Expand Up @@ -1022,7 +1029,7 @@ impl<'a, W: Write> Formatter<'a, W> {
trace!("Visiting {}", {
let n = std::any::type_name::<V>();
n.strip_prefix("solang_parser::pt::").unwrap_or(n)
},);
});
item.visit(self)?;
}
}
Expand Down Expand Up @@ -1053,6 +1060,7 @@ impl<'a, W: Write> Formatter<'a, W> {
.map(|w| w.strip_suffix('\r').unwrap_or(w))
.unwrap_or(unwritten_whitespace);
}
trace!("Unwritten whitespace: {unwritten_whitespace:?}");
self.write_raw(unwritten_whitespace)?;
}

Expand Down Expand Up @@ -2310,26 +2318,15 @@ impl<'a, W: Write> Visitor for Formatter<'a, W> {
}

#[instrument(name = "var_declaration", skip_all)]
fn visit_var_declaration(
&mut self,
var: &mut VariableDeclaration,
is_assignment: bool,
) -> Result<()> {
fn visit_var_declaration(&mut self, var: &mut VariableDeclaration) -> Result<()> {
return_source_if_disabled!(self, var.loc);
self.grouped(|fmt| {
var.ty.visit(fmt)?;
if let Some(storage) = &var.storage {
write_chunk!(fmt, storage.loc().end(), "{}", storage)?;
write_chunk!(fmt, storage.loc().end(), "{storage}")?;
}
let var_name = var.name.safe_unwrap();
write_chunk!(
fmt,
var_name.loc.end(),
"{}{}",
var_name.name,
if is_assignment { " =" } else { "" }
)?;
Ok(())
write_chunk!(fmt, var_name.loc.end(), "{var_name}")
})?;
Ok(())
}
Expand Down Expand Up @@ -2840,28 +2837,20 @@ impl<'a, W: Write> Visitor for Formatter<'a, W> {
loc: Loc,
declaration: &mut VariableDeclaration,
expr: &mut Option<Expression>,
semicolon: bool,
) -> Result<()> {
if semicolon {
return_source_if_disabled!(self, loc, ';');
} else {
return_source_if_disabled!(self, loc);
}
return_source_if_disabled!(self, loc, ';');

let declaration = self.chunked(declaration.loc.start(), None, |fmt| {
fmt.visit_var_declaration(declaration, expr.is_some())
})?;
let declaration = self
.chunked(declaration.loc.start(), None, |fmt| fmt.visit_var_declaration(declaration))?;
let multiline = declaration.content.contains('\n');
self.write_chunk(&declaration)?;

expr.as_mut()
.map(|expr| self.indented_if(multiline, 1, |fmt| fmt.visit_assignment(expr)))
.transpose()?;

if semicolon {
self.write_semicolon()?;
if let Some(expr) = expr {
write!(self.buf(), " =")?;
self.indented_if(multiline, 1, |fmt| fmt.visit_assignment(expr))?;
}
Ok(())

self.write_semicolon()
}

#[instrument(name = "for", skip_all)]
Expand All @@ -2881,43 +2870,31 @@ impl<'a, W: Write> Visitor for Formatter<'a, W> {
SurroundingChunk::new(")", None, next_byte_end),
|fmt, _| {
let mut write_for_loop_header = |fmt: &mut Self, multiline: bool| -> Result<()> {
init.as_mut()
.map(|stmt| {
match **stmt {
Statement::VariableDefinition(loc, ref mut decl, ref mut expr) => {
fmt.visit_var_definition_stmt(loc, decl, expr, false)
}
Statement::Expression(loc, ref mut expr) => {
fmt.visit_expr(loc, expr)
}
_ => stmt.visit(fmt), // unreachable
}
})
.transpose()?;
fmt.write_semicolon()?;
match init {
Some(stmt) => stmt.visit(fmt),
None => fmt.write_semicolon(),
}?;
if multiline {
fmt.write_whitespace_separator(true)?;
}
cond.as_mut().map(|expr| expr.visit(fmt)).transpose()?;

cond.visit(fmt)?;
fmt.write_semicolon()?;
if multiline {
fmt.write_whitespace_separator(true)?;
}
update
.as_mut()
.map(|stmt| {
match **stmt {
Statement::VariableDefinition(_, ref mut decl, ref mut expr) => {
fmt.visit_var_definition_stmt(loc, decl, expr, false)
}
Statement::Expression(loc, ref mut expr) => {
fmt.visit_expr(loc, expr)
}
_ => stmt.visit(fmt), // unreachable
}
})
.transpose()?;
Ok(())

// Don't write a semi after the update expression
// This should be just an `Expression`, but it is parsed as a `Statement` for
// some reason in solang-parser
// See https://github.com/hyperledger/solang/issues/1283
match update.as_deref_mut() {
Some(Statement::Expression(loc, expr)) => fmt.visit_expr(*loc, expr),
Some(stmt) => {
unreachable!("Invalid Solidity for loop `update` expression: {stmt:?}")
}
_ => Ok(()),
}
};
let multiline = !fmt.try_on_single_line(|fmt| write_for_loop_header(fmt, false))?;
if multiline {
Expand Down
2 changes: 2 additions & 0 deletions fmt/src/macros.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ macro_rules! return_source_if_disabled {
($self:expr, $loc:expr) => {{
let loc = $loc;
if $self.inline_config.is_disabled(loc) {
trace!("Returning because disabled: {loc:?}");
return $self.visit_source(loc)
}
}};
Expand All @@ -98,6 +99,7 @@ macro_rules! return_source_if_disabled {
let has_suffix = $self.extend_loc_until(&mut loc, $suffix);
if $self.inline_config.is_disabled(loc) {
$self.visit_source(loc)?;
trace!("Returning because disabled: {loc:?}");
if !has_suffix {
write!($self.buf(), "{}", $suffix)?;
}
Expand Down
17 changes: 4 additions & 13 deletions fmt/src/visit.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,21 +120,12 @@ pub trait Visitor {
loc: Loc,
_declaration: &mut VariableDeclaration,
_expr: &mut Option<Expression>,
_semicolon: bool,
) -> Result<(), Self::Error> {
self.visit_source(loc)?;
self.visit_stray_semicolon()?;

Ok(())
self.visit_stray_semicolon()
}

/// Don't write semicolon at the end because variable declarations can appear in both
/// struct definition and function body as a statement
fn visit_var_declaration(
&mut self,
var: &mut VariableDeclaration,
_is_assignment: bool,
) -> Result<(), Self::Error> {
fn visit_var_declaration(&mut self, var: &mut VariableDeclaration) -> Result<(), Self::Error> {
self.visit_source(var.loc)
}

Expand Down Expand Up @@ -539,7 +530,7 @@ impl Visitable for Statement {
v.visit_stray_semicolon()
}
Statement::VariableDefinition(loc, declaration, expr) => {
v.visit_var_definition_stmt(*loc, declaration, expr, true)
v.visit_var_definition_stmt(*loc, declaration, expr)
}
Statement::For(loc, init, cond, update, body) => {
v.visit_for(*loc, init, cond, update, body)
Expand Down Expand Up @@ -593,7 +584,7 @@ impl Visitable for VariableDeclaration {
where
V: Visitor,
{
v.visit_var_declaration(self, false)
v.visit_var_declaration(self)
}
}

Expand Down
3 changes: 3 additions & 0 deletions fmt/testdata/InlineDisable/fmt.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,9 @@ function literalTest() {
hex"001122FF";
0xc02aaa39b223Fe8D0A0e5C4F27ead9083c756Cc2;
// forgefmt: disable-end

// forgefmt: disable-next-line
bytes memory bytecode = hex"ff";
}

function returnTest() {
Expand Down
4 changes: 4 additions & 0 deletions fmt/testdata/InlineDisable/original.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,10 @@ function literalTest() {
hex"001122FF";
0xc02aaa39b223Fe8D0A0e5C4F27ead9083c756Cc2;
// forgefmt: disable-end

// forgefmt: disable-next-line
bytes memory bytecode =
hex"ff";
}

function returnTest() {
Expand Down

0 comments on commit e99cf83

Please sign in to comment.