Revert statement. #11037

chriseth · 2021-03-03T17:38:55Z

No description provided.

mijovic

Few small comments and suggestions for first round.

Also this needs to be rebased again, as there are some commits from other PRs

libsolidity/analysis/PostTypeChecker.cpp

libsolidity/ast/AST.h

libsolidity/codegen/ir/IRGeneratorForStatements.cpp

libsolidity/ast/AST.cpp

cameel

Only minor suggestions. I haven't found anything serious.

libsolidity/analysis/PostTypeContractLevelChecker.cpp

libsolidity/ast/AST.cpp

libsolidity/codegen/ExpressionCompiler.cpp

libsolidity/formal/SMTEncoder.cpp

test/libsolidity/syntaxTests/revertStatement/regular.sol

test/libsolidity/semanticTests/errors/panic_via_import.sol

test/libsolidity/semanticTests/errors/using_structs.sol

cameel

LTGM

mijovic · 2021-03-30T08:38:00Z

libsolidity/interface/CompilerStack.cpp

@@ -433,6 +434,14 @@ bool CompilerStack::analyze()
 				noErrors = false;
 		}

+		if (noErrors)
+		{


I think brackets are not needed, but not mandatory to change

mijovic

Looks good.

hrkrshnn · 2021-03-30T10:19:20Z

Can you also add this test:

struct error { uint error; }
contract C {
	uint error = 0;
	error test();
	function f() public {
		revert test();
	}
}
contract D {
	error _struct;
	error test();
}

should succeed (already does) because error is not a keyword.

chriseth · 2021-03-30T14:01:24Z

It actually fails :(

chriseth · 2021-03-30T14:06:59Z

Added the test as error_structs.sol.

aarlt · 2021-03-30T15:21:24Z

libsolidity/analysis/FunctionCallGraph.cpp

@@ -125,6 +125,8 @@ bool FunctionCallGraphBuilder::visit(FunctionCall const& _functionCall)
 		// change at runtime). All we can do is to add an edge to the dispatch which in turn has
 		// edges to all functions could possibly be called.
 		add(m_currentNode, CallGraph::SpecialNode::InternalDispatch);
+	else if (functionType->kind() == FunctionType::Kind::Error)
+		m_graph.usedErrors.insert(&dynamic_cast<ErrorDefinition const&>(functionType->declaration()));


maybe we should add an assert here?

You mean as else? Not really. There are other case possible here, the graph just skips them.

There is a check and I would say it is highly unlikely for this assert to trigger...

hrkrshnn · 2021-03-30T15:51:45Z

  struct S {uint x;}
  contract C {
	  function f() public {
		revert S(10);
	  }
  }

produces an ICE:

: Internal compiler error during compilation:
: /solidity/libsolidity/codegen/ContractCompiler.cpp(79): Throw in function void {anonymous}::StackHeightChecker::check()
: Dynamic exception type: boost::wrapexcept<solidity::langutil::InternalCompilerError>
: std::exception::what: I sense a disturbance in the stack: 2 vs 1
: [solidity::util::tag_comment*] = I sense a disturbance in the stack: 2 vs 1

But the following works!

  struct S {uint x;}
  contract C {
	  error S(uint y);
	  function f() public {
		revert S(10);
	  }
  }

aarlt · 2021-03-30T16:39:52Z

test/libsolidity/syntaxTests/revertStatement/revert_event.sol

+contract C {
+    event E();
+    function f() public pure {
+        revert E();


I was wondering why pure functions are allowed to revert. I think that should be forbidden.

This has been discussed a lot in the past. Pure means the result only depends on the arguments, and that is the case here.

chriseth · 2021-03-30T17:35:43Z

@hrkrshnn oh wow! Good that you found this! I could swear the code for that was there before...

chriseth · 2021-03-30T17:42:02Z

@hrkrshnn added the check to TypeChecker and added tests.

The base branch was changed.

It was introduced alongside the `revert` contextual keyword, see ethereum/solidity#11037.

The statement was introduced alongside the `revert` contextual keyword, see ethereum/solidity#11037.

chriseth force-pushed the revertStatement branch 2 times, most recently from 046f830 to 5dd3058 Compare March 3, 2021 17:58

chriseth force-pushed the customErrorDeclaration branch from 4b8159f to b4ba6d1 Compare March 9, 2021 15:47

chriseth force-pushed the revertStatement branch 4 times, most recently from 201bfc1 to 4e0438e Compare March 10, 2021 17:46

chriseth force-pushed the customErrorDeclaration branch from b4ba6d1 to ae5fa34 Compare March 16, 2021 18:18

chriseth force-pushed the revertStatement branch 2 times, most recently from 4174c6a to c621909 Compare March 16, 2021 18:47

mijovic reviewed Mar 17, 2021

View reviewed changes

chriseth force-pushed the revertStatement branch 2 times, most recently from 51857d7 to 728bc1b Compare March 18, 2021 12:28

chriseth commented Mar 18, 2021

View reviewed changes

libsolidity/ast/AST.cpp Show resolved Hide resolved

chriseth force-pushed the revertStatement branch from 728bc1b to 69d6b62 Compare March 18, 2021 13:46

cameel mentioned this pull request Mar 19, 2021

Syntax for defining custom errors. #10987

Merged

cameel reviewed Mar 19, 2021

View reviewed changes

chriseth force-pushed the customErrorDeclaration branch from ae5fa34 to 1aad33d Compare March 24, 2021 13:10

chriseth force-pushed the revertStatement branch from 69d6b62 to d4421a4 Compare March 24, 2021 13:10

chriseth force-pushed the customErrorDeclaration branch 6 times, most recently from 8184bea to 4438a59 Compare March 25, 2021 10:17

chriseth force-pushed the revertStatement branch from d4421a4 to 7d7b6b8 Compare March 25, 2021 10:23

chriseth force-pushed the customErrorDeclaration branch from 4438a59 to d0cec87 Compare March 25, 2021 11:00

chriseth force-pushed the revertStatement branch 2 times, most recently from 59523fa to 5a033f0 Compare March 29, 2021 13:58

cameel previously approved these changes Mar 29, 2021

View reviewed changes

mijovic reviewed Mar 30, 2021

View reviewed changes

mijovic approved these changes Mar 30, 2021

View reviewed changes

chriseth force-pushed the revertStatement branch from 5a033f0 to ea0e693 Compare March 30, 2021 14:06

mijovic previously approved these changes Mar 30, 2021

View reviewed changes

aarlt reviewed Mar 30, 2021

View reviewed changes

chriseth force-pushed the revertStatement branch from ea0e693 to 002f79f Compare March 30, 2021 17:41

chriseth force-pushed the customErrorDeclaration branch from d0cec87 to b04b189 Compare March 30, 2021 19:15

chriseth added 7 commits March 30, 2021 21:15

Revert statement.

fb67051

AST import and export for revert statement.

b552e5a

Grammar for revert statement.

3353107

Code generation for errors.

d566969

Take revert statement into account in control flow graph.

1057fd5

Use all referenced errors.

e877e2b

Skip certain test for grammar test.

d80059f

chriseth force-pushed the revertStatement branch from 002f79f to d80059f Compare March 30, 2021 19:15

Base automatically changed from customErrorDeclaration to develop March 30, 2021 21:03

chriseth merged commit 4fd6192 into develop Mar 30, 2021

chriseth deleted the revertStatement branch March 30, 2021 21:03

Xanewok added a commit to Xanewok/slang that referenced this pull request Sep 19, 2023

fix: Mark RevertStatement as introduced in 0.8.4

510d8cf

It was introduced alongside the `revert` contextual keyword, see ethereum/solidity#11037.

Xanewok added a commit to Xanewok/slang that referenced this pull request Sep 21, 2023

fix: Mark Revert{Keyword,Statement} as introduced in 0.8.4

be71af9

The statement was introduced alongside the `revert` contextual keyword, see ethereum/solidity#11037.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert statement. #11037

Revert statement. #11037

chriseth commented Mar 3, 2021

mijovic left a comment

cameel left a comment

cameel left a comment

mijovic Mar 30, 2021

mijovic left a comment

hrkrshnn commented Mar 30, 2021

chriseth commented Mar 30, 2021

chriseth commented Mar 30, 2021

aarlt Mar 30, 2021

cameel Mar 30, 2021

chriseth Mar 30, 2021

hrkrshnn commented Mar 30, 2021 •

edited

Loading

aarlt Mar 30, 2021

chriseth Mar 30, 2021

chriseth commented Mar 30, 2021

chriseth commented Mar 30, 2021

Revert statement. #11037

Revert statement. #11037

Conversation

chriseth commented Mar 3, 2021

mijovic left a comment

Choose a reason for hiding this comment

cameel left a comment

Choose a reason for hiding this comment

cameel left a comment

Choose a reason for hiding this comment

mijovic Mar 30, 2021

Choose a reason for hiding this comment

mijovic left a comment

Choose a reason for hiding this comment

hrkrshnn commented Mar 30, 2021

chriseth commented Mar 30, 2021

chriseth commented Mar 30, 2021

aarlt Mar 30, 2021

Choose a reason for hiding this comment

cameel Mar 30, 2021

Choose a reason for hiding this comment

chriseth Mar 30, 2021

Choose a reason for hiding this comment

hrkrshnn commented Mar 30, 2021 • edited Loading

aarlt Mar 30, 2021

Choose a reason for hiding this comment

chriseth Mar 30, 2021

Choose a reason for hiding this comment

chriseth commented Mar 30, 2021

chriseth commented Mar 30, 2021

hrkrshnn commented Mar 30, 2021 •

edited

Loading