[BREAKING] Remove emit workaround by erak · Pull Request #4179 · argotorg/solidity

erak · 2018-05-23T13:40:37Z

Closes #4107.

This PR introduces emit as a proper keyword instead of parsing it as identifier.

chriseth

Please add an assert to parseEmitStatement which checks for the emit keyword.

chriseth · 2018-05-23T14:21:55Z

should this target 050?

erak · 2018-05-25T10:51:17Z

@chriseth I added the assertion you've requested.

axic · 2018-05-25T15:37:59Z

Changelog.md

 ### 0.5.0 (unreleased)

+Compiler features:
+ * Parser: Introduce emit as a keyword instead of parsing it as identifier.


Need to escape emit with the double backticks.

axic · 2018-05-25T15:38:28Z

libsolidity/parsing/Parser.cpp


 ASTPointer<EmitStatement> Parser::parseEmitStatement(ASTPointer<ASTString> const& _docString)
 {
+	solAssert(m_scanner->currentToken() == Token::Emit, "Expected emit token.");


This should use expectToken.

@axic, @chriseth requested an assertion here, but using expectToken seems to be more consistent with the rest of the parser. I'd be in favor of rather reporting a fatal parser error than raising an interal compiler error.

Well it will not be triggered, but mostly we use expectToken and not solAssert in this file.

I thought that it would be invalid use of the function to call it when there is no emit token, but yeah, expectToken is probably still better.

axic · 2018-05-25T15:38:46Z

test/libsolidity/syntaxTests/parsing/emit_event.sol

+contract C {
+	event A();
+	function f() {
+		emit A();


I'm sure there's a test like this already?

@axic We have some syntax tests that cover the usage of emit with non-event types, but none that doesn't expect parser / type errors. So I think this should be added.

Are you sure? event_emit_simple in SolidityNameAndTypeResolution seems to be a duplicate of this

@axic Oh, you're right. I forgot to search in SolidityNameAndTypeResolution. Will remove this one then.

erak · 2018-05-30T11:41:05Z

@axic @chriseth I think this is ready now for a final review.

axic · 2018-05-30T14:12:13Z

It is failing:

*** 14 failures are detected in the test module "SolidityTests"

axic · 2018-05-30T14:14:04Z

Also please rebase after #4178 was merged, perhaps some tests would need to be changed.

erak · 2018-05-30T14:14:43Z

@axic I will have a look why they are failing.

axic · 2018-06-11T12:01:59Z

It is weird that no tests seem to need an update. Does this fully disable invoking events without emit outside of 0.5.0?

axic · 2018-06-11T12:04:07Z

The check in TypeChecker should have the warning (Invoking events without "emit" prefix is deprecated.) removed and only keeping the error. I'd also tend to say that instead of TypeError, this should be a SyntaxError and as such may deserve its place in SyntaxChecker, if it not too big of an overhead.

axic · 2018-06-11T20:18:19Z

It may be better to keep this for using the keyword and having a separate PR cleaning up the rest.

axic · 2018-06-11T20:20:50Z

Changelog.md

 Language Features:
 * General: Support ``pop()`` for storage arrays.
+Compiler features:
+ * Parser: Introduce ``emit`` as a keyword instead of parsing it as identifier.


I think this is actually a breaking change, because emit cannot be used as an identifier anymore. Moved.

Change has been done.

chriseth previously requested changes May 23, 2018

View reviewed changes

erak force-pushed the emitWorkaround branch from 0cdf79a to 9c1744b Compare May 25, 2018 10:49

axic reviewed May 25, 2018

View reviewed changes

erak force-pushed the emitWorkaround branch 2 times, most recently from 9b2c622 to 520e252 Compare May 30, 2018 11:40

axic previously approved these changes May 30, 2018

View reviewed changes

erak dismissed axic’s stale review via f61b13d May 30, 2018 14:35

erak force-pushed the emitWorkaround branch 3 times, most recently from 0f5f378 to d54dbff Compare June 5, 2018 17:24

axic changed the title ~~Remove emit workaround~~ [BREAKING] Remove emit workaround Jun 11, 2018

axic force-pushed the emitWorkaround branch from d54dbff to 8f2de39 Compare June 11, 2018 17:10

axic reviewed Jun 11, 2018

View reviewed changes

Introduces emit token and removes identifier workaround.

2e9f5d1

axic force-pushed the emitWorkaround branch from 8f2de39 to 2e9f5d1 Compare June 11, 2018 20:21

axic approved these changes Jun 11, 2018

View reviewed changes

axic mentioned this pull request Jun 11, 2018

[BREAKING] Remove non-0.5.0 warning for emit keyword (make it mandatory) #4269

Merged

ekpyron approved these changes Jun 11, 2018

View reviewed changes

axic merged commit 8999a2f into develop Jun 11, 2018

axic deleted the emitWorkaround branch June 11, 2018 22:00

Conversation

erak commented May 23, 2018

Uh oh!

chriseth left a comment

Choose a reason for hiding this comment

Uh oh!

chriseth commented May 23, 2018

Uh oh!

erak commented May 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic May 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erak commented May 30, 2018

Uh oh!

axic commented May 30, 2018

Uh oh!

axic commented May 30, 2018

Uh oh!

erak commented May 30, 2018

Uh oh!

axic commented Jun 11, 2018

Uh oh!

axic commented Jun 11, 2018

Uh oh!

axic commented Jun 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

axic May 30, 2018 •

edited

Loading