New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn about side-effect free statements. #2152

Merged
merged 4 commits into from Apr 24, 2017

Conversation

Projects
None yet
3 participants
@chriseth
Contributor

chriseth commented Apr 21, 2017

Fixes #2121
Fixes #1816

@chriseth chriseth requested review from axic and pirapira Apr 21, 2017

if (auto functionType = dynamic_cast<FunctionType const*>(annotation.type.get()))
annotation.isPure = functionType->isPure();
if (dynamic_cast<FunctionType const*>(annotation.type.get()))
annotation.isPure = true;

This comment has been minimized.

@chriseth

chriseth Apr 21, 2017

Contributor

Please check that this change is correct. This is the difference between f and f(). The first should be pure even if f is not a pure function.

@chriseth

chriseth Apr 21, 2017

Contributor

Please check that this change is correct. This is the difference between f and f(). The first should be pure even if f is not a pure function.

This comment has been minimized.

@pirapira

pirapira Apr 21, 2017

Member

Why is this just for functions? What about other magic variables?

@pirapira

pirapira Apr 21, 2017

Member

Why is this just for functions? What about other magic variables?

This comment has been minimized.

@pirapira

pirapira Apr 21, 2017

Member

For instance, msg might be pure although msg.sender is not pure.

@pirapira

pirapira Apr 21, 2017

Member

For instance, msg might be pure although msg.sender is not pure.

This comment has been minimized.

@chriseth

chriseth Apr 24, 2017

Contributor

Sure, but do we really need that? When we do the follow-up story, we can add a positive and a negative list and error whenever both are set to "false".

@chriseth

chriseth Apr 24, 2017

Contributor

Sure, but do we really need that? When we do the follow-up story, we can add a positive and a negative list and error whenever both are set to "false".

This comment has been minimized.

@pirapira

pirapira Apr 24, 2017

Member

I don't think positives or negatives are relevant. I'm seeing only booleans. If an expression reads, and if an expression writes.

We can also do this in the follow-up. In that case, I'll add this point in the follow-up issue.

@pirapira

pirapira Apr 24, 2017

Member

I don't think positives or negatives are relevant. I'm seeing only booleans. If an expression reads, and if an expression writes.

We can also do this in the follow-up. In that case, I'll add this point in the follow-up issue.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Apr 21, 2017

Member

Can you add a test for var a = revert; a; ?

Member

axic commented Apr 21, 2017

Can you add a test for var a = revert; a; ?

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Apr 21, 2017

Contributor

@axic such a test would be identical to uint a; a;, but I'll add it.

Contributor

chriseth commented Apr 21, 2017

@axic such a test would be identical to uint a; a;, but I'll add it.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Apr 21, 2017

Contributor

@axic it turns out such code will not trigger the warning, because a is not considered pure by the current code - it could be changed in a loop or so. I guess we will still catch most of the cases.

Contributor

chriseth commented Apr 21, 2017

@axic it turns out such code will not trigger the warning, because a is not considered pure by the current code - it could be changed in a loop or so. I guess we will still catch most of the cases.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Apr 21, 2017

Contributor

Note that this is not detecting all cases of side-effect-free statements, the rest will be done in #1380

Contributor

chriseth commented Apr 21, 2017

Note that this is not detecting all cases of side-effect-free statements, the rest will be done in #1380

@@ -3653,54 +3653,56 @@ BOOST_AUTO_TEST_CASE(conditional_with_all_types)
// integers
uint x;
uint y;
true ? x : y;
uint g = true ? x : y;

This comment has been minimized.

@pirapira

pirapira Apr 21, 2017

Member

Do we have an issue to warn about unread variables?

@pirapira

pirapira Apr 21, 2017

Member

Do we have an issue to warn about unread variables?

This comment has been minimized.

@chriseth

chriseth Apr 21, 2017

Contributor

Yes, someone else is working on it already.

@chriseth

chriseth Apr 21, 2017

Contributor

Yes, someone else is working on it already.

contract C {
function f() {
for (uint x = 0; x < 10; true)
x++;

This comment has been minimized.

@pirapira

pirapira Apr 21, 2017

Member

Is this a pure statement?

@pirapira

pirapira Apr 21, 2017

Member

Is this a pure statement?

This comment has been minimized.

@chriseth

chriseth Apr 21, 2017

Contributor

No it is not.

@chriseth

chriseth Apr 21, 2017

Contributor

No it is not.

This comment has been minimized.

@chriseth

chriseth Apr 21, 2017

Contributor

but the true is.

@chriseth

chriseth Apr 21, 2017

Contributor

but the true is.

This comment has been minimized.

@pirapira

pirapira Apr 21, 2017

Member

Ah, OK.

Will you add a test about for (uint x = 0; true; x++)?

@pirapira

pirapira Apr 21, 2017

Member

Ah, OK.

Will you add a test about for (uint x = 0; true; x++)?

This comment has been minimized.

@chriseth

chriseth Apr 24, 2017

Contributor

Added the test.

@chriseth

chriseth Apr 24, 2017

Contributor

Added the test.

@pirapira

Looks good to me. I'm restarting the emscripten test.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Apr 24, 2017

Member

I know it is useless because they are regular functions, but would it make sense having a test for all the critical ones (revert, assert, require, selfdestruct)? Just in case some other future feature would mess them up.

Member

axic commented Apr 24, 2017

I know it is useless because they are regular functions, but would it make sense having a test for all the critical ones (revert, assert, require, selfdestruct)? Just in case some other future feature would mess them up.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Apr 24, 2017

Contributor

Added the tests.

Contributor

chriseth commented Apr 24, 2017

Added the tests.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Apr 24, 2017

Contributor

Travis was a sporadic single failure, windows error was race condition with bytecode upload.

Contributor

chriseth commented Apr 24, 2017

Travis was a sporadic single failure, windows error was race condition with bytecode upload.

@chriseth chriseth merged commit 4d111e3 into develop Apr 24, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Apr 25, 2017

Member

@chriseth can there be an issue about the race condition?

Member

pirapira commented Apr 25, 2017

@chriseth can there be an issue about the race condition?

@pirapira pirapira deleted the warnRevert branch Apr 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment