Bugfix: Accessing storage struct members is always a storage read

[xermicus]

Fixes a bug: StorageLoad after a struct member access is a storage read, this case was not considered in the mutability check.

The regression in the EVM test is fine; it's caused by a problem in solc

[LucasSte]

Why are you restricting the diagnostics to only one per statement? Both the solc and Solang in the main branch detect multiple access violations in a single statement:

contract Test {
    struct CC {
        bytes name;
        uint32 age;
    }

    CC vari;
    uint32 cc;

    function read() public pure returns (uint32) {
        return vari.age + cc;
    }
}

We detect the problem in vari.age and in cc, but your branch only finds cc.
Two diagnostics for the same access level in a statement does not mean they refer to the same problem.

[xermicus]

Yeah good point. I've analyzed the situation a bit further. There are situations where the same error will be reported twice, if Expression::StorageLoad will be considered during expr recursing. If this can be made consistent somehow, there's no need to restrict diagnostics to a context.

[xermicus]

Just added the edge case for storageloads ond struct members

[seanyoung]

So why is a storageload of a struct member an exception? Any storage load should require view mutability.

The comment just explains how struct member works but nothing about why this is an exception.

This is just an ugly hack, I am not going to approve it.

[xermicus]

So why is a storageload of a struct member an exception? Any storage load should require view mutability.

We discussed this in the standup a while ago. 🙂 So, why isn't there already a match arm covering any storage load. The problem is that we need to consider the context, as 2 subsequent expressions might emit the same error (although with slightly different locations). Which we don't want. Is there another solution? Maybe, however I explored different options which all didn't work. This is why I decided to just fix this bug in the end 🙂

The comment just explains how struct member works but nothing about why this is an exception.

I don't understand why you think this is an exception and why you think that the comment doesn't explain the match arm thoroughly enough 🙂 I think that the comment explaining that struct member accesses are only a view if they come together with a storage load would be sufficient 🙃 Could you please go into more detail what you are missing from this comment?

Also, this is the only match arm with a comment anyways. My contribution being opposed to like this,, when there are usually not even comments or unreachable messages in the code base, makes me feel quite confused and makes for a unsatisfying contributor experience..

This is just an ugly hack, I am not going to approve it.

Could you please elaborate how you imagine the solution to look like without it being an ugly hack?

#1544 was supposed to solve #1493, which states needs a major overhaul. #1544 was approved without any fuss. But #1544 did clearly not do a major overhaul, while also missing out to fix the bug addressed here. In #1544, not a single comment was added, not even the PR itself did not have any description. Yet, despite all of that, #1544 was approved without any fuss 🙂 Under these circumstances, opposing my contribution, nitpicking on unreachable messages and comments, and calling it an ugly hack makes me feel unproductive and that my contribution is not well received and welcomed.

However, I still would like to get this bug fixed. Thus I kindly ask you to reconsider your previous review. And if you are still unhappy with it, I'm open for constructive input on

• How to go forward with this contribution
• How to craft future contributions, so that they are being accepted and welcomed in similar fashion to Fix mutability check #1544

[seanyoung]

Let's go through the history of this PR:

• The first version of the PR made any struct member on contract storage require view which is just wrong, we had to explain what struct member does
• Your second version just kept the last error which no good and a regression
• Your third version retained one error per statement which is also no good
• Your fourth version is the similar as your first rejected version

I kept on saying you need to make Expression::LoadStorage require view and do not stop recursing. I agree that #1544 should not have been merged. But shitty code elsewhere is no excuse for introducing more shitty code. We need to fix problems rather just paper over the cracks.

It's fixed here #1563

[xermicus]

I kept on saying you need to make Expression::LoadStorage require view and do not stop recursing

No, I perceive that entirely different. The first and only time that was stated is after the last iteration of this PR 🙂 Neither was it mentioned in previous PRs attempting to "fix mutability checks". The opposite is the case: It was removed in #1544. And when I asked about it during the standup, I was told that having this leads to duplicate error messages, which we don't want.

So I tried to find a different solution, based on the advice suggested to me initially. And then, the only thing that has been highlighted repeatedly in this PR (in various other PRs I made too btw) instead is that my comments are unsatisfactory, my function names are unsatisfactory, and calling my contribution an ugly hack and shitty code. Which leaves me feeling discouraged 😕