Discussion: when should we split parameters, modifiers and return types in multiple lines?

[fvictorio]

Previous discussions: #139, #256, #454.

Before releasing a stable version of Prettier Solidity, we should decide what we want to do with respect to long function signatures. This is by far the most contentious issue, and chances are we won't make everyone happy. But I would like to, at least, get as many opinions as possible.

Right now, if I recall correctly, we split if there are more than two parameters/modifiers/return types. I personally don't like this approach, and I think it's not consistent with what prettier core does.

An alternative more similar to what prettier does (with long typescript functions) is to split the parameter lists first and then the modifiers list.

The return types are not treated different than a modifier with several arguments, so the question is what happens if they are too long (or if a modifier has a looot of arguments). For example:

function foo () modifier1 modifier2(aVeryLongArgument) returns (AVeryLongType, AnotherVeryVeryVeryVeryLongType)  {

An analogue situation in typescript is this:

function foo(): [[number, number, number, number, number], number, number] {}

and prettier does this:

function foo(): [
  [number, number, number, number, number],
  number,
  number
] {}

or, if the array doesn't fit, this:

function foo(): [
  [
    number,
    number,
    number,
    number,
    number
  ],
  number,
  number
] {}

So, using that as a guide, that solidity function should be formatted like this:

function foo ()
  modifier1
  modifier2(aVeryLongArgument)
  returns (AVeryLongType, AnotherVeryVeryVeryVeryLongType)  {

and, if the return types or a modifier have too many arguments/types, we split them further:

function foo ()
  modifier1
  modifier2(aVeryLongArgument)
  returns (
    AVeryLongType,
    AnotherVeryVeryVeryVeryLongType
  )  {

An unfortunate effect of this is that the first line of the function body is at the same indentation level that the ) of the return types. But this can also happen in typescript:

function foo():
  | AVeryLongLongLongType
  | AnotherVeryVeryVeryLongType {
  console.log();
}

Anyway, I guess my point is that we can create somewhat similar scenarios in typescript and see what prettier core does, and just imitate it as much as possible. But, again, I would like to hear more opinions on this.

[frangio]

An unfortunate effect of this is that the first line of the function body is at the same indentation level

This is why in these cases we place the { on its own line in OZ:

    function _checkOnERC721Received(address from, address to, uint256 tokenId, bytes memory _data)
        private returns (bool)
    {
        if (to.isContract()) {

But this is definitely controversial.

I would agree with: "split the parameter lists first and then the modifiers list."

---

I suppose in the example above it would result in:

    function _checkOnERC721Received(
        address from,
        address to,
        uint256 tokenId,
        bytes memory _data
    ) private returns (bool) {
        if (to.isContract()) {

This looks okay to me. I guess the problem is when long modifiers are involved.

[fvictorio]

But this is definitely controversial.

I'm not against this. While prettier core doesn't do this, the situations where it can happen for typescript are edge cases, while in solidity this is (arguably) more common.

[Janther]

This is why in these cases we place the { on its own line in OZ:

    function _checkOnERC721Received(address from, address to, uint256 tokenId, bytes memory _data)
        private returns (bool)
    {
        if (to.isContract()) {

But this is definitely controversial.

Not only I'm happy with this, we are already doing it here.

[fvictorio]

Not only I'm happy with this, we are already doing it here.

Oh, nice, I had forgotten about that (or never noticed 😅).

[frangio]

This is not exactly what this issue is about, but may be related. I found this example that I think is not ideal:

            try IERC1155Receiver(to).onERC1155BatchReceived(operator, from, ids, amounts, data) returns (
                bytes4 response
            ) {

It's prioritizing breaking in the parenthesis of the return parameters. I think of the following two should be preferred:

            try IERC1155Receiver(to).onERC1155BatchReceived(
                operator, from, ids, amounts, data
            ) returns (bytes4 response) {

            try IERC1155Receiver(to).onERC1155BatchReceived(
                operator,
                from,
                ids,
                amounts,
                data
            ) returns (bytes4 response) {

Note: This is not a dealbreaker for me, but I do think it can be improved.

[adriandelgg]

How can I set the default Solidity Prettier tab amount to 2? Everytime I save it does 4 space.

[Janther]

How can I set the default Solidity Prettier tab amount to 2? Everytime I save it does 4 space.

Hi @adriandelgg,

This is a question that probably belongs to our telegram channel but here's the answer.

The following configuration is our default for the .prettierrc file:

{
  "overrides": [
    {
      "files": "*.sol",
      "options": {
        "printWidth": 80,
        "tabWidth": 4,
        "useTabs": false,
        "singleQuote": false,
        "bracketSpacing": false,
        "explicitTypes": "always"
      }
    }
  ]
}

Just change the tabWidth parameter two whatever length you need.

[adriandelgg]

How can I set the default Solidity Prettier tab amount to 2? Everytime I save it does 4 space.

Hi @adriandelgg,

This is a question that probably belongs to our telegram channel but here's the answer.

The following configuration is our default for the .prettierrc file:

{
  "overrides": [
    {
      "files": "*.sol",
      "options": {
        "printWidth": 80,
        "tabWidth": 4,
        "useTabs": false,
        "singleQuote": false,
        "bracketSpacing": false,
        "explicitTypes": "always"
      }
    }
  ]
}

Just change the tabWidth parameter two whatever length you need.

Thank you @Janther !
Where do I find the .prettierrc file at though? I've looked around & can't seem to find it. :/

[mattiaerre]

if you are a VSCode user @adriandelgg https://github.com/prettier-solidity/prettier-plugin-solidity#vscode

[adriandelgg]

if you are a VSCode user @adriandelgg https://github.com/prettier-solidity/prettier-plugin-solidity#vscode

Ah, I see. Thank you! Is there a way to change the values globally & not just within each project?

[mattiaerre]

I believe that changes the values globally @adriandelgg

[Janther]

No there's no global config on prettier by philosophy. It is by project.
https://prettier.io/docs/en/configuration.html
I'd also appreciate to keep threads on topic and open new issues or contact us via Telegram rather than deviating.

[mds1]

Hey, just curious what the latest on this issue is!

As described in #454 (comment), one thing that always bugs me a bit is when function parameters are split on multiple lines even though the whole function signature is shorter than printWidth. It just happened to me again now which is why I bring this up 🙂

Since there's a lot of opinions here, one idea is to add an option like ignoreFunctionSignatures that, when true, doesn't run prettier against function signatures. This way people can format function signatures by hand if they don't like the prettier output

[fvictorio]

Ok, my current proposal is this.

Signature fits in print width

function f(uint x) public {
  doSomething();
}

Nothing to do here, the line is kept as-is.

Signature fits in print width and has more than two parameters

function f(uint x, uint y, uint z) public {
  doSomething();
}

Same as before, the line is not modified. This is different from what we do now, where we split signatures that have more than two parameters. I think we should stop doing that. And this is probably what's happening to you, @mds1.

Signature is too long, modifiers fit in print width

function f(
  uint longParam,
  uint anotherLongParam,
  uint aVeryLongPara
) public onlyOwner {
  doSomething();
}

Signature is too long, modifiers don't fit in print width

function f(
  uint longParam,
  uint anotherLongParam,
  uint aVeryLongPara
)
  public
  onlyOwner
  aModifier
  anotherModifier
{
  doSomething();
}

Notice that here the opening brace goes to its own line. This is a bit inconsistent, but it seems worth it.

Also, in this case we would split the params even if they would fit in print width. That is:

function f(
  uint x
)
  public
  onlyOwner
  aModifier
  anotherModifier
{
  doSomething();
}

The exception would be when there are no parameters:

function f()
  public
  onlyOwner
  aModifier
  anotherModifier
{
  doSomething();
}

The reason for this is to make implementation easier, but I'm not 100% sure that's the case. I mean, if for any reason it's easier to keep params in one line when they fit, I'd be happy with that too.

---

In summary:

• If the signature fits inside the print width, we don't do anything, not matter how many parameters it has.
• If it doesn't fit, and if it has one or more parameters, we split them.
• If the rest of the signature still doesn't fit, we split it and move the brace to its won line.

Of course, if the returns (...) part doesn't fit, we'll also split it.

Is anyone strongly opposed to some part of this?

[mds1]

LGTM overall, thanks @fvictorio! You've addressed this, but my one comment is:

Also, in this case we would split the params even if they would fit in print width

that in this case we wouldn't split the params. But to your point, if it makes implementation a lot simpler that seems ok to me

[fvictorio]

that in this case we wouldn't split the params

Yeah, I would say that is a nice-to-have but not a blocker for 1.0.0 (unlike the rest of the examples).

[fvictorio]

The final version of this is that parameters will be split first, then modifiers, then return parameters. We are going to publish an RC this week with this change.