From e99cf83396c4320e8c7e7f6650916183ad3051b4 Mon Sep 17 00:00:00 2001 From: DaniPopes <57450786+DaniPopes@users.noreply.github.com> Date: Thu, 20 Apr 2023 20:24:55 +0200 Subject: [PATCH] fix(fmt): `Variable{Declaration,Definition}` visit and fmt implementations (#4785) --- cli/src/cmd/forge/fmt.rs | 9 +- cli/src/cmd/forge/geiger/visitor.rs | 7 +- fmt/src/formatter.rs | 105 +++++++++--------------- fmt/src/macros.rs | 2 + fmt/src/visit.rs | 17 +--- fmt/testdata/InlineDisable/fmt.sol | 3 + fmt/testdata/InlineDisable/original.sol | 4 + 7 files changed, 60 insertions(+), 87 deletions(-) diff --git a/cli/src/cmd/forge/fmt.rs b/cli/src/cmd/forge/fmt.rs index 82d31a92996c..6fe6b58fe68d 100644 --- a/cli/src/cmd/forge/fmt.rs +++ b/cli/src/cmd/forge/fmt.rs @@ -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(_)) { diff --git a/cli/src/cmd/forge/geiger/visitor.rs b/cli/src/cmd/forge/geiger/visitor.rs index be0c673e7b04..3b0e2f881df4 100644 --- a/cli/src/cmd/forge/geiger/visitor.rs +++ b/cli/src/cmd/forge/geiger/visitor.rs @@ -260,17 +260,12 @@ impl Visitor for CheatcodeVisitor { _: Loc, declaration: &mut VariableDeclaration, expr: &mut Option, - _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) } diff --git a/fmt/src/formatter.rs b/fmt/src/formatter.rs index cc7cf09cf44d..0932592e3187 100644 --- a/fmt/src/formatter.rs +++ b/fmt/src/formatter.rs @@ -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, } } @@ -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 { @@ -1022,7 +1029,7 @@ impl<'a, W: Write> Formatter<'a, W> { trace!("Visiting {}", { let n = std::any::type_name::(); n.strip_prefix("solang_parser::pt::").unwrap_or(n) - },); + }); item.visit(self)?; } } @@ -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)?; } @@ -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(()) } @@ -2840,28 +2837,20 @@ impl<'a, W: Write> Visitor for Formatter<'a, W> { loc: Loc, declaration: &mut VariableDeclaration, expr: &mut Option, - 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)] @@ -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 { diff --git a/fmt/src/macros.rs b/fmt/src/macros.rs index 4ea922aba63f..c5c9d31a7d0e 100644 --- a/fmt/src/macros.rs +++ b/fmt/src/macros.rs @@ -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) } }}; @@ -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)?; } diff --git a/fmt/src/visit.rs b/fmt/src/visit.rs index 138332ff3a99..6d42edd2f063 100644 --- a/fmt/src/visit.rs +++ b/fmt/src/visit.rs @@ -120,21 +120,12 @@ pub trait Visitor { loc: Loc, _declaration: &mut VariableDeclaration, _expr: &mut Option, - _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) } @@ -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) @@ -593,7 +584,7 @@ impl Visitable for VariableDeclaration { where V: Visitor, { - v.visit_var_declaration(self, false) + v.visit_var_declaration(self) } } diff --git a/fmt/testdata/InlineDisable/fmt.sol b/fmt/testdata/InlineDisable/fmt.sol index 282e085de6dc..03979bc18893 100644 --- a/fmt/testdata/InlineDisable/fmt.sol +++ b/fmt/testdata/InlineDisable/fmt.sol @@ -229,6 +229,9 @@ function literalTest() { hex"001122FF"; 0xc02aaa39b223Fe8D0A0e5C4F27ead9083c756Cc2; // forgefmt: disable-end + + // forgefmt: disable-next-line + bytes memory bytecode = hex"ff"; } function returnTest() { diff --git a/fmt/testdata/InlineDisable/original.sol b/fmt/testdata/InlineDisable/original.sol index da66c57a1bc5..617da7719c2c 100644 --- a/fmt/testdata/InlineDisable/original.sol +++ b/fmt/testdata/InlineDisable/original.sol @@ -209,6 +209,10 @@ function literalTest() { hex"001122FF"; 0xc02aaa39b223Fe8D0A0e5C4F27ead9083c756Cc2; // forgefmt: disable-end + + // forgefmt: disable-next-line + bytes memory bytecode = + hex"ff"; } function returnTest() {