Implement parsing of override keyword #7233

Marenz · 2019-08-13T11:58:50Z

Doesn't break any compability yet, so still targeting develop at this point.
prepares for #5424

chriseth · 2019-08-13T12:26:27Z

Needs changes in ast export (and import...)

chriseth · 2019-08-13T12:26:57Z

libsolidity/ast/AST.h

@@ -624,6 +625,7 @@ class FunctionDefinition: public CallableDeclaration, public Documented, public
 		ImplementationOptional(_body != nullptr),
 		m_stateMutability(_stateMutability),
 		m_isConstructor(_isConstructor),
+		m_isOverriding(_isOverriding),


maybe just overriding?

wouldn't that be inconsistent with the existing naming isContructor and isIndexed etc?

actually the coding style says that is prefixes should be avoided, but yeah, we have a lot of them here.

chriseth · 2019-08-13T12:27:37Z

libsolidity/ast/AST.h

@@ -753,6 +760,7 @@ class VariableDeclaration: public Declaration
 	bool m_isStateVariable; ///< Whether or not this is a contract state variable
 	bool m_isIndexed; ///< Whether this is an indexed variable (used by events).
 	bool m_isConstant; ///< Whether the variable is a compile-time constant.
+	bool m_isOverriding; ///< whether the variable is overriding a public base class variable


Suggested change

bool m_isOverriding; ///< whether the variable is overriding a public base class variable

bool m_isOverriding; ///< whether the variable is overriding an external base contract function

A state variable can override a function fro the base contract?

The only situation where override is meaningful for state variables is if the getter of the state variable overrides a function, and for that, the function in the base contract has to be external.

chriseth · 2019-08-13T12:38:12Z

Looks good otherwise!

Marenz · 2019-08-13T16:02:27Z

updated

leonardoalt · 2019-08-14T08:52:24Z

Tests failing, I think expectations need to be updated.

Marenz · 2019-08-14T11:14:24Z

fixed now. Had to investigate another bug that turned out to be just due to a missing make clean

chriseth · 2019-08-15T13:12:20Z

Are you planning to change this again to support the arguments?

Marenz · 2019-08-15T13:55:10Z

Yes, currently working on exactly that

ekpyron · 2019-08-15T16:57:14Z

libsolidity/parsing/Parser.cpp

