[Yul] Fix registration of functions in scopes.

[chriseth]

Fixes #6446

[bshastry]

I reran the fuzzer on this patch and it told me that the following assembly code compiles fine, but optimization throws an exception

{
        function f(a, b) -> x
        {
                x := x
                {
                         x := f(g(), h())
                }
                let r := f(b, a)
        }
        function g() -> y
        {
                let r := f(g(), h())
        }
        function h() -> z
        {
                mstore(5, 4)
                z := mload(0)
        }
        let r := f(g(), h())
}

Assertion is thrown here

https://github.com/ethereum/solidity/blob/3df4936b6f76426f38129d7a7e57e3c8ac35ddd0/libyul/AsmAnalysis.cpp#L77

The (optimized) code at the time assertion is thrown is the following

{
    let z := 0
    let _1 := 4
    let _2 := 5
    mstore(_2, _1)
    let z_1 := mload(z)
    mstore(_2, _1)
    pop(f(g(), mload(z)))
    mstore(_2, _1)
    pop(f(g(), mload(z)))
    mstore(_2, _1)
    pop(f(g(), mload(z)))
    mstore(_2, _1)
    pop(f(g(), mload(z)))
    mstore(_2, _1)
    pop(f(g(), mload(z)))
    mstore(_2, _1)
    pop(f(g(), mload(z)))
    mstore(_2, _1)
    pop(f(g(), mload(z)))
    mstore(_2, _1)
    pop(f(g(), mload(z)))
    pop(f(z_1, z))
    function f(a, b) -> x
    {
        mstore(5, 4)
        x := f(g(), mload(0))
        pop(f(b, a))
    }
    function g() -> y
    {
        mstore(5, 4)
        let z := mload(0)
        let _1 := g()
        mstore(5, 4)
        pop(f(g(), mload(0)))
        pop(f(z, _1))
    }
}

I tried to print the errorList at the time this happened and it returned the following text

bugs/opt-not-yul.yul:17:3: Error: Variable name z already taken in this scope.
		z := mload(0)
		^-----------^
bugs/opt-not-yul.yul:12:14: Error: Variable name _1 already taken in this scope.
		let r := f(g(), h())
		           ^-^

Since the errors don't match either the original source or the optimized source, I don't know what to make of it.

General question: What do scoping rules say about

{
  let x := 0
  function f() { x := 1 }
}

Is x in scope inside f()?

[chriseth]

Ready for review. Fixed another might-be-bug.

[chriseth]

Yes, that should be fixed, now, too @bshastry

[chriseth]

The scoping rules were not implemented correctly: https://solidity.readthedocs.io/en/latest/yul.html#scoping-rules

Shadowing is disallowed, i.e. you cannot declare an identifier at a point where another identifier with the same name is also visible, even if it is not accessible.
Inside functions, it is not possible to access a variable that was declared outside of that function.

Visible means "defined in the same block as the function (or a common parent block)". Accessible means "visible and reaching to it does not cross function boundaries".

[bshastry]

Yes, that should be fixed, now, too @bshastry

Okay, will rerun the fuzzer on the diff

[bshastry]

Visible means "defined in the same block as the function (or a common parent block)". Accessible means "visible and reaching to it does not cross function boundaries".

So in the previous example (reproducing for convenience)

{
  let x := 0
  function f() { x := 1 }
}

x is visible in f() but not accessible, right? Does accessible subsume both read and write i.e., inside f() one can neither read from nor write to x?

Ah, I see why the original test input which is

{
  pop(f())
  function f() -> b { let g := mload(0) b := 0 }
  function g() {}
}

led to problems. g() is accessible inside f() so declaring it (albeit as a variable) inside f() is disallowed.

[bshastry]

Please rebase and squash.

[ekpyron]

Took me a while to understand what's happening here, but now it looks good to me!

What's that macos failure?

[ekpyron]

Ah, it's a compilation test failing in both runs (i.e. linux and mac) due to an additional scope...

[ekpyron]

Which is due to running the function grouper, so it's fine and only the test expectations need to be updated.

[chriseth]

Rebased and updated some expectations.

[ekpyron]

Fine, if the tests are passing.

[codecov]

Codecov Report

Merging #6456 into develop will increase coverage by <.01%.
The diff coverage is 95.23%.

@@             Coverage Diff             @@
##           develop    #6456      +/-   ##
===========================================
+ Coverage    87.85%   87.86%   +<.01%     
===========================================
  Files          387      387              
  Lines        37938    37942       +4     
  Branches      4474     4473       -1     
===========================================
+ Hits         33331    33336       +5     
+ Misses        3070     3069       -1     
  Partials      1537     1537

Flag	Coverage Δ
#all	87.86% <95.23%> (ø)	⬆️
#syntax	26.2% <52.38%> (-0.01%)	⬇️