feat: warnings as errors

[jatkz]

I would like to submit a new feature that would allow to create a setting for compile warnings to be included as errors.

Motivation

I wanted to work on this feature request here:
foundry-rs/foundry#3476

Following the guidance from that thread I am starting with making the additions here and I would be interested to also make the feature in Foundry after this is resolved.

With this setting turned on, the compiler when checking for errors, will also look for warnings

Solution

I have made the code changes that added a property to Project/ProjectBuilder and it goes all the way down to the compiler output, quite similar to how "ignored_error_codes" is done. I also added one test. I feel that I should mention I am a beginner to rust and I would greatly appreciate any feedback or guidance 😄

PR Checklist

• Added Tests
• Added Documentation
• Updated the changelog

[davidiola]

i had also been flirting with implementing this feature and happened to stumble across your PR so i put some of my thoughts in a review here 😄

cheers to learning together!

[davidiola]

this treats errors with severity type Info (rather than just Warning) as errors as well (unsure if this is intended). also, users may have populated ignored_error_codes and this logic will disregard those preferences.

here is what i had implemented locally:

pub fn has_error(&self, ignored_error_codes: &[u64], warnings_as_errors: bool) -> bool {
        if warnings_as_errors && self.has_warning(ignored_error_codes) {
            return true
        }
        self.errors.iter().any(|err| err.severity.is_error())
}

only errors w/Warning severity & warnings that are not ignored will be treated as errors in this case. unsure which solution is correct, just wanted to point out some inherent assumptions being made here

[gakonst]

implementation pattern LGTM - defer to @mattsse on what the right choice to do here is

[davidiola]

perhaps worth adding a test where set_warnings_as_errors is used in conjunction with ignored_error_codes?

[gakonst]

Yes please!

[jatkz]

Great catch, I wrote a new test which failed as predicted by David. I adjusted the has_error() fn to be a conjunction of the two solutions. Ty for the guidance everyone 🙏

[davidiola]

perhaps follow convention here and change to:

let warnings_as_errors = cache.project().warnings_as_errors.clone();

[gakonst]

implementation pattern LGTM - defer to @mattsse on what the right choice to do here is

[mattsse]

let's replace the bool arg with Severity

[mattsse]

I think this is fine.

Instead of the bool argument this could just take the Severity enum as argument and find all errors that are >= (needs Ord impl)

[jatkz]

I pushed a second commit that changed to using Severity as argument. I'm assuming it was following the correct pattern. If you think there are any specifics that could be adjusted, feel free to let me know.

During implement I had to choose between adding a Copy trait to Severity or passing a pointer down. I chose pointer since it was similar to "ignored_error_codes" and wasn't sure if Copy is something that is generally not preferred?

[jatkz]

I added a second commit to go with Severity as an argument, since that sounds like it could be the preferred solution.

[jatkz]

I took a look at the logs during the workflows lint step and noticed it exited when it got to my test.

I made another commit to see if formatting that code will fix it, but I am not really sure what went wrong.

[mattsse]

no need to take this as ref

Suggested change

	pub fn diagnostics<'a>(&'a self, ignored_error_codes: &'a [u64], warnings_as_errors: &'a Severity) -> OutputDiagnostics {
	pub fn diagnostics<'a>(&'a self, ignored_error_codes: &'a [u64], warnings_as_errors: Severity) -> OutputDiagnostics {

[mattsse]

Suggested change

	warnings_as_errors: &'a Severity,
	warnings_as_errors: Severity,

[mattsse]

please rename to something akin to severity

[gakonst]

CI failure unrelated, good progress and thank you for taking the time to address our review nits ^^, can merge once Matt's comments are 👍

[jatkz]

It was my pleasure 💯

[mattsse]

lgtm, ty!

[gakonst]

This is looking great, but let's add that one final test just to make sure we don't break anything on Foundry.

[gakonst]

Yes please!

[jatkz]

One thing to note, the first test checks all smart contracts in that test-data dir I created.

The second test I was much more specific and only tested 1 smart contract and the error code that goes with it.

[jatkz]

I made another commit based on the idea of retaining >= on the Severity enum.