search

hyperledger / solang

Solidity Compiler for Solana and Polkadot
https://solang.readthedocs.io/
Apache License 2.0
1.25k stars 208 forks source link

Bugfix: Accessing storage struct members is always a storage read #1553

Closed xermicus closed 11 months ago

xermicus commented 11 months ago

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

xermicus commented 11 months ago

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. :slightly_smiling_face: 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 :slightly_smiling_face:

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 :slightly_smiling_face: 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 :upside_down_face: 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 :slightly_smiling_face: 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

seanyoung commented 11 months ago

Let's go through the history of this PR:

I kept on saying you need to make Expression::LoadStorage require view and do not stop recursing. I agree that https://github.com/hyperledger/solang/pull/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 https://github.com/hyperledger/solang/pull/1563

xermicus commented 11 months ago

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 :slightly_smiling_face: 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 :confused: