From 02c9e3b85dc285daccae9870c9d4af144139cb26 Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Tue, 9 Jan 2024 17:09:33 +0100 Subject: [PATCH 1/2] fix: dont strip empty () on constructors --- crates/fmt/src/formatter.rs | 44 ++++++++++++++++++- .../testdata/ConstructorModifierStyle/fmt.sol | 13 ++++++ .../ConstructorModifierStyle/original.sol | 13 ++++++ crates/fmt/tests/formatter.rs | 1 + 4 files changed, 69 insertions(+), 2 deletions(-) create mode 100644 crates/fmt/testdata/ConstructorModifierStyle/fmt.sol create mode 100644 crates/fmt/testdata/ConstructorModifierStyle/original.sol diff --git a/crates/fmt/src/formatter.rs b/crates/fmt/src/formatter.rs index 774cd3345264a..82833fa113134 100644 --- a/crates/fmt/src/formatter.rs +++ b/crates/fmt/src/formatter.rs @@ -80,6 +80,13 @@ struct Context { if_stmt_single_line: Option, } +impl Context { + /// Returns true if the current function context is the constructor + pub(crate) fn is_constructor_function(&self) -> bool { + self.function.as_ref().map_or(false, |f| matches!(f.ty, FunctionTy::Constructor)) + } +} + /// A Solidity formatter #[derive(Debug)] pub struct Formatter<'a, W> { @@ -3047,6 +3054,18 @@ impl<'a, W: Write> Visitor for Formatter<'a, W> { self.visit_list("", args, None, Some(loc.end()), false)? } FunctionAttribute::BaseOrModifier(loc, base) => { + // here we need to find out if this attribute belongs to the constructor because the + // modifier need to include the trailing parenthesis + // This is very ambiguous because the modifier can either by an inherited contract + // or a modifier here: e.g.: This is valid constructor: + // `constructor() public Ownable() OnlyOwner {}` + let is_constructor = self.context.is_constructor_function(); + // we can't make any decisions here regarding trailing `()` because we'd need to + // find out if the `base` is a solidity modifier or an + // interface/contract therefor we we its raw content. + + // we can however check if the contract `is` the `base`, this however also does + // not cover all cases let is_contract_base = self.context.contract.as_ref().map_or(false, |contract| { contract.base.iter().any(|contract_base| { contract_base @@ -3058,8 +3077,27 @@ impl<'a, W: Write> Visitor for Formatter<'a, W> { }) }); - if is_contract_base { - base.visit(self)?; + if is_constructor || is_contract_base { + if is_contract_base { + base.visit(self)?; + } else { + // This is ambiguous because the modifier can either by an inherited + // contract modifiers with empty parenthesis are + // valid, but not required so we make the assumption + // here that modifiers are lowercase + let mut base_or_modifier = + self.visit_to_chunk(loc.start(), Some(loc.end()), base)?; + let is_lowercase = base_or_modifier + .content + .chars() + .next() + .map_or(false, |c| c.is_lowercase()); + if is_lowercase && base_or_modifier.content.ends_with("()") { + base_or_modifier.content.truncate(base_or_modifier.content.len() - 2); + } + + self.write_chunk(&base_or_modifier)?; + } } else { let mut base_or_modifier = self.visit_to_chunk(loc.start(), Some(loc.end()), base)?; @@ -3110,6 +3148,8 @@ impl<'a, W: Write> Visitor for Formatter<'a, W> { })?; if base.args.is_none() || base.args.as_ref().unwrap().is_empty() { + // This is ambiguous because the modifier can either by an inherited contract or a + // modifier if self.context.function.is_some() { name.content.push_str("()"); } diff --git a/crates/fmt/testdata/ConstructorModifierStyle/fmt.sol b/crates/fmt/testdata/ConstructorModifierStyle/fmt.sol new file mode 100644 index 0000000000000..88694860aded2 --- /dev/null +++ b/crates/fmt/testdata/ConstructorModifierStyle/fmt.sol @@ -0,0 +1,13 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.5.2; + +import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; +import {ERC1155} from "solmate/tokens/ERC1155.sol"; + +import {IAchievements} from "./interfaces/IAchievements.sol"; +import {SoulBound1155} from "./abstracts/SoulBound1155.sol"; + +contract Achievements is IAchievements, SoulBound1155, Ownable { + constructor(address owner) Ownable() ERC1155() {} +} diff --git a/crates/fmt/testdata/ConstructorModifierStyle/original.sol b/crates/fmt/testdata/ConstructorModifierStyle/original.sol new file mode 100644 index 0000000000000..88694860aded2 --- /dev/null +++ b/crates/fmt/testdata/ConstructorModifierStyle/original.sol @@ -0,0 +1,13 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.5.2; + +import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; +import {ERC1155} from "solmate/tokens/ERC1155.sol"; + +import {IAchievements} from "./interfaces/IAchievements.sol"; +import {SoulBound1155} from "./abstracts/SoulBound1155.sol"; + +contract Achievements is IAchievements, SoulBound1155, Ownable { + constructor(address owner) Ownable() ERC1155() {} +} diff --git a/crates/fmt/tests/formatter.rs b/crates/fmt/tests/formatter.rs index 4a391ee773dd0..2e5d77054051f 100644 --- a/crates/fmt/tests/formatter.rs +++ b/crates/fmt/tests/formatter.rs @@ -181,6 +181,7 @@ macro_rules! test_directories { test_directories! { ConstructorDefinition, + ConstructorModifierStyle, ContractDefinition, DocComments, EnumDefinition, From 22bde7c7b7a121bfc145c6d1bf7b53fae09bb0f8 Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Tue, 9 Jan 2024 18:22:01 +0100 Subject: [PATCH 2/2] else if --- crates/fmt/src/formatter.rs | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/crates/fmt/src/formatter.rs b/crates/fmt/src/formatter.rs index 82833fa113134..89829a59b053e 100644 --- a/crates/fmt/src/formatter.rs +++ b/crates/fmt/src/formatter.rs @@ -3077,27 +3077,22 @@ impl<'a, W: Write> Visitor for Formatter<'a, W> { }) }); - if is_constructor || is_contract_base { - if is_contract_base { - base.visit(self)?; - } else { - // This is ambiguous because the modifier can either by an inherited - // contract modifiers with empty parenthesis are - // valid, but not required so we make the assumption - // here that modifiers are lowercase - let mut base_or_modifier = - self.visit_to_chunk(loc.start(), Some(loc.end()), base)?; - let is_lowercase = base_or_modifier - .content - .chars() - .next() - .map_or(false, |c| c.is_lowercase()); - if is_lowercase && base_or_modifier.content.ends_with("()") { - base_or_modifier.content.truncate(base_or_modifier.content.len() - 2); - } - - self.write_chunk(&base_or_modifier)?; + if is_contract_base { + base.visit(self)?; + } else if is_constructor { + // This is ambiguous because the modifier can either by an inherited + // contract modifiers with empty parenthesis are + // valid, but not required so we make the assumption + // here that modifiers are lowercase + let mut base_or_modifier = + self.visit_to_chunk(loc.start(), Some(loc.end()), base)?; + let is_lowercase = + base_or_modifier.content.chars().next().map_or(false, |c| c.is_lowercase()); + if is_lowercase && base_or_modifier.content.ends_with("()") { + base_or_modifier.content.truncate(base_or_modifier.content.len() - 2); } + + self.write_chunk(&base_or_modifier)?; } else { let mut base_or_modifier = self.visit_to_chunk(loc.start(), Some(loc.end()), base)?;