Bugfix struct member read access (take 2)

[seanyoung]

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]

In this version, this code generates multiple errors for the same read/write violations:

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

    CC[] vari;
    uint32[] cc;

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

error: function declared 'pure' but this expression reads from state
   ┌─ /Users/lucasste/Documents/solang/examples/test.sol:11:16
   │
11 │         return vari[1].age + cc[2];
   │                ^^^^

error: function declared 'pure' but this expression reads from state
   ┌─ /Users/lucasste/Documents/solang/examples/test.sol:11:16
   │
11 │         return vari[1].age + cc[2];
   │                ^^^^^^^^^^^

error: function declared 'pure' but this expression reads from state
   ┌─ /Users/lucasste/Documents/solang/examples/test.sol:11:30
   │
11 │         return vari[1].age + cc[2];
   │                              ^^

error: function declared 'pure' but this expression reads from state
   ┌─ /Users/lucasste/Documents/solang/examples/test.sol:11:30
   │
11 │         return vari[1].age + cc[2];
   │                              ^^^^^

In one hand, you are not wrong as solc generates the same four errors. On the other hand, they clutter the diagnostics, don't they?

[seanyoung]

In one hand, you are not wrong as solc generates the same four errors. On the other hand, they clutter the diagnostics, don't they?

Thanks for finding those, I was thinking this could happen in some situations.

• Generating the same errors as solc is nice in its own way
• The code which removes overlapping errors works, but it is not the most elegant solution.
• I don't think those extra errors are too ugly (that's an opinion of course)

[xermicus]

In one hand, you are not wrong as solc generates the same four errors. On the other hand, they clutter the diagnostics, don't they?

Thanks for finding those, I was thinking this could happen in some situations.

* Generating the same errors as solc is nice in its own way

* The code which removes overlapping errors works, but it is not the most elegant solution.

* I don't think those extra errors are too ugly (that's an opinion of course)

I'm on the same page. Not ideal to have those errors, but I'm also not sure if it's worth the effort. What about fixing those cluttering messages when users complain, WDYT @LucasSte ?

[seanyoung]

I think what we want is some marker you can set which says "I've already warned for read on this expression, don't generate any more when recursing the current expression. I am not entirely sure how would you do that with current code though.

[LucasSte]

This is a working solution that copies solc behavior, so it is valid for now.

[xermicus]

I think what we want is some marker you can set which says "I've already warned for read on this expression, don't generate any more when recursing the current expression. I am not entirely sure how would you do that with current code though.

Hmm yeah, I've tried that out in my previous attempt, I agree it's not easily possible with the current implementation

[codecov[bot]]

Codecov Report

Merging #1563 (51d273f) into main (2c29fd7) will increase coverage by 0.02%. Report is 1 commits behind head on main. The diff coverage is 98.46%.

@@            Coverage Diff             @@
##             main    #1563      +/-   ##
==========================================
+ Coverage   87.33%   87.36%   +0.02%     
==========================================
  Files         133      133              
  Lines       64143    64126      -17     
==========================================
+ Hits        56021    56022       +1     
+ Misses       8122     8104      -18     

Files	Coverage Δ
src/sema/statements.rs	93.80% <100.00%> (+0.04%)	:arrow_up:
src/sema/mutability.rs	95.38% <97.87%> (+0.10%)	:arrow_up:

... and 5 files with indirect coverage changes