alexfertel / bulloak

A Solidity test generator based on the Branching Tree Technique.
Apache License 2.0
225 stars 13 forks source link

feat: add support for multiple trees per file #51

Closed giovannidisiena closed 4 months ago

giovannidisiena commented 6 months ago

WIP attempt at handling multiple trees per .tree file. Currently, it appears I have broken some of the existing tests but I'm not yet sure how/why.

Steps taken:

@alexfertel it would be great to get some initial feedback here, when you get some time please, to make sure I'm not completely barking up the wrong tree or making too much of a mess. I have left a few follow-up comments for both my and your benefit - I'm sure there's lots here that could be improved or otherwise written in a more idiomatic fashion so please do let me know where that is the case as I'm definitely still getting to grips with Rust!

Partially closes #50.

alexfertel commented 6 months ago

This looks very promising! Thanks for such an effort.

Reading the code made me realize that we might be able to do something simpler: What if we parallelized the inner phases? That is, we currently roughly have the following compiler pipeline:

.tree -> &str -> scaffold entrypoint -> tokenize & parse -> semantic analysis -> HIR -> PT -> emit 

The above works for one (1) tree. Would you be willing to try feeding each separate tree to the pipeline? You would only touch two points in the pipeline:

.tree -> &str -> split input -> scaffold entrypoint -> ... -> HIR -> Combine -> PT -> emit
                 ^^^^^^^^^^^                                         ^^^^^^^

Which ends up looking like:

                                scaffold entrypoint -> ... -> Hir 
                              /                                   \
.tree -> &str -> split input -> scaffold entrypoint -> ... -> Hir -> Combine -> PT -> emit
                              \                                   /
                                scaffold entrypoint -> ... -> Hir 

Where the split is a simple:

let trees = tree.split("\n\n").collect::<Vec<_>>();

And the Combine phase looks similar to what you have.

If this makes sense to you I think it would simplify a lot. Wdyt?

On the other hand, I think the combine step is a good place for verifying that all the trees have the same contract name 👌🏻

giovannidisiena commented 6 months ago

Thanks @alexfertel, the review was very helpful - I have just pushed a new commit with the simplified pipeline. I have also identified the cause of most of the existing test failures due to some slight modifications I made to the use of identifier mode in syntax::tokenizer, which I believe to be correct but have downstream effects, so if you could take a look at the remaining updated follow-up comments and provide your thoughts please that would be awesome.

alexfertel commented 6 months ago

I don't have much time right now to review, but I will do so soon after New Year's!

codecov[bot] commented 5 months ago

Codecov Report

Attention: Patch coverage is 98.38275% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 92.5%. Comparing base (02c171e) to head (8f80e68).

