Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn about unused return values. #677

Merged
merged 2 commits into from
Jun 27, 2016
Merged

Warn about unused return values. #677

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc6314c
Warn about unused return values.
chriseth Jun 21, 2016
25a64c7
Only warn about unused return in low-level functions.
chriseth Jun 24, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions libsolidity/analysis/TypeChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -830,6 +830,22 @@ void TypeChecker::endVisit(ExpressionStatement const& _statement)
if (type(_statement.expression())->category() == Type::Category::RationalNumber)
if (!dynamic_cast<RationalNumberType const&>(*type(_statement.expression())).mobileType())
typeError(_statement.expression().location(), "Invalid rational number.");

if (auto call = dynamic_cast<FunctionCall const*>(&_statement.expression()))
{
if (auto callType = dynamic_cast<FunctionType const*>(type(call->expression()).get()))
{
using Location = FunctionType::Location;
Location location = callType->location();
if (
location == Location::Bare ||
location == Location::BareCallCode ||
location == Location::BareDelegateCall ||
location == Location::Send
)
warning(_statement.location(), "Return value of low-level calls not used.");
}
}
}

bool TypeChecker::visit(Conditional const& _conditional)
Expand Down Expand Up @@ -1570,6 +1586,16 @@ void TypeChecker::typeError(SourceLocation const& _location, string const& _desc
m_errors.push_back(err);
}

void TypeChecker::warning(SourceLocation const& _location, string const& _description)
{
auto err = make_shared<Error>(Error::Type::Warning);
*err <<
errinfo_sourceLocation(_location) <<
errinfo_comment(_description);

m_errors.push_back(err);
}

void TypeChecker::fatalTypeError(SourceLocation const& _location, string const& _description)
{
typeError(_location, _description);
Expand Down
3 changes: 3 additions & 0 deletions libsolidity/analysis/TypeChecker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ class TypeChecker: private ASTConstVisitor
/// Adds a new error to the list of errors.
void typeError(SourceLocation const& _location, std::string const& _description);

/// Adds a new warning to the list of errors.
void warning(SourceLocation const& _location, std::string const& _description);

/// Adds a new error to the list of errors and throws to abort type checking.
void fatalTypeError(SourceLocation const& _location, std::string const& _description);

Expand Down
73 changes: 73 additions & 0 deletions test/libsolidity/SolidityNameAndTypeResolution.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3750,6 +3750,79 @@ BOOST_AUTO_TEST_CASE(one_divided_by_three_integer_conversion)
BOOST_CHECK(!success(text));
}

BOOST_AUTO_TEST_CASE(unused_return_value)
{
char const* text = R"(
contract test {
function g() returns (uint) {}
function f() {
g();
}
}
)";
BOOST_CHECK(success(text));
}

BOOST_AUTO_TEST_CASE(unused_return_value_send)
{
char const* text = R"(
contract test {
function f() {
address(0x12).send(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually really quick, just trying to understand this bit. Why does this one throw a warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the return value of send is ignored.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of me says this is smart, but then the other part of me says this can get annoying when it comes to void contracts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VoR0220 Do void contracts ignore return values? (BTW, what's a void contract?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have said void contract function. That is my terminology for a contract function that doesn't return a value. Although with the send...yeah you should be checking to make sure it happened.

Copy link
Contributor

@redsquirrel redsquirrel Jun 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

address(0x12).send(1); this call's return value is ignored. this is the source of the warning, not f().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, but that should be irrelevant if it's inside function f's scope.

Copy link
Contributor

@redsquirrel redsquirrel Jun 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if what I'm about to say is obvious or wrong. Bear with me as I'm learning lots at once. After a quick review of the code (disclaimer: I never learned C/C++ very well), it looks like this is part of the visitor pattern. If that's the case, then it looks like this code is being run on every single ExpressionStatement in the AST. So I think that means scope is irrelevant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, and I'm saying scope should be relevant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually...I just realized I'm wrong. This is good to merge.

}
}
)";
BOOST_CHECK(expectError(text, true) == Error::Type::Warning);
}

BOOST_AUTO_TEST_CASE(unused_return_value_call)
{
char const* text = R"(
contract test {
function f() {
address(0x12).call("abc");
}
}
)";
BOOST_CHECK(expectError(text, true) == Error::Type::Warning);
}

BOOST_AUTO_TEST_CASE(unused_return_value_call_value)
{
char const* text = R"(
contract test {
function f() {
address(0x12).call.value(2)("abc");
}
}
)";
BOOST_CHECK(expectError(text, true) == Error::Type::Warning);
}

BOOST_AUTO_TEST_CASE(unused_return_value_callcode)
{
char const* text = R"(
contract test {
function f() {
address(0x12).callcode("abc");
}
}
)";
BOOST_CHECK(expectError(text, true) == Error::Type::Warning);
}

BOOST_AUTO_TEST_CASE(unused_return_value_delegatecall)
{
char const* text = R"(
contract test {
function f() {
address(0x12).delegatecall("abc");
}
}
)";
BOOST_CHECK(expectError(text, true) == Error::Type::Warning);
}

BOOST_AUTO_TEST_SUITE_END()

}
Expand Down