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

[xermicus]

When I initially proposed this solution in the standup. I was pointed out that #1544 removed it because it leads to duplicate error messages. And I should try a different solution. But I agree that a StorageLoad must always require view. So I guess without changing the structure of the tree there is no other solution than tracking the diagnostics. 🤷

[LucasSte]

Although this solution of removing duplicate diagnostics based on their location is a working solution, I wonder whether this code is really necessary. In codegen, we have explicitly Isntr::StorageStore and Instr::LoadStorage that can be used to detect storage mutability. In fact, moving the mutability check to codegen was my original idea for #1544, but I found a quick fix for the problem (which led to other unforeseen errors, unfortunately).

I know this would require a lot more work than this PR, so we might solve this later and I would be happy to implement this. Please, just share your thoughts about this.

[seanyoung]

I don't think this will work in codegen, if we want to be compatible with solc. Take this example:

contract C {
	int[] arr;

	function foo() internal pure returns (int[] storage y) {
		y = arr;
	}
}

solc says:

Error: Function declared as pure, but this expression (potentially) reads from the environment or state and thus requires "view".

codegen:

# function C::C::function::foo public:false selector:8ae62232ad41e1fb nonpayable:true
# params: 
# returns: int256[] storage y
block0: # entry
	ty:int256[] storage %y = uint32 16
	return %y

(There is no Instr::LoadStorage)

[seanyoung]

After some more fixes, I think the fn remove_overlapping() is entirely unnecessary.

[LucasSte]

It is interesting that solc considers that a view function, even if we do not read anything from storage and only assign a constant value to the return variable.

After some more fixes, I think the fn remove_overlapping() is entirely unnecessary.

Great! That makes your code much cleaner!

[seanyoung]

It is interesting that solc considers that a view function, even if we do not read anything from storage and only assign a constant value to the return variable.

Yes, I don't understand this either. I guess it's not just any old constant value, it's a storage key.

[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]

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%)	⬆️
src/sema/mutability.rs	95.38% <97.87%> (+0.10%)	⬆️

... and 5 files with indirect coverage changes