From a5ceaac8df80f89112bb4d7bbfd9da165d370aa3 Mon Sep 17 00:00:00 2001 From: Federico Bond Date: Mon, 17 Jul 2017 16:47:44 -0300 Subject: Improve override changes signature error message --- Changelog.md | 1 + libsolidity/analysis/TypeChecker.cpp | 32 +++++++++++++++++++++++++++++++- libsolidity/analysis/TypeChecker.h | 3 +++ 3 files changed, 35 insertions(+), 1 deletion(-) diff --git a/Changelog.md b/Changelog.md index 4609da92..b4ca745f 100644 --- a/Changelog.md +++ b/Changelog.md @@ -4,6 +4,7 @@ Features: * Parser: Display previous visibility specifier in error if multiple are found. * Syntax Checker: Support ``pragma experimental ;`` to turn on experimental features. * Metadata: Store experimental flag in metadata CBOR. + * Type Checker: More detailed error message for invalid overrides. Bugfixes: * Parser: Enforce commas between array and tuple elements. diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp index 6852c13d..a7433250 100644 --- a/libsolidity/analysis/TypeChecker.cpp +++ b/libsolidity/analysis/TypeChecker.cpp @@ -290,7 +290,7 @@ void TypeChecker::checkContractIllegalOverrides(ContractDefinition const& _contr overriding->isPayable() != function->isPayable() || overridingType != functionType ) - m_errorReporter.typeError(overriding->location(), "Override changes extended function signature."); + overrideTypeError(*overriding, *function); } functions[name].push_back(function); } @@ -1950,3 +1950,33 @@ void TypeChecker::requireLValue(Expression const& _expression) m_errorReporter.typeError(_expression.location(), "Expression has to be an lvalue."); } + +void TypeChecker::overrideTypeError(FunctionDefinition const& function, FunctionDefinition const& super) +{ + string message; + + if (function.visibility() != super.visibility()) + message = "Overriding function visibility differs from extended function."; + else if (function.isDeclaredConst() && !super.isDeclaredConst()) + message = "Overriding function should not be declared constant."; + else if (!function.isDeclaredConst() && super.isDeclaredConst()) + message = "Overriding function should be declared constant."; + else if (function.isPayable() && !super.isPayable()) + message = "Overriding function should not be declared payable."; + else if (!function.isPayable() && super.isPayable()) + message = "Overriding function should be declared payable."; + + if (message.empty()) + { + FunctionType functionType(function); + FunctionType superType(super); + + if (functionType != superType) + message = "Overriding function return types differ from extended function."; + } + + if (message.empty()) + message = "Override changes extended function signature."; + + m_errorReporter.typeError(function.location(), message); +} diff --git a/libsolidity/analysis/TypeChecker.h b/libsolidity/analysis/TypeChecker.h index ee43d13a..17dde81a 100644 --- a/libsolidity/analysis/TypeChecker.h +++ b/libsolidity/analysis/TypeChecker.h @@ -120,6 +120,9 @@ private: /// Runs type checks on @a _expression to infer its type and then checks that it is an LValue. void requireLValue(Expression const& _expression); + /// Reports a type error with an appropiate message when overriden function signature differs. + void overrideTypeError(FunctionDefinition const& function, FunctionDefinition const& super); + ContractDefinition const* m_scope = nullptr; ErrorReporter& m_errorReporter; -- cgit From ff5bb54e3c19bedfadd06dd60f032fd7cdc55389 Mon Sep 17 00:00:00 2001 From: Federico Bond Date: Mon, 17 Jul 2017 19:12:34 -0300 Subject: Use fully qualified name of super in message --- libsolidity/analysis/TypeChecker.cpp | 7 +++---- libsolidity/ast/AST.cpp | 9 +++++++++ libsolidity/ast/AST.h | 1 + 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp index a7433250..9bccac12 100644 --- a/libsolidity/analysis/TypeChecker.cpp +++ b/libsolidity/analysis/TypeChecker.cpp @@ -1950,13 +1950,12 @@ void TypeChecker::requireLValue(Expression const& _expression) m_errorReporter.typeError(_expression.location(), "Expression has to be an lvalue."); } - void TypeChecker::overrideTypeError(FunctionDefinition const& function, FunctionDefinition const& super) { string message; if (function.visibility() != super.visibility()) - message = "Overriding function visibility differs from extended function."; + message = "Overriding function visibility differs from " + super.fullyQualifiedName() + "."; else if (function.isDeclaredConst() && !super.isDeclaredConst()) message = "Overriding function should not be declared constant."; else if (!function.isDeclaredConst() && super.isDeclaredConst()) @@ -1972,11 +1971,11 @@ void TypeChecker::overrideTypeError(FunctionDefinition const& function, Function FunctionType superType(super); if (functionType != superType) - message = "Overriding function return types differ from extended function."; + message = "Overriding function return types differ from " + super.fullyQualifiedName() + "."; } if (message.empty()) - message = "Override changes extended function signature."; + message = "Overriding function signature differs from " + super.fullyQualifiedName() + "."; m_errorReporter.typeError(function.location(), message); } diff --git a/libsolidity/ast/AST.cpp b/libsolidity/ast/AST.cpp index 1d68231e..e173237e 100644 --- a/libsolidity/ast/AST.cpp +++ b/libsolidity/ast/AST.cpp @@ -371,6 +371,15 @@ string FunctionDefinition::externalSignature() const return FunctionType(*this).externalSignature(); } +string FunctionDefinition::fullyQualifiedName() const +{ + auto const* contract = dynamic_cast(scope()); + solAssert(contract, "Enclosing scope of function definition was not set."); + + auto fname = name().empty() ? "" : name(); + return sourceUnitName() + ":" + contract->name() + "." + fname; +} + FunctionDefinitionAnnotation& FunctionDefinition::annotation() const { if (!m_annotation) diff --git a/libsolidity/ast/AST.h b/libsolidity/ast/AST.h index bdf20e81..d32cf573 100644 --- a/libsolidity/ast/AST.h +++ b/libsolidity/ast/AST.h @@ -613,6 +613,7 @@ public: std::vector> const& modifiers() const { return m_functionModifiers; } std::vector> const& returnParameters() const { return m_returnParameters->parameters(); } Block const& body() const { solAssert(m_body, ""); return *m_body; } + std::string fullyQualifiedName() const; virtual bool isVisibleInContract() const override { return Declaration::isVisibleInContract() && !isConstructor() && !isFallback(); -- cgit From f0dc5720551a12febbe1a61461218de24e18e07a Mon Sep 17 00:00:00 2001 From: Federico Bond Date: Tue, 18 Jul 2017 01:10:57 -0300 Subject: Improve and add missing tests --- test/libsolidity/SolidityNameAndTypeResolution.cpp | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/test/libsolidity/SolidityNameAndTypeResolution.cpp b/test/libsolidity/SolidityNameAndTypeResolution.cpp index f7648bd7..49a23e13 100644 --- a/test/libsolidity/SolidityNameAndTypeResolution.cpp +++ b/test/libsolidity/SolidityNameAndTypeResolution.cpp @@ -924,16 +924,25 @@ BOOST_AUTO_TEST_CASE(illegal_override_visibility) contract B { function f() internal {} } contract C is B { function f() public {} } )"; - CHECK_ERROR(text, TypeError, "Override changes extended function signature."); + CHECK_ERROR(text, TypeError, "Overriding function visibility differs"); } -BOOST_AUTO_TEST_CASE(illegal_override_constness) +BOOST_AUTO_TEST_CASE(illegal_override_remove_constness) { char const* text = R"( contract B { function f() constant {} } contract C is B { function f() {} } )"; - CHECK_ERROR(text, TypeError, "Override changes extended function signature."); + CHECK_ERROR(text, TypeError, "Overriding function should be declared constant."); +} + +BOOST_AUTO_TEST_CASE(illegal_override_add_constness) +{ + char const* text = R"( + contract B { function f() {} } + contract C is B { function f() constant {} } + )"; + CHECK_ERROR(text, TypeError, "Overriding function should not be declared constant."); } BOOST_AUTO_TEST_CASE(complex_inheritance) @@ -2491,7 +2500,7 @@ BOOST_AUTO_TEST_CASE(override_changes_return_types) function f(uint a) returns (uint8) { } } )"; - CHECK_ERROR(sourceCode, TypeError, "Override changes extended function signature."); + CHECK_ERROR(sourceCode, TypeError, "Overriding function return types differ"); } BOOST_AUTO_TEST_CASE(multiple_constructors) @@ -4732,7 +4741,7 @@ BOOST_AUTO_TEST_CASE(illegal_override_payable) contract B { function f() payable {} } contract C is B { function f() {} } )"; - CHECK_ERROR(text, TypeError, "Override changes extended function signature."); + CHECK_ERROR(text, TypeError, "Overriding function should be declared payable."); } BOOST_AUTO_TEST_CASE(illegal_override_payable_nonpayable) @@ -4741,7 +4750,7 @@ BOOST_AUTO_TEST_CASE(illegal_override_payable_nonpayable) contract B { function f() {} } contract C is B { function f() payable {} } )"; - CHECK_ERROR(text, TypeError, "Override changes extended function signature."); + CHECK_ERROR(text, TypeError, "Overriding function should not be declared payable."); } BOOST_AUTO_TEST_CASE(function_variable_mixin) -- cgit From a6949851f332df8d05bef3cb2cd45105a02100a5 Mon Sep 17 00:00:00 2001 From: Federico Bond Date: Wed, 19 Jul 2017 14:32:26 -0300 Subject: Refactor function override check to remove duplicate logic --- libsolidity/analysis/TypeChecker.cpp | 92 +++++++++++++++++++----------------- libsolidity/analysis/TypeChecker.h | 5 +- 2 files changed, 50 insertions(+), 47 deletions(-) diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp index 9bccac12..df791be9 100644 --- a/libsolidity/analysis/TypeChecker.cpp +++ b/libsolidity/analysis/TypeChecker.cpp @@ -277,21 +277,10 @@ void TypeChecker::checkContractIllegalOverrides(ContractDefinition const& _contr string const& name = function->name(); if (modifiers.count(name)) m_errorReporter.typeError(modifiers[name]->location(), "Override changes function to modifier."); - FunctionType functionType(*function); - // function should not change the return type + for (FunctionDefinition const* overriding: functions[name]) - { - FunctionType overridingType(*overriding); - if (!overridingType.hasEqualArgumentTypes(functionType)) - continue; - if ( - overriding->visibility() != function->visibility() || - overriding->isDeclaredConst() != function->isDeclaredConst() || - overriding->isPayable() != function->isPayable() || - overridingType != functionType - ) - overrideTypeError(*overriding, *function); - } + checkFunctionOverride(*overriding, *function); + functions[name].push_back(function); } for (ModifierDefinition const* modifier: contract->functionModifiers()) @@ -308,6 +297,51 @@ void TypeChecker::checkContractIllegalOverrides(ContractDefinition const& _contr } } +void TypeChecker::checkFunctionOverride(FunctionDefinition const& function, FunctionDefinition const& super) +{ + FunctionType functionType(function); + FunctionType superType(super); + + if (!functionType.hasEqualArgumentTypes(superType)) + return; + + if (function.visibility() != super.visibility()) + m_errorReporter.typeError( + function.location(), + "Overriding function visibility differs from " + super.fullyQualifiedName() + "." + ); + + if (function.isDeclaredConst() && !super.isDeclaredConst()) + m_errorReporter.typeError( + function.location(), + "Overriding function should not be declared constant." + ); + + if (!function.isDeclaredConst() && super.isDeclaredConst()) + m_errorReporter.typeError( + function.location(), + "Overriding function should be declared constant." + ); + + if (function.isPayable() && !super.isPayable()) + m_errorReporter.typeError( + function.location(), + "Overriding function should not be declared payable." + ); + + if (!function.isPayable() && super.isPayable()) + m_errorReporter.typeError( + function.location(), + "Overriding function should be declared payable." + ); + + if (functionType != superType) + m_errorReporter.typeError( + function.location(), + "Overriding function return types differ from " + super.fullyQualifiedName() + "." + ); +} + void TypeChecker::checkContractExternalTypeClashes(ContractDefinition const& _contract) { map>> externalDeclarations; @@ -1949,33 +1983,3 @@ void TypeChecker::requireLValue(Expression const& _expression) else if (!_expression.annotation().isLValue) m_errorReporter.typeError(_expression.location(), "Expression has to be an lvalue."); } - -void TypeChecker::overrideTypeError(FunctionDefinition const& function, FunctionDefinition const& super) -{ - string message; - - if (function.visibility() != super.visibility()) - message = "Overriding function visibility differs from " + super.fullyQualifiedName() + "."; - else if (function.isDeclaredConst() && !super.isDeclaredConst()) - message = "Overriding function should not be declared constant."; - else if (!function.isDeclaredConst() && super.isDeclaredConst()) - message = "Overriding function should be declared constant."; - else if (function.isPayable() && !super.isPayable()) - message = "Overriding function should not be declared payable."; - else if (!function.isPayable() && super.isPayable()) - message = "Overriding function should be declared payable."; - - if (message.empty()) - { - FunctionType functionType(function); - FunctionType superType(super); - - if (functionType != superType) - message = "Overriding function return types differ from " + super.fullyQualifiedName() + "."; - } - - if (message.empty()) - message = "Overriding function signature differs from " + super.fullyQualifiedName() + "."; - - m_errorReporter.typeError(function.location(), message); -} diff --git a/libsolidity/analysis/TypeChecker.h b/libsolidity/analysis/TypeChecker.h index 17dde81a..f1004e92 100644 --- a/libsolidity/analysis/TypeChecker.h +++ b/libsolidity/analysis/TypeChecker.h @@ -62,6 +62,8 @@ private: /// arguments and that there is at most one constructor. void checkContractDuplicateFunctions(ContractDefinition const& _contract); void checkContractIllegalOverrides(ContractDefinition const& _contract); + /// Reports a type error with an appropiate message if overriden function signature differs. + void checkFunctionOverride(FunctionDefinition const& function, FunctionDefinition const& super); void checkContractAbstractFunctions(ContractDefinition const& _contract); void checkContractAbstractConstructors(ContractDefinition const& _contract); /// Checks that different functions with external visibility end up having different @@ -120,9 +122,6 @@ private: /// Runs type checks on @a _expression to infer its type and then checks that it is an LValue. void requireLValue(Expression const& _expression); - /// Reports a type error with an appropiate message when overriden function signature differs. - void overrideTypeError(FunctionDefinition const& function, FunctionDefinition const& super); - ContractDefinition const* m_scope = nullptr; ErrorReporter& m_errorReporter; -- cgit From d4997dd9a3ec10767d8ee289207fdf0ea6b45f0a Mon Sep 17 00:00:00 2001 From: Federico Bond Date: Wed, 19 Jul 2017 19:22:36 -0300 Subject: Use a secondary location for function override errors --- libsolidity/analysis/TypeChecker.cpp | 39 ++++++++++++++--------------------- libsolidity/analysis/TypeChecker.h | 1 + libsolidity/interface/ErrorReporter.h | 4 ++-- 3 files changed, 18 insertions(+), 26 deletions(-) diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp index df791be9..2a9ca5a8 100644 --- a/libsolidity/analysis/TypeChecker.cpp +++ b/libsolidity/analysis/TypeChecker.cpp @@ -306,40 +306,31 @@ void TypeChecker::checkFunctionOverride(FunctionDefinition const& function, Func return; if (function.visibility() != super.visibility()) - m_errorReporter.typeError( - function.location(), - "Overriding function visibility differs from " + super.fullyQualifiedName() + "." - ); + overrideError(function, super, "Overriding function visibility differs."); if (function.isDeclaredConst() && !super.isDeclaredConst()) - m_errorReporter.typeError( - function.location(), - "Overriding function should not be declared constant." - ); + overrideError(function, super, "Overriding function should not be declared constant."); if (!function.isDeclaredConst() && super.isDeclaredConst()) - m_errorReporter.typeError( - function.location(), - "Overriding function should be declared constant." - ); + overrideError(function, super, "Overriding function should be declared constant."); if (function.isPayable() && !super.isPayable()) - m_errorReporter.typeError( - function.location(), - "Overriding function should not be declared payable." - ); + overrideError(function, super, "Overriding function should not be declared payable."); if (!function.isPayable() && super.isPayable()) - m_errorReporter.typeError( - function.location(), - "Overriding function should be declared payable." - ); + overrideError(function, super, "Overriding function should be declared payable."); if (functionType != superType) - m_errorReporter.typeError( - function.location(), - "Overriding function return types differ from " + super.fullyQualifiedName() + "." - ); + overrideError(function, super, "Overriding function return types differ."); +} + +void TypeChecker::overrideError(FunctionDefinition const& function, FunctionDefinition const& super, string message) +{ + m_errorReporter.typeError( + function.location(), + SecondarySourceLocation().append("Overriden function is here:", super.location()), + message + ); } void TypeChecker::checkContractExternalTypeClashes(ContractDefinition const& _contract) diff --git a/libsolidity/analysis/TypeChecker.h b/libsolidity/analysis/TypeChecker.h index f1004e92..f2e13765 100644 --- a/libsolidity/analysis/TypeChecker.h +++ b/libsolidity/analysis/TypeChecker.h @@ -64,6 +64,7 @@ private: void checkContractIllegalOverrides(ContractDefinition const& _contract); /// Reports a type error with an appropiate message if overriden function signature differs. void checkFunctionOverride(FunctionDefinition const& function, FunctionDefinition const& super); + void overrideError(FunctionDefinition const& function, FunctionDefinition const& super, std::string message); void checkContractAbstractFunctions(ContractDefinition const& _contract); void checkContractAbstractConstructors(ContractDefinition const& _contract); /// Checks that different functions with external visibility end up having different diff --git a/libsolidity/interface/ErrorReporter.h b/libsolidity/interface/ErrorReporter.h index 42b0c8b6..12f4e8df 100644 --- a/libsolidity/interface/ErrorReporter.h +++ b/libsolidity/interface/ErrorReporter.h @@ -75,8 +75,8 @@ public: void typeError( SourceLocation const& _location, - SecondarySourceLocation const& _secondaryLocation, - std::string const& _description + SecondarySourceLocation const& _secondaryLocation = SecondarySourceLocation(), + std::string const& _description = std::string() ); void typeError(SourceLocation const& _location, std::string const& _description); -- cgit From 3571db6e3f54476cdabaf4861100564ae4b00a6a Mon Sep 17 00:00:00 2001 From: Federico Bond Date: Fri, 21 Jul 2017 01:11:16 -0300 Subject: Avoid duplicate errors due to function overrides --- libsolidity/analysis/TypeChecker.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp index 2a9ca5a8..8dd45a90 100644 --- a/libsolidity/analysis/TypeChecker.cpp +++ b/libsolidity/analysis/TypeChecker.cpp @@ -308,19 +308,19 @@ void TypeChecker::checkFunctionOverride(FunctionDefinition const& function, Func if (function.visibility() != super.visibility()) overrideError(function, super, "Overriding function visibility differs."); - if (function.isDeclaredConst() && !super.isDeclaredConst()) + else if (function.isDeclaredConst() && !super.isDeclaredConst()) overrideError(function, super, "Overriding function should not be declared constant."); - if (!function.isDeclaredConst() && super.isDeclaredConst()) + else if (!function.isDeclaredConst() && super.isDeclaredConst()) overrideError(function, super, "Overriding function should be declared constant."); - if (function.isPayable() && !super.isPayable()) + else if (function.isPayable() && !super.isPayable()) overrideError(function, super, "Overriding function should not be declared payable."); - if (!function.isPayable() && super.isPayable()) + else if (!function.isPayable() && super.isPayable()) overrideError(function, super, "Overriding function should be declared payable."); - if (functionType != superType) + else if (functionType != superType) overrideError(function, super, "Overriding function return types differ."); } -- cgit