feature request: support for multiline `it` statements

[mds1]

I have a function that looks like this:

/**
 * @dev Generates pseudo-randomly a salt value using a diverse selection of block and
 * transaction properties.
 * @return salt The 32-byte pseudo-random salt value.
 */
function _generateSalt() internal view returns (bytes32 salt) {
    salt = keccak256(
        abi.encode(
            blockhash(block.number - 32),
            block.coinbase,
            block.number,
            block.timestamp,
            block.prevrandao,
            block.chainid,
            msg.sender
        )
    );
}

The rationale is add a lot of block properties to make it as unique as possible across chains/txs. But this isn't all possible properties, so a spec might look like this:

CreateX_GenerateSalt_Internal_Test
├── It never reverts.
└── It should be a function of multiple block properties and the caller.

But that's not fully specified as it doesn't specify which block properties. So it might be nice to support something like this:

CreateX_GenerateSalt_Internal_Test
├── It never reverts.
└── It should be a function of the below block properties and the caller:
    ├── - blockhash(block.number - 32),
    ├── - block.coinbase,
    ├── - block.number,
    ├── - block.timestamp,
    ├── - block.prevrandao,
    ├── - block.chainid,
    └── - msg.sender.

Which would get rendered into a test like this:

function test_ShouldBeAFunctionOfMultipleBlockPropertiesAndTheCaller() external {
  // It should be a function of multiple block properties and the caller.
  // The full set of dependencies is:
  //    - blockhash(block.number - 32),
  //    - block.coinbase,
  //    - block.number,
  //    - block.timestamp,
  //    - block.prevrandao,
  //    - block.chainid,
  //    - msg.sender.
}

In other words, the rule here is: Anything nested under an it gets included in the comments for that it, and it gets included at the respective indent level. So since there's one level of nesting for all those bullet points, in the comment they are indented by two spaces. As another example:

CreateX_GenerateSalt_Internal_Test
├── It never reverts.
└── It should be a function of the below block properties and the caller:
    ├── Here is some text
    │   ├── More examples
    │   └── Bulloak is great
    ├── Add block.chainid,
    └── Add msg.sender

would generate:

function test_ShouldBeAFunctionOfMultipleBlockPropertiesAndTheCaller() external {
  // It should be a function of the below block properties and the caller:
  //   Here is some text
  //     More examples
  //     Bulloak is great
  //   Add block.chainid,
  //   Add msg.sender
}

[alexfertel]

Cool! This sounds like a good use case 👍🏻 I'm happy to implement it.

If you'd like to give it a go lmk first, and I'll ping you after I merge a big revamp for bulloak check I'm working on.

[mds1]

Yea happy to take a shot! And any guidance/tips on where to implement/test would be great, as I’m not yet too familiar with the codebase :)

[alexfertel]

Okay, sorry for the delay, I finally have a structure that I'm happy with for bulloak check, so I can share some pointers with you. I haven't merged it because I need to finish implementing the rules, add docs & more tests, but you can take a look at https://github.com/alexfertel/bulloak/pull/31 if you want to start getting a hang of the new repo.

After thinking a bit more about this feature, I realized this is a non-trivial amount of work. However, the steps are roughly as follows:

• Take a look at the _parse function (arguably the obscurest part of the code because it's recursive), https://github.com/alexfertel/bulloak/blob/5ebf414ee6b5b930d03f21bdfc50ca660a589b85/src/syntax/parser.rs#L202 specifically at the TokenKind::It branch https://github.com/alexfertel/bulloak/blob/5ebf414ee6b5b930d03f21bdfc50ca660a589b85/src/syntax/parser.rs#L223-L230
  • One solution is to _parse recursively until it finds something that isn't a TokenKind::Word.
  • Another is to add a new keyword which we remove further down the compiler so that it doesn't show up in the comments.
  • Note that this should track the indentation level, unsure how this might look, but my first thought is to use a stack and add the current level to the ast::Ast variant created in the next bullet.
• Then, you can add a new ast::Ast variant -- something like ActionDetail or ActionDescription.
• Then, you can extend ast::Action to include a children: Vec<Ast> and the compiler should guide you into filling all the gaps introduced, which should roughly be:
  • Add a corresponding visit_action_detail function to the syntax::visitor::Visitor trait. This will also break the project in a few places (semantics, modifiers & translator).
  • Map all of the child ActionDetail of an action into the lexeme field of hir::Comment in hir::translator.
• Then (or before), test it all out, make sure we don't create the new AST nodes when we shouldn't and add documentation.

It's quite a lot of work, so feel free to leave it to me. Also, the parsing step is a bit overcomplicated because it was more natural for me to do it recursively, but it should probably be done iteratively. If you decide to dive in, lmk if you have any questions!

Also, it's probably best if you wait a bit to give me time to finish https://github.com/alexfertel/bulloak/pull/31.

[mds1]

Thanks for all the details! It definitely is more involved than I thought so I probably won't have the time to implement it anytime soon, and would happily defer the work to you if you have the bandwidth 😅

From a UX standpoint, I think the recursive parsing option is better than requiring a new keyword in tree files

[alexfertel]

Thanks for all the details! It definitely is more involved than I thought so I probably won't have the time to implement it anytime soon, and would happily defer the work to you if you have the bandwidth 😅

Ofc! No worries. I'll get to it when I can!