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

fix: dont strip empty () on constructors #6740

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
44 changes: 42 additions & 2 deletions crates/fmt/src/formatter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,13 @@ struct Context {
if_stmt_single_line: Option<bool>,
}

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> {
Expand Down Expand Up @@ -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
Expand All @@ -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 {
mattsse marked this conversation as resolved.
Show resolved Hide resolved
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)?;
Expand Down Expand Up @@ -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("()");
}
Expand Down
13 changes: 13 additions & 0 deletions crates/fmt/testdata/ConstructorModifierStyle/fmt.sol
Original file line number Diff line number Diff line change
@@ -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() {}
}
13 changes: 13 additions & 0 deletions crates/fmt/testdata/ConstructorModifierStyle/original.sol
Original file line number Diff line number Diff line change
@@ -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() {}
}
1 change: 1 addition & 0 deletions crates/fmt/tests/formatter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ macro_rules! test_directories {

test_directories! {
ConstructorDefinition,
ConstructorModifierStyle,
ContractDefinition,
DocComments,
EnumDefinition,
Expand Down
Loading