From fb67051467988f787f3e614b7613c9b0ded5c1d9 Mon Sep 17 00:00:00 2001 From: chriseth Date: Wed, 24 Feb 2021 10:55:49 +0100 Subject: [PATCH 1/7] Revert statement. --- libsolidity/analysis/PostTypeChecker.cpp | 67 +++++++++++++------ libsolidity/analysis/PostTypeChecker.h | 3 + libsolidity/analysis/TypeChecker.cpp | 12 ++++ libsolidity/analysis/TypeChecker.h | 1 + libsolidity/ast/AST.h | 25 +++++++ libsolidity/ast/ASTVisitor.h | 4 ++ libsolidity/ast/AST_accept.h | 14 ++++ libsolidity/parsing/Parser.cpp | 46 +++++++++++-- libsolidity/parsing/Parser.h | 1 + .../semanticTests/errors/panic_via_import.sol | 18 +++++ .../semanticTests/errors/using_structs.sol | 20 ++++++ .../syntaxTests/errors/require_custom.sol | 8 +++ .../syntaxTests/errors/revert_parentheses.sol | 8 +++ .../revertStatement/at_file_level.sol | 3 + .../revertStatement/error_used_elsewhere.sol | 6 ++ .../revertStatement/in_global_function.sol | 4 ++ .../revertStatement/non_called.sol | 6 ++ .../syntaxTests/revertStatement/non_error.sol | 5 ++ .../syntaxTests/revertStatement/regular.sol | 6 ++ .../revertStatement/revert_event.sol | 8 +++ .../revertStatement/revert_revert.sol | 8 +++ .../syntaxTests/revertStatement/scoped.sol | 8 +++ .../revertStatement/using_function.sol | 9 +++ .../revertStatement/using_struct.sol | 8 +++ 24 files changed, 272 insertions(+), 26 deletions(-) create mode 100644 test/libsolidity/semanticTests/errors/panic_via_import.sol create mode 100644 test/libsolidity/semanticTests/errors/using_structs.sol create mode 100644 test/libsolidity/syntaxTests/errors/require_custom.sol create mode 100644 test/libsolidity/syntaxTests/errors/revert_parentheses.sol create mode 100644 test/libsolidity/syntaxTests/revertStatement/at_file_level.sol create mode 100644 test/libsolidity/syntaxTests/revertStatement/error_used_elsewhere.sol create mode 100644 test/libsolidity/syntaxTests/revertStatement/in_global_function.sol create mode 100644 test/libsolidity/syntaxTests/revertStatement/non_called.sol create mode 100644 test/libsolidity/syntaxTests/revertStatement/non_error.sol create mode 100644 test/libsolidity/syntaxTests/revertStatement/regular.sol create mode 100644 test/libsolidity/syntaxTests/revertStatement/revert_event.sol create mode 100644 test/libsolidity/syntaxTests/revertStatement/revert_revert.sol create mode 100644 test/libsolidity/syntaxTests/revertStatement/scoped.sol create mode 100644 test/libsolidity/syntaxTests/revertStatement/using_function.sol create mode 100644 test/libsolidity/syntaxTests/revertStatement/using_struct.sol diff --git a/libsolidity/analysis/PostTypeChecker.cpp b/libsolidity/analysis/PostTypeChecker.cpp index 3294088328ea..e5c3ec2f83b4 100644 --- a/libsolidity/analysis/PostTypeChecker.cpp +++ b/libsolidity/analysis/PostTypeChecker.cpp @@ -86,6 +86,16 @@ void PostTypeChecker::endVisit(EmitStatement const& _emit) callEndVisit(_emit); } +bool PostTypeChecker::visit(RevertStatement const& _revert) +{ + return callVisit(_revert); +} + +void PostTypeChecker::endVisit(RevertStatement const& _revert) +{ + callEndVisit(_revert); +} + bool PostTypeChecker::visit(FunctionCall const& _functionCall) { return callVisit(_functionCall); @@ -281,42 +291,59 @@ struct ModifierContextChecker: public PostTypeChecker::Checker bool m_insideModifierInvocation = false; }; -struct EventOutsideEmitChecker: public PostTypeChecker::Checker +struct EventOutsideEmitErrorOutsideRevertChecker: public PostTypeChecker::Checker { - EventOutsideEmitChecker(ErrorReporter& _errorReporter): + EventOutsideEmitErrorOutsideRevertChecker(ErrorReporter& _errorReporter): Checker(_errorReporter) {} - bool visit(EmitStatement const&) override + bool visit(EmitStatement const& _emitStatement) override { - m_insideEmitStatement = true; + m_currentStatement = &_emitStatement; return true; } void endVisit(EmitStatement const&) override { - m_insideEmitStatement = true; + m_currentStatement = nullptr; } - bool visit(FunctionCall const& _functionCall) override + bool visit(RevertStatement const& _revertStatement) override { - if (*_functionCall.annotation().kind != FunctionCallKind::FunctionCall) - return true; + m_currentStatement = &_revertStatement; + return true; + } - if (FunctionTypePointer const functionType = dynamic_cast(_functionCall.expression().annotation().type)) - // Check for event outside of emit statement - if (!m_insideEmitStatement && functionType->kind() == FunctionType::Kind::Event) - m_errorReporter.typeError( - 3132_error, - _functionCall.location(), - "Event invocations have to be prefixed by \"emit\"." - ); + void endVisit(RevertStatement const&) override + { + m_currentStatement = nullptr; + } + + bool visit(FunctionCall const& _functionCall) override + { + if (*_functionCall.annotation().kind == FunctionCallKind::FunctionCall) + if (auto const* functionType = dynamic_cast(_functionCall.expression().annotation().type)) + { + // Check for event outside of emit statement + if (!dynamic_cast(m_currentStatement) && functionType->kind() == FunctionType::Kind::Event) + m_errorReporter.typeError( + 3132_error, + _functionCall.location(), + "Event invocations have to be prefixed by \"emit\"." + ); + else if (!dynamic_cast(m_currentStatement) && functionType->kind() == FunctionType::Kind::Error) + m_errorReporter.typeError( + 7757_error, + _functionCall.location(), + "Errors can only be used with revert statements: \"revert MyError();\"." + ); + } + m_currentStatement = nullptr; return true; } private: - /// Flag indicating whether we are currently inside an EmitStatement. - bool m_insideEmitStatement = false; + Statement const* m_currentStatement = nullptr; }; struct NoVariablesInInterfaceChecker: public PostTypeChecker::Checker @@ -395,14 +422,16 @@ struct ReservedErrorSelector: public PostTypeChecker::Checker } } }; + } + PostTypeChecker::PostTypeChecker(langutil::ErrorReporter& _errorReporter): m_errorReporter(_errorReporter) { m_checkers.push_back(make_shared(_errorReporter)); m_checkers.push_back(make_shared(_errorReporter)); m_checkers.push_back(make_shared(_errorReporter)); - m_checkers.push_back(make_shared(_errorReporter)); + m_checkers.push_back(make_shared(_errorReporter)); m_checkers.push_back(make_shared(_errorReporter)); m_checkers.push_back(make_shared(_errorReporter)); } diff --git a/libsolidity/analysis/PostTypeChecker.h b/libsolidity/analysis/PostTypeChecker.h index f35330fa1fae..04b4fbe7a29e 100644 --- a/libsolidity/analysis/PostTypeChecker.h +++ b/libsolidity/analysis/PostTypeChecker.h @@ -83,6 +83,9 @@ class PostTypeChecker: private ASTConstVisitor bool visit(EmitStatement const& _emit) override; void endVisit(EmitStatement const& _emit) override; + bool visit(RevertStatement const& _revert) override; + void endVisit(RevertStatement const& _revert) override; + bool visit(FunctionCall const& _functionCall) override; bool visit(Identifier const& _identifier) override; diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp index 8c99c8396020..f5aecd252ef3 100644 --- a/libsolidity/analysis/TypeChecker.cpp +++ b/libsolidity/analysis/TypeChecker.cpp @@ -1167,6 +1167,18 @@ void TypeChecker::endVisit(EmitStatement const& _emit) m_errorReporter.typeError(9292_error, _emit.eventCall().expression().location(), "Expression has to be an event invocation."); } +void TypeChecker::endVisit(RevertStatement const& _revert) +{ + FunctionCall const& errorCall = _revert.errorCall(); + if ( + *errorCall.annotation().kind != FunctionCallKind::FunctionCall || + type(errorCall.expression())->category() != Type::Category::Function || + dynamic_cast(*type(errorCall.expression())).kind() != FunctionType::Kind::Error + ) + m_errorReporter.typeError(1885_error, errorCall.expression().location(), "Expression has to be an error."); +} + + bool TypeChecker::visit(VariableDeclarationStatement const& _statement) { if (!_statement.initialValue()) diff --git a/libsolidity/analysis/TypeChecker.h b/libsolidity/analysis/TypeChecker.h index efbf99adaf71..a9604bdec966 100644 --- a/libsolidity/analysis/TypeChecker.h +++ b/libsolidity/analysis/TypeChecker.h @@ -134,6 +134,7 @@ class TypeChecker: private ASTConstVisitor bool visit(ForStatement const& _forStatement) override; void endVisit(Return const& _return) override; void endVisit(EmitStatement const& _emit) override; + void endVisit(RevertStatement const& _revert) override; bool visit(VariableDeclarationStatement const& _variable) override; void endVisit(ExpressionStatement const& _statement) override; bool visit(Conditional const& _conditional) override; diff --git a/libsolidity/ast/AST.h b/libsolidity/ast/AST.h index 1f36875e5105..6d58f504a52c 100644 --- a/libsolidity/ast/AST.h +++ b/libsolidity/ast/AST.h @@ -1726,6 +1726,31 @@ class Throw: public Statement void accept(ASTConstVisitor& _visitor) const override; }; +/** + * The revert statement is used to revert state changes and return error data. + */ +class RevertStatement: public Statement +{ +public: + explicit RevertStatement( + int64_t _id, + SourceLocation const& _location, + ASTPointer const& _docString, + ASTPointer _functionCall + ): + Statement(_id, _location, _docString), m_errorCall(std::move(_functionCall)) + { + solAssert(m_errorCall, ""); + } + void accept(ASTVisitor& _visitor) override; + void accept(ASTConstVisitor& _visitor) const override; + + FunctionCall const& errorCall() const { return *m_errorCall; } + +private: + ASTPointer m_errorCall; +}; + /** * The emit statement is used to emit events: emit EventName(arg1, ..., argn) */ diff --git a/libsolidity/ast/ASTVisitor.h b/libsolidity/ast/ASTVisitor.h index 4acb472736a7..398387b33b4a 100644 --- a/libsolidity/ast/ASTVisitor.h +++ b/libsolidity/ast/ASTVisitor.h @@ -90,6 +90,7 @@ class ASTVisitor virtual bool visit(Return& _node) { return visitNode(_node); } virtual bool visit(Throw& _node) { return visitNode(_node); } virtual bool visit(EmitStatement& _node) { return visitNode(_node); } + virtual bool visit(RevertStatement& _node) { return visitNode(_node); } virtual bool visit(VariableDeclarationStatement& _node) { return visitNode(_node); } virtual bool visit(ExpressionStatement& _node) { return visitNode(_node); } virtual bool visit(Conditional& _node) { return visitNode(_node); } @@ -144,6 +145,7 @@ class ASTVisitor virtual void endVisit(Return& _node) { endVisitNode(_node); } virtual void endVisit(Throw& _node) { endVisitNode(_node); } virtual void endVisit(EmitStatement& _node) { endVisitNode(_node); } + virtual void endVisit(RevertStatement& _node) { endVisitNode(_node); } virtual void endVisit(VariableDeclarationStatement& _node) { endVisitNode(_node); } virtual void endVisit(ExpressionStatement& _node) { endVisitNode(_node); } virtual void endVisit(Conditional& _node) { endVisitNode(_node); } @@ -220,6 +222,7 @@ class ASTConstVisitor virtual bool visit(Return const& _node) { return visitNode(_node); } virtual bool visit(Throw const& _node) { return visitNode(_node); } virtual bool visit(EmitStatement const& _node) { return visitNode(_node); } + virtual bool visit(RevertStatement const& _node) { return visitNode(_node); } virtual bool visit(VariableDeclarationStatement const& _node) { return visitNode(_node); } virtual bool visit(ExpressionStatement const& _node) { return visitNode(_node); } virtual bool visit(Conditional const& _node) { return visitNode(_node); } @@ -274,6 +277,7 @@ class ASTConstVisitor virtual void endVisit(Return const& _node) { endVisitNode(_node); } virtual void endVisit(Throw const& _node) { endVisitNode(_node); } virtual void endVisit(EmitStatement const& _node) { endVisitNode(_node); } + virtual void endVisit(RevertStatement const& _node) { endVisitNode(_node); } virtual void endVisit(VariableDeclarationStatement const& _node) { endVisitNode(_node); } virtual void endVisit(ExpressionStatement const& _node) { endVisitNode(_node); } virtual void endVisit(Conditional const& _node) { endVisitNode(_node); } diff --git a/libsolidity/ast/AST_accept.h b/libsolidity/ast/AST_accept.h index 678a9ef7e5c9..075e6fe5c136 100644 --- a/libsolidity/ast/AST_accept.h +++ b/libsolidity/ast/AST_accept.h @@ -682,6 +682,20 @@ void Throw::accept(ASTConstVisitor& _visitor) const _visitor.endVisit(*this); } +void RevertStatement::accept(ASTVisitor& _visitor) +{ + if (_visitor.visit(*this)) + m_errorCall->accept(_visitor); + _visitor.endVisit(*this); +} + +void RevertStatement::accept(ASTConstVisitor& _visitor) const +{ + if (_visitor.visit(*this)) + m_errorCall->accept(_visitor); + _visitor.endVisit(*this); +} + void EmitStatement::accept(ASTVisitor& _visitor) { if (_visitor.visit(*this)) diff --git a/libsolidity/parsing/Parser.cpp b/libsolidity/parsing/Parser.cpp index e051343102bc..10d396c4c462 100644 --- a/libsolidity/parsing/Parser.cpp +++ b/libsolidity/parsing/Parser.cpp @@ -1250,11 +1250,13 @@ ASTPointer Parser::parseStatement(bool _allowUnchecked) statement = parseEmitStatement(docString); break; case Token::Identifier: - if (m_insideModifier && m_scanner->currentLiteral() == "_") - { - statement = ASTNodeFactory(*this).createNode(docString); - m_scanner->next(); - } + if (m_scanner->currentLiteral() == "revert" && m_scanner->peekNextToken() == Token::Identifier) + statement = parseRevertStatement(docString); + else if (m_insideModifier && m_scanner->currentLiteral() == "_") + { + statement = ASTNodeFactory(*this).createNode(docString); + m_scanner->next(); + } else statement = parseSimpleStatement(docString); break; @@ -1474,8 +1476,38 @@ ASTPointer Parser::parseEmitStatement(ASTPointer const nodeFactory.markEndPosition(); expectToken(Token::RParen); auto eventCall = eventCallNodeFactory.createNode(eventName, arguments, names); - auto statement = nodeFactory.createNode(_docString, eventCall); - return statement; + return nodeFactory.createNode(_docString, eventCall); +} + +ASTPointer Parser::parseRevertStatement(ASTPointer const& _docString) +{ + ASTNodeFactory nodeFactory(*this); + solAssert(*expectIdentifierToken() == "revert", ""); + + ASTNodeFactory errorCallNodeFactory(*this); + + solAssert(m_scanner->currentToken() == Token::Identifier, ""); + + IndexAccessedPath iap; + while (true) + { + iap.path.push_back(parseIdentifier()); + if (m_scanner->currentToken() != Token::Period) + break; + m_scanner->next(); + } + + auto errorName = expressionFromIndexAccessStructure(iap); + expectToken(Token::LParen); + + vector> arguments; + vector> names; + std::tie(arguments, names) = parseFunctionCallArguments(); + errorCallNodeFactory.markEndPosition(); + nodeFactory.markEndPosition(); + expectToken(Token::RParen); + auto errorCall = errorCallNodeFactory.createNode(errorName, arguments, names); + return nodeFactory.createNode(_docString, errorCall); } ASTPointer Parser::parseSimpleStatement(ASTPointer const& _docString) diff --git a/libsolidity/parsing/Parser.h b/libsolidity/parsing/Parser.h index dd9125e5bd76..559f804c2c3d 100644 --- a/libsolidity/parsing/Parser.h +++ b/libsolidity/parsing/Parser.h @@ -127,6 +127,7 @@ class Parser: public langutil::ParserBase ASTPointer parseDoWhileStatement(ASTPointer const& _docString); ASTPointer parseForStatement(ASTPointer const& _docString); ASTPointer parseEmitStatement(ASTPointer const& docString); + ASTPointer parseRevertStatement(ASTPointer const& docString); /// A "simple statement" can be a variable declaration statement or an expression statement. ASTPointer parseSimpleStatement(ASTPointer const& _docString); ASTPointer parseVariableDeclarationStatement( diff --git a/test/libsolidity/semanticTests/errors/panic_via_import.sol b/test/libsolidity/semanticTests/errors/panic_via_import.sol new file mode 100644 index 000000000000..bdd8d5e4b8ba --- /dev/null +++ b/test/libsolidity/semanticTests/errors/panic_via_import.sol @@ -0,0 +1,18 @@ +==== Source: s1.sol ==== +error E(uint); +==== Source: s2.sol ==== +import { E as Panic } from "s1.sol"; +contract C { + error E(uint); + function a() public pure { + revert Panic(1); + } + function b() public pure { + revert E(1); + } +} +// ==== +// compileViaYul: also +// ---- +// a() -> FAILURE, hex"002ff067", hex"0000000000000000000000000000000000000000000000000000000000000001" +// b() -> FAILURE, hex"002ff067", hex"0000000000000000000000000000000000000000000000000000000000000001" \ No newline at end of file diff --git a/test/libsolidity/semanticTests/errors/using_structs.sol b/test/libsolidity/semanticTests/errors/using_structs.sol new file mode 100644 index 000000000000..c9422c4ac703 --- /dev/null +++ b/test/libsolidity/semanticTests/errors/using_structs.sol @@ -0,0 +1,20 @@ +pragma abicoder v2; +struct S { uint a; string b; } +error E(uint a, S s, uint b); +contract C { + S s; + function f(bool c) public { + if (c) { + s.a = 9; + s.b = "abc"; + revert E(2, s, 7); + } else { + revert E({b: 7, a: 2, s: S({b: "abc", a: 9})}); + } + } +} +// ==== +// compileViaYul: also +// ---- +// f(bool): true -> FAILURE, hex"e96e07f0", hex"0000000000000000000000000000000000000000000000000000000000000002", hex"0000000000000000000000000000000000000000000000000000000000000060", hex"0000000000000000000000000000000000000000000000000000000000000007", hex"0000000000000000000000000000000000000000000000000000000000000009", hex"0000000000000000000000000000000000000000000000000000000000000040", hex"0000000000000000000000000000000000000000000000000000000000000003", hex"6162630000000000000000000000000000000000000000000000000000000000" +// f(bool): false -> FAILURE, hex"e96e07f0", hex"0000000000000000000000000000000000000000000000000000000000000002", hex"0000000000000000000000000000000000000000000000000000000000000060", hex"0000000000000000000000000000000000000000000000000000000000000007", hex"0000000000000000000000000000000000000000000000000000000000000009", hex"0000000000000000000000000000000000000000000000000000000000000040", hex"0000000000000000000000000000000000000000000000000000000000000003", hex"6162630000000000000000000000000000000000000000000000000000000000" diff --git a/test/libsolidity/syntaxTests/errors/require_custom.sol b/test/libsolidity/syntaxTests/errors/require_custom.sol new file mode 100644 index 000000000000..f139a4fe5a7c --- /dev/null +++ b/test/libsolidity/syntaxTests/errors/require_custom.sol @@ -0,0 +1,8 @@ +error E(uint a, uint b); +contract C { + function f(bool c) public pure { + require(c, E(2, 7)); + } +} +// ---- +// TypeError 9322: (83-90): No matching declaration found after argument-dependent lookup. diff --git a/test/libsolidity/syntaxTests/errors/revert_parentheses.sol b/test/libsolidity/syntaxTests/errors/revert_parentheses.sol new file mode 100644 index 000000000000..27c7044e5d84 --- /dev/null +++ b/test/libsolidity/syntaxTests/errors/revert_parentheses.sol @@ -0,0 +1,8 @@ +error E(uint a, uint b); +contract C { + function f() public pure { + revert(E(2, 7)); + } +} +// ---- +// TypeError 9322: (77-83): No matching declaration found after argument-dependent lookup. diff --git a/test/libsolidity/syntaxTests/revertStatement/at_file_level.sol b/test/libsolidity/syntaxTests/revertStatement/at_file_level.sol new file mode 100644 index 000000000000..b2d53bd8bc71 --- /dev/null +++ b/test/libsolidity/syntaxTests/revertStatement/at_file_level.sol @@ -0,0 +1,3 @@ +revert X(); +// ---- +// ParserError 2314: (8-9): Expected ';' but got '(' diff --git a/test/libsolidity/syntaxTests/revertStatement/error_used_elsewhere.sol b/test/libsolidity/syntaxTests/revertStatement/error_used_elsewhere.sol new file mode 100644 index 000000000000..208a7326be14 --- /dev/null +++ b/test/libsolidity/syntaxTests/revertStatement/error_used_elsewhere.sol @@ -0,0 +1,6 @@ +error E(); +function f() pure { + E(); +} +// ---- +// TypeError 7757: (35-38): Errors can only be used with revert statements: "revert MyError();". diff --git a/test/libsolidity/syntaxTests/revertStatement/in_global_function.sol b/test/libsolidity/syntaxTests/revertStatement/in_global_function.sol new file mode 100644 index 000000000000..a8ea0e444f4e --- /dev/null +++ b/test/libsolidity/syntaxTests/revertStatement/in_global_function.sol @@ -0,0 +1,4 @@ +error E(); +function f() pure { + revert E(); +} diff --git a/test/libsolidity/syntaxTests/revertStatement/non_called.sol b/test/libsolidity/syntaxTests/revertStatement/non_called.sol new file mode 100644 index 000000000000..63ecfbc6e01e --- /dev/null +++ b/test/libsolidity/syntaxTests/revertStatement/non_called.sol @@ -0,0 +1,6 @@ +error E(); +function f() public pure { + revert E; +} +// ---- +// ParserError 2314: (50-51): Expected '(' but got ';' diff --git a/test/libsolidity/syntaxTests/revertStatement/non_error.sol b/test/libsolidity/syntaxTests/revertStatement/non_error.sol new file mode 100644 index 000000000000..329b68f938ea --- /dev/null +++ b/test/libsolidity/syntaxTests/revertStatement/non_error.sol @@ -0,0 +1,5 @@ +function f() public pure { + revert 1; +} +// ---- +// ParserError 2314: (38-39): Expected ';' but got 'Number' diff --git a/test/libsolidity/syntaxTests/revertStatement/regular.sol b/test/libsolidity/syntaxTests/revertStatement/regular.sol new file mode 100644 index 000000000000..57de7a0e4136 --- /dev/null +++ b/test/libsolidity/syntaxTests/revertStatement/regular.sol @@ -0,0 +1,6 @@ +error E(); +contract C { + function f() public pure { + revert E(); + } +} \ No newline at end of file diff --git a/test/libsolidity/syntaxTests/revertStatement/revert_event.sol b/test/libsolidity/syntaxTests/revertStatement/revert_event.sol new file mode 100644 index 000000000000..6b2832c876ff --- /dev/null +++ b/test/libsolidity/syntaxTests/revertStatement/revert_event.sol @@ -0,0 +1,8 @@ +contract C { + event E(); + function f() public pure { + revert E(); + } +} +// ---- +// TypeError 1885: (74-75): Expression has to be an error. diff --git a/test/libsolidity/syntaxTests/revertStatement/revert_revert.sol b/test/libsolidity/syntaxTests/revertStatement/revert_revert.sol new file mode 100644 index 000000000000..f3b1e080551d --- /dev/null +++ b/test/libsolidity/syntaxTests/revertStatement/revert_revert.sol @@ -0,0 +1,8 @@ +error revert(); +contract C { + function f() public pure { + revert revert(); + } +} +// ---- +// Warning 2319: (0-15): This declaration shadows a builtin symbol. diff --git a/test/libsolidity/syntaxTests/revertStatement/scoped.sol b/test/libsolidity/syntaxTests/revertStatement/scoped.sol new file mode 100644 index 000000000000..ec8b6e601fd0 --- /dev/null +++ b/test/libsolidity/syntaxTests/revertStatement/scoped.sol @@ -0,0 +1,8 @@ +contract A { + error E(); +} +contract C { + function f() public pure { + revert A.E(); + } +} \ No newline at end of file diff --git a/test/libsolidity/syntaxTests/revertStatement/using_function.sol b/test/libsolidity/syntaxTests/revertStatement/using_function.sol new file mode 100644 index 000000000000..e10384cd75b4 --- /dev/null +++ b/test/libsolidity/syntaxTests/revertStatement/using_function.sol @@ -0,0 +1,9 @@ +error f(uint, uint); +contract C { + function f(uint) public { + revert f(10); + } +} +// ---- +// Warning 2519: (38-91): This declaration shadows an existing declaration. +// TypeError 1885: (79-80): Expression has to be an error. diff --git a/test/libsolidity/syntaxTests/revertStatement/using_struct.sol b/test/libsolidity/syntaxTests/revertStatement/using_struct.sol new file mode 100644 index 000000000000..f04b0d598af6 --- /dev/null +++ b/test/libsolidity/syntaxTests/revertStatement/using_struct.sol @@ -0,0 +1,8 @@ +struct S { uint x; } +contract C { + function f() public { + revert S(10); + } +} +// ---- +// TypeError 1885: (75-76): Expression has to be an error. From b552e5aeeb993f08b2857e2497e3737f5b6abdc5 Mon Sep 17 00:00:00 2001 From: chriseth Date: Tue, 16 Mar 2021 19:17:57 +0100 Subject: [PATCH 2/7] AST import and export for revert statement. --- libsolidity/ast/ASTJsonConverter.cpp | 8 ++++++++ libsolidity/ast/ASTJsonConverter.h | 1 + libsolidity/ast/ASTJsonImporter.cpp | 11 +++++++++++ libsolidity/ast/ASTJsonImporter.h | 1 + 4 files changed, 21 insertions(+) diff --git a/libsolidity/ast/ASTJsonConverter.cpp b/libsolidity/ast/ASTJsonConverter.cpp index 40f72bde796d..ae8d57baf107 100644 --- a/libsolidity/ast/ASTJsonConverter.cpp +++ b/libsolidity/ast/ASTJsonConverter.cpp @@ -675,6 +675,14 @@ bool ASTJsonConverter::visit(EmitStatement const& _node) return false; } +bool ASTJsonConverter::visit(RevertStatement const& _node) +{ + setJsonNode(_node, "RevertStatement", { + make_pair("errorCall", toJson(_node.errorCall())) + }); + return false; +} + bool ASTJsonConverter::visit(VariableDeclarationStatement const& _node) { Json::Value varDecs(Json::arrayValue); diff --git a/libsolidity/ast/ASTJsonConverter.h b/libsolidity/ast/ASTJsonConverter.h index 90474900443b..34903c7a17b3 100644 --- a/libsolidity/ast/ASTJsonConverter.h +++ b/libsolidity/ast/ASTJsonConverter.h @@ -107,6 +107,7 @@ class ASTJsonConverter: public ASTConstVisitor bool visit(Return const& _node) override; bool visit(Throw const& _node) override; bool visit(EmitStatement const& _node) override; + bool visit(RevertStatement const& _node) override; bool visit(VariableDeclarationStatement const& _node) override; bool visit(ExpressionStatement const& _node) override; bool visit(Conditional const& _node) override; diff --git a/libsolidity/ast/ASTJsonImporter.cpp b/libsolidity/ast/ASTJsonImporter.cpp index cb58885f7111..0fa88ae3b6c7 100644 --- a/libsolidity/ast/ASTJsonImporter.cpp +++ b/libsolidity/ast/ASTJsonImporter.cpp @@ -191,6 +191,8 @@ ASTPointer ASTJsonImporter::convertJsonToASTNode(Json::Value const& _js return createReturn(_json); if (nodeType == "EmitStatement") return createEmitStatement(_json); + if (nodeType == "RevertStatement") + return createRevertStatement(_json); if (nodeType == "Throw") return createThrow(_json); if (nodeType == "VariableDeclarationStatement") @@ -747,6 +749,15 @@ ASTPointer ASTJsonImporter::createEmitStatement(Json::Value const ); } +ASTPointer ASTJsonImporter::createRevertStatement(Json::Value const& _node) +{ + return createASTNode( + _node, + nullOrASTString(_node, "documentation"), + createFunctionCall(member(_node, "errorCall")) + ); +} + ASTPointer ASTJsonImporter::createVariableDeclarationStatement(Json::Value const& _node) { std::vector> variables; diff --git a/libsolidity/ast/ASTJsonImporter.h b/libsolidity/ast/ASTJsonImporter.h index 3ef6db384931..a4d43293fc61 100644 --- a/libsolidity/ast/ASTJsonImporter.h +++ b/libsolidity/ast/ASTJsonImporter.h @@ -107,6 +107,7 @@ class ASTJsonImporter ASTPointer createReturn(Json::Value const& _node); ASTPointer createThrow(Json::Value const& _node); ASTPointer createEmitStatement(Json::Value const& _node); + ASTPointer createRevertStatement(Json::Value const& _node); ASTPointer createVariableDeclarationStatement(Json::Value const& _node); ASTPointer createExpressionStatement(Json::Value const& _node); ASTPointer createConditional(Json::Value const& _node); From 3353107779169dca8430a5c77506822bf7868b96 Mon Sep 17 00:00:00 2001 From: chriseth Date: Tue, 9 Mar 2021 16:50:29 +0100 Subject: [PATCH 3/7] Grammar for revert statement. --- docs/grammar/Solidity.g4 | 7 ++++++- docs/grammar/SolidityLexer.g4 | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/docs/grammar/Solidity.g4 b/docs/grammar/Solidity.g4 index bd2d1ad5ab96..69da6b4056ee 100644 --- a/docs/grammar/Solidity.g4 +++ b/docs/grammar/Solidity.g4 @@ -381,7 +381,7 @@ inlineArrayExpression: LBrack (expression ( Comma expression)* ) RBrack; /** * Besides regular non-keyword Identifiers, some keywords like 'from' and 'error' can also be used as identifiers. */ -identifier: Identifier | From | Error; +identifier: Identifier | From | Error | Revert; literal: stringLiteral | numberLiteral | booleanLiteral | hexStringLiteral | unicodeStringLiteral; booleanLiteral: True | False; @@ -422,6 +422,7 @@ statement: | tryStatement | returnStatement | emitStatement + | revertStatement | assemblyStatement ; @@ -459,6 +460,10 @@ returnStatement: Return expression? Semicolon; * An emit statement. The contained expression needs to refer to an event. */ emitStatement: Emit expression callArgumentList Semicolon; +/** + * A revert statement. The contained expression needs to refer to an error. + */ +revertStatement: Revert expression callArgumentList Semicolon; /** * An inline assembly block. * The contents of an inline assembly block use a separate scanner/lexer, i.e. the set of keywords and diff --git a/docs/grammar/SolidityLexer.g4 b/docs/grammar/SolidityLexer.g4 index 560d78e6abb8..6b5209c3e05f 100644 --- a/docs/grammar/SolidityLexer.g4 +++ b/docs/grammar/SolidityLexer.g4 @@ -30,6 +30,7 @@ Else: 'else'; Emit: 'emit'; Enum: 'enum'; Error: 'error'; // not a real keyword +Revert: 'revert'; // not a real keyword Event: 'event'; External: 'external'; Fallback: 'fallback'; From d5669696d51a039315009c5e8c548ce081753436 Mon Sep 17 00:00:00 2001 From: chriseth Date: Thu, 28 Jan 2021 12:56:22 +0100 Subject: [PATCH 4/7] Code generation for errors. --- libsolidity/ast/ASTUtils.h | 4 - libsolidity/codegen/CompilerUtils.cpp | 16 ++++ libsolidity/codegen/CompilerUtils.h | 6 ++ libsolidity/codegen/ContractCompiler.cpp | 9 ++ libsolidity/codegen/ContractCompiler.h | 1 + libsolidity/codegen/ExpressionCompiler.cpp | 17 +++- .../codegen/ir/IRGeneratorForStatements.cpp | 88 +++++++++++-------- .../codegen/ir/IRGeneratorForStatements.h | 9 ++ .../errors/error_in_library_and_interface.sol | 24 +++++ .../semanticTests/errors/named_error_args.sol | 10 +++ .../errors/revert_conversion.sol | 12 +++ .../semanticTests/errors/simple.sol | 10 +++ .../errors/via_contract_type.sol | 18 ++++ .../semanticTests/errors/via_import.sol | 25 ++++++ .../semanticTests/errors/weird_name.sol | 10 +++ .../semanticTests/reverts/error_struct.sol | 17 ++++ 16 files changed, 236 insertions(+), 40 deletions(-) create mode 100644 test/libsolidity/semanticTests/errors/error_in_library_and_interface.sol create mode 100644 test/libsolidity/semanticTests/errors/named_error_args.sol create mode 100644 test/libsolidity/semanticTests/errors/revert_conversion.sol create mode 100644 test/libsolidity/semanticTests/errors/simple.sol create mode 100644 test/libsolidity/semanticTests/errors/via_contract_type.sol create mode 100644 test/libsolidity/semanticTests/errors/via_import.sol create mode 100644 test/libsolidity/semanticTests/errors/weird_name.sol create mode 100644 test/libsolidity/semanticTests/reverts/error_struct.sol diff --git a/libsolidity/ast/ASTUtils.h b/libsolidity/ast/ASTUtils.h index 44caa24ddf35..5b1a7d4e5225 100644 --- a/libsolidity/ast/ASTUtils.h +++ b/libsolidity/ast/ASTUtils.h @@ -25,10 +25,6 @@ class VariableDeclaration; class Declaration; class Expression; -/// @returns the declaration referenced from the expression which has to be MemberAccess -/// or Identifier. Returns nullptr otherwise. -Declaration const* referencedDeclaration(Expression const& _expression); - /// Find the topmost referenced constant variable declaration when the given variable /// declaration value is an identifier. Works only for constant variable declarations. /// Returns nullptr if an identifier in the chain is not referencing a constant variable declaration. diff --git a/libsolidity/codegen/CompilerUtils.cpp b/libsolidity/codegen/CompilerUtils.cpp index c11c635c714b..01143bf42578 100644 --- a/libsolidity/codegen/CompilerUtils.cpp +++ b/libsolidity/codegen/CompilerUtils.cpp @@ -103,6 +103,22 @@ void CompilerUtils::revertWithStringData(Type const& _argumentType) m_context << Instruction::REVERT; } +void CompilerUtils::revertWithError( + string const& _signature, + vector const& _parameterTypes, + vector const& _argumentTypes +) +{ + fetchFreeMemoryPointer(); + m_context << util::selectorFromSignature(_signature); + m_context << Instruction::DUP2 << Instruction::MSTORE; + m_context << u256(4) << Instruction::ADD; + // Stack: + abiEncode(_argumentTypes, _parameterTypes); + toSizeAfterFreeMemoryPointer(); + m_context << Instruction::REVERT; +} + void CompilerUtils::returnDataToArray() { if (m_context.evmVersion().supportsReturndata()) diff --git a/libsolidity/codegen/CompilerUtils.h b/libsolidity/codegen/CompilerUtils.h index 7bc26d971f99..0930ff1c877e 100644 --- a/libsolidity/codegen/CompilerUtils.h +++ b/libsolidity/codegen/CompilerUtils.h @@ -69,6 +69,12 @@ class CompilerUtils /// Stack post: void revertWithStringData(Type const& _argumentType); + void revertWithError( + std::string const& _errorName, + std::vector const& _parameterTypes, + std::vector const& _argumentTypes + ); + /// Allocates a new array and copies the return data to it. /// If the EVM does not support return data, creates an empty array. void returnDataToArray(); diff --git a/libsolidity/codegen/ContractCompiler.cpp b/libsolidity/codegen/ContractCompiler.cpp index 843dc3f6cef6..10604b4f2c80 100644 --- a/libsolidity/codegen/ContractCompiler.cpp +++ b/libsolidity/codegen/ContractCompiler.cpp @@ -1281,6 +1281,15 @@ bool ContractCompiler::visit(EmitStatement const& _emit) return false; } +bool ContractCompiler::visit(RevertStatement const& _revert) +{ + CompilerContext::LocationSetter locationSetter(m_context, _revert); + StackHeightChecker checker(m_context); + compileExpression(_revert.errorCall()); + checker.check(); + return false; +} + bool ContractCompiler::visit(VariableDeclarationStatement const& _variableDeclarationStatement) { CompilerContext::LocationSetter locationSetter(m_context, _variableDeclarationStatement); diff --git a/libsolidity/codegen/ContractCompiler.h b/libsolidity/codegen/ContractCompiler.h index c08c9a38e40c..74a2e3b5f7d3 100644 --- a/libsolidity/codegen/ContractCompiler.h +++ b/libsolidity/codegen/ContractCompiler.h @@ -117,6 +117,7 @@ class ContractCompiler: private ASTConstVisitor bool visit(Return const& _return) override; bool visit(Throw const& _throw) override; bool visit(EmitStatement const& _emit) override; + bool visit(RevertStatement const& _revert) override; bool visit(VariableDeclarationStatement const& _variableDeclarationStatement) override; bool visit(ExpressionStatement const& _expressionStatement) override; bool visit(PlaceholderStatement const&) override; diff --git a/libsolidity/codegen/ExpressionCompiler.cpp b/libsolidity/codegen/ExpressionCompiler.cpp index fb88e0dd205d..a8e7dd5b7263 100644 --- a/libsolidity/codegen/ExpressionCompiler.cpp +++ b/libsolidity/codegen/ExpressionCompiler.cpp @@ -29,6 +29,7 @@ #include #include +#include #include #include @@ -914,7 +915,20 @@ bool ExpressionCompiler::visit(FunctionCall const& _functionCall) } case FunctionType::Kind::Error: { - solAssert(false, ""); + _functionCall.expression().accept(*this); + vector argumentTypes; + for (ASTPointer const& arg: _functionCall.sortedArguments()) + { + arg->accept(*this); + argumentTypes.push_back(arg->annotation().type); + } + solAssert(dynamic_cast(&function.declaration()), ""); + utils().revertWithError( + function.externalSignature(), + function.parameterTypes(), + argumentTypes + ); + break; } case FunctionType::Kind::BlockHash: { @@ -1868,6 +1882,7 @@ bool ExpressionCompiler::visit(MemberAccess const& _memberAccess) solAssert( dynamic_cast(_memberAccess.annotation().referencedDeclaration) || dynamic_cast(_memberAccess.annotation().referencedDeclaration) || + dynamic_cast(_memberAccess.annotation().referencedDeclaration) || category == Type::Category::TypeType || category == Type::Category::Module, "" diff --git a/libsolidity/codegen/ir/IRGeneratorForStatements.cpp b/libsolidity/codegen/ir/IRGeneratorForStatements.cpp index dddf8b80c377..c129ae646ae5 100644 --- a/libsolidity/codegen/ir/IRGeneratorForStatements.cpp +++ b/libsolidity/codegen/ir/IRGeneratorForStatements.cpp @@ -1045,7 +1045,14 @@ void IRGeneratorForStatements::endVisit(FunctionCall const& _functionCall) } case FunctionType::Kind::Error: { - solAssert(false, ""); + ErrorDefinition const* error = dynamic_cast(ASTNode::referencedDeclaration(_functionCall.expression())); + solAssert(error, ""); + revertWithError( + error->functionType(true)->externalSignature(), + error->functionType(true)->parameterTypes(), + _functionCall.sortedArguments() + ); + break; } case FunctionType::Kind::Assert: case FunctionType::Kind::Require: @@ -1199,41 +1206,19 @@ void IRGeneratorForStatements::endVisit(FunctionCall const& _functionCall) case FunctionType::Kind::Revert: { solAssert(arguments.size() == parameterTypes.size(), ""); - if (arguments.empty()) + solAssert(arguments.size() <= 1, ""); + solAssert( + arguments.empty() || + arguments.front()->annotation().type->isImplicitlyConvertibleTo(*TypeProvider::stringMemory()), + ""); + if (m_context.revertStrings() == RevertStrings::Strip || arguments.empty()) m_code << "revert(0, 0)\n"; else - { - solAssert(arguments.size() == 1, ""); - - if (m_context.revertStrings() == RevertStrings::Strip) - m_code << "revert(0, 0)\n"; - else - { - solAssert(type(*arguments.front()).isImplicitlyConvertibleTo(*TypeProvider::stringMemory()),""); - - Whiskers templ(R"({ - let := () - mstore(, ) - let := (add(, 4) ) - revert(, sub(, )) - })"); - templ("pos", m_context.newYulVariable()); - templ("end", m_context.newYulVariable()); - templ("hash", util::selectorFromSignature("Error(string)").str()); - templ("allocateUnbounded", m_utils.allocateUnboundedFunction()); - templ( - "argumentVars", - joinHumanReadablePrefixed(IRVariable{*arguments.front()}.stackSlots()) - ); - templ("encode", m_context.abiFunctions().tupleEncoder( - {&type(*arguments.front())}, - {TypeProvider::stringMemory()} - )); - - m_code << templ.render(); - } - } - + revertWithError( + "Error(string)", + {TypeProvider::stringMemory()}, + {arguments.front()} + ); break; } // Array creation using new @@ -1995,7 +1980,7 @@ void IRGeneratorForStatements::endVisit(MemberAccess const& _memberAccess) dynamic_cast(_memberAccess.annotation().referencedDeclaration), "Error not found" ); - // the call will do the resolving + // The function call will resolve the selector. break; case FunctionType::Kind::DelegateCall: define(IRVariable(_memberAccess).part("address"), _memberAccess.expression()); @@ -2043,6 +2028,7 @@ void IRGeneratorForStatements::endVisit(MemberAccess const& _memberAccess) solAssert( dynamic_cast(_memberAccess.annotation().referencedDeclaration) || dynamic_cast(_memberAccess.annotation().referencedDeclaration) || + dynamic_cast(_memberAccess.annotation().referencedDeclaration) || category == Type::Category::TypeType || category == Type::Category::Module, "" @@ -3140,6 +3126,38 @@ void IRGeneratorForStatements::rethrow() m_code << "revert(0, 0) // rethrow\n"s; } +void IRGeneratorForStatements::revertWithError( + string const& _signature, + vector const& _parameterTypes, + vector> const& _errorArguments +) +{ + Whiskers templ(R"({ + let := () + mstore(, ) + let := (add(, 4) ) + revert(, sub(, )) + })"); + templ("pos", m_context.newYulVariable()); + templ("end", m_context.newYulVariable()); + templ("hash", util::selectorFromSignature(_signature).str()); + templ("allocateUnbounded", m_utils.allocateUnboundedFunction()); + + vector errorArgumentVars; + vector errorArgumentTypes; + for (ASTPointer const& arg: _errorArguments) + { + errorArgumentVars += IRVariable(*arg).stackSlots(); + solAssert(arg->annotation().type, ""); + errorArgumentTypes.push_back(arg->annotation().type); + } + templ("argumentVars", joinHumanReadablePrefixed(errorArgumentVars)); + templ("encode", m_context.abiFunctions().tupleEncoder(errorArgumentTypes, _parameterTypes)); + + m_code << templ.render(); +} + + bool IRGeneratorForStatements::visit(TryCatchClause const& _clause) { _clause.block().accept(*this); diff --git a/libsolidity/codegen/ir/IRGeneratorForStatements.h b/libsolidity/codegen/ir/IRGeneratorForStatements.h index e212398b8b8f..90504aa7fa31 100644 --- a/libsolidity/codegen/ir/IRGeneratorForStatements.h +++ b/libsolidity/codegen/ir/IRGeneratorForStatements.h @@ -106,6 +106,15 @@ class IRGeneratorForStatements: public ASTConstVisitor /// Generates code to rethrow an exception. void rethrow(); + /// Generates code to revert with an error. The error arguments are assumed to + /// be already evaluated and available in local IRVariables, but not yet + /// converted. + void revertWithError( + std::string const& _signature, + std::vector const& _parameterTypes, + std::vector> const& _errorArguments + ); + void handleVariableReference( VariableDeclaration const& _variable, Expression const& _referencingExpression diff --git a/test/libsolidity/semanticTests/errors/error_in_library_and_interface.sol b/test/libsolidity/semanticTests/errors/error_in_library_and_interface.sol new file mode 100644 index 000000000000..81f6c1548449 --- /dev/null +++ b/test/libsolidity/semanticTests/errors/error_in_library_and_interface.sol @@ -0,0 +1,24 @@ +error E(uint a); +library L { + error E(uint a, uint b); +} +interface I { + error E(uint a, uint b, uint c); +} +contract C { + function f() public pure { + revert E(1); + } + function g() public pure { + revert L.E(1, 2); + } + function h() public pure { + revert I.E(1, 2, 3); + } +} +// ==== +// compileViaYul: also +// ---- +// f() -> FAILURE, hex"002ff067", hex"0000000000000000000000000000000000000000000000000000000000000001" +// g() -> FAILURE, hex"85208890", hex"0000000000000000000000000000000000000000000000000000000000000001", hex"0000000000000000000000000000000000000000000000000000000000000002" +// h() -> FAILURE, hex"7924ea7c", hex"0000000000000000000000000000000000000000000000000000000000000001", hex"0000000000000000000000000000000000000000000000000000000000000002", hex"0000000000000000000000000000000000000000000000000000000000000003" diff --git a/test/libsolidity/semanticTests/errors/named_error_args.sol b/test/libsolidity/semanticTests/errors/named_error_args.sol new file mode 100644 index 000000000000..7c3f35f31eb4 --- /dev/null +++ b/test/libsolidity/semanticTests/errors/named_error_args.sol @@ -0,0 +1,10 @@ +error E(uint a, uint b); +contract C { + function f() public pure { + revert E({b: 7, a: 2}); + } +} +// ==== +// compileViaYul: also +// ---- +// f() -> FAILURE, hex"85208890", hex"0000000000000000000000000000000000000000000000000000000000000002", hex"0000000000000000000000000000000000000000000000000000000000000007" diff --git a/test/libsolidity/semanticTests/errors/revert_conversion.sol b/test/libsolidity/semanticTests/errors/revert_conversion.sol new file mode 100644 index 000000000000..fd8fb7dc9357 --- /dev/null +++ b/test/libsolidity/semanticTests/errors/revert_conversion.sol @@ -0,0 +1,12 @@ +error E(string a, uint[] b); +contract C { + uint[] x; + function f() public { + x.push(7); + revert E("abc", x); + } +} +// ==== +// compileViaYul: also +// ---- +// f() -> FAILURE, hex"59e4d4df", 0x40, 0x80, 3, "abc", 1, 7 diff --git a/test/libsolidity/semanticTests/errors/simple.sol b/test/libsolidity/semanticTests/errors/simple.sol new file mode 100644 index 000000000000..26b40c8d3780 --- /dev/null +++ b/test/libsolidity/semanticTests/errors/simple.sol @@ -0,0 +1,10 @@ +error E(uint a, uint b); +contract C { + function f() public pure { + revert E(2, 7); + } +} +// ==== +// compileViaYul: also +// ---- +// f() -> FAILURE, hex"85208890", 2, 7 diff --git a/test/libsolidity/semanticTests/errors/via_contract_type.sol b/test/libsolidity/semanticTests/errors/via_contract_type.sol new file mode 100644 index 000000000000..767628c2a09e --- /dev/null +++ b/test/libsolidity/semanticTests/errors/via_contract_type.sol @@ -0,0 +1,18 @@ +contract A { + error E(uint); +} +contract X { + error E(string); +} +contract B is A { + function f() public pure { revert E(1); } + function g() public pure { revert A.E(1); } + function h() public pure { revert X.E("abc"); } + +} +// ==== +// compileViaYul: also +// ---- +// f() -> FAILURE, hex"002ff067", hex"0000000000000000000000000000000000000000000000000000000000000001" +// g() -> FAILURE, hex"002ff067", hex"0000000000000000000000000000000000000000000000000000000000000001" +// h() -> FAILURE, hex"3e9992c9", hex"0000000000000000000000000000000000000000000000000000000000000020", hex"0000000000000000000000000000000000000000000000000000000000000003", hex"6162630000000000000000000000000000000000000000000000000000000000" diff --git a/test/libsolidity/semanticTests/errors/via_import.sol b/test/libsolidity/semanticTests/errors/via_import.sol new file mode 100644 index 000000000000..9dd9a2cd34f5 --- /dev/null +++ b/test/libsolidity/semanticTests/errors/via_import.sol @@ -0,0 +1,25 @@ +==== Source: s1.sol ==== +error E(uint); +==== Source: s2.sol ==== +import "s1.sol" as S; +==== Source: s3.sol ==== +import "s1.sol" as S; +import "s2.sol" as T; +import "s1.sol"; +contract C { + function x() public pure { + revert E(1); + } + function y() public pure { + revert S.E(2); + } + function z() public pure { + revert T.S.E(3); + } +} +// ==== +// compileViaYul: also +// ---- +// x() -> FAILURE, hex"002ff067", hex"0000000000000000000000000000000000000000000000000000000000000001" +// y() -> FAILURE, hex"002ff067", hex"0000000000000000000000000000000000000000000000000000000000000002" +// z() -> FAILURE, hex"002ff067", hex"0000000000000000000000000000000000000000000000000000000000000003" diff --git a/test/libsolidity/semanticTests/errors/weird_name.sol b/test/libsolidity/semanticTests/errors/weird_name.sol new file mode 100644 index 000000000000..080522346454 --- /dev/null +++ b/test/libsolidity/semanticTests/errors/weird_name.sol @@ -0,0 +1,10 @@ +error error(uint a); +contract C { + function f() public pure { + revert error(2); + } +} +// ==== +// compileViaYul: also +// ---- +// f() -> FAILURE, hex"b48fb6cf", hex"0000000000000000000000000000000000000000000000000000000000000002" diff --git a/test/libsolidity/semanticTests/reverts/error_struct.sol b/test/libsolidity/semanticTests/reverts/error_struct.sol new file mode 100644 index 000000000000..38e24b4ac79d --- /dev/null +++ b/test/libsolidity/semanticTests/reverts/error_struct.sol @@ -0,0 +1,17 @@ +struct error { uint error; } +contract C { + error test(); + error _struct; + function f() public { + revert test(); + } + function g(uint x) public returns (uint) { + _struct.error = x; + return _struct.error; + } +} +// ==== +// compileViaYul: also +// ---- +// f() -> FAILURE, hex"f8a8fd6d" +// g(uint256): 7 -> 7 From 1057fd53558a5f64ca7b037d660c502c6673042c Mon Sep 17 00:00:00 2001 From: chriseth Date: Thu, 18 Mar 2021 13:21:09 +0100 Subject: [PATCH 5/7] Take revert statement into account in control flow graph. --- libsolidity/analysis/ControlFlowBuilder.cpp | 10 +++++++++ libsolidity/analysis/ControlFlowBuilder.h | 1 + .../syntaxTests/errors/duplicate.sol | 21 +++++++++++++++++++ .../syntaxTests/errors/unreachable.sol | 10 +++++++++ 4 files changed, 42 insertions(+) create mode 100644 test/libsolidity/syntaxTests/errors/duplicate.sol create mode 100644 test/libsolidity/syntaxTests/errors/unreachable.sol diff --git a/libsolidity/analysis/ControlFlowBuilder.cpp b/libsolidity/analysis/ControlFlowBuilder.cpp index 975e4ec6d9ce..8946e6ed3925 100644 --- a/libsolidity/analysis/ControlFlowBuilder.cpp +++ b/libsolidity/analysis/ControlFlowBuilder.cpp @@ -236,6 +236,16 @@ bool ControlFlowBuilder::visit(Throw const& _throw) return false; } +bool ControlFlowBuilder::visit(RevertStatement const& _revert) +{ + solAssert(!!m_currentNode, ""); + solAssert(!!m_revertNode, ""); + visitNode(_revert); + connect(m_currentNode, m_revertNode); + m_currentNode = newLabel(); + return false; +} + bool ControlFlowBuilder::visit(PlaceholderStatement const&) { solAssert(!!m_currentNode, ""); diff --git a/libsolidity/analysis/ControlFlowBuilder.h b/libsolidity/analysis/ControlFlowBuilder.h index 0efd55d77bcb..879d9b241c04 100644 --- a/libsolidity/analysis/ControlFlowBuilder.h +++ b/libsolidity/analysis/ControlFlowBuilder.h @@ -56,6 +56,7 @@ class ControlFlowBuilder: private ASTConstVisitor, private yul::ASTWalker bool visit(Break const&) override; bool visit(Continue const&) override; bool visit(Throw const&) override; + bool visit(RevertStatement const&) override; bool visit(PlaceholderStatement const&) override; bool visit(FunctionCall const& _functionCall) override; bool visit(ModifierInvocation const& _modifierInvocation) override; diff --git a/test/libsolidity/syntaxTests/errors/duplicate.sol b/test/libsolidity/syntaxTests/errors/duplicate.sol new file mode 100644 index 000000000000..a9ef4cc927d4 --- /dev/null +++ b/test/libsolidity/syntaxTests/errors/duplicate.sol @@ -0,0 +1,21 @@ +==== Source: A ==== + +error E(); + +==== Source: B ==== + +error E(); + +==== Source: C ==== + +import "A" as A; +import "B" as B; + +contract Test { + function f() public pure { + revert A.E(); + } + function g() public pure { + revert B.E(); + } +} \ No newline at end of file diff --git a/test/libsolidity/syntaxTests/errors/unreachable.sol b/test/libsolidity/syntaxTests/errors/unreachable.sol new file mode 100644 index 000000000000..c3ee54d177d6 --- /dev/null +++ b/test/libsolidity/syntaxTests/errors/unreachable.sol @@ -0,0 +1,10 @@ +contract C { + error E(); + uint x = 2; + function f() public { + revert E(); + x = 4; + } +} +// ---- +// Warning 5740: (98-103): Unreachable code. From e877e2bba745619f433e94c5d211debed5478a98 Mon Sep 17 00:00:00 2001 From: chriseth Date: Thu, 18 Mar 2021 13:28:04 +0100 Subject: [PATCH 6/7] Use all referenced errors. --- libsolidity/CMakeLists.txt | 2 + libsolidity/analysis/ContractLevelChecker.cpp | 18 ----- libsolidity/analysis/FunctionCallGraph.cpp | 2 + libsolidity/analysis/FunctionCallGraph.h | 4 +- .../analysis/PostTypeContractLevelChecker.cpp | 72 +++++++++++++++++++ .../analysis/PostTypeContractLevelChecker.h | 56 +++++++++++++++ libsolidity/ast/AST.cpp | 12 +++- libsolidity/ast/AST.h | 3 +- libsolidity/ast/CallGraph.h | 3 + libsolidity/interface/CompilerStack.cpp | 6 ++ .../syntaxTests/errors/hash_collision.sol | 2 +- .../errors/hash_collision_external.sol | 14 ++++ 12 files changed, 170 insertions(+), 24 deletions(-) create mode 100644 libsolidity/analysis/PostTypeContractLevelChecker.cpp create mode 100644 libsolidity/analysis/PostTypeContractLevelChecker.h create mode 100644 test/libsolidity/syntaxTests/errors/hash_collision_external.sol diff --git a/libsolidity/CMakeLists.txt b/libsolidity/CMakeLists.txt index 6d2c095a5657..0598cae86489 100644 --- a/libsolidity/CMakeLists.txt +++ b/libsolidity/CMakeLists.txt @@ -30,6 +30,8 @@ set(sources analysis/OverrideChecker.h analysis/PostTypeChecker.cpp analysis/PostTypeChecker.h + analysis/PostTypeContractLevelChecker.cpp + analysis/PostTypeContractLevelChecker.h analysis/ReferencesResolver.cpp analysis/ReferencesResolver.h analysis/Scoper.cpp diff --git a/libsolidity/analysis/ContractLevelChecker.cpp b/libsolidity/analysis/ContractLevelChecker.cpp index aece2f8cbee5..560702921e90 100644 --- a/libsolidity/analysis/ContractLevelChecker.cpp +++ b/libsolidity/analysis/ContractLevelChecker.cpp @@ -433,24 +433,6 @@ void ContractLevelChecker::checkHashCollisions(ContractDefinition const& _contra ); hashes.insert(hash); } - - map errorHashes; - for (ErrorDefinition const* error: _contract.interfaceErrors()) - { - if (!error->functionType(true)->interfaceFunctionType()) - // This will result in an error later on, so we can ignore it here. - continue; - uint32_t hash = selectorFromSignature32(error->functionType(true)->externalSignature()); - if (errorHashes.count(hash)) - m_errorReporter.typeError( - 4883_error, - _contract.location(), - SecondarySourceLocation{}.append("This error has the same selector: "s, errorHashes[hash]), - "Error signature hash collision for " + error->functionType(true)->externalSignature() - ); - else - errorHashes[hash] = error->location(); - } } void ContractLevelChecker::checkLibraryRequirements(ContractDefinition const& _contract) diff --git a/libsolidity/analysis/FunctionCallGraph.cpp b/libsolidity/analysis/FunctionCallGraph.cpp index 17cc67914a21..92cd10cd9101 100644 --- a/libsolidity/analysis/FunctionCallGraph.cpp +++ b/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(functionType->declaration())); return true; } diff --git a/libsolidity/analysis/FunctionCallGraph.h b/libsolidity/analysis/FunctionCallGraph.h index 1ec1d4d8956c..3ea1f5b9e2e0 100644 --- a/libsolidity/analysis/FunctionCallGraph.h +++ b/libsolidity/analysis/FunctionCallGraph.h @@ -65,8 +65,8 @@ class FunctionCallGraphBuilder: private ASTConstVisitor private: FunctionCallGraphBuilder(ContractDefinition const& _contract): - m_contract(_contract), - m_graph{{}, {}, {}} {} + m_contract(_contract) + {} bool visit(FunctionCall const& _functionCall) override; bool visit(EmitStatement const& _emitStatement) override; diff --git a/libsolidity/analysis/PostTypeContractLevelChecker.cpp b/libsolidity/analysis/PostTypeContractLevelChecker.cpp new file mode 100644 index 000000000000..cfc6f4f29ca2 --- /dev/null +++ b/libsolidity/analysis/PostTypeContractLevelChecker.cpp @@ -0,0 +1,72 @@ +/* + This file is part of solidity. + + solidity is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation, either version 3 of the License, or + (at your option) any later version. + + solidity is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with solidity. If not, see . +*/ +// SPDX-License-Identifier: GPL-3.0 +/** + * Component that verifies overloads, abstract contracts, function clashes and others + * checks at contract or function level. + */ + +#include + +#include +#include +#include + +using namespace std; +using namespace solidity; +using namespace solidity::langutil; +using namespace solidity::frontend; + +bool PostTypeContractLevelChecker::check(SourceUnit const& _sourceUnit) +{ + bool noErrors = true; + for (auto* contract: ASTNode::filteredNodes(_sourceUnit.nodes())) + if (!check(*contract)) + noErrors = false; + return noErrors; +} + +bool PostTypeContractLevelChecker::check(ContractDefinition const& _contract) +{ + solAssert( + _contract.annotation().creationCallGraph.set() && + _contract.annotation().deployedCallGraph.set(), + "" + ); + + map> errorHashes; + for (ErrorDefinition const* error: _contract.interfaceErrors()) + { + string signature = error->functionType(true)->externalSignature(); + uint32_t hash = selectorFromSignature32(signature); + // Fail if there is a different signature for the same hash. + if (!errorHashes[hash].empty() && !errorHashes[hash].count(signature)) + { + SourceLocation& otherLocation = errorHashes[hash].begin()->second; + m_errorReporter.typeError( + 4883_error, + error->nameLocation(), + SecondarySourceLocation{}.append("This error has a different signature but the same hash: ", otherLocation), + "Error signature hash collision for " + error->functionType(true)->externalSignature() + ); + } + else + errorHashes[hash][signature] = error->location(); + } + + return Error::containsOnlyWarnings(m_errorReporter.errors()); +} diff --git a/libsolidity/analysis/PostTypeContractLevelChecker.h b/libsolidity/analysis/PostTypeContractLevelChecker.h new file mode 100644 index 000000000000..e12e37184687 --- /dev/null +++ b/libsolidity/analysis/PostTypeContractLevelChecker.h @@ -0,0 +1,56 @@ +/* + This file is part of solidity. + + solidity is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation, either version 3 of the License, or + (at your option) any later version. + + solidity is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with solidity. If not, see . +*/ +// SPDX-License-Identifier: GPL-3.0 +/** + * Component that verifies properties at contract level, after the type checker has run. + */ + +#pragma once + +#include + +namespace solidity::langutil +{ +class ErrorReporter; +} + +namespace solidity::frontend +{ + +/** + * Component that verifies properties at contract level, after the type checker has run. + */ +class PostTypeContractLevelChecker +{ +public: + explicit PostTypeContractLevelChecker(langutil::ErrorReporter& _errorReporter): + m_errorReporter(_errorReporter) + {} + + /// Performs checks on the given source ast. + /// @returns true iff all checks passed. Note even if all checks passed, errors() can still contain warnings + bool check(SourceUnit const& _sourceUnit); + +private: + /// Performs checks on the given contract. + /// @returns true iff all checks passed. Note even if all checks passed, errors() can still contain warnings + bool check(ContractDefinition const& _contract); + + langutil::ErrorReporter& m_errorReporter; +}; + +} diff --git a/libsolidity/ast/AST.cpp b/libsolidity/ast/AST.cpp index 44acbe2f0715..513906b81822 100644 --- a/libsolidity/ast/AST.cpp +++ b/libsolidity/ast/AST.cpp @@ -23,6 +23,7 @@ #include +#include #include #include #include @@ -174,12 +175,19 @@ vector const& ContractDefinition::interfaceEvents() cons }); } -vector ContractDefinition::interfaceErrors() const +vector ContractDefinition::interfaceErrors(bool _requireCallGraph) const { set result; - // TODO add all referenced errors for (ContractDefinition const* contract: annotation().linearizedBaseContracts) result += filteredNodes(contract->m_subNodes); + solAssert(annotation().creationCallGraph.set() == annotation().deployedCallGraph.set(), ""); + if (_requireCallGraph) + solAssert(annotation().creationCallGraph.set(), ""); + if (annotation().creationCallGraph.set()) + { + result += (*annotation().creationCallGraph)->usedErrors; + result += (*annotation().deployedCallGraph)->usedErrors; + } return convertContainer>(move(result)); } diff --git a/libsolidity/ast/AST.h b/libsolidity/ast/AST.h index 6d58f504a52c..4438620c778f 100644 --- a/libsolidity/ast/AST.h +++ b/libsolidity/ast/AST.h @@ -515,7 +515,8 @@ class ContractDefinition: public Declaration, public StructurallyDocumented, pub std::vector const& interfaceEvents() const; /// @returns all errors defined in this contract or any base contract /// and all errors referenced during execution. - std::vector interfaceErrors() const; + /// @param _requireCallGraph if false, do not fail if the call graph has not been computed yet. + std::vector interfaceErrors(bool _requireCallGraph = true) const; bool isInterface() const { return m_contractKind == ContractKind::Interface; } bool isLibrary() const { return m_contractKind == ContractKind::Library; } diff --git a/libsolidity/ast/CallGraph.h b/libsolidity/ast/CallGraph.h index 8589c6c7bba7..f746d02e88e0 100644 --- a/libsolidity/ast/CallGraph.h +++ b/libsolidity/ast/CallGraph.h @@ -67,6 +67,9 @@ struct CallGraph /// Events that may get emitted by functions present in the graph. std::set emittedEvents; + + /// Errors that are used by functions present in the graph. + std::set usedErrors; }; } diff --git a/libsolidity/interface/CompilerStack.cpp b/libsolidity/interface/CompilerStack.cpp index bb323a72fa24..19d6ec7e3aa8 100644 --- a/libsolidity/interface/CompilerStack.cpp +++ b/libsolidity/interface/CompilerStack.cpp @@ -34,6 +34,7 @@ #include #include #include +#include #include #include #include @@ -433,6 +434,11 @@ bool CompilerStack::analyze() noErrors = false; } + if (noErrors) + for (Source const* source: m_sourceOrder) + if (source->ast && !PostTypeContractLevelChecker{m_errorReporter}.check(*source->ast)) + noErrors = false; + // Check that immutable variables are never read in c'tors and assigned // exactly once if (noErrors) diff --git a/test/libsolidity/syntaxTests/errors/hash_collision.sol b/test/libsolidity/syntaxTests/errors/hash_collision.sol index 37b95c822e40..d5ff09af9f5f 100644 --- a/test/libsolidity/syntaxTests/errors/hash_collision.sol +++ b/test/libsolidity/syntaxTests/errors/hash_collision.sol @@ -3,4 +3,4 @@ contract test { error tgeo(); } // ---- -// TypeError 4883: (0-52): Error signature hash collision for tgeo() +// TypeError 4883: (43-47): Error signature hash collision for tgeo() diff --git a/test/libsolidity/syntaxTests/errors/hash_collision_external.sol b/test/libsolidity/syntaxTests/errors/hash_collision_external.sol new file mode 100644 index 000000000000..8102a85a19fa --- /dev/null +++ b/test/libsolidity/syntaxTests/errors/hash_collision_external.sol @@ -0,0 +1,14 @@ +library L { + error gsf(); +} +contract test { + error tgeo(); + function f(bool a) public { + if (a) + revert L.gsf(); + else + revert tgeo(); + } +} +// ---- +// TypeError 4883: (57-61): Error signature hash collision for tgeo() From d80059fb98332dbce79d9deb090577216910d430 Mon Sep 17 00:00:00 2001 From: chriseth Date: Mon, 29 Mar 2021 15:58:16 +0200 Subject: [PATCH 7/7] Skip certain test for grammar test. --- scripts/test_antlr_grammar.sh | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/scripts/test_antlr_grammar.sh b/scripts/test_antlr_grammar.sh index 360cc00d258a..4b02f6585367 100755 --- a/scripts/test_antlr_grammar.sh +++ b/scripts/test_antlr_grammar.sh @@ -117,9 +117,12 @@ done < <( "^\/\/ (Syntax|Type|Declaration)Error|^\/\/ ParserError (1684|2837|3716|3997|5333|6275|6281|6933|7319)|^==== Source:" \ "${ROOT_DIR}/test/libsolidity/syntaxTests" \ "${ROOT_DIR}/test/libsolidity/semanticTests" | - grep -v -E 'comments/.*_direction_override.*.sol' | - grep -v -E 'literals/.*_direction_override.*.sol' # Skipping the unicode tests as I couldn't adapt the lexical grammar to recursively counting RLO/LRO/PDF's. + grep -v -E 'comments/.*_direction_override.*.sol' | + grep -v -E 'literals/.*_direction_override.*.sol' | + # Skipping a test with "revert E;" because ANTLR cannot distinguish it from + # a variable declaration. + grep -v -E 'revertStatement/non_called.sol' ) YUL_FILES=()