From 8e736a9f4982315dc36a47e24f23e8048d0fe880 Mon Sep 17 00:00:00 2001 From: chriseth Date: Thu, 5 Sep 2019 20:02:34 +0200 Subject: [PATCH] Type Checker for try/catch. --- libsolidity/analysis/ControlFlowBuilder.cpp | 2 +- libsolidity/analysis/NameAndTypeResolver.cpp | 12 ++ libsolidity/analysis/NameAndTypeResolver.h | 2 + libsolidity/analysis/ReferencesResolver.cpp | 2 +- libsolidity/analysis/StaticAnalyzer.cpp | 6 +- libsolidity/analysis/TypeChecker.cpp | 129 ++++++++++++++++++- libsolidity/analysis/TypeChecker.h | 1 + libsolidity/ast/AST.cpp | 25 ++-- libsolidity/ast/AST.h | 5 +- 9 files changed, 166 insertions(+), 18 deletions(-) diff --git a/libsolidity/analysis/ControlFlowBuilder.cpp b/libsolidity/analysis/ControlFlowBuilder.cpp index 6774b6e38..85566b32d 100644 --- a/libsolidity/analysis/ControlFlowBuilder.cpp +++ b/libsolidity/analysis/ControlFlowBuilder.cpp @@ -396,7 +396,7 @@ bool ControlFlowBuilder::visit(VariableDeclaration const& _variableDeclaration) _variableDeclaration.value().get() ); // Function arguments are considered to be immediately assigned as well (they are "externally assigned"). - else if (_variableDeclaration.isCallableParameter() && !_variableDeclaration.isReturnParameter()) + else if (_variableDeclaration.isCallableOrCatchParameter() && !_variableDeclaration.isReturnParameter()) m_currentNode->variableOccurrences.emplace_back( _variableDeclaration, VariableOccurrence::Kind::Assignment, diff --git a/libsolidity/analysis/NameAndTypeResolver.cpp b/libsolidity/analysis/NameAndTypeResolver.cpp index 727dc5f35..a1a266db1 100644 --- a/libsolidity/analysis/NameAndTypeResolver.cpp +++ b/libsolidity/analysis/NameAndTypeResolver.cpp @@ -624,6 +624,18 @@ void DeclarationRegistrationHelper::endVisit(FunctionDefinition&) closeCurrentScope(); } +bool DeclarationRegistrationHelper::visit(TryCatchClause& _tryCatchClause) +{ + _tryCatchClause.setScope(m_currentScope); + enterNewSubScope(_tryCatchClause); + return true; +} + +void DeclarationRegistrationHelper::endVisit(TryCatchClause&) +{ + closeCurrentScope(); +} + bool DeclarationRegistrationHelper::visit(ModifierDefinition& _modifier) { registerDeclaration(_modifier, true); diff --git a/libsolidity/analysis/NameAndTypeResolver.h b/libsolidity/analysis/NameAndTypeResolver.h index e5f5774ab..1f8d19419 100644 --- a/libsolidity/analysis/NameAndTypeResolver.h +++ b/libsolidity/analysis/NameAndTypeResolver.h @@ -177,6 +177,8 @@ private: bool visit(EnumValue& _value) override; bool visit(FunctionDefinition& _function) override; void endVisit(FunctionDefinition& _function) override; + bool visit(TryCatchClause& _tryCatchClause) override; + void endVisit(TryCatchClause& _tryCatchClause) override; bool visit(ModifierDefinition& _modifier) override; void endVisit(ModifierDefinition& _modifier) override; bool visit(FunctionTypeName& _funTypeName) override; diff --git a/libsolidity/analysis/ReferencesResolver.cpp b/libsolidity/analysis/ReferencesResolver.cpp index a1df47abe..43095cb09 100644 --- a/libsolidity/analysis/ReferencesResolver.cpp +++ b/libsolidity/analysis/ReferencesResolver.cpp @@ -388,7 +388,7 @@ void ReferencesResolver::endVisit(VariableDeclaration const& _variable) ", ", " or " ); - if (_variable.isCallableParameter()) + if (_variable.isCallableOrCatchParameter()) errorString += " for " + string(_variable.isReturnParameter() ? "return " : "") + diff --git a/libsolidity/analysis/StaticAnalyzer.cpp b/libsolidity/analysis/StaticAnalyzer.cpp index 2a0aaddcb..01c218218 100644 --- a/libsolidity/analysis/StaticAnalyzer.cpp +++ b/libsolidity/analysis/StaticAnalyzer.cpp @@ -118,10 +118,12 @@ void StaticAnalyzer::endVisit(FunctionDefinition const&) for (auto const& var: m_localVarUseCount) if (var.second == 0) { - if (var.first.second->isCallableParameter()) + if (var.first.second->isCallableOrCatchParameter()) m_errorReporter.warning( var.first.second->location(), - "Unused function parameter. Remove or comment out the variable name to silence this warning." + "Unused " + + string(var.first.second->isTryCatchParameter() ? "try/catch" : "function") + + " parameter. Remove or comment out the variable name to silence this warning." ); else m_errorReporter.warning(var.first.second->location(), "Unused local variable."); diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp index fcf81c2da..0b31fbf95 100644 --- a/libsolidity/analysis/TypeChecker.cpp +++ b/libsolidity/analysis/TypeChecker.cpp @@ -433,7 +433,7 @@ bool TypeChecker::visit(VariableDeclaration const& _variable) // * or inside of a struct definition. if ( m_scope->isInterface() - && !_variable.isCallableParameter() + && !_variable.isCallableOrCatchParameter() && !m_insideStruct ) m_errorReporter.typeError(_variable.location(), "Variables cannot be declared in interfaces."); @@ -743,6 +743,133 @@ bool TypeChecker::visit(IfStatement const& _ifStatement) return false; } +void TypeChecker::endVisit(TryStatement const& _tryStatement) +{ + FunctionCall const* externalCall = dynamic_cast(&_tryStatement.externalCall()); + if (!externalCall || externalCall->annotation().kind != FunctionCallKind::FunctionCall) + { + m_errorReporter.typeError( + _tryStatement.externalCall().location(), + "Try can only be used with external function calls and contract creation calls." + ); + return; + } + + FunctionType const& functionType = dynamic_cast(*externalCall->expression().annotation().type); + if ( + functionType.kind() != FunctionType::Kind::External && + functionType.kind() != FunctionType::Kind::Creation && + functionType.kind() != FunctionType::Kind::DelegateCall + ) + { + m_errorReporter.typeError( + _tryStatement.externalCall().location(), + "Try can only be used with external function calls and contract creation calls." + ); + return; + } + + externalCall->annotation().tryCall = true; + + solAssert(_tryStatement.clauses().size() >= 2, ""); + solAssert(_tryStatement.clauses().front(), ""); + + TryCatchClause const& successClause = *_tryStatement.clauses().front(); + if (successClause.parameters()) + { + TypePointers returnTypes = + m_evmVersion.supportsReturndata() ? + functionType.returnParameterTypes() : + functionType.returnParameterTypesWithoutDynamicTypes(); + std::vector> const& parameters = + successClause.parameters()->parameters(); + if (returnTypes.size() != parameters.size()) + m_errorReporter.typeError( + successClause.location(), + "Function returns " + + to_string(functionType.returnParameterTypes().size()) + + " values, but returns clause has " + + to_string(parameters.size()) + + " variables." + ); + size_t len = min(returnTypes.size(), parameters.size()); + for (size_t i = 0; i < len; ++i) + { + solAssert(returnTypes[i], ""); + if (parameters[i] && *parameters[i]->annotation().type != *returnTypes[i]) + m_errorReporter.typeError( + parameters[i]->location(), + "Invalid type, expected " + + returnTypes[i]->toString(false) + + " but got " + + parameters[i]->annotation().type->toString() + + "." + ); + } + } + + TryCatchClause const* errorClause = nullptr; + TryCatchClause const* lowLevelClause = nullptr; + for (size_t i = 1; i < _tryStatement.clauses().size(); ++i) + { + TryCatchClause const& clause = *_tryStatement.clauses()[i]; + if (clause.errorName() == "") + { + if (lowLevelClause) + m_errorReporter.typeError( + clause.location(), + SecondarySourceLocation{}.append("The first clause is here:", lowLevelClause->location()), + "This try statement already has a low-level catch clause." + ); + lowLevelClause = &clause; + if (clause.parameters() && !clause.parameters()->parameters().empty()) + { + if ( + clause.parameters()->parameters().size() != 1 || + *clause.parameters()->parameters().front()->type() != *TypeProvider::bytesMemory() + ) + m_errorReporter.typeError(clause.location(), "Expected `catch (bytes memory ...) { ... }` or `catch { ... }`."); + if (!m_evmVersion.supportsReturndata()) + m_errorReporter.typeError( + clause.location(), + "This catch clause type cannot be used on the selected EVM version (" + + m_evmVersion.name() + + "). You need at least a Byzantium-compatible EVM or use `catch { ... }`." + ); + } + } + else if (clause.errorName() == "Error") + { + if (!m_evmVersion.supportsReturndata()) + m_errorReporter.typeError( + clause.location(), + "This catch clause type cannot be used on the selected EVM version (" + + m_evmVersion.name() + + "). You need at least a Byzantium-compatible EVM or use `catch { ... }`." + ); + + if (errorClause) + m_errorReporter.typeError( + clause.location(), + SecondarySourceLocation{}.append("The first clause is here:", errorClause->location()), + "This try statement already has an \"Error\" catch clause." + ); + errorClause = &clause; + if ( + !clause.parameters() || + clause.parameters()->parameters().size() != 1 || + *clause.parameters()->parameters().front()->type() != *TypeProvider::stringMemory() + ) + m_errorReporter.typeError(clause.location(), "Expected `catch Error(string memory ...) { ... }`."); + } + else + m_errorReporter.typeError( + clause.location(), + "Invalid catch clause name. Expected either `catch (...)` or `catch Error(...)`." + ); + } +} + bool TypeChecker::visit(WhileStatement const& _whileStatement) { expectType(_whileStatement.condition(), *TypeProvider::boolean()); diff --git a/libsolidity/analysis/TypeChecker.h b/libsolidity/analysis/TypeChecker.h index c962ad2ed..9e5d46d94 100644 --- a/libsolidity/analysis/TypeChecker.h +++ b/libsolidity/analysis/TypeChecker.h @@ -120,6 +120,7 @@ private: void endVisit(FunctionTypeName const& _funType) override; bool visit(InlineAssembly const& _inlineAssembly) override; bool visit(IfStatement const& _ifStatement) override; + void endVisit(TryStatement const& _tryStatement) override; bool visit(WhileStatement const& _whileStatement) override; bool visit(ForStatement const& _forStatement) override; void endVisit(Return const& _return) override; diff --git a/libsolidity/ast/AST.cpp b/libsolidity/ast/AST.cpp index 2fec31fdd..cc834143c 100644 --- a/libsolidity/ast/AST.cpp +++ b/libsolidity/ast/AST.cpp @@ -452,12 +452,9 @@ bool VariableDeclaration::isLocalVariable() const dynamic_cast(s); } -bool VariableDeclaration::isCallableParameter() const +bool VariableDeclaration::isCallableOrCatchParameter() const { - // TODO Adjust for catch params. - // I think catch params should return true here. - // some of the error strings would need to be adjusted. - if (isReturnParameter() || dynamic_cast(scope())) + if (isReturnParameter() || isTryCatchParameter()) return true; vector> const* parameters = nullptr; @@ -476,8 +473,7 @@ bool VariableDeclaration::isCallableParameter() const bool VariableDeclaration::isLocalOrReturn() const { - // TODO adjust for catch params - return isReturnParameter() || (isLocalVariable() && !isCallableParameter()); + return isReturnParameter() || (isLocalVariable() && !isCallableOrCatchParameter()); } bool VariableDeclaration::isReturnParameter() const @@ -497,9 +493,14 @@ bool VariableDeclaration::isReturnParameter() const return false; } +bool VariableDeclaration::isTryCatchParameter() const +{ + return dynamic_cast(scope()); +} + bool VariableDeclaration::isExternalCallableParameter() const { - if (!isCallableParameter()) + if (!isCallableOrCatchParameter()) return false; if (auto const* callable = dynamic_cast(scope())) @@ -511,7 +512,7 @@ bool VariableDeclaration::isExternalCallableParameter() const bool VariableDeclaration::isInternalCallableParameter() const { - if (!isCallableParameter()) + if (!isCallableOrCatchParameter()) return false; if (auto const* funTypeName = dynamic_cast(scope())) @@ -523,7 +524,7 @@ bool VariableDeclaration::isInternalCallableParameter() const bool VariableDeclaration::isLibraryFunctionParameter() const { - if (!isCallableParameter()) + if (!isCallableOrCatchParameter()) return false; if (auto const* funDef = dynamic_cast(scope())) return dynamic_cast(*funDef->scope()).isLibrary(); @@ -559,10 +560,10 @@ set VariableDeclaration::allowedDataLocations() c locations.insert(Location::Storage); return locations; } - else if (isCallableParameter()) + else if (isCallableOrCatchParameter()) { set locations{ Location::Memory }; - if (isInternalCallableParameter() || isLibraryFunctionParameter()) + if (isInternalCallableParameter() || isLibraryFunctionParameter() || isTryCatchParameter()) locations.insert(Location::Storage); return locations; } diff --git a/libsolidity/ast/AST.h b/libsolidity/ast/AST.h index ce0eb5d62..5742cc74e 100644 --- a/libsolidity/ast/AST.h +++ b/libsolidity/ast/AST.h @@ -738,9 +738,12 @@ public: /// (or function type name or event) or declared inside a function body. bool isLocalVariable() const; /// @returns true if this variable is a parameter or return parameter of a function. - bool isCallableParameter() const; + bool isCallableOrCatchParameter() const; /// @returns true if this variable is a return parameter of a function. bool isReturnParameter() const; + /// @returns true if this variable is a parameter of the success or failure clausse + /// of a try/catch statement. + bool isTryCatchParameter() const; /// @returns true if this variable is a local variable or return parameter. bool isLocalOrReturn() const; /// @returns true if this variable is a parameter (not return parameter) of an external function.