diff --git a/crates/fmt/src/formatter.rs b/crates/fmt/src/formatter.rs index 774cd3345264a..89829a59b053e 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 @@ -3060,6 +3079,20 @@ impl<'a, W: Write> Visitor for Formatter<'a, W> { 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)?; @@ -3110,6 +3143,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,