sway
sway copied to clipboard
FuelLabs
Should we emit warnings for storage variables that are only written?
We currently emit a warning for storage variables that are never read (even if they are written). For example, the following emits a warning:
contract;
storage {
x: u64 = 0,
}
abi MyContract {
#[storage(write)]
fn set();
}
impl MyContract for Contract {
#[storage(write)]
fn set() {
storage.x = 42;
}
}
Warning we get:
2 |
3 | storage {
4 | x: u64 = 0,
| - This storage declaration is never accessed and can be removed.
5 | }
|
____
Is this expected? Is there any value to have storage variables that are only ever written (or not be written at all)? Or should we continue emitting warnings for them? At the very least, the warning above should say "... never read ..." instead of "... never accessed ...".
When/if the SDK allows to directly access storage variable values, unaccessed variables might be useful offchain
That's right.. partially what prompted me to open this issue. In this case, maybe we don't need to emit any warnings here even if the storage variable is never read or written.
I think we could add something like #[write-only] annotation for storage variables like that, so that the user can convey the mode of use explicitly. What do you think?
In my experience with contract audits, having read-only or write-only storage vars sometimes might indicate a bug, so it's worth it to emit warnings in these cases, unless the user is explicit about that.
I think we could add something like #[write-only] annotation for storage variables like that, so that the user can convey the mode of use explicitly. What do you think?
@anton-trunov oh so that's annotation at the storage variable level? Interesting. I think Rust has similar annotations for dead code analysis.
It's reasonable to assume that storage variables that are only written but not read are generally indicative of a bug. I just worry that we still don't have a way to silence specific warnings.
@mohammadfawaz It seems #3453 deserves at least P: high tag (which I just added)?
A user was also confused by this: https://forum.fuel.network/t/forc-build-linter-issues-always-says-unused/2779
It's reasonable to assume that storage variables that are only written but not read are generally indicative of a bug. I just worry that we still don't have a way to silence specific warnings.
If we're going to keep this behavior, we should consider updating the warning message to be more clear. never accessed sounds like never used. Maybe something like This storage declaration is never read which could indicate a bug. for any storage variables that aren't read.