+	if (m_scanner->currentToken() == Token::LParen)
+	{
+		m_scanner->next();
+		while (m_scanner->currentToken() == Token::Identifier)


What happens for let's say override(A, address)?

I think using while (m_scanner->currentToken() != Token::RParen) should fix that.

That might still fail (resp. "fail to fail") for override(A,), right? (which could be fixed by expectToken(Token::RParen) after the loop - by the way expectToken advances by default, doesn't it? Which means the m->scanner->next() after the expectToken(Token::Comma); might also have to be removed.)

Both should be test cases in any case.

Also override() should be a test case - I'd disallow that as well, but we could also say it's fine.

Oh of course, I did not mean that just modifying the while condition is the only required change.

ekpyron · 2019-08-15T19:32:15Z

test/libsolidity/ASTJSON/override.json

+					"modifiers" : [],
+					"name" : "faa",
+					"nodeType" : "FunctionDefinition",
+					"overrides" : 


Yep, this swallowed the B due to advancing after the expectToken above.

Marenz · 2019-08-19T09:41:44Z

Updated. Should fix all your concerns :>

chriseth · 2019-08-19T10:04:09Z

libsolidity/ast/AST.h

@@ -604,6 +604,28 @@ class CallableDeclaration: public Declaration, public VariableScope
 	ASTPointer<ParameterList> m_returnParameters;
 };



Please document.

chriseth · 2019-08-19T10:04:23Z

libsolidity/ast/AST.h

+	void accept(ASTVisitor& _visitor) override;
+	void accept(ASTConstVisitor& _visitor) const override;
+
+	///< Returns the list of specific overrides, if any


Suggested change

///< Returns the list of specific overrides, if any

/// @returns the list of specific overrides, if any

chriseth · 2019-08-19T10:04:40Z

libsolidity/ast/AST.h

+	void accept(ASTConstVisitor& _visitor) const override;
+
+	///< Returns the list of specific overrides, if any
+	std::vector<ASTPointer<UserDefinedTypeName>> overrides() const { return m_overrides; }


Why not return by const&?

chriseth · 2019-08-19T10:05:49Z

libsolidity/ast/AST.h

@@ -613,6 +635,7 @@ class FunctionDefinition: public CallableDeclaration, public Documented, public
 		Declaration::Visibility _visibility,
 		StateMutability _stateMutability,
 		bool _isConstructor,
+		ASTPointer<OverrideSpecifier> const& _overrides,


Does it make sense to move this all the way up to Declaration?

libsolidity/parsing/Parser.cpp

chriseth · 2019-08-19T10:24:33Z

Please also update grammar.txt

chriseth · 2019-08-19T10:26:40Z

libsolidity/parsing/Parser.cpp

@@ -493,6 +522,13 @@ Parser::FunctionHeaderParserResult Parser::parseFunctionHeader(bool _forceEmptyN
 			else
 				result.stateMutability = parseStateMutability();
 		}
+		else if (token == Token::Override)


I think this should also take allowModifiers &&, at least override should not be allowed for function types, where allowModifiers is set to false. Maybe allowModifiers could be renamed.

chriseth · 2019-08-19T10:28:34Z

libsolidity/parsing/Parser.cpp

@@ -540,6 +576,7 @@ ASTPointer<ASTNode> Parser::parseFunctionDefinitionOrFunctionTypeStateVariable()
 			header.visibility,


Could also add header.overrides to the condition in line 520/556, I think.

Marenz · 2019-08-19T13:31:32Z

Could also add header.overrides to the condition in line 520/556, I think.

fixed all points but this one, wasn't sure how you meant this

chriseth · 2019-08-19T13:56:27Z

libsolidity/ast/AST.h

@@ -604,6 +604,32 @@ class CallableDeclaration: public Declaration, public VariableScope
 	ASTPointer<ParameterList> m_returnParameters;
 };

