Skip to content
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

Recommended use of 'emit' invalid inside tuples #3887

Closed
area opened this issue Apr 16, 2018 · 8 comments
Closed

Recommended use of 'emit' invalid inside tuples #3887

area opened this issue Apr 16, 2018 · 8 comments
Labels

Comments

@area
Copy link

area commented Apr 16, 2018

Solidity 0.4.21 provides a warning when not using emit with events:

pragma solidity ^0.4.3;

contract Test {
event __SomeEvent();

    function a() public {
        bool x = true;
        bool y = true;
        (x) ? ( __SomeEvent(), y=false) : (__SomeEvent(), y = false);
    }
}

However, inserting emit causes compilation to fail with: ParserError: Expected token Comma got 'Identifier' (x) ? ( emit __SomeEvent(), y=false) : (emit __SomeEvent(), y = false);

Was this use never really supported? Or was this regression an oversight when emit was introduced? I could certainly believe either scenario!

@axic axic added the bug 🐛 label Apr 16, 2018
@chriseth
Copy link
Contributor

Oh wow, someone found an edge case in the parser :)

Did you use an automated tool for that?

The thing is that emit is not really a keyword. It will be a keyword in 0.5.0 and until then, we just try to parse otherwise invalid input correctly according to the new rules.

I would actually say that the specific code you gave should be invalid even with the old rules, since you create a tuple with a component that has a null type (events do not return anything).

@area
Copy link
Author

area commented Apr 16, 2018

Did you use an automated tool for that?

Yeah, this is how we've been instrumenting ternary statements in solidity-coverage, because it was the only way to use the comma 'operator'.

The thing is that emit is not really a keyword. It will be a keyword in 0.5.0

Does this mean this could be expected to start working again in 0.5.0? (Assuming that you don't nix tuples with components with null types... it sounds like I may have shot myself in the foot!)

@chriseth
Copy link
Contributor

it might work with 0.5.0, but let me actually check, this bug might lie deeper than just what is visible with emit.

@chriseth
Copy link
Contributor

Yes, this should also be an error:

pragma solidity ^0.4.3;

contract Test {
    function f() {}

    function a() public {
        bool x = true;
        bool y = true;
        (x) ? ( f(), y=false) : (f(), y = false);
    }
}

@chriseth
Copy link
Contributor

In general, it should not be possible to use events outside of the statement level. #3889 should get us closer there.

@chriseth
Copy link
Contributor

Oh I'm sorry, now I only understand that this breaks your functionality.

@chriseth
Copy link
Contributor

I guess you have to translate ternary statements even more into proper if statements.

@area
Copy link
Author

area commented Apr 16, 2018

Oh I'm sorry, now I only understand that this breaks your functionality.

That's fine. For the greater good!

I guess you have to translate ternary statements even more into proper if statements.

Yeah, I'm pretty sure this is where we're going to have to go now. Thanks for being so responsive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants