Linter: Warn when storage can never be freed up

[cmichi]

pallet-contracts uses storage deposit as a mechanism to counter state bloat. In short, the idea is that there is an economic incentive to free up space on the chain. When a user executes a contract function that writes to storage, the user has to put a deposit down for the amount of storage space used. Whoever frees up that storage at some later point gets the deposit back.

For this to work, developers should whenever possible provide means for users to free up storage space again.

We could introduce a lint that checks in a simple manner:

• If there is a Vec.push()in the contract, then there also has to be a Vec.pop().
• If there is a Mapping.insert()in the contract, then there also has to be a Mapping.remove().
• If there is a Option = Some(…)in the contract, then there also has to be a Option = None.

There's surely contexts where it makes sense to ignore this rule, we could have something like #[allow(ink::never_freed)] to explicitly mark those.

I'm a bit torn if this linter rule makes sense, I would like to hear opinions from others. My concern is that I'm not sure if it's realistic to have inverse methods on a general base and if we would not be causing more user friction this way, possibly resulting in people just always allowing never_freed for the entire contract.

[athei]

I think this issue should be in ink!. This is where the lints live now :)

[deuszx]

You could also leave lints out of the compiler and move it to a specialised tool, just for that (just like rustc is not a linter and outsources that to clippy). We (Aleph Zero) are working with CoinFabrik team on a static analysis tool - https://github.com/CoinFabrik/scout - maybe you can consider "contributing" the idea there :)

[jubnzv]

@cmichi I suggest to split this into two lints:

1. storage_never_freed that checks if the storage used by complex structures is never freed (i.e. if there is Vec.push, there should be Vec.pop).
2. unused_option which checks if the None value of Option is used.

The point is that the implementation of these lints seems to be different.

storage_never_freed should work with storage fields and ignore local bindings. The main idea there is to find all the uses of fields (including mutable borrowings and propagations across function calls as it is done in #1914) that have specific types and then check if there the specific methods are called on them. We could check these cases for Vec, Mapping and the available collection types.

OTOH, unused_option should find all values of the Option type, in both local bindings and storage fields. Then it should check if they are used in pattern-matching arms (if let and match constructions). In my opinion, to simplify and speed up the implementation, we could make that analysis work only on HIR without propagating values across function calls. For instance, if the Option value is used as an argument in a function call, we may assume that its None value is somehow used.

What do you think?

[cmichi]

@jubnzv I think it makes a lot of sense.

unused_option seems quite reasonable, but I think clippy already contains a lint for that? Could you check?

I'm a bit torn on storage_never_freed. The idea was to create it to check if the contract contains methods for freeing up the storage it creates again. The idea came from this thought: our storage deposit system to reduce storage bloat works only properly if storage can indeed be freed up again.

I'm torn because there's also legitimate use-cases for writing storage once and never freeing it up again. An example is our link URL shortener, where the whole idea is to write a short URL and its long URL once and make sure no entity can ever remove that mapping again. So currently I'm leaning to not work on storage_never_freed right now. Wdyt?

[jubnzv]

unused_option seems quite reasonable, but I think clippy already contains a lint for that? Could you check?

It seems that there is no such lint in clippy. I'll try to ask them, maybe it is possible to contribute a new lint to clippy, since even the naive implementation seems to be useful in some cases.

I'm torn because there's also legitimate use-cases for writing storage once and never freeing it up again.

That's a fair point, I agree. In some cases it is absolutely legit to write code that doesn't free storage.

My suggestion here is to set allow as a default warning level for this lint. If the user wants to make the linter to be as annoying as possible, they can explicitly enable all the "pedantic" lints and run this analysis.

What do you think?

[jubnzv]

Here's the issue in clippy regarding unnecessary Option: rust-lang/rust-clippy#11612