Conversation
| } | ||
| )"; | ||
| compileAndRun(sourceCode); | ||
| BOOST_REQUIRE(callContractFunction("getGasLeft()") == encodeArgs(99978604)); |
There was a problem hiding this comment.
This test is probably way too unstable.
| )"; | ||
| CHECK_ERROR(text, TypeError, "Member \"gas\" not found or not visible after argument-dependent lookup in msg"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Please add a test where gasleft is a local variable or function and check that resolution works correctly.
It could also test what happens if the signature of the function is different and whether overload resolution is performed.
There was a problem hiding this comment.
Regarding overload resolution: what's the expected behaviour here?
I just checked the behaviour of the global function "now" and noticed, that if "now" is overwritten/shadowed by a local function, then the global "now" will be inaccessible, even if the local function has a different signature... so at the moment overload resolution does not seem to work in such cases.
As of now "gasleft" has the same behaviour. I'm about to add tests for shadowing gasleft with local functions/variables, but I'm not sure about overload resolution - should overload resolution find the global function, if a local function has a different signature? If so, this should probably also be adjusted for the case of "now"...
There was a problem hiding this comment.
Yes, it should be consistent with the other cases: Shadowing between scopes, overloading in the same scope.
docs/contracts.rst
Outdated
| that accesses storage, blockchain data (e.g. ``now``, ``this.balance`` or | ||
| ``block.number``) or | ||
| execution data (``msg.gas``) or make calls to external contracts are disallowed. Expressions | ||
| execution data (``msg.value``) or make calls to external contracts are disallowed. Expressions |
There was a problem hiding this comment.
I would still keep the gas example, since this is a little special. I actually only wondered today whether we allow gasleft in pure functions or not.
There was a problem hiding this comment.
Hm, perhaps it is best to provide both value and gas.
libsolidity/ast/Types.cpp
Outdated
| } | ||
|
|
||
| MemberList::MemberMap MagicType::nativeMembers(ContractDefinition const*) const | ||
| MemberList::MemberMap MagicType::nativeMembers(ContractDefinition const *_contract) const |
libsolidity/ast/Types.cpp
Outdated
| }; | ||
| if (!v050) | ||
| members.emplace_back("gas", make_shared<IntegerType>(256)); | ||
| return MemberList::MemberMap(members); |
There was a problem hiding this comment.
The type of members should be identical to memberList::MemberMap shouldn't it?
|
|
||
| bool ExpressionCompiler::visit(MemberAccess const& _memberAccess) | ||
| { | ||
| const bool v050 = m_context.experimentalFeatureActive(ExperimentalFeature::V050); |
| else if (member == "origin") | ||
| m_context << Instruction::ORIGIN; | ||
| else if (member == "gas") | ||
| else if (!v050 && member == "gas") |
There was a problem hiding this comment.
Please use solAssert(!v050, "") here instead of just checking.
|
Re-triggered test, seems to be infrastructure issue. |
| function f() public returns (uint256 val) { return msg.gas; } | ||
| } | ||
| )"; | ||
| CHECK_SUCCESS(text); |
There was a problem hiding this comment.
Please always use CHECK_SUCCESS_NO_WARNINGS
| char const* text = R"( | ||
| contract C { | ||
| function gasleft() public pure returns (bytes32 val) { return "abc"; } | ||
| function f() public pure { bytes32 val = gasleft(); assert (val == "abc"); } |
There was a problem hiding this comment.
This code is not executed, so the assert is not an actual test.
There was a problem hiding this comment.
The "assert (val == "abc");" will not trigger an error (I only added it to supress an "unused variable" warning) - but if the compiler were to resolve to global "gasleft", which returns an uint256, the assignement "bytes32 val = gasleft();" should trigger an invalid conversion error, so it's still a valid test for the resolution... I will move that part to an EndToEnd test nonetheless.
|
Also should raise a warning if using |
|
@axic created an issue for the warning, but the description of your issue was rather clear that it should be a function. We discussed this and concluded that indeed, it should be a function. Note that both |
|
The issue had both options, but I don't remember any discussion. I had no preference, I like the function better as having volatile variables isn't a good idea at the moment. |
| "block.difficulty", | ||
| "block.number", | ||
| "block.gaslimit", | ||
| "gasleft()", |
There was a problem hiding this comment.
block.blockhash(7) above also uses parentheses. We should have tests for both versions if such a magic global is a function.
There was a problem hiding this comment.
Oh cool, I think it must be correct this way, the others are variables, while these two are functions.
3 participants
Am I right in assuming that due to scoping introducing the gasleft as a new global in fact does not break existing code?
I wasn't sure what kind of tests to add, so I added several tests - not all of them may be necessary. Also: the tests share the same name - is that ok, since they test the same feature or should they still have distinct names?