Add modifier areas #3939

randomnetcat · 2018-04-18T21:23:35Z

Closes #623.

Yes, the code here is pretty sloppy. Feedback (from maintainers and others) is welcome and encouraged!
I'm especially interested in hearing suggestions people have for syntax.

Syntax:

apply ... { ... }

The ellipsis after apply can contain any modifier invocations, a state mutability specifier, and a visibility mutability specifier, or any combination of the above. Inside the area there can be more modifier areas (modifier invocations are cumulative) and function declarations. Nested modifier areas and function declarations cannot have a different visibility or mutability than their modifier areas.

randomnetcat · 2018-04-19T00:01:31Z

Look at that... already found a (significant) bug. (now fixed)

ekpyron

At least the dependency cycle between modifier areas and function definitions needs to be resolved.

ekpyron · 2018-06-11T17:15:13Z

docs/grammar.txt

@@ -12,7 +12,7 @@ ContractDefinition = ( 'contract' | 'library' | 'interface' ) Identifier
                     '{' ContractPart* '}'

 ContractPart = StateVariableDeclaration | UsingForDeclaration
-             | StructDefinition | ModifierDefinition | FunctionDefinition | EventDefinition | EnumDefinition
+             | StructDefinition | ModifierDefinition | FunctionDefinition | EventDefinition | EnumDefinition | ModifierArea


Maybe a line break may be good.

ekpyron · 2018-06-11T17:17:47Z

docs/grammar.txt

@@ -32,6 +32,9 @@ EventDefinition = 'event' Identifier EventParameterList 'anonymous'? ';'
 EnumValue = Identifier
 EnumDefinition = 'enum' Identifier '{' EnumValue? (',' EnumValue)* '}'

+ModifierArea = 'using modifier' ModifierInvocation (',' ModifierInvocation)*
+               '{' (ModifierArea | FunctionDefinition) '}'


Is this correct? Can't there be multiple function definitions and modifier areas in a modifier area? Actually there should also be test cases for that (at the moment I only see test cases with a single function definition in a modifier area).

Yes. I think I just forgot the asterisk.

ekpyron · 2018-06-11T17:19:11Z

docs/structure-of-a-contract.rst

+
+Multiple modifier invocations can be used in a single modifier area:
+`using modifier A, B { /* ... */ }` declares a modifier area
+where both modifiers are used on every function.a


I guess the a in the end is a typo?

ekpyron · 2018-06-11T17:30:14Z

libsolidity/ast/AST.cpp

+	std::vector<FunctionDefinition const*> totalFunctions = filteredNodes<FunctionDefinition>(m_subNodes);
+	std::vector<ModifierArea const*> baseModifierAreas = modifierAreas();
+	std::vector<ModifierArea const*> nextGroups;
+	while (!baseModifierAreas.empty())


I think this may be easier to read, if you used a queue of ModifierArea const*. Then you could extract the front element into a local variable, pop it from the queue, process it and while doing so just add the subareas back to the queue (and continue until the queue is empty).

This may only be my personal taste, though, and it should also be fine as it is now.

ekpyron · 2018-06-11T17:34:05Z

libsolidity/ast/AST.cpp

