diff --git a/libsolidity/AST.cpp b/libsolidity/AST.cpp index 10464726e..b07959b39 100644 --- a/libsolidity/AST.cpp +++ b/libsolidity/AST.cpp @@ -133,7 +133,7 @@ void ContractDefinition::checkIllegalOverrides() const FunctionDefinition const*& override = functions[name]; if (!override) override = function.get(); - else if (override->isPublic() != function->isPublic() || + else if (override->getVisibility() != function->getVisibility() || override->isDeclaredConst() != function->isDeclaredConst() || FunctionType(*override) != FunctionType(*function)) BOOST_THROW_EXCEPTION(override->createTypeError("Override changes extended function signature.")); diff --git a/libsolidity/AST.h b/libsolidity/AST.h index 950553cd4..8e8641c78 100755 --- a/libsolidity/AST.h +++ b/libsolidity/AST.h @@ -133,12 +133,17 @@ class Declaration: public ASTNode { public: enum class LValueType { NONE, LOCAL, STORAGE }; + enum class Visibility { DEFAULT, PUBLIC, PROTECTED, PRIVATE }; - Declaration(Location const& _location, ASTPointer const& _name): - ASTNode(_location), m_name(_name), m_scope(nullptr) {} + Declaration(Location const& _location, ASTPointer const& _name, + Visibility _visibility = Visibility::DEFAULT): + ASTNode(_location), m_name(_name), m_visibility(_visibility), m_scope(nullptr) {} /// @returns the declared name. ASTString const& getName() const { return *m_name; } + Visibility getVisibility() const { return m_visibility == Visibility::DEFAULT ? getDefaultVisibility() : m_visibility; } + bool isPublic() const { return getVisibility() == Visibility::PUBLIC; } + /// @returns the scope this declaration resides in. Can be nullptr if it is the global scope. /// Available only after name and type resolution step. Declaration const* getScope() const { return m_scope; } @@ -151,8 +156,12 @@ public: /// @returns the lvalue type of expressions referencing this declaration virtual LValueType getLValueType() const { return LValueType::NONE; } +protected: + virtual Visibility getDefaultVisibility() const { return Visibility::PUBLIC; } + private: ASTPointer m_name; + Visibility m_visibility; Declaration const* m_scope; }; @@ -330,16 +339,15 @@ class FunctionDefinition: public Declaration, public VariableScope, public Docum { public: FunctionDefinition(Location const& _location, ASTPointer const& _name, - bool _isPublic, - bool _isConstructor, + Declaration::Visibility _visibility, bool _isConstructor, ASTPointer const& _documentation, ASTPointer const& _parameters, bool _isDeclaredConst, std::vector> const& _modifiers, ASTPointer const& _returnParameters, ASTPointer const& _body): - Declaration(_location, _name), Documented(_documentation), - m_isPublic(_isPublic), m_isConstructor(_isConstructor), + Declaration(_location, _name, _visibility), Documented(_documentation), + m_isConstructor(_isConstructor), m_parameters(_parameters), m_isDeclaredConst(_isDeclaredConst), m_functionModifiers(_modifiers), @@ -350,7 +358,6 @@ public: virtual void accept(ASTVisitor& _visitor) override; virtual void accept(ASTConstVisitor& _visitor) const override; - bool isPublic() const { return m_isPublic; } bool isConstructor() const { return m_isConstructor; } bool isDeclaredConst() const { return m_isDeclaredConst; } std::vector> const& getModifiers() const { return m_functionModifiers; } @@ -371,7 +378,6 @@ public: std::string getCanonicalSignature() const; private: - bool m_isPublic; bool m_isConstructor; ASTPointer m_parameters; bool m_isDeclaredConst; @@ -388,10 +394,10 @@ class VariableDeclaration: public Declaration { public: VariableDeclaration(Location const& _location, ASTPointer const& _type, - ASTPointer const& _name, bool _isPublic, bool _isStateVar = false, - bool _isIndexed = false): - Declaration(_location, _name), m_typeName(_type), - m_isPublic(_isPublic), m_isStateVariable(_isStateVar), m_isIndexed(_isIndexed) {} + ASTPointer const& _name, Visibility _visibility, + bool _isStateVar = false, bool _isIndexed = false): + Declaration(_location, _name, _visibility), m_typeName(_type), + m_isStateVariable(_isStateVar), m_isIndexed(_isIndexed) {} virtual void accept(ASTVisitor& _visitor) override; virtual void accept(ASTConstVisitor& _visitor) const override; @@ -404,13 +410,14 @@ public: virtual LValueType getLValueType() const override; bool isLocalVariable() const { return !!dynamic_cast(getScope()); } - bool isPublic() const { return m_isPublic; } bool isStateVariable() const { return m_isStateVariable; } bool isIndexed() const { return m_isIndexed; } +protected: + Visibility getDefaultVisibility() const override { return Visibility::PROTECTED; } + private: ASTPointer m_typeName; ///< can be empty ("var") - bool m_isPublic; ///< Whether there is an accessor for it or not bool m_isStateVariable; ///< Whether or not this is a contract state variable bool m_isIndexed; ///< Whether this is an indexed variable (used by events). diff --git a/libsolidity/Parser.cpp b/libsolidity/Parser.cpp index d2e888a8d..cc75fc0a2 100644 --- a/libsolidity/Parser.cpp +++ b/libsolidity/Parser.cpp @@ -131,27 +131,19 @@ ASTPointer Parser::parseContractDefinition() } while (m_scanner->getCurrentToken() == Token::COMMA); expectToken(Token::LBRACE); - bool visibilityIsPublic = true; while (true) { Token::Value currentToken = m_scanner->getCurrentToken(); if (currentToken == Token::RBRACE) break; - else if (currentToken == Token::PUBLIC || currentToken == Token::PRIVATE) - { - visibilityIsPublic = (m_scanner->getCurrentToken() == Token::PUBLIC); - m_scanner->next(); - expectToken(Token::COLON); - } else if (currentToken == Token::FUNCTION) - functions.push_back(parseFunctionDefinition(visibilityIsPublic, name.get())); + functions.push_back(parseFunctionDefinition(name.get())); else if (currentToken == Token::STRUCT) structs.push_back(parseStructDefinition()); else if (currentToken == Token::IDENTIFIER || currentToken == Token::MAPPING || Token::isElementaryTypeName(currentToken)) { VarDeclParserOptions options; - options.isPublic = visibilityIsPublic; options.isStateVariable = true; stateVariables.push_back(parseVariableDeclaration(options)); expectToken(Token::SEMICOLON); @@ -186,7 +178,22 @@ ASTPointer Parser::parseInheritanceSpecifier() return nodeFactory.createNode(name, arguments); } -ASTPointer Parser::parseFunctionDefinition(bool _isPublic, ASTString const* _contractName) +Declaration::Visibility Parser::parseVisibilitySpecifier(Token::Value _token) +{ + Declaration::Visibility visibility; + if (_token == Token::PUBLIC) + visibility = Declaration::Visibility::PUBLIC; + else if (_token == Token::PROTECTED) + visibility = Declaration::Visibility::PROTECTED; + else if (_token == Token::PRIVATE) + visibility = Declaration::Visibility::PRIVATE; + else + solAssert(false, "Invalid visibility specifier."); + m_scanner->next(); + return visibility; +} + +ASTPointer Parser::parseFunctionDefinition(ASTString const* _contractName) { ASTNodeFactory nodeFactory(*this); ASTPointer docstring; @@ -201,16 +208,24 @@ ASTPointer Parser::parseFunctionDefinition(bool _isPublic, A name = expectIdentifierToken(); ASTPointer parameters(parseParameterList()); bool isDeclaredConst = false; + Declaration::Visibility visibility(Declaration::Visibility::DEFAULT); vector> modifiers; while (true) { - if (m_scanner->getCurrentToken() == Token::CONST) + Token::Value token = m_scanner->getCurrentToken(); + if (token == Token::CONST) { isDeclaredConst = true; m_scanner->next(); } - else if (m_scanner->getCurrentToken() == Token::IDENTIFIER) + else if (token == Token::IDENTIFIER) modifiers.push_back(parseModifierInvocation()); + else if (Token::isVisibilitySpecifier(token)) + { + if (visibility != Declaration::Visibility::DEFAULT) + BOOST_THROW_EXCEPTION(createParserError("Multiple visibility specifiers.")); + visibility = parseVisibilitySpecifier(token); + } else break; } @@ -226,7 +241,7 @@ ASTPointer Parser::parseFunctionDefinition(bool _isPublic, A ASTPointer block = parseBlock(); nodeFactory.setEndPositionFromNode(block); bool const c_isConstructor = (_contractName && *name == *_contractName); - return nodeFactory.createNode(name, _isPublic, c_isConstructor, docstring, + return nodeFactory.createNode(name, visibility, c_isConstructor, docstring, parameters, isDeclaredConst, modifiers, returnParameters, block); } @@ -253,14 +268,18 @@ ASTPointer Parser::parseVariableDeclaration(VarDeclParserOp ASTNodeFactory nodeFactory(*this); ASTPointer type = parseTypeName(_options.allowVar); bool isIndexed = false; - if (_options.allowIndexed && m_scanner->getCurrentToken() == Token::INDEXED) + Token::Value token = m_scanner->getCurrentToken(); + if (_options.allowIndexed && token == Token::INDEXED) { isIndexed = true; m_scanner->next(); } + Declaration::Visibility visibility(Declaration::Visibility::DEFAULT); + if (_options.isStateVariable && Token::isVisibilitySpecifier(token)) + visibility = parseVisibilitySpecifier(token); nodeFactory.markEndPosition(); return nodeFactory.createNode(type, expectIdentifierToken(), - _options.isPublic, _options.isStateVariable, + visibility, _options.isStateVariable, isIndexed); } diff --git a/libsolidity/Parser.h b/libsolidity/Parser.h index 413a2711e..388fd7a96 100644 --- a/libsolidity/Parser.h +++ b/libsolidity/Parser.h @@ -48,7 +48,6 @@ private: struct VarDeclParserOptions { VarDeclParserOptions() {} bool allowVar = false; - bool isPublic = false; bool isStateVariable = false; bool allowIndexed = false; }; @@ -58,7 +57,8 @@ private: ASTPointer parseImportDirective(); ASTPointer parseContractDefinition(); ASTPointer parseInheritanceSpecifier(); - ASTPointer parseFunctionDefinition(bool _isPublic, ASTString const* _contractName); + Declaration::Visibility parseVisibilitySpecifier(Token::Value _token); + ASTPointer parseFunctionDefinition(ASTString const* _contractName); ASTPointer parseStructDefinition(); ASTPointer parseVariableDeclaration(VarDeclParserOptions const& _options = VarDeclParserOptions()); ASTPointer parseModifierDefinition(); diff --git a/libsolidity/Token.h b/libsolidity/Token.h index ed42f90cc..76e504499 100644 --- a/libsolidity/Token.h +++ b/libsolidity/Token.h @@ -165,6 +165,7 @@ namespace solidity K(NEW, "new", 0) \ K(PUBLIC, "public", 0) \ K(PRIVATE, "private", 0) \ + K(PROTECTED, "protected", 0) \ K(RETURN, "return", 0) \ K(RETURNS, "returns", 0) \ K(STRUCT, "struct", 0) \ @@ -376,6 +377,7 @@ public: static bool isUnaryOp(Value op) { return (NOT <= op && op <= DELETE) || op == ADD || op == SUB; } static bool isCountOp(Value op) { return op == INC || op == DEC; } static bool isShiftOp(Value op) { return (SHL <= op) && (op <= SHR); } + static bool isVisibilitySpecifier(Value op) { return op == PUBLIC || op == PRIVATE || op == PROTECTED; } // Returns a string corresponding to the JS token string // (.e., "<" for the token LT) or NULL if the token doesn't diff --git a/libsolidity/grammar.txt b/libsolidity/grammar.txt index b97dac5db..1785b516c 100644 --- a/libsolidity/grammar.txt +++ b/libsolidity/grammar.txt @@ -1,14 +1,15 @@ ContractDefinition = 'contract' Identifier ( 'is' InheritanceSpecifier (',' InheritanceSpecifier )* )? '{' ContractPart* '}' -ContractPart = VariableDeclaration ';' | StructDefinition | ModifierDefinition | - FunctionDefinition | 'public:' | 'private:' +ContractPart = StateVariableDeclaration | StructDefinition | ModifierDefinition | FunctionDefinition InheritanceSpecifier = Identifier ( '(' Expression ( ',' Expression )* ')' )? StructDefinition = 'struct' Identifier '{' ( VariableDeclaration (';' VariableDeclaration)* )? '} +StateVariableDeclaration = TypeName ( 'public' | 'protected' | 'private' )? Identifier ';' ModifierDefinition = 'modifier' Identifier ParameterList? Block -FunctionDefinition = 'function' Identifier ParameterList ( Identifier | 'constant' )* +FunctionDefinition = 'function' Identifier ParameterList + ( Identifier | 'constant' | 'public' | 'protected' | 'private' )* ( 'returns' ParameterList )? Block ParameterList = '(' ( VariableDeclaration (',' VariableDeclaration)* )? ')' // semantic restriction: mappings and structs (recursively) containing mappings diff --git a/test/SolidityEndToEndTest.cpp b/test/SolidityEndToEndTest.cpp index 9a04e02d3..ced9785ef 100644 --- a/test/SolidityEndToEndTest.cpp +++ b/test/SolidityEndToEndTest.cpp @@ -885,7 +885,7 @@ BOOST_AUTO_TEST_CASE(constructor) BOOST_AUTO_TEST_CASE(simple_accessor) { char const* sourceCode = "contract test {\n" - " uint256 data;\n" + " uint256 public data;\n" " function test() {\n" " data = 8;\n" " }\n" @@ -897,10 +897,10 @@ BOOST_AUTO_TEST_CASE(simple_accessor) BOOST_AUTO_TEST_CASE(multiple_elementary_accessors) { char const* sourceCode = "contract test {\n" - " uint256 data;\n" - " string6 name;\n" - " hash a_hash;\n" - " address an_address;\n" + " uint256 public data;\n" + " string6 public name;\n" + " hash public a_hash;\n" + " address public an_address;\n" " function test() {\n" " data = 8;\n" " name = \"Celina\";\n" @@ -908,7 +908,6 @@ BOOST_AUTO_TEST_CASE(multiple_elementary_accessors) " an_address = address(0x1337);\n" " super_secret_data = 42;\n" " }\n" - " private:" " uint256 super_secret_data;" "}\n"; compileAndRun(sourceCode); @@ -1511,8 +1510,7 @@ BOOST_AUTO_TEST_CASE(functions_called_by_constructor) setName("abc"); } function getName() returns (string3 ret) { return name; } - private: - function setName(string3 _name) { name = _name; } + function setName(string3 _name) private { name = _name; } })"; compileAndRun(sourceCode); BOOST_REQUIRE(callContractFunction("getName()") == encodeArgs("abc")); diff --git a/test/SolidityNameAndTypeResolution.cpp b/test/SolidityNameAndTypeResolution.cpp index 2fe3288ad..1a087592c 100644 --- a/test/SolidityNameAndTypeResolution.cpp +++ b/test/SolidityNameAndTypeResolution.cpp @@ -467,6 +467,24 @@ BOOST_AUTO_TEST_CASE(illegal_override_indirect) BOOST_CHECK_THROW(parseTextAndResolveNames(text), TypeError); } +BOOST_AUTO_TEST_CASE(illegal_override_visibility) +{ + char const* text = R"( + contract B { function f() protected {} } + contract C is B { function f() public {} } + )"; + BOOST_CHECK_THROW(parseTextAndResolveNames(text), TypeError); +} + +BOOST_AUTO_TEST_CASE(illegal_override_constness) +{ + char const* text = R"( + contract B { function f() constant {} } + contract C is B { function f() {} } + )"; + BOOST_CHECK_THROW(parseTextAndResolveNames(text), TypeError); +} + BOOST_AUTO_TEST_CASE(complex_inheritance) { char const* text = R"( @@ -636,9 +654,9 @@ BOOST_AUTO_TEST_CASE(state_variable_accessors) " function fun() {\n" " uint64(2);\n" " }\n" - "uint256 foo;\n" - "mapping(uint=>string4) map;\n" - "mapping(uint=>mapping(uint=>string4)) multiple_map;\n" + "uint256 public foo;\n" + "mapping(uint=>string4) public map;\n" + "mapping(uint=>mapping(uint=>string4)) public multiple_map;\n" "}\n"; ASTPointer source; @@ -687,16 +705,19 @@ BOOST_AUTO_TEST_CASE(private_state_variable) " function fun() {\n" " uint64(2);\n" " }\n" - "private:\n" - "uint256 foo;\n" + "uint256 private foo;\n" + "uint256 protected bar;\n" "}\n"; ASTPointer source; ContractDefinition const* contract; BOOST_CHECK_NO_THROW(source = parseTextAndResolveNamesWithChecks(text)); BOOST_CHECK((contract = retrieveContract(source, 0)) != nullptr); - FunctionTypePointer function = retrieveFunctionBySignature(contract, "foo()"); + FunctionTypePointer function; + function = retrieveFunctionBySignature(contract, "foo()"); BOOST_CHECK_MESSAGE(function == nullptr, "Accessor function of a private variable should not exist"); + function = retrieveFunctionBySignature(contract, "bar()"); + BOOST_CHECK_MESSAGE(function == nullptr, "Accessor function of a protected variable should not exist"); } BOOST_AUTO_TEST_CASE(fallback_function) @@ -799,6 +820,54 @@ BOOST_AUTO_TEST_CASE(multiple_events_argument_clash) BOOST_CHECK_NO_THROW(parseTextAndResolveNames(text)); } +BOOST_AUTO_TEST_CASE(access_to_default_function_visibility) +{ + char const* text = R"( + contract c { + function f() {} + } + contract d { + function g() { c(0).f(); } + })"; + BOOST_CHECK_NO_THROW(parseTextAndResolveNames(text)); +} + +BOOST_AUTO_TEST_CASE(access_to_protected_function) +{ + char const* text = R"( + contract c { + function f() protected {} + } + contract d { + function g() { c(0).f(); } + })"; + BOOST_CHECK_THROW(parseTextAndResolveNames(text), TypeError); +} + +BOOST_AUTO_TEST_CASE(access_to_default_state_variable_visibility) +{ + char const* text = R"( + contract c { + uint a; + } + contract d { + function g() { c(0).a(); } + })"; + BOOST_CHECK_THROW(parseTextAndResolveNames(text), TypeError); +} + +BOOST_AUTO_TEST_CASE(access_to_protected_state_variable) +{ + char const* text = R"( + contract c { + uint public a; + } + contract d { + function g() { c(0).a(); } + })"; + BOOST_CHECK_NO_THROW(parseTextAndResolveNames(text)); +} + BOOST_AUTO_TEST_SUITE_END() } diff --git a/test/SolidityParser.cpp b/test/SolidityParser.cpp index 4adee9c66..4ccdcd57a 100644 --- a/test/SolidityParser.cpp +++ b/test/SolidityParser.cpp @@ -129,9 +129,7 @@ BOOST_AUTO_TEST_CASE(function_natspec_documentation) ASTPointer contract; ASTPointer function; char const* text = "contract test {\n" - " private:\n" - " uint256 stateVar;\n" - " public:\n" + " uint256 stateVar;\n" " /// This is a test function\n" " function functionName(hash hashin) returns (hash hashout) {}\n" "}\n"; @@ -162,9 +160,7 @@ BOOST_AUTO_TEST_CASE(multiple_functions_natspec_documentation) ASTPointer contract; ASTPointer function; char const* text = "contract test {\n" - " private:\n" " uint256 stateVar;\n" - " public:\n" " /// This is test function 1\n" " function functionName1(hash hashin) returns (hash hashout) {}\n" " /// This is test function 2\n" @@ -621,6 +617,31 @@ BOOST_AUTO_TEST_CASE(event_arguments_indexed) BOOST_CHECK_NO_THROW(parseText(text)); } +BOOST_AUTO_TEST_CASE(visibility_specifiers) +{ + char const* text = R"( + contract c { + uint private a; + uint protected b; + uint public c; + uint d; + function f() {} + function f_priv() private {} + function f_public() public {} + function f_protected() protected {} + })"; + BOOST_CHECK_NO_THROW(parseText(text)); +} + +BOOST_AUTO_TEST_CASE(multiple_visibility_specifiers) +{ + char const* text = R"( + contract c { + uint private protected a; + })"; + BOOST_CHECK_THROW(parseText(text), ParserError); +} + BOOST_AUTO_TEST_SUITE_END() }