Solidity: The function dev::test::Option::get() should be avoided

[chriseth]

The function dev::test::Option::get() only works properly when run via soltest, it fails to work when used inside isoltest. Because of that, it should be avoided.

The SolidityExecutionFramework uses it to set the evm version, for example.

[chriseth]

It looks like isoltest --evm-version is not properly passed on to the compiler.

[erak]

I checked the following and it looks like passing the evm version works:

test/libsolidity/semanticTests/shifts.sol

contract C {
    function f(uint x) public returns (uint y) {
        assembly { y := shl(2, x) }
    }
}
// ====
// compileViaYul: also
// EVMVersion: >=constantinople
// ----
// f(uint256): 7 -> 28

calling isoltest -t 'semanticTests/*shifts*' --evm-version=homestead results in the test above not being run. Changing the required version like this:

// EVMVersion: >=homestead

results in the test being run and the expected pre-constantinople parser error.

[erak]

We could add a --no-silent argument (similar to no-color) that prints debug information and start with printing the concrete compiler arguments.

[chriseth]

It does get passed to isoltest, but the contract is not compiled with the right version.

[erak]

Do you have a source line or something already? I thought that if the following contract fails to compile:

contract C {
    function f(uint x) public returns (uint y) {
        assembly { y := shl(2, x) }
    }
}
...
// EVMVersion: >=homestead
...

when the default version in isoltest is petersburg, the test defines homestead as the minimal version (it would not be run for >=constantinople) and --evm-version=homestead gets passed to the compiler? It must have been compiled with the right, at least pre-constantinople, version, doesn't it?

[chriseth]

Maybe it's just in my evmone pull request, but I thought I didn't change things there...

Hm, I think only semantics tests are affected, can you try that?

[ekpyron]

@erak and me just rechecked and hard-coded aleth to always run on homestead. As expected a solidity semantics test with a shift still worked for isoltest --evm-version homestead and failed for other evm version settings, so isoltest seems to indeed correctly honour the evm version passed as argument.
We also rechecked that the filter setting worked, so we haven't found anything wrong.

[chriseth]

ExecutionFramework (and maybe others) use a singleton options object while isoltest uses its own isoltestoptions object. The (boost) options class should be merged into the common options class and the options should be explicitly created. The get function can still be used, but should throw an error if it is used before the object has been created.

[ekpyron]

In https://github.com/ethereum/solidity/pull/7010#discussion_r310720625 we noticed that currently <test/Options.h> in fact produces a link time dependency on the boost unit test framework - as long as <test/Options.h> is only used by soltest which links against that anyways, that might be fine, but in the long run and especially if we merge the Options classes it's better to avoid that - which should be straightforward with explicitly creating the options object (i.e. not include the boost unit test headers in <test/Options.h> and move the depending parts of parse to boostTest.cpp).

[erak]

Couldn't find the time to start with this yet, so I unassigned myself for now.

[ekpyron]

I'll take this on now.

[ekpyron]

Unassigned it again - I didn't actually get around to do it :-/.