Merge pull request #4430 from ethereum/enforceVisibilitySpecifier

[BREAKING] Enforce visibility specifier

18 files changed, 48 insertions, 53 deletions

diff --git a/Changelog.md b/Changelog.md
index f3c06d90..1064d8f6 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -3,6 +3,7 @@
 How to update your code:
  * Change every ``.call()`` to a ``.call("")`` and every ``.call(signature, a, b, c)`` to use ``.call(abi.encodeWithSignature(signature, a, b, c))`` (the last one only works for value types).
  * Change every ``keccak256(a, b, c)`` to ``keccak256(abi.encodePacked(a, b, c))``.
+ * Add ``public`` to every function and ``external`` to every fallback or interface function that does not specify its visibility already.
  * Make your fallback functions ``external``.
  * Explicitly state the storage location for local variables of struct and array types, e.g. change ``uint[] x = m_x`` to ``uint[] storage x = m_x``.
 
@@ -49,6 +50,7 @@ Breaking Changes:
  * Remove obsolete ``std`` directory from the Solidity repository. This means accessing ``https://github.com/ethereum/soldity/blob/develop/std/*.sol`` (or ``https://github.com/ethereum/solidity/std/*.sol`` in Remix) will not be possible.
  * References Resolver: Turn missing storage locations into an error. This was already the case in the experimental 0.5.0 mode.
  * Syntax Checker: Named return values in function types are an error.
+ * Syntax Checker: Strictly require visibility specifier for functions. This was already the case in the experimental 0.5.0 mode.
  * Syntax Checker: Disallow unary ``+``. This was already the case in the experimental 0.5.0 mode.
  * View Pure Checker: Strictly enfore state mutability. This was already the case in the experimental 0.5.0 mode.
 
diff --git a/docs/contracts.rst b/docs/contracts.rst
index 033e9a45..51d7923d 100644
--- a/docs/contracts.rst
+++ b/docs/contracts.rst
@@ -131,10 +131,9 @@ a "message call") and external
 ones that do), there are four types of visibilities for
 functions and state variables.
 
-Functions can be specified as being ``external``,
-``public``, ``internal`` or ``private``, where the default is
-``public``. For state variables, ``external`` is not possible
-and the default is ``internal``.
+Functions have to be specified as being ``external``,
+``public``, ``internal`` or ``private``.
+For state variables, ``external`` is not possible.
 
 ``external``:
     External functions are part of the contract
@@ -850,10 +849,10 @@ Details are given in the following example.
 
 ::
 
-    pragma solidity ^0.4.22;
+    pragma solidity >0.4.24;
 
     contract owned {
-        constructor() { owner = msg.sender; }
+        constructor() public { owner = msg.sender; }
         address owner;
     }
 
