Fix incorrect block scoping rules for legacy code

[d1ll0n]

The scoping rules in ast/definitions.ts incorrectly handle pre-0.5 blocks. Looking up a variable in a block should resolve to any VariableDeclaration anywhere in the block.

In the code below, y is equal to 16 because the scope is shared throughout the entire block, but resolveAny will resolve every xn to the contract's storage values.

Scenario

pragma solidity ^0.4.0;

contract A {
    uint256 x1; // Declaration ID = 3
    uint256 x2; // Declaration ID = 5
    uint256 x3; // Declaration ID = 7
    uint256 x4; // Declaration ID = 9

    function f() {
        {
            uint256 x1 = 1; // Declaration ID = 13
            if (x1 > 0) {
                uint256 x2 = 2; // Declaration ID = 20
            }
            {
                uint256 x3 = 3; // Declaration ID = 26
            }
        }
        // Declaration ID = 32
        for (uint256 x4; x4 < 10; x4++) {}
        // Declaration ID = 51
        uint256 y = x1 + x2 + x3 + x4;
    }
}

const result = await compileSol(sample, "0.4.26");
const reader = new ASTReader();
const [sourceUnits] = reader.read(result.data, astKind);
const yDeclaration = reader.context.locate(51) as VariableDeclarationStatement;
const [[x1], [x2], [x3], [x4]] = ["x1", "x2", "x3", "x4"].map((name) =>
    resolveAny(name, yDeclaration, "0.4.26")
) as Array<Set<VariableDeclaration>>;

Expected behavior

x1,x2,x3,x4 resolve to declarations within function f with IDs 13, 20, 26, 32.

Current behavior

x1,x2,x3,x4 resolve to declarations within contract with IDs 3,5,7,9

Solution

The fix is to search for all VariableDeclaration nodes when performing a lookup in the scope of a block when the version is <0.5. I modified lookupInBlock to perform this check and had to change lookupInScope and resolveAny to pass along the version. I also updated the block_04 and block_05 tests in test/samples/solidity/resolving to test this behavior.

[blitz-1306]

Hello there and thanks for the contribution. This also would be affected by #159 (not a blocker, just a reference).

[codecov-commenter]

Codecov Report

Merging #165 (68880dd) into master (745e34c) will decrease coverage by 0.05%.
The diff coverage is 63.63%.

@@            Coverage Diff             @@
##           master     #165      +/-   ##
==========================================
- Coverage   91.09%   91.03%   -0.06%     
==========================================
  Files         269      269              
  Lines        6291     6294       +3     
  Branches     1228     1229       +1     
==========================================
- Hits         5731     5730       -1     
- Misses        311      314       +3     
- Partials      249      250       +1     

Impacted Files	Coverage Δ
src/ast/definitions.ts	82.78% <63.63%> (-2.36%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[cd1m0]

Good catch! Thank you for the contriubtion. I am surprised that variables in sub-blocks are visible in the parent block, but I guess it was a really old compiler version? Thank you for also adding tests to your PR

[cd1m0]

lgtm