From 283cdef98ca1b215a0357ce0c146a2f216f06da0 Mon Sep 17 00:00:00 2001 From: chriseth Date: Mon, 4 Jun 2018 17:00:37 +0200 Subject: Fix view/pure error ordering problem. --- libsolidity/analysis/ViewPureChecker.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'libsolidity/analysis/ViewPureChecker.cpp') diff --git a/libsolidity/analysis/ViewPureChecker.cpp b/libsolidity/analysis/ViewPureChecker.cpp index d9843012..107eb3aa 100644 --- a/libsolidity/analysis/ViewPureChecker.cpp +++ b/libsolidity/analysis/ViewPureChecker.cpp @@ -117,7 +117,7 @@ private: bool ViewPureChecker::check() { // The bool means "enforce view with errors". - map contracts; + vector> contracts; for (auto const& node: m_ast) { @@ -125,7 +125,7 @@ bool ViewPureChecker::check() solAssert(source, ""); bool enforceView = source->annotation().experimentalFeatures.count(ExperimentalFeature::V050); for (ContractDefinition const* c: source->filteredNodes(source->nodes())) - contracts[c] = enforceView; + contracts.emplace_back(c, enforceView); } // Check modifiers first to infer their state mutability. -- cgit v1.2.3 From 7ea8365ab08fae7a30cfff55e7d53f78bb1a2c36 Mon Sep 17 00:00:00 2001 From: Daniel Kirchner Date: Fri, 11 May 2018 10:40:32 +0200 Subject: Remove v050 check for enforcing "view" in ViewPureChecker. --- libsolidity/analysis/ViewPureChecker.cpp | 38 +++++++++++--------------------- 1 file changed, 13 insertions(+), 25 deletions(-) (limited to 'libsolidity/analysis/ViewPureChecker.cpp') diff --git a/libsolidity/analysis/ViewPureChecker.cpp b/libsolidity/analysis/ViewPureChecker.cpp index 107eb3aa..18c642c3 100644 --- a/libsolidity/analysis/ViewPureChecker.cpp +++ b/libsolidity/analysis/ViewPureChecker.cpp @@ -116,31 +116,22 @@ private: bool ViewPureChecker::check() { - // The bool means "enforce view with errors". - vector> contracts; + vector contracts; for (auto const& node: m_ast) { SourceUnit const* source = dynamic_cast(node.get()); solAssert(source, ""); - bool enforceView = source->annotation().experimentalFeatures.count(ExperimentalFeature::V050); - for (ContractDefinition const* c: source->filteredNodes(source->nodes())) - contracts.emplace_back(c, enforceView); + contracts += source->filteredNodes(source->nodes()); } // Check modifiers first to infer their state mutability. for (auto const& contract: contracts) - { - m_enforceViewWithError = contract.second; - for (ModifierDefinition const* mod: contract.first->functionModifiers()) + for (ModifierDefinition const* mod: contract->functionModifiers()) mod->accept(*this); - } for (auto const& contract: contracts) - { - m_enforceViewWithError = contract.second; - contract.first->accept(*this); - } + contract->accept(*this); return !m_errors; } @@ -232,17 +223,20 @@ void ViewPureChecker::reportMutability(StateMutability _mutability, SourceLocati { if (m_currentFunction && m_currentFunction->stateMutability() < _mutability) { - string text; if (_mutability == StateMutability::View) - text = + m_errorReporter.typeError( + _location, "Function declared as pure, but this expression (potentially) reads from the " - "environment or state and thus requires \"view\"."; + "environment or state and thus requires \"view\"." + ); else if (_mutability == StateMutability::NonPayable) - text = + m_errorReporter.typeError( + _location, "Function declared as " + stateMutabilityToString(m_currentFunction->stateMutability()) + ", but this expression (potentially) modifies the state and thus " - "requires non-payable (the default) or payable."; + "requires non-payable (the default) or payable." + ); else solAssert(false, ""); @@ -251,13 +245,7 @@ void ViewPureChecker::reportMutability(StateMutability _mutability, SourceLocati m_currentFunction->stateMutability() == StateMutability::Pure, "" ); - if (!m_enforceViewWithError && m_currentFunction->stateMutability() == StateMutability::View) - m_errorReporter.warning(_location, text); - else - { - m_errors = true; - m_errorReporter.typeError(_location, text); - } + m_errors = true; } if (_mutability > m_currentBestMutability) m_currentBestMutability = _mutability; -- cgit v1.2.3 From 3fa0ac5822f9920e9e75a3786189eb10edc4c39c Mon Sep 17 00:00:00 2001 From: chriseth Date: Fri, 20 Jul 2018 10:50:05 +0200 Subject: Do not handle `balance` member of contract types specially. --- libsolidity/analysis/ViewPureChecker.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'libsolidity/analysis/ViewPureChecker.cpp') diff --git a/libsolidity/analysis/ViewPureChecker.cpp b/libsolidity/analysis/ViewPureChecker.cpp index 18c642c3..d936ada0 100644 --- a/libsolidity/analysis/ViewPureChecker.cpp +++ b/libsolidity/analysis/ViewPureChecker.cpp @@ -287,9 +287,8 @@ void ViewPureChecker::endVisit(MemberAccess const& _memberAccess) ASTString const& member = _memberAccess.memberName(); switch (_memberAccess.expression().annotation().type->category()) { - case Type::Category::Contract: case Type::Category::Integer: - if (member == "balance" && !_memberAccess.annotation().referencedDeclaration) + if (member == "balance") mutability = StateMutability::View; break; case Type::Category::Magic: -- cgit v1.2.3 From 9328ea4c3c023950e59405941cfc25ec4c0ed1b4 Mon Sep 17 00:00:00 2001 From: chriseth Date: Sat, 30 Jun 2018 18:09:13 +0200 Subject: Add abi.decode(bytes data, (...)) --- libsolidity/analysis/ViewPureChecker.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'libsolidity/analysis/ViewPureChecker.cpp') diff --git a/libsolidity/analysis/ViewPureChecker.cpp b/libsolidity/analysis/ViewPureChecker.cpp index d936ada0..e92ad2fa 100644 --- a/libsolidity/analysis/ViewPureChecker.cpp +++ b/libsolidity/analysis/ViewPureChecker.cpp @@ -295,7 +295,7 @@ void ViewPureChecker::endVisit(MemberAccess const& _memberAccess) { // we can ignore the kind of magic and only look at the name of the member set static const pureMembers{ - "encode", "encodePacked", "encodeWithSelector", "encodeWithSignature", "data", "sig", "blockhash" + "encode", "encodePacked", "encodeWithSelector", "encodeWithSignature", "decode", "data", "sig", "blockhash" }; if (!pureMembers.count(member)) mutability = StateMutability::View; -- cgit v1.2.3 From 75a92b0ffd0946c17a27a58e6e02abe96cd3fa00 Mon Sep 17 00:00:00 2001 From: Erik Kundt Date: Thu, 26 Jul 2018 21:45:24 +0200 Subject: Warns if modifier uses msg.value in non-payable function. --- libsolidity/analysis/ViewPureChecker.cpp | 123 ++++++++++++++++++++++--------- 1 file changed, 87 insertions(+), 36 deletions(-) (limited to 'libsolidity/analysis/ViewPureChecker.cpp') diff --git a/libsolidity/analysis/ViewPureChecker.cpp b/libsolidity/analysis/ViewPureChecker.cpp index e92ad2fa..291b4681 100644 --- a/libsolidity/analysis/ViewPureChecker.cpp +++ b/libsolidity/analysis/ViewPureChecker.cpp @@ -142,7 +142,7 @@ bool ViewPureChecker::visit(FunctionDefinition const& _funDef) { solAssert(!m_currentFunction, ""); m_currentFunction = &_funDef; - m_currentBestMutability = StateMutability::Pure; + m_bestMutabilityAndLocation = {StateMutability::Pure, _funDef.location()}; return true; } @@ -150,7 +150,7 @@ void ViewPureChecker::endVisit(FunctionDefinition const& _funDef) { solAssert(m_currentFunction == &_funDef, ""); if ( - m_currentBestMutability < _funDef.stateMutability() && + m_bestMutabilityAndLocation.mutability < _funDef.stateMutability() && _funDef.stateMutability() != StateMutability::Payable && _funDef.isImplemented() && !_funDef.isConstructor() && @@ -159,22 +159,22 @@ void ViewPureChecker::endVisit(FunctionDefinition const& _funDef) ) m_errorReporter.warning( _funDef.location(), - "Function state mutability can be restricted to " + stateMutabilityToString(m_currentBestMutability) + "Function state mutability can be restricted to " + stateMutabilityToString(m_bestMutabilityAndLocation.mutability) ); m_currentFunction = nullptr; } -bool ViewPureChecker::visit(ModifierDefinition const&) +bool ViewPureChecker::visit(ModifierDefinition const& _modifier) { solAssert(m_currentFunction == nullptr, ""); - m_currentBestMutability = StateMutability::Pure; + m_bestMutabilityAndLocation = {StateMutability::Pure, _modifier.location()}; return true; } void ViewPureChecker::endVisit(ModifierDefinition const& _modifierDef) { solAssert(m_currentFunction == nullptr, ""); - m_inferredMutability[&_modifierDef] = m_currentBestMutability; + m_inferredMutability[&_modifierDef] = std::move(m_bestMutabilityAndLocation); } void ViewPureChecker::endVisit(Identifier const& _identifier) @@ -219,36 +219,70 @@ void ViewPureChecker::endVisit(InlineAssembly const& _inlineAssembly) }(_inlineAssembly.operations()); } -void ViewPureChecker::reportMutability(StateMutability _mutability, SourceLocation const& _location) +void ViewPureChecker::reportMutability( + StateMutability _mutability, + SourceLocation const& _location, + boost::optional const& _nestedLocation +) { - if (m_currentFunction && m_currentFunction->stateMutability() < _mutability) + if (_mutability > m_bestMutabilityAndLocation.mutability) + m_bestMutabilityAndLocation = MutabilityAndLocation{_mutability, _location}; + if (!m_currentFunction || _mutability <= m_currentFunction->stateMutability()) + return; + + // Check for payable here, because any occurrence of `msg.value` + // will set mutability to payable. + if (_mutability == StateMutability::View || ( + _mutability == StateMutability::Payable && + m_currentFunction->stateMutability() == StateMutability::Pure + )) { - if (_mutability == StateMutability::View) - m_errorReporter.typeError( - _location, - "Function declared as pure, but this expression (potentially) reads from the " - "environment or state and thus requires \"view\"." - ); - else if (_mutability == StateMutability::NonPayable) - m_errorReporter.typeError( - _location, - "Function declared as " + - stateMutabilityToString(m_currentFunction->stateMutability()) + - ", but this expression (potentially) modifies the state and thus " - "requires non-payable (the default) or payable." - ); - else - solAssert(false, ""); - - solAssert( - m_currentFunction->stateMutability() == StateMutability::View || - m_currentFunction->stateMutability() == StateMutability::Pure, - "" + m_errorReporter.typeError( + _location, + "Function declared as pure, but this expression (potentially) reads from the " + "environment or state and thus requires \"view\"." ); m_errors = true; } - if (_mutability > m_currentBestMutability) - m_currentBestMutability = _mutability; + else if (_mutability == StateMutability::NonPayable) + { + m_errorReporter.typeError( + _location, + "Function declared as " + + stateMutabilityToString(m_currentFunction->stateMutability()) + + ", but this expression (potentially) modifies the state and thus " + "requires non-payable (the default) or payable." + ); + m_errors = true; + } + else if (_mutability == StateMutability::Payable) + { + // We do not warn for library functions because they cannot be payable anyway. + // Also internal functions should be allowed to use `msg.value`. + if (m_currentFunction->isPublic() && m_currentFunction->inContractKind() != ContractDefinition::ContractKind::Library) + { + if (_nestedLocation) + m_errorReporter.warning( + _location, + "This modifier uses \"msg.value\" and thus the function should be payable.", + SecondarySourceLocation().append("\"msg.value\" appears here inside the modifier.", *_nestedLocation) + ); + else + m_errorReporter.warning( + _location, + "\"msg.value\" used in non-payable function. Do you want to add the \"payable\" modifier to this function?" + ); + } + } + else + solAssert(false, ""); + + solAssert( + m_currentFunction->stateMutability() == StateMutability::View || + m_currentFunction->stateMutability() == StateMutability::Pure || + m_currentFunction->stateMutability() == StateMutability::NonPayable, + "" + ); } void ViewPureChecker::endVisit(FunctionCall const& _functionCall) @@ -293,12 +327,28 @@ void ViewPureChecker::endVisit(MemberAccess const& _memberAccess) break; case Type::Category::Magic: { - // we can ignore the kind of magic and only look at the name of the member - set static const pureMembers{ - "encode", "encodePacked", "encodeWithSelector", "encodeWithSignature", "decode", "data", "sig", "blockhash" + using MagicMember = pair; + set static const pureMembers{ + {MagicType::Kind::ABI, "decode"}, + {MagicType::Kind::ABI, "encode"}, + {MagicType::Kind::ABI, "encodePacked"}, + {MagicType::Kind::ABI, "encodeWithSelector"}, + {MagicType::Kind::ABI, "encodeWithSignature"}, + {MagicType::Kind::Block, "blockhash"}, + {MagicType::Kind::Message, "data"}, + {MagicType::Kind::Message, "sig"} }; - if (!pureMembers.count(member)) + set static const payableMembers{ + {MagicType::Kind::Message, "value"} + }; + + auto const& type = dynamic_cast(*_memberAccess.expression().annotation().type); + MagicMember magicMember(type.kind(), member); + + if (!pureMembers.count(magicMember)) mutability = StateMutability::View; + if (payableMembers.count(magicMember)) + mutability = StateMutability::Payable; break; } case Type::Category::Struct: @@ -338,7 +388,8 @@ void ViewPureChecker::endVisit(ModifierInvocation const& _modifier) if (ModifierDefinition const* mod = dynamic_cast(_modifier.name()->annotation().referencedDeclaration)) { solAssert(m_inferredMutability.count(mod), ""); - reportMutability(m_inferredMutability.at(mod), _modifier.location()); + auto const& mutAndLocation = m_inferredMutability.at(mod); + reportMutability(mutAndLocation.mutability, _modifier.location(), mutAndLocation.location); } else solAssert(dynamic_cast(_modifier.name()->annotation().referencedDeclaration), ""); -- cgit v1.2.3 From 431c2fbcf370926d6c3e7e93667d8472f2f6b73b Mon Sep 17 00:00:00 2001 From: chriseth Date: Thu, 16 Aug 2018 17:28:49 +0200 Subject: Turn warning into error. --- libsolidity/analysis/ViewPureChecker.cpp | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) (limited to 'libsolidity/analysis/ViewPureChecker.cpp') diff --git a/libsolidity/analysis/ViewPureChecker.cpp b/libsolidity/analysis/ViewPureChecker.cpp index 291b4681..be6b7ef7 100644 --- a/libsolidity/analysis/ViewPureChecker.cpp +++ b/libsolidity/analysis/ViewPureChecker.cpp @@ -262,16 +262,18 @@ void ViewPureChecker::reportMutability( if (m_currentFunction->isPublic() && m_currentFunction->inContractKind() != ContractDefinition::ContractKind::Library) { if (_nestedLocation) - m_errorReporter.warning( + m_errorReporter.typeError( _location, - "This modifier uses \"msg.value\" and thus the function should be payable.", - SecondarySourceLocation().append("\"msg.value\" appears here inside the modifier.", *_nestedLocation) + SecondarySourceLocation().append("\"msg.value\" appears here inside the modifier.", *_nestedLocation), + "This modifier uses \"msg.value\" and thus the function has to be payable or internal." ); else - m_errorReporter.warning( + m_errorReporter.typeError( _location, - "\"msg.value\" used in non-payable function. Do you want to add the \"payable\" modifier to this function?" + "\"msg.value\" can only be used in payable public functions. Make the function " + "\"payable\" or use an internal function to avoid this error." ); + m_errors = true; } } else -- cgit v1.2.3 From 87804b6419a5894601441efe511015adda5fb119 Mon Sep 17 00:00:00 2001 From: Daniel Kirchner Date: Mon, 3 Sep 2018 17:45:58 +0200 Subject: Split IntegerType into IntegerType and AddressType. --- libsolidity/analysis/ViewPureChecker.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'libsolidity/analysis/ViewPureChecker.cpp') diff --git a/libsolidity/analysis/ViewPureChecker.cpp b/libsolidity/analysis/ViewPureChecker.cpp index be6b7ef7..113a3177 100644 --- a/libsolidity/analysis/ViewPureChecker.cpp +++ b/libsolidity/analysis/ViewPureChecker.cpp @@ -323,7 +323,7 @@ void ViewPureChecker::endVisit(MemberAccess const& _memberAccess) ASTString const& member = _memberAccess.memberName(); switch (_memberAccess.expression().annotation().type->category()) { - case Type::Category::Integer: + case Type::Category::Address: if (member == "balance") mutability = StateMutability::View; break; -- cgit v1.2.3 From d76bfcd935407e7249cfb8480a29da24615667cf Mon Sep 17 00:00:00 2001 From: chriseth Date: Thu, 4 Oct 2018 13:03:55 +0200 Subject: Fix typos. --- libsolidity/analysis/ViewPureChecker.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'libsolidity/analysis/ViewPureChecker.cpp') diff --git a/libsolidity/analysis/ViewPureChecker.cpp b/libsolidity/analysis/ViewPureChecker.cpp index 113a3177..b0cacc43 100644 --- a/libsolidity/analysis/ViewPureChecker.cpp +++ b/libsolidity/analysis/ViewPureChecker.cpp @@ -292,11 +292,11 @@ void ViewPureChecker::endVisit(FunctionCall const& _functionCall) if (_functionCall.annotation().kind != FunctionCallKind::FunctionCall) return; - StateMutability mut = dynamic_cast(*_functionCall.expression().annotation().type).stateMutability(); + StateMutability mutability = dynamic_cast(*_functionCall.expression().annotation().type).stateMutability(); // We only require "nonpayable" to call a payble function. - if (mut == StateMutability::Payable) - mut = StateMutability::NonPayable; - reportMutability(mut, _functionCall.location()); + if (mutability == StateMutability::Payable) + mutability = StateMutability::NonPayable; + reportMutability(mutability, _functionCall.location()); } bool ViewPureChecker::visit(MemberAccess const& _memberAccess) -- cgit v1.2.3