Merge pull request #2749 from ethereum/require-visibility

Warn if no visibility is specified on contract functions.

Changelog.md

@@ -4,6 +4,7 @@ Features:
  * Support ``pragma experimental v0.5.0;`` to turn on upcoming breaking changes.
  * Code Generator: Added ``.selector`` member on external function types to retrieve their signature.
  * Optimizer: Add new optimization step to remove unused ``JUMPDEST``s.
+ * Syntax Checker: Warn if no visibility is specified on contract functions.
  * Type Checker: Display helpful warning for unused function arguments/return parameters.
  * Type Checker: Do not show the same error multiple times for events.
  * Type Checker: Greatly reduce the number of duplicate errors shown for duplicate constructors and functions.

libsolidity/analysis/SyntaxChecker.cpp

@@ -193,6 +193,18 @@ bool SyntaxChecker::visit(PlaceholderStatement const&)
 	return true;
 }
 
+bool SyntaxChecker::visit(FunctionDefinition const& _function)
+{
+	if (_function.noVisibilitySpecified())
+		m_errorReporter.warning(
+			_function.location(),
+			"No visibility specified. Defaulting to \"" +
+			Declaration::visibilityToString(_function.visibility()) +
+			"\"."
+		);
+	return true;
+}
+
 bool SyntaxChecker::visit(FunctionTypeName const& _node)
 {
 	for (auto const& decl: _node.parameterTypeList()->parameters())

libsolidity/analysis/SyntaxChecker.h

@@ -66,6 +66,7 @@ private:
 
 	virtual bool visit(PlaceholderStatement const& _placeholderStatement) override;
 
+	virtual bool visit(FunctionDefinition const& _function) override;
 	virtual bool visit(FunctionTypeName const& _node) override;
 
 	ErrorReporter& m_errorReporter;

libsolidity/ast/AST.h

@@ -180,6 +180,7 @@ public:
 
 	/// @returns the declared name.
 	ASTString const& name() const { return *m_name; }
+	bool noVisibilitySpecified() const { return m_visibility == Visibility::Default; }
 	Visibility visibility() const { return m_visibility == Visibility::Default ? defaultVisibility() : m_visibility; }
 	bool isPublic() const { return visibility() >= Visibility::Public; }
 	virtual bool isVisibleInContract() const { return visibility() != Visibility::External; }

std/StandardToken.sol

@@ -8,24 +8,24 @@ contract StandardToken is Token {
 	mapping (address =>
 		mapping (address => uint256)) m_allowance;
 
-	function StandardToken(address _initialOwner, uint256 _supply) {
+	function StandardToken(address _initialOwner, uint256 _supply) public {
 		supply = _supply;
 		balance[_initialOwner] = _supply;
 	}
 
-	function balanceOf(address _account) constant returns (uint) {
+	function balanceOf(address _account) constant public returns (uint) {
 		return balance[_account];
 	}
 
-	function totalSupply() constant returns (uint) {
+	function totalSupply() constant public returns (uint) {
 		return supply;
 	}
 
-	function transfer(address _to, uint256 _value) returns (bool success) {
+	function transfer(address _to, uint256 _value) public returns (bool success) {
 		return doTransfer(msg.sender, _to, _value);
 	}
 
-	function transferFrom(address _from, address _to, uint256 _value) returns (bool) {
+	function transferFrom(address _from, address _to, uint256 _value) public returns (bool) {
 		if (m_allowance[_from][msg.sender] >= _value) {
 			if (doTransfer(_from, _to, _value)) {
 				m_allowance[_from][msg.sender] -= _value;
@@ -47,13 +47,13 @@ contract StandardToken is Token {
 		}
 	}
 
-	function approve(address _spender, uint256 _value) returns (bool success) {
+	function approve(address _spender, uint256 _value) public returns (bool success) {
 		m_allowance[msg.sender][_spender] = _value;
 		Approval(msg.sender, _spender, _value);
 		return true;
 	}
 
-	function allowance(address _owner, address _spender) constant returns (uint256) {
+	function allowance(address _owner, address _spender) constant public returns (uint256) {
 		return m_allowance[_owner][_spender];
 	}
 }

std/Token.sol

@@ -4,10 +4,10 @@ contract Token {
 	event Transfer(address indexed _from, address indexed _to, uint256 _value);
 	event Approval(address indexed _owner, address indexed _spender, uint256 _value);
 
-	function totalSupply() constant returns (uint256 supply);
+	function totalSupply() constant public returns (uint256 supply);
-	function balanceOf(address _owner) constant returns (uint256 balance);
+	function balanceOf(address _owner) constant public returns (uint256 balance);
-	function transfer(address _to, uint256 _value) returns (bool success);
+	function transfer(address _to, uint256 _value) public returns (bool success);
-	function transferFrom(address _from, address _to, uint256 _value) returns (bool success);
+	function transferFrom(address _from, address _to, uint256 _value) public returns (bool success);
-	function approve(address _spender, uint256 _value) returns (bool success);
+	function approve(address _spender, uint256 _value) public returns (bool success);
-	function allowance(address _owner, address _spender) constant returns (uint256 remaining);
+	function allowance(address _owner, address _spender) constant public returns (uint256 remaining);
 }

std/mortal.sol

@@ -3,7 +3,7 @@ pragma solidity ^0.4.0;
 import "./owned.sol";
 
 contract mortal is owned {
-    function kill() {
+    function kill() public {
         if (msg.sender == owner)
             selfdestruct(owner);
     }

std/owned.sol

@@ -9,7 +9,7 @@ contract owned {
         }
     }
 
-    function owned() {
+    function owned() public {
         owner = msg.sender;
     }
 }

test/libsolidity/ViewPureChecker.cpp

@@ -40,10 +40,10 @@ BOOST_AUTO_TEST_CASE(smoke_test)
 	char const* text = R"(
 		contract C {
 			uint x;
-			function g() pure {}
+			function g() pure public {}
-			function f() view returns (uint) { return now; }
+			function f() view public returns (uint) { return now; }
-			function h() { x = 2; }
+			function h() public { x = 2; }
-			function i() payable { x = 2; }
+			function i() payable public { x = 2; }
 		}
 	)";
 	CHECK_SUCCESS_NO_WARNINGS(text);
@@ -53,10 +53,10 @@ BOOST_AUTO_TEST_CASE(call_internal_functions_success)
 {
 	char const* text = R"(
 		contract C {
-			function g() pure { g(); }
+			function g() pure public { g(); }
-			function f() view returns (uint) { f(); g(); }
+			function f() view public returns (uint) { f(); g(); }
-			function h() { h(); g(); f(); }
+			function h() public { h(); g(); f(); }
-			function i() payable { i(); h(); g(); f(); }
+			function i() payable public { i(); h(); g(); f(); }
 		}
 	)";
 	CHECK_SUCCESS_NO_WARNINGS(text);
@@ -66,7 +66,7 @@ BOOST_AUTO_TEST_CASE(suggest_pure)
 {
 	char const* text = R"(
 		contract C {
-			function g() view { }
+			function g() view public { }
 		}
 	)";
 	CHECK_WARNING(text, "can be restricted to pure");
@@ -77,7 +77,7 @@ BOOST_AUTO_TEST_CASE(suggest_view)
 	char const* text = R"(
 		contract C {
 			uint x;
-			function g() returns (uint) { return x; }
+			function g() public returns (uint) { return x; }
 		}
 	)";
 	CHECK_WARNING(text, "can be restricted to view");
@@ -86,7 +86,7 @@ BOOST_AUTO_TEST_CASE(suggest_view)
 BOOST_AUTO_TEST_CASE(call_internal_functions_fail)
 {
 	CHECK_ERROR(
-		"contract C{ function f() pure { g(); } function g() view {} }",
+		"contract C{ function f() pure public { g(); } function g() view public {} }",
 		TypeError,
 		"Function declared as pure, but this expression (potentially) reads from the environment or state and thus requires \"view\""
 	);
@@ -95,7 +95,7 @@ BOOST_AUTO_TEST_CASE(call_internal_functions_fail)
 BOOST_AUTO_TEST_CASE(write_storage_fail)
 {
 	CHECK_WARNING(
-		"contract C{ uint x; function f() view { x = 2; } }",
+		"contract C{ uint x; function f() view public { x = 2; } }",
 		"Function declared as view, but this expression (potentially) modifies the state and thus requires non-payable (the default) or payable."
 	);
 }
@@ -129,7 +129,7 @@ BOOST_AUTO_TEST_CASE(environment_access)
 	for (string const& x: view)
 	{
 		CHECK_ERROR(
-			"contract C { function f() pure { var x = " + x + "; x; } }",
+			"contract C { function f() pure public { var x = " + x + "; x; } }",
 			TypeError,
 			"Function declared as pure, but this expression (potentially) reads from the environment or state and thus requires \"view\""
 		);
@@ -137,7 +137,7 @@ BOOST_AUTO_TEST_CASE(environment_access)
 	for (string const& x: pure)
 	{
 		CHECK_WARNING(
-			"contract C { function f() view { var x = " + x + "; x; } }",
+			"contract C { function f() view public { var x = " + x + "; x; } }",
 			"restricted to pure"
 		);
 	}
@@ -153,15 +153,15 @@ BOOST_AUTO_TEST_CASE(modifiers)
 			modifier nonpayablem(uint) { x = 2; _; }
 		}
 		contract C is D {
-			function f() purem(0) pure {}
+			function f() purem(0) pure public {}
-			function g() viewm(0) view {}
+			function g() viewm(0) view public {}
-			function h() nonpayablem(0) {}
+			function h() nonpayablem(0) public {}
-			function i() purem(x) view {}
+			function i() purem(x) view public {}
-			function j() viewm(x) view {}
+			function j() viewm(x) view public {}
-			function k() nonpayablem(x) {}
+			function k() nonpayablem(x) public {}
-			function l() purem(x = 2) {}
+			function l() purem(x = 2) public {}
-			function m() viewm(x = 2) {}
+			function m() viewm(x = 2) public {}
-			function n() nonpayablem(x = 2) {}
+			function n() nonpayablem(x = 2) public {}
 		}
 	)";
 	CHECK_SUCCESS_NO_WARNINGS(text);
@@ -171,10 +171,10 @@ BOOST_AUTO_TEST_CASE(interface)
 {
 	string text = R"(
 		interface D {
-			function f() view;
+			function f() view public;
 		}
 		contract C is D {
-			function f() view {}
+			function f() view public {}
 		}
 	)";
 	CHECK_SUCCESS_NO_WARNINGS(text);
@@ -185,10 +185,10 @@ BOOST_AUTO_TEST_CASE(overriding)
 	string text = R"(
 		contract D {
 			uint x;
-			function f() { x = 2; }
+			function f() public { x = 2; }
 		}
 		contract C is D {
-			function f() {}
+			function f() public {}
 		}
 	)";
 	CHECK_SUCCESS_NO_WARNINGS(text);
@@ -203,12 +203,10 @@ BOOST_AUTO_TEST_CASE(returning_structs)
 			function f() view internal returns (S storage) {
 				return s;
 			}
-			function g()
+			function g() public {
-			{
 				f().x = 2;
 			}
-			function h() view
+			function h() view public {
-			{
 				f();
 				f().x;
 			}
@@ -222,13 +220,13 @@ BOOST_AUTO_TEST_CASE(mappings)
 	string text = R"(
 		contract C {
 			mapping(uint => uint) a;
-			function f() view {
+			function f() view public {
 				a;
 			}
-			function g() view {
+			function g() view public {
 				a[2];
 			}
-			function h() {
+			function h() public {
 				a[2] = 3;
 			}
 		}
@@ -242,18 +240,18 @@ BOOST_AUTO_TEST_CASE(local_storage_variables)
 		contract C {
 			struct S { uint a; }
 			S s;
-			function f() view {
+			function f() view public {
 				S storage x = s;
 				x;
 			}
-			function g() view {
+			function g() view public {
 				S storage x = s;
 				x = s;
 			}
-			function i() {
+			function i() public {
 				s.a = 2;
 			}
-			function h() {
+			function h() public {
 				S storage x = s;
 				x.a = 2;
 			}
@@ -266,14 +264,14 @@ BOOST_AUTO_TEST_CASE(builtin_functions)
 {
 	string text = R"(
 		contract C {
-			function f() {
+			function f() public {
 				this.transfer(1);
 				require(this.send(2));
 				selfdestruct(this);
 				require(this.delegatecall());
 				require(this.call());
 			}
-			function g() pure {
+			function g() pure public {
 				var x = keccak256("abc");
 				var y = sha256("abc");
 				var z = ecrecover(1, 2, 3, 4);
@@ -281,7 +279,7 @@ BOOST_AUTO_TEST_CASE(builtin_functions)
 				assert(true);
 				x; y; z;
 			}
-			function() payable {}
+			function() payable public {}
 		}
 	)";
 	CHECK_SUCCESS_NO_WARNINGS(text);
@@ -291,7 +289,7 @@ BOOST_AUTO_TEST_CASE(function_types)
 {
 	string text = R"(
 		contract C {
-			function f() pure {
+			function f() pure public {
 				function () external nonpayFun;
 				function () external view viewFun;
 				function () external pure pureFun;
@@ -301,12 +299,12 @@ BOOST_AUTO_TEST_CASE(function_types)
 				pureFun;
 				pureFun();
 			}
-			function g() view {
+			function g() view public {
 				function () external view viewFun;
 
 				viewFun();
 			}
-			function h() {
+			function h() public {
 				function () external nonpayFun;
 
 				nonpayFun();
@@ -321,7 +319,7 @@ BOOST_AUTO_TEST_CASE(creation)
 	string text = R"(
 		contract D {}
 		contract C {
-			function f() { new D(); }
+			function f() public { new D(); }
 		}
 	)";
 	CHECK_SUCCESS_NO_WARNINGS(text);
@@ -333,20 +331,20 @@ BOOST_AUTO_TEST_CASE(assembly)
 		contract C {
 			struct S { uint x; }
 			S s;
-			function e() pure {
+			function e() pure public {
 				assembly { mstore(keccak256(0, 20), mul(s_slot, 2)) }
 			}
-			function f() pure {
+			function f() pure public {
 				uint x;
 				assembly { x := 7 }
 			}
-			function g() view {
+			function g() view public {
 				assembly { for {} 1 { pop(sload(0)) } { } }
 			}
-			function h() view {
+			function h() view public {
 				assembly { function g() { pop(blockhash(20)) } }
 			}
-			function j()  {
+			function j() public {
 				assembly { pop(call(0, 1, 2, 3, 4, 5, 6)) }
 			}
 		}
@@ -358,7 +356,7 @@ BOOST_AUTO_TEST_CASE(assembly_staticcall)
 {
 	string text = R"(
 		contract C {
-			function i() view {
+			function i() view public {
 				assembly { pop(staticcall(0, 1, 2, 3, 4, 5)) }
 			}
 		}
@@ -370,7 +368,7 @@ BOOST_AUTO_TEST_CASE(assembly_jump)
 {
 	string text = R"(
 		contract C {
-			function k()  {
+			function k() public {
 				assembly { jump(2) }
 			}
 		}
@@ -383,7 +381,7 @@ BOOST_AUTO_TEST_CASE(constant)
 	string text = R"(
 		contract C {
 			uint constant x = 2;
-			function k() pure returns (uint) {
+			function k() pure public returns (uint) {
 				return x;
 			}
 		}