+/**
+ * Function override specifier. Consists of a single override keyword or a list of


Suggested change

* Function override specifier. Consists of a single override keyword or a list of

* Function override specifier. Consists of a single override keyword potentially followed by a parenthesized list of base contract names.

chriseth · 2019-08-19T14:03:12Z

Hm, test seems to be failing, can you check, please?

Marenz · 2019-08-19T14:05:07Z

The failing test is the thing I mentioned in the meeting.. it seems the order of values in an json array is not consistent

Marenz · 2019-08-19T14:05:44Z

I'll take care of it

Marenz · 2019-08-20T07:27:46Z

This will now work once current develop is merged into develop_060

Marenz · 2019-08-26T08:31:50Z

I pushed some small fixes and rebased on latest develop_060. Here is the diff of my fixes only: https://github.com/ethereum/solidity/compare/aa0a53c0282391ae45dd7c118f918fea6ed72ac5..f4f0f3e84c2e2c02bb6319ab7418977260b5e3f6

Marenz · 2019-08-26T11:03:17Z

I investigated the doc failure. It's caused by a change @ekpyron sponsored. solhint runs on older grammar and thus will keep failing for this until we released 060 and they have updated their grammar.

So it's safe to ignore this for this PR.

libsolidity/parsing/Parser.cpp

test/libsolidity/ASTJSON/override.json

test/libsolidity/ASTJSON/non_utf8.sol

erak

Looks good! Need to track the removal of chk_docs_examples from Circle for develop_060 as suggested by @ekpyron though.

Marenz · 2019-08-26T15:35:08Z

I forgot to add one test case that is now added.

Marenz force-pushed the override-5424 branch from 06a73f3 to 4261135 Compare August 13, 2019 12:02

chriseth reviewed Aug 13, 2019

View reviewed changes

Marenz force-pushed the override-5424 branch from 4261135 to 889008b Compare August 13, 2019 16:00

Marenz force-pushed the override-5424 branch from 889008b to d822f99 Compare August 14, 2019 11:06

Marenz force-pushed the override-5424 branch from d822f99 to c5abb5c Compare August 15, 2019 14:39

ekpyron reviewed Aug 15, 2019

View reviewed changes

Marenz force-pushed the override-5424 branch from c5abb5c to dddf6ea Compare August 19, 2019 09:40

chriseth reviewed Aug 19, 2019

View reviewed changes

Marenz changed the base branch from develop to develop_060 August 19, 2019 10:17

chriseth reviewed Aug 19, 2019

View reviewed changes

libsolidity/parsing/Parser.cpp Show resolved Hide resolved

Marenz force-pushed the override-5424 branch from dddf6ea to dfdf1c3 Compare August 19, 2019 10:18

chriseth reviewed Aug 19, 2019

View reviewed changes

libsolidity/parsing/Parser.cpp Show resolved Hide resolved

chriseth reviewed Aug 19, 2019

View reviewed changes

Marenz force-pushed the override-5424 branch from dfdf1c3 to 6f83344 Compare August 19, 2019 13:29

Marenz force-pushed the override-5424 branch from 6f83344 to aa0a53c Compare August 19, 2019 13:51

chriseth reviewed Aug 19, 2019

View reviewed changes

chriseth approved these changes Aug 19, 2019

View reviewed changes

Marenz force-pushed the override-5424 branch 2 times, most recently from f4f0f3e to a624237 Compare August 26, 2019 08:31

Marenz force-pushed the override-5424 branch from a624237 to 321d48e Compare August 26, 2019 09:25

erak reviewed Aug 26, 2019

View reviewed changes

libsolidity/parsing/Parser.cpp Outdated Show resolved Hide resolved

erak reviewed Aug 26, 2019

View reviewed changes

test/libsolidity/ASTJSON/override.json Show resolved Hide resolved

erak reviewed Aug 26, 2019

View reviewed changes

test/libsolidity/ASTJSON/non_utf8.sol Show resolved Hide resolved

erak approved these changes Aug 26, 2019

View reviewed changes

Parse override keyword

2471640

Marenz force-pushed the override-5424 branch from 4c6cbfb to 2471640 Compare August 26, 2019 15:34

Marenz merged commit 25a3a83 into develop_060 Aug 26, 2019

Marenz deleted the override-5424 branch August 26, 2019 16:12

	bool m_isOverriding; ///< whether the variable is overriding a public base class variable
	bool m_isOverriding; ///< whether the variable is overriding an external base contract function

		@@ -604,6 +604,28 @@ class CallableDeclaration: public Declaration, public VariableScope
		ASTPointer<ParameterList> m_returnParameters;
		};

	///< Returns the list of specific overrides, if any
	/// @returns the list of specific overrides, if any

		@@ -540,6 +576,7 @@ ASTPointer<ASTNode> Parser::parseFunctionDefinitionOrFunctionTypeStateVariable()
		header.visibility,

	* Function override specifier. Consists of a single override keyword or a list of
	* Function override specifier. Consists of a single override keyword potentially followed by a parenthesized list of base contract names.

Implement parsing of override keyword #7233

Implement parsing of override keyword #7233

Conversation

Marenz commented Aug 13, 2019

chriseth commented Aug 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth commented Aug 13, 2019

Marenz commented Aug 13, 2019

leonardoalt commented Aug 14, 2019

Marenz commented Aug 14, 2019

chriseth commented Aug 15, 2019

Marenz commented Aug 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekpyron Aug 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marenz commented Aug 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth commented Aug 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marenz commented Aug 19, 2019

Choose a reason for hiding this comment

chriseth commented Aug 19, 2019

Marenz commented Aug 19, 2019

Marenz commented Aug 19, 2019

Marenz commented Aug 20, 2019

Marenz commented Aug 26, 2019

Marenz commented Aug 26, 2019

erak left a comment

Choose a reason for hiding this comment

Marenz commented Aug 26, 2019

ekpyron Aug 15, 2019 •

edited

Loading