Forge fmt sometimes introduces incorrect spacing for multi-line comments with `wrap_comments` enabled

[emo-eth]

Component

Forge

Have you ensured that all of these are up to date?

• Foundry
• Foundryup

What version of Foundry are you on?

forge 0.2.0 (9256265 2023-01-12T00:04:39.480435Z)

What command(s) is the bug in?

fmt

Operating System

None

Describe the bug

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.17;

contract WeirdCommentFormat {
    function weirdComment() internal pure {
        assembly { 
            /*
             * Checks:
             * 1. Order parameters struct offset == 0x20
             * 2. Additional recipients arr offset == 0x240
             * 3. Signature offset == 0x260 + (recipients.length * 0x40)
             * 4. BasicOrderType between 0 and 23 (i.e. < 24)
             */
        }
    }
}

becomes

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.17;

contract WeirdCommentFormat {
    function weirdComment() internal pure {
        assembly {
            /*
             * Checks:
             * 1. Order parameters struct offset == 0x20
             * 2. Additional recipients arr offset == 0x240
            * 3. Signature offset == 0x260 + (recipients.length * 0x40)
             * 4. BasicOrderType between 0 and 23 (i.e. < 24)
             */
        }
    }
}

Note that item 3. was shifted to the left by one space.
(Strangely enough, removing the numbers after recipients.length * results in correct formatting again.)

Similarly happens with this multi-line comment

contract WeirdCommentFormat {
    function weirdComment() internal pure {
            /*
            * Calculate EIP712 ConsiderationItem hashes for original
            * additional recipients and add a ReceivedItem for each to the
            * consideration array in the OrderFulfilled event. The original
            * additional recipients are all the consideration items signed
            * by the offerer aside from the primary consideration items of
            * the order. Uses memory region from 0x80-0x160 as a buffer for
            * calculating EIP712 ConsiderationItem hashes.
            */
    }
}

becomes

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.17;

contract WeirdCommentFormat {
    function weirdComment() internal pure {
        /*
            * Calculate EIP712 ConsiderationItem hashes for original
        * additional recipients and add a ReceivedItem for each to the
        * consideration array in the OrderFulfilled event. The original
        * additional recipients are all the consideration items signed
        * by the offerer aside from the primary consideration items of
        * the order. Uses memory region from 0x80-0x160 as a buffer for
            * calculating EIP712 ConsiderationItem hashes.
            */
    }
}

However, if spacing is "correct" the first time, sometimes (perhaps outside of assembly blocks?), it will not dis-align the *s:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.17;

contract WeirdCommentFormat {
    function weirdComment() internal pure {
        /*
         * Calculate EIP712 ConsiderationItem hashes for original
         * additional recipients and add a ReceivedItem for each to the
         * consideration array in the OrderFulfilled event. The original
         * additional recipients are all the consideration items signed
         * by the offerer aside from the primary consideration items of
         * the order. Uses memory region from 0x80-0x160 as a buffer for
         * calculating EIP712 ConsiderationItem hashes.
         */
    }
}

is stable

[rkrasiuk]

@jameswenzel thanks for the bug report! this issue will be addressed in scope of #3786, will make sure to add tests

[sambacha]

@jameswenzel thanks for the bug report! this issue will be addressed in scope of #3786, will make sure to add tests

Why not fuzz the fmter? Can fuzz both the config and fmt results

[mds1]

I think this is now resolved.

This example is left alone and no longer changed.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.17;

contract WeirdCommentFormat {
    function weirdComment() internal pure {
        assembly { 
            /*
             * Checks:
             * 1. Order parameters struct offset == 0x20
             * 2. Additional recipients arr offset == 0x240
             * 3. Signature offset == 0x260 + (recipients.length * 0x40)
             * 4. BasicOrderType between 0 and 23 (i.e. < 24)
             */
        }
    }
}

The second example:

contract WeirdCommentFormat {
    function weirdComment() internal pure {
            /*
            * Calculate EIP712 ConsiderationItem hashes for original
            * additional recipients and add a ReceivedItem for each to the
            * consideration array in the OrderFulfilled event. The original
            * additional recipients are all the consideration items signed
            * by the offerer aside from the primary consideration items of
            * the order. Uses memory region from 0x80-0x160 as a buffer for
            * calculating EIP712 ConsiderationItem hashes.
            */
    }
}

Gets changed to

contract WeirdCommentFormat {
    function weirdComment() internal pure {
        /*
            * Calculate EIP712 ConsiderationItem hashes for original
            * additional recipients and add a ReceivedItem for each to the
            * consideration array in the OrderFulfilled event. The original
            * additional recipients are all the consideration items signed
            * by the offerer aside from the primary consideration items of
            * the order. Uses memory region from 0x80-0x160 as a buffer for
            * calculating EIP712 ConsiderationItem hashes.
            */
    }
}

In that case my understanding is that the first `/* gets moved to the expected spot, and the rest is left where it is because, imagine if the asterisks were indented hyphens/bullet points, then the formatter wants to leave them where they are.

So I'm going to optimistically close this, but @jameswenzel feel free to create a new issue if you have a good suggested behavior for that last case (e.g. maybe if they are asterisks also shift them left, but treat other characters as bullets? but then you can't use asterisks to represent markdown bullets for forge doc, and have to use hyphens, which maybe is ok)

[sambacha]

what about the case of /*!

[mds1]

Sorry, what about /*!? I don't see that mentioned in this issue, but if it is not being handled properly please open a new issue for it 🙂