@@ -862,7 +861,7 @@ Details are given in the following example.
     // internal functions and state variables. These cannot be
     // accessed externally via `this`, though.
     contract mortal is owned {
-        function kill() {
+        function kill() public {
             if (msg.sender == owner) selfdestruct(owner);
         }
     }
@@ -884,7 +883,7 @@ Details are given in the following example.
     // also a base class of `mortal`, yet there is only a single
     // instance of `owned` (as for virtual inheritance in C++).
     contract named is owned, mortal {
-        constructor(bytes32 name) {
+        constructor(bytes32 name) public {
             Config config = Config(0xD5f9D8D94886E70b06E474c3fB14Fd43E2f23970);
             NameReg(config.lookup(1)).register(name);
         }
diff --git a/docs/control-structures.rst b/docs/control-structures.rst
index ead236c4..9fd017db 100644
--- a/docs/control-structures.rst
+++ b/docs/control-structures.rst
@@ -465,10 +465,10 @@ The following example shows how an error string can be used together with revert
 
 ::
 
-    pragma solidity ^0.4.22;
+    pragma solidity >0.4.24;
 
     contract VendingMachine {
-        function buy(uint amount) payable {
+        function buy(uint amount) public payable {
             if (amount > msg.value / 2 ether)
                 revert("Not enough Ether provided.");
             // Alternative way to do it:
diff --git a/docs/layout-of-source-files.rst b/docs/layout-of-source-files.rst
index 81faf816..46ef3d57 100644
--- a/docs/layout-of-source-files.rst
+++ b/docs/layout-of-source-files.rst
@@ -205,7 +205,7 @@ for the two input parameters and two returned values.
           * @return s The calculated surface.
           * @return p The calculated perimeter.
           */
-        function rectangle(uint w, uint h) returns (uint s, uint p) {
+        function rectangle(uint w, uint h) public returns (uint s, uint p) {
             s = w * h;
             p = 2 * (w + h);
         }
diff --git a/docs/types.rst b/docs/types.rst
index 0fe36757..c81dee33 100644
--- a/docs/types.rst
+++ b/docs/types.rst
@@ -483,7 +483,7 @@ Another example that uses external function types::
 
     contract OracleUser {
       Oracle constant oracle = Oracle(0x1234567); // known contract
-      function buySomething() {
+      function buySomething() public {
         oracle.query("USD", this.oracleResponse);
       }
       function oracleResponse(bytes memory response) public {
@@ -979,4 +979,3 @@ converted to a matching size. This makes alignment and padding explicit::
     uint16 x = 0xffff;
     bytes32(uint256(x)); // pad on the left
     bytes32(bytes2(x)); // pad on the right
-
diff --git a/libsolidity/analysis/StaticAnalyzer.cpp b/libsolidity/analysis/StaticAnalyzer.cpp
index 323282ca..60a58665 100644
--- a/libsolidity/analysis/StaticAnalyzer.cpp
+++ b/libsolidity/analysis/StaticAnalyzer.cpp
@@ -51,16 +51,6 @@ void StaticAnalyzer::endVisit(ContractDefinition const&)
 
 bool StaticAnalyzer::visit(FunctionDefinition const& _function)
 {
-	const bool isInterface = m_currentContract->contractKind() == ContractDefinition::ContractKind::Interface;
-
-	if (_function.noVisibilitySpecified())
-		m_errorReporter.warning(
-			_function.location(),
-			"No visibility specified. Defaulting to \"" +
-			Declaration::visibilityToString(_function.visibility()) +
-			"\"." +
-			(isInterface ? " In interfaces it defaults to external." : "")
-		);
 	if (_function.isImplemented())
 		m_currentFunction = &_function;
 	else
diff --git a/libsolidity/analysis/SyntaxChecker.cpp b/libsolidity/analysis/SyntaxChecker.cpp
index fba18a45..4d09e36d 100644
--- a/libsolidity/analysis/SyntaxChecker.cpp
+++ b/libsolidity/analysis/SyntaxChecker.cpp
@@ -197,12 +197,24 @@ bool SyntaxChecker::visit(PlaceholderStatement const&)
 	return true;
 }
 
+bool SyntaxChecker::visit(ContractDefinition const& _contract)
+{
+	m_isInterface = _contract.contractKind() == ContractDefinition::ContractKind::Interface;
+	return true;
+}
+
 bool SyntaxChecker::visit(FunctionDefinition const& _function)
 {
 	bool const v050 = m_sourceUnit->annotation().experimentalFeatures.count(ExperimentalFeature::V050);
 
-	if (v050 && _function.noVisibilitySpecified())
-		m_errorReporter.syntaxError(_function.location(), "No visibility specified.");
+	if (_function.noVisibilitySpecified())
+	{
+		string suggestedVisibility = _function.isFallback() || m_isInterface ? "external" : "public";
+		m_errorReporter.syntaxError(
+			_function.location(),
+			"No visibility specified. Did you intend to add \"" + suggestedVisibility + "\"?"
+		);
+	}
 
 	if (_function.isOldStyleConstructor())
 	{
diff --git a/libsolidity/analysis/SyntaxChecker.h b/libsolidity/analysis/SyntaxChecker.h
index 5460e3be..28a0f66e 100644
--- a/libsolidity/analysis/SyntaxChecker.h
+++ b/libsolidity/analysis/SyntaxChecker.h
@@ -66,6 +66,7 @@ private:
 
 	virtual bool visit(PlaceholderStatement const& _placeholderStatement) override;
 
+	virtual bool visit(ContractDefinition const& _contract) override;
 	virtual bool visit(FunctionDefinition const& _function) override;
 	virtual bool visit(FunctionTypeName const& _node) override;
 
@@ -82,6 +83,7 @@ private:
 	bool m_versionPragmaFound = false;
 
 	int m_inLoopDepth = 0;
+	bool m_isInterface = false;
 
 	SourceUnit const* m_sourceUnit = nullptr;
 };
diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp
index 0fe21c4a..d8f2f531 100644
--- a/test/libsolidity/SolidityEndToEndTest.cpp
+++ b/test/libsolidity/SolidityEndToEndTest.cpp
@@ -670,7 +670,7 @@ BOOST_AUTO_TEST_CASE(nested_loops_multiple_local_vars)
 	// and free local variables properly
 	char const* sourceCode = R"(
 		contract test {
-			function f(uint x) returns(uint y) {
+			function f(uint x) public returns(uint y) {
 				while (x > 0) {
 					uint z = x + 10;
 					uint k = z + 1;
@@ -9536,7 +9536,7 @@ BOOST_AUTO_TEST_CASE(continue_in_modifier)
 					_;
 				}
 			}
-			function f() run {
+			function f() run public {
 				uint k = x;
 				uint t = k + 1;
 				x = t;
@@ -9560,7 +9560,7 @@ BOOST_AUTO_TEST_CASE(return_in_modifier)
 					_;
 				}
 			}
-			function f() run {
+			function f() run public {
 				uint k = x;
 				uint t = k + 1;
 				x = t;
diff --git a/test/libsolidity/syntaxTests/constructor/constructor_no_visibility.sol b/test/libsolidity/syntaxTests/constructor/constructor_no_visibility.sol
index 88553084..586329b1 100644
--- a/test/libsolidity/syntaxTests/constructor/constructor_no_visibility.sol
+++ b/test/libsolidity/syntaxTests/constructor/constructor_no_visibility.sol
@@ -1,3 +1,3 @@
 contract A { constructor() {} }
 // ----
-// Warning: (13-29): No visibility specified. Defaulting to "public".
+// SyntaxError: (13-29): No visibility specified. Did you intend to add "public"?
diff --git a/test/libsolidity/syntaxTests/constructor/constructor_no_visibility_050.sol b/test/libsolidity/syntaxTests/constructor/constructor_no_visibility_050.sol
deleted file mode 100644
index 0f57a41f..00000000
--- a/test/libsolidity/syntaxTests/constructor/constructor_no_visibility_050.sol
+++ /dev/null
@@ -1,4 +0,0 @@
-pragma experimental "v0.5.0";
-contract A { constructor() {} }
-// ----
-// SyntaxError: (43-59): No visibility specified.
diff --git a/test/libsolidity/syntaxTests/fallback/default_visibility.sol b/test/libsolidity/syntaxTests/fallback/default_visibility.sol
index 31123d59..6fbb15a5 100644
--- a/test/libsolidity/syntaxTests/fallback/default_visibility.sol
+++ b/test/libsolidity/syntaxTests/fallback/default_visibility.sol
@@ -3,4 +3,5 @@ contract C {
     function() {}
 }
 // ----
+// SyntaxError: (90-103): No visibility specified. Did you intend to add "external"?
 // TypeError: (90-103): Fallback function must be defined as "external".
diff --git a/test/libsolidity/syntaxTests/nameAndTypeResolution/567_require_visibility_specifiers_v050.sol b/test/libsolidity/syntaxTests/nameAndTypeResolution/567_require_visibility_specifiers_v050.sol
deleted file mode 100644
index ec7c0937..00000000
--- a/test/libsolidity/syntaxTests/nameAndTypeResolution/567_require_visibility_specifiers_v050.sol
+++ /dev/null
@@ -1,6 +0,0 @@
-pragma experimental "v0.5.0";
-contract C {
-    function f() pure { }
-}
-// ----
-// SyntaxError: (47-68): No visibility specified.
diff --git a/test/libsolidity/syntaxTests/visibility/function_no_visibility.sol b/test/libsolidity/syntaxTests/visibility/function_no_visibility.sol
index ecc36f04..4fc7900f 100644
--- a/test/libsolidity/syntaxTests/visibility/function_no_visibility.sol
+++ b/test/libsolidity/syntaxTests/visibility/function_no_visibility.sol
@@ -2,4 +2,4 @@ contract C {
     function f() pure { }
 }
 // ----
-// Warning: (17-38): No visibility specified. Defaulting to "public".
+// SyntaxError: (17-38): No visibility specified. Did you intend to add "public"?
diff --git a/test/libsolidity/syntaxTests/visibility/function_no_visibility_050.sol b/test/libsolidity/syntaxTests/visibility/function_no_visibility_050.sol
deleted file mode 100644
index ec7c0937..00000000
--- a/test/libsolidity/syntaxTests/visibility/function_no_visibility_050.sol
+++ /dev/null
@@ -1,6 +0,0 @@
-pragma experimental "v0.5.0";
-contract C {
-    function f() pure { }
-}
-// ----
-// SyntaxError: (47-68): No visibility specified.
diff --git a/test/libsolidity/syntaxTests/visibility/interface/function_default.sol b/test/libsolidity/syntaxTests/visibility/interface/function_default.sol
index 161d66e1..b7e96e5e 100644
--- a/test/libsolidity/syntaxTests/visibility/interface/function_default.sol
+++ b/test/libsolidity/syntaxTests/visibility/interface/function_default.sol
@@ -2,4 +2,5 @@ interface I {
 	function f();
 }
 // ----
+// SyntaxError: (15-28): No visibility specified. Did you intend to add "external"?
 // TypeError: (15-28): Functions in interfaces must be declared external.
diff --git a/test/libsolidity/syntaxTests/visibility/interface/function_default050.sol b/test/libsolidity/syntaxTests/visibility/interface/function_default050.sol
deleted file mode 100644
index 513df26b..00000000
--- a/test/libsolidity/syntaxTests/visibility/interface/function_default050.sol
+++ /dev/null
@@ -1,7 +0,0 @@
-pragma experimental "v0.5.0";
-interface I {
-	function f();
-}
-// ----
-// SyntaxError: (45-58): No visibility specified.
-// TypeError: (45-58): Functions in interfaces must be declared external.
diff --git a/test/libsolidity/syntaxTests/visibility/interface/interface_contract_function_default.sol b/test/libsolidity/syntaxTests/visibility/interface/interface_contract_function_default.sol
new file mode 100644
index 00000000..b1a820ed
--- /dev/null
+++ b/test/libsolidity/syntaxTests/visibility/interface/interface_contract_function_default.sol
@@ -0,0 +1,12 @@
+// State of the syntax checker has to be reset after the interface
+// was visited. The suggested visibility for g() should not be external.
+interface I {
+    function f();
+}
+contract C {
+    function g();
+}
+// ----
+// SyntaxError: (158-171): No visibility specified. Did you intend to add "external"?
+// SyntaxError: (191-204): No visibility specified. Did you intend to add "public"?
+// TypeError: (158-171): Functions in interfaces must be declared external.