+	if (m_modifierArea)
+	{
+		auto const areaModifiers = _modifierArea->totalModifiers();
+		realModifiers.insert(realModifiers.begin(), areaModifiers.begin(), areaModifiers.end());


There are overloads for + and += for vectors in libdevcore (I think) - so you should be able to just use
realModifiers = areaModifiers + realModifiers;.

ekpyron · 2018-06-11T17:37:49Z

libsolidity/ast/AST.cpp

+	if (m_parent)
+	{
+		std::vector<ASTPointer<ModifierInvocation>> parentModifiers = m_parent->totalModifiers();
+		completeVector.insert(completeVector.begin(), parentModifiers.begin(), parentModifiers.end());


Same as above. Also: why do you need the local variable completeVector? You could just start by setting m_totalModifiers = m_declaredModifiers; and then continue with m_totalModifiers = parentModifiers + m_totalModifiers;. Even better: do it the other way round: if there is a parent first set m_totalModifiers = parentModifers; and then use m_totalModifiers += m_declaredModifiers; after the ìf.

ekpyron · 2018-06-11T17:43:17Z

libsolidity/ast/AST_accept.h

+	if (_visitor.visit(*this))
+	{
+		listAccept(m_declaredModifiers, _visitor);
+		listAccept(*m_functions, _visitor);


Somewhere you should probably assert that these members (m_functions, m_subAreas) aren't null pointers. Either here or in the constructor of ModifierArea or even at both places (or otherwise this should be an if (m_functions) listAccept(...) if the pointer may actually be null, although it doesn't look like they can... as stated above it may make sense for them not to be pointer at all).

ekpyron · 2018-06-11T17:44:29Z

libsolidity/parsing/Parser.cpp

+	RecursionGuard recursionGuard(*this);
+
+	if (m_scanner->peekNextToken() == Token::Identifier)
+	{


We usually don't use brackets for single line if, resp. else.

ekpyron · 2018-06-11T17:47:17Z

libsolidity/ast/AST.h

+private:
+	std::vector<ASTPointer<ModifierInvocation>> m_totalModifiers;
+	std::vector<ASTPointer<ModifierInvocation>> m_declaredModifiers;
+	ASTPointer<std::vector<ASTPointer<FunctionDefinition>> const> m_functions;


Actually: it doesn't seem like those can ever be null or that they are passed around, resp. shared - so why use ASTPointers and not just the vectors themselves as members?

ekpyron · 2018-06-11T17:56:36Z

libsolidity/parsing/Parser.cpp

+	ASTPointer<std::vector<ASTPointer<FunctionDefinition>>> functions = std::make_shared<std::vector<ASTPointer<FunctionDefinition>>>();
+	ASTPointer<std::vector<ASTPointer<ModifierArea>>> subAreas = std::make_shared<std::vector<ASTPointer<ModifierArea>>>();
+
+	ASTPointer<ModifierArea> modifierArea = nodeFactory.createNode<ModifierArea>(modifiers, functions, subAreas, _parent);


OK - this is probably why you use ASTPointer's for the functions and subareas in ModifierArea - so that you can create the modifier area here and still fill in the functions and subareas below. However: are you sure you don't introduce cyclic dependencies of shared pointers here? The modifier areas contains a list of shared pointers to function definitions and the function definitions contain a shared pointer back to the modifier area? You probably at least need to change the parent pointers in the function definitions to a plain pointer instead of a shared pointer, although it would even be better if you could avoid such cyclic constructions altogether, if at all possible.

randomnetcat · 2018-06-12T02:37:35Z

@ekpyron , thanks for all of the feedback. I can't actually do anything for 5 days for personal reasons, but I will definitely look at it then :).

ekpyron · 2018-06-12T08:14:18Z

@meowingtwurtle No worries :-)!

federicobond · 2018-06-12T08:53:24Z

What would be the syntax if we add visibility eventually? Something like this?

using visibility public {
    using modifier [mod], [mod], ... {
        <functions, sub-modifier areas>
    }
}

Or is it going to be possible to put visibility besides other modifiers? In that case, are we ok with prefixing the whole list with the keyword modifier?

Edit: perhaps I should have brought this up in #623 or #2608 (comment)

chriseth · 2018-06-12T14:57:13Z

I would actually prefer to have visibility (and payability) specifiers and modifiers treated in the same way, although I would not use a comma as separator (we also do not have a comma in the function syntax).

randomnetcat · 2018-06-16T01:47:09Z

Okay, I see two ways to do this, and I would love ideas:

Have no ModifierArea ASTNode, just add modifiers/visibility in parser. I'm not sure how I feel about this, really, since it kind of hides the fact that they exist at all.
Have a ModifierArea ASTNode. However, this introduces the problems already seen in my code (cyclic dependencies). One way to fix this would just to not give FunctionDefinitions a shared_ptr to their ModifierArea, but I don't really like this idea, since it means that FunctionDefinitions would no longer have all of their context necessary for generation.

ekpyron · 2018-06-16T10:29:34Z

@meowingtwurtle You should definitely have AST nodes for the modifier areas.

The main problem with the current code is that it leaks memory. Since the modifier area AST node contains shared pointers to the function definitions and the function definitions contain shared pointers to the modifier area, they can never be freed, since the reference counters of the shared pointers will never fall to zero. Do you see the problem?

The easiest solution for that would be to use a plain, non-reference-counted pointer to the modifier area in the function definitions. That way you still have access to the modifier area from the function definitions, but you still break the reference cycle. You can compare with e.g. Scopable in the AST. It contains a plain pointer to the scope instead of an ASTPointer (i.e. shared_ptr) for the very same reason. In general you want to use shared pointers from parent to child, but never from child to parent in the AST. That way the life-time of objects is guaranteed, but you still don't introduce cycles (i.e. reference cycles of shared pointers).

An alternative would be to not have a pointer to the modifier area in the function definitions themselves, but instead to add a (plain) pointer to FunctionDefinitionAnnotation and fill them in while traversing the AST later on. The solution above is probably simpler and perfectly fine (probably even better) in this case, though.

randomnetcat · 2018-06-16T14:54:47Z

Okay, thank you!

randomnetcat · 2018-06-16T20:26:58Z

Okay, taking @ekpyron 's advice about pointers and @chriseth 's comment about visibility, mutability, and comma separation. (haven't finished yet)

Right now, the only way to differentiate between modifier area and using directive for parsing is whether or not the token after using is an identifier. This means that we have to have modifier or some other keyword/token after using. Or, we could use a different reserved keyword, like apply, here to signal it.

randomnetcat · 2018-06-18T17:10:55Z

apply has now been selected to replace using modifier

Add ModifierArea class Add methods to appropriate classes

axic · 2018-08-08T19:31:33Z

libsolidity/parsing/Token.h

@@ -182,6 +182,7 @@ namespace solidity
 	K(Var, "var", 0)                                                   \
 	K(View, "view", 0)                                                 \
 	K(While, "while", 0)                                               \
+	K(Apply, "apply", 0)                                               \


This section is alphabetically ordered.

axic · 2018-08-08T19:32:20Z

libsolidity/parsing/Parser.cpp

+			else if (_modifierArea && _modifierArea->visibility() != Declaration::Visibility::Default)
+			{
+				parserError("Cannot override modifier area's visibility.");
+				m_scanner->next();


Can these checks be moved to SyntaxtChecker?

axic · 2018-08-08T19:35:46Z

libsolidity/ast/AST.h

@@ -723,6 +721,42 @@ class VariableDeclaration: public Declaration
 	Location m_location; ///< Location of the variable if it is of reference type.
 };

+class ModifierArea: public ASTNode
+{
+public:


I think indentation here is off.

I just copied it from surrounding classes

axic · 2018-08-08T19:36:04Z

libsolidity/ast/AST.h

@@ -646,6 +643,7 @@ class FunctionDefinition: public CallableDeclaration, public Documented, public
 	bool m_isConstructor;
 	std::vector<ASTPointer<ModifierInvocation>> m_functionModifiers;
 	ASTPointer<Block> m_body;
+	ModifierArea* m_modifierArea;


Perhaps use ASTPointer?

This was something @ekpyron mentioned. We can''t use a smart pointer here because then there is a dependency loop and neither ModifierArea nor FunctionDefinition would be destructed.

axic · 2018-08-08T19:36:22Z

libsolidity/ast/AST.h

@@ -620,6 +614,9 @@ class FunctionDefinition: public CallableDeclaration, public Documented, public
 	std::vector<ASTPointer<ModifierInvocation>> const& modifiers() const { return m_functionModifiers; }
 	std::vector<ASTPointer<VariableDeclaration>> const& returnParameters() const { return m_returnParameters->parameters(); }
 	Block const& body() const { solAssert(m_body, ""); return *m_body; }
+	ModifierArea* modifierArea() { return m_modifierArea; }
+
+	std::string fullyQualifiedName() const;


I think this isn't needed anymore?

Remove contract name parameters Fix function visibility/state mutability

codecov · 2018-08-08T21:22:29Z

Codecov Report

Merging #3939 into develop will decrease coverage by 0.02%.
The diff coverage is 92%.

@@             Coverage Diff             @@
##           develop    #3939      +/-   ##
===========================================
- Coverage    87.56%   87.54%   -0.03%     
===========================================
  Files          313      313              
  Lines        30813    30954     +141     
  Branches      3662     3677      +15     
===========================================
+ Hits         26981    27098     +117     
- Misses        2579     2589      +10     
- Partials      1253     1267      +14

Flag	Coverage Δ
#all	`87.54% <92%> (-0.03%)`	⬇️
#syntax	`28.45% <68.66%> (+0.14%)`	⬆️

Update tests

Dismissing outdated review.

chriseth · 2019-11-04T11:57:40Z

Closing since this is unfortunately too much outdate for now :(

randomnetcat force-pushed the modifierAreas branch from 608a004 to 58824b8 Compare April 19, 2018 00:00

randomnetcat force-pushed the modifierAreas branch from 58824b8 to da4fe8d Compare April 28, 2018 22:53

ekpyron previously requested changes Jun 11, 2018

View reviewed changes

randomnetcat force-pushed the modifierAreas branch 5 times, most recently from cec8e15 to dccdbce Compare June 18, 2018 17:10

randomnetcat force-pushed the modifierAreas branch from dccdbce to 935198b Compare June 18, 2018 17:32

axic added the external contribution ⭐ label Aug 8, 2018

randomnetcat added 6 commits August 8, 2018 12:19

Move Apply token with other keywords

bb1c74a

Make changes in AST files

b66d15f

Add ModifierArea class Add methods to appropriate classes

Implement in Parser

fd81f00

Update ASTPrinter and ASTJsonConverter

06baee1

Update tests

17524b3

Update documentation

69d6bef

randomnetcat force-pushed the modifierAreas branch from 935198b to 69d6bef Compare August 8, 2018 16:43

randomnetcat added 2 commits August 8, 2018 12:48

Update tests for 0.5.0

812e4de

Update syntaxTests for 0.5.0

0c6c673

axic reviewed Aug 8, 2018

View reviewed changes

randomnetcat added 2 commits August 8, 2018 16:04

Update Parser

0c1faa7

Remove contract name parameters Fix function visibility/state mutability

Remove FunctionDefinition::fullyQualifiedName

bac10ee

randomnetcat added 3 commits August 9, 2018 21:30

Put apply token in alphabetical order

4c9d283

Update a few AST node methods

9109367

Move error checking to SyntaxChecker

9bd8646

Update tests

chriseth closed this Nov 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add modifier areas #3939

Add modifier areas #3939

randomnetcat commented Apr 18, 2018 •

edited

Loading

randomnetcat commented Apr 19, 2018

ekpyron left a comment

ekpyron Jun 11, 2018

ekpyron Jun 11, 2018

randomnetcat Jun 12, 2018

ekpyron Jun 11, 2018

ekpyron Jun 11, 2018

ekpyron Jun 11, 2018

ekpyron Jun 11, 2018

ekpyron Jun 11, 2018

ekpyron Jun 11, 2018

ekpyron Jun 11, 2018

ekpyron Jun 11, 2018

randomnetcat commented Jun 12, 2018

ekpyron commented Jun 12, 2018

federicobond commented Jun 12, 2018 •

edited

Loading

chriseth commented Jun 12, 2018

randomnetcat commented Jun 16, 2018

ekpyron commented Jun 16, 2018 •

edited

Loading

randomnetcat commented Jun 16, 2018

randomnetcat commented Jun 16, 2018 •

edited

Loading

randomnetcat commented Jun 18, 2018

axic Aug 8, 2018

axic Aug 8, 2018

randomnetcat Aug 9, 2018

axic Aug 8, 2018

randomnetcat Aug 10, 2018

axic Aug 8, 2018

randomnetcat Aug 9, 2018

axic Aug 8, 2018

randomnetcat Aug 8, 2018 •

edited

Loading

codecov bot commented Aug 8, 2018 •

edited

Loading

chriseth commented Nov 4, 2019

Add modifier areas #3939

Add modifier areas #3939

Conversation

randomnetcat commented Apr 18, 2018 • edited Loading

randomnetcat commented Apr 19, 2018

ekpyron left a 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

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

Choose a reason for hiding this comment

randomnetcat commented Jun 12, 2018

ekpyron commented Jun 12, 2018

federicobond commented Jun 12, 2018 • edited Loading

chriseth commented Jun 12, 2018

randomnetcat commented Jun 16, 2018

ekpyron commented Jun 16, 2018 • edited Loading

randomnetcat commented Jun 16, 2018

randomnetcat commented Jun 16, 2018 • edited Loading

randomnetcat commented Jun 18, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

randomnetcat Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 8, 2018 • edited Loading

Codecov Report

chriseth commented Nov 4, 2019

randomnetcat commented Apr 18, 2018 •

edited

Loading

federicobond commented Jun 12, 2018 •

edited

Loading

ekpyron commented Jun 16, 2018 •

edited

Loading

randomnetcat commented Jun 16, 2018 •

edited

Loading

randomnetcat Aug 8, 2018 •

edited

Loading

codecov bot commented Aug 8, 2018 •

edited

Loading