Additional details and impacted files | [Files](https://app.codecov.io/gh/alexfertel/bulloak/pull/51?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alexander+Gonz%C3%A1lez) | Coverage Δ | | |---|---|---| | [src/check/rules/structural\_match.rs](https://app.codecov.io/gh/alexfertel/bulloak/pull/51?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alexander+Gonz%C3%A1lez#diff-c3JjL2NoZWNrL3J1bGVzL3N0cnVjdHVyYWxfbWF0Y2gucnM=) | `96.1% <ø> (ø)` | | | [src/check/violation.rs](https://app.codecov.io/gh/alexfertel/bulloak/pull/51?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alexander+Gonz%C3%A1lez#diff-c3JjL2NoZWNrL3Zpb2xhdGlvbi5ycw==) | `88.4% <100.0%> (+0.2%)` | :arrow_up: | | [src/error.rs](https://app.codecov.io/gh/alexfertel/bulloak/pull/51?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alexander+Gonz%C3%A1lez#diff-c3JjL2Vycm9yLnJz) | `91.3% <100.0%> (+0.6%)` | :arrow_up: | | [src/hir/mod.rs](https://app.codecov.io/gh/alexfertel/bulloak/pull/51?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alexander+Gonz%C3%A1lez#diff-c3JjL2hpci9tb2QucnM=) | `100.0% <100.0%> (ø)` | | | [src/hir/translator.rs](https://app.codecov.io/gh/alexfertel/bulloak/pull/51?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alexander+Gonz%C3%A1lez#diff-c3JjL2hpci90cmFuc2xhdG9yLnJz) | `98.3% <100.0%> (ø)` | | | [src/scaffold/emitter.rs](https://app.codecov.io/gh/alexfertel/bulloak/pull/51?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alexander+Gonz%C3%A1lez#diff-c3JjL3NjYWZmb2xkL2VtaXR0ZXIucnM=) | `97.9% <100.0%> (-0.1%)` | :arrow_down: | | [src/scaffold/mod.rs](https://app.codecov.io/gh/alexfertel/bulloak/pull/51?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alexander+Gonz%C3%A1lez#diff-c3JjL3NjYWZmb2xkL21vZC5ycw==) | `87.9% <100.0%> (+2.6%)` | :arrow_up: | | [src/scaffold/modifiers.rs](https://app.codecov.io/gh/alexfertel/bulloak/pull/51?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alexander+Gonz%C3%A1lez#diff-c3JjL3NjYWZmb2xkL21vZGlmaWVycy5ycw==) | `90.8% <ø> (ø)` | | | [src/syntax/tokenizer.rs](https://app.codecov.io/gh/alexfertel/bulloak/pull/51?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alexander+Gonz%C3%A1lez#diff-c3JjL3N5bnRheC90b2tlbml6ZXIucnM=) | `94.6% <100.0%> (-0.1%)` | :arrow_down: | | [src/utils.rs](https://app.codecov.io/gh/alexfertel/bulloak/pull/51?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alexander+Gonz%C3%A1lez#diff-c3JjL3V0aWxzLnJz) | `99.2% <100.0%> (+4.6%)` | :arrow_up: | | ... and [1 more](https://app.codecov.io/gh/alexfertel/bulloak/pull/51?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alexander+Gonz%C3%A1lez) | |
alexfertel commented 5 months ago

Heyo @giovannidisiena ! Sorry about the delay, I've been very busy. Finally had some time to look at this.

Very good job! This is basically what I had in mind. What's left is testing, which I'll leave in your hands. I'd expect unit tests + integration tests in the tests folder.

I pushed a couple of commits: one with a few structural changes that are basically nits from me and another one fixing what you brought up -- you were right that identifier_mode should start as false. This was a bug given that we sanitize contract names now.

One more thing we need to add to the TODO list is that we need to handle trees that are more than \n\n apart. I think it's reasonable to just ignore any whitespace around and between the \n\n.

Thank you very much, excited about merging this PR!

giovannidisiena commented 5 months ago

Hey @alexfertel, that's not a problem at all. I'm also very busy for the foreseeable so will have to attempt to chip away at the testing as and when possible.

Brilliant, thank you for the assist there, and yes agreed regarding the additional whitespace.

Thank you very much, excited about merging this PR!

Likewise!

alexfertel commented 4 months ago

Hey @giovannidisiena, I just wanted to check how we're feeling about this one. I'd really like this not to get stale, so if you're not up for it I'm happy to take over!

giovannidisiena commented 4 months ago

Absolutely @alexfertel, thanks for checking in. I may get some time toward the end of this week, so will keep you updated; otherwise, it may be best for you to take over since the next opportunity I have will likely be toward the end of March/April, which is obviously not ideal for keeping this from getting stale. I'll let you know how it goes!

giovannidisiena commented 4 months ago

@alexfertel I think that's almost everything you asked for now, except Combiner unit tests that I will add. The only other thing I am unsure about is whether we need to handle any violation cases to be fixed when running bulloak check --fix?

giovannidisiena commented 4 months ago

This is looking awesome! I want to do some checks myself locally, which I'll do as soon as I get some time, hopefully today. Thanks for all this work ❤️

Great! And thanks for all the help ❤️

alexfertel commented 4 months ago

Okay, local checks done!

any violation cases to be fixed when running bulloak check --fix

Nothing comes to mind right now, but any work there can be done in a different PR, it doesn't need to be a part of this one. If you think of anything feel free to open an issue.

I think what's missing are the Combiner unit tests, and then I think we need to check the README and update it to reflect the new functionality. Does that make sense to you?

giovannidisiena commented 4 months ago

Nothing comes to mind right now, but any work there can be done in a different PR, it doesn't need to be a part of this one. If you think of anything feel free to open an issue.

The only thing I can think of at the moment is perhaps fixing the contract name mismatch by applying the first identifier to all subsequent roots, but I'm happy to open it as a separate issue if you think it's not worth tackling right now.

alexfertel commented 4 months ago

The only thing I can think of at the moment is perhaps fixing the contract name mismatch by applying the first identifier to all subsequent roots, but I'm happy to open it as a separate issue if you think it's not worth tackling right now.

Ah, you're right. Yeah, I think we can tackle it separately. If you feel like working on it, don't let me hold you back, though. I'm happy either way.

giovannidisiena commented 4 months ago

Ah, you're right. Yeah, I think we can tackle it separately. If you feel like working on it, don't let me hold you back, though. I'm happy either way.

Okay, no problem; I'll see if I have time left over after the Combiner tests and README are done 👍

giovannidisiena commented 4 months ago

@alexfertel, Combiner tests have been added, and the corresponding logic has been updated accordingly. I'm not sure I'll get around to refactoring the structural match violation logic, although if you are willing to provide some pointers then that could speed things up and I may be able to take a crack at it.

alexfertel commented 4 months ago

I'm not sure I'll get around to refactoring the structural match violation logic

That's okay, that part of the code is not polished and needs lots of love. I'm merging this in!

When you're happy with this, feel free to change it to ready for review.

giovannidisiena commented 4 months ago

@alexfertel just realised I haven't updated the README, doing that now.

alexfertel commented 4 months ago

I forgot about that as well, thank you!

giovannidisiena commented 4 months ago

@alexfertel While writing up an example for the README, I noticed that the resulting Solidity test function names did not include the root identifier function name, so I have also added that and refactored the Combiner. May be worth another review to make sure you're still happy with it.

alexfertel commented 4 months ago

Ah, I see, yeah that makes sense. I'll take a better look tomorrow morn, and if everything looks good, I'll merge 👍🏻