Merge pull request #5617 from ethereum/controlFlowRework

Rework of ControlFlowGraph and analysis.

Changelog.md

@@ -1,6 +1,7 @@
 ### 0.5.2 (unreleased)
 
 Language Features:
+ * Control Flow Graph: Detect every access to uninitialized storage pointers.
 
 
 Compiler Features:

libsolidity/analysis/ControlFlowAnalyzer.cpp

@@ -17,6 +17,7 @@
 
 #include <libsolidity/analysis/ControlFlowAnalyzer.h>
 #include <liblangutil/SourceLocation.h>
+#include <boost/range/algorithm/sort.hpp>
 
 using namespace std;
 using namespace langutil;
@@ -33,131 +34,112 @@ bool ControlFlowAnalyzer::visit(FunctionDefinition const& _function)
 	if (_function.isImplemented())
 	{
 		auto const& functionFlow = m_cfg.functionFlow(_function);
-		checkUnassignedStorageReturnValues(_function, functionFlow.entry, functionFlow.exit);
+		checkUninitializedAccess(functionFlow.entry, functionFlow.exit);
 	}
 	return false;
 }
 
-set<VariableDeclaration const*> ControlFlowAnalyzer::variablesAssignedInNode(CFGNode const *node)
+void ControlFlowAnalyzer::checkUninitializedAccess(CFGNode const* _entry, CFGNode const* _exit) const
 {
-	set<VariableDeclaration const*> result;
+	struct NodeInfo
-	for (auto expression: node->block.expressions)
 	{
-		if (auto const* assignment = dynamic_cast<Assignment const*>(expression))
+		set<VariableDeclaration const*> unassignedVariablesAtEntry;
+		set<VariableDeclaration const*> unassignedVariablesAtExit;
+		set<VariableOccurrence const*> uninitializedVariableAccesses;
+		/// Propagate the information from another node to this node.
+		/// To be used to propagate information from a node to its exit nodes.
+		/// Returns true, if new variables were added and thus the current node has
+		/// to be traversed again.
+		bool propagateFrom(NodeInfo const& _entryNode)
 		{
-			stack<Expression const*> expressions;
+			size_t previousUnassignedVariablesAtEntry = unassignedVariablesAtEntry.size();
-			expressions.push(&assignment->leftHandSide());
+			size_t previousUninitializedVariableAccessess = uninitializedVariableAccesses.size();
-			while (!expressions.empty())
+			unassignedVariablesAtEntry += _entryNode.unassignedVariablesAtExit;
-			{
+			uninitializedVariableAccesses += _entryNode.uninitializedVariableAccesses;
-				Expression const* expression = expressions.top();
+			return
-				expressions.pop();
+				unassignedVariablesAtEntry.size() > previousUnassignedVariablesAtEntry ||
-
+				uninitializedVariableAccesses.size() > previousUninitializedVariableAccessess
-				if (auto const *tuple = dynamic_cast<TupleExpression const*>(expression))
+			;
-					for (auto const& component: tuple->components())
-						expressions.push(component.get());
-				else if (auto const* identifier = dynamic_cast<Identifier const*>(expression))
-					if (auto const* variableDeclaration = dynamic_cast<VariableDeclaration const*>(
-						identifier->annotation().referencedDeclaration
-					))
-						result.insert(variableDeclaration);
-			}
 		}
-	}
+	};
-	return result;
+	map<CFGNode const*, NodeInfo> nodeInfos;
-}
+	set<CFGNode const*> nodesToTraverse;
+	nodesToTraverse.insert(_entry);
 
-void ControlFlowAnalyzer::checkUnassignedStorageReturnValues(
+	// Walk all paths starting from the nodes in ``nodesToTraverse`` until ``NodeInfo::propagateFrom``
-	FunctionDefinition const& _function,
+	// returns false for all exits, i.e. until all paths have been walked with maximal sets of unassigned
-	CFGNode const* _functionEntry,
+	// variables and accesses.
-	CFGNode const* _functionExit
-) const
-{
-	if (_function.returnParameterList()->parameters().empty())
-		return;
-
-	map<CFGNode const*, set<VariableDeclaration const*>> unassigned;
-
-	{
-		auto& unassignedAtFunctionEntry = unassigned[_functionEntry];
-		for (auto const& returnParameter: _function.returnParameterList()->parameters())
-			if (
-				returnParameter->type()->dataStoredIn(DataLocation::Storage) ||
-				returnParameter->type()->category() == Type::Category::Mapping
-			)
-				unassignedAtFunctionEntry.insert(returnParameter.get());
-	}
-
-	stack<CFGNode const*> nodesToTraverse;
-	nodesToTraverse.push(_functionEntry);
-
-	// walk all paths from entry with maximal set of unassigned return values
 	while (!nodesToTraverse.empty())
 	{
-		auto node = nodesToTraverse.top();
+		CFGNode const* currentNode = *nodesToTraverse.begin();
-		nodesToTraverse.pop();
+		nodesToTraverse.erase(nodesToTraverse.begin());
 
-		auto& unassignedAtNode = unassigned[node];
+		auto& nodeInfo = nodeInfos[currentNode];
-
+		auto unassignedVariables = nodeInfo.unassignedVariablesAtEntry;
-		if (node->block.returnStatement != nullptr)
+		for (auto const& variableOccurrence: currentNode->variableOccurrences)
-			if (node->block.returnStatement->expression())
-				unassignedAtNode.clear();
-		if (!unassignedAtNode.empty())
 		{
-			// kill all return values to which a value is assigned
+			switch (variableOccurrence.kind())
-			for (auto const* variableDeclaration: variablesAssignedInNode(node))
+			{
-				unassignedAtNode.erase(variableDeclaration);
+				case VariableOccurrence::Kind::Assignment:
-
+					unassignedVariables.erase(&variableOccurrence.declaration());
-			// kill all return values referenced in inline assembly
+					break;
-			// a reference is enough, checking whether there actually was an assignment might be overkill
+				case VariableOccurrence::Kind::InlineAssembly:
-			for (auto assembly: node->block.inlineAssemblyStatements)
+					// We consider all variables referenced in inline assembly as accessed.
-				for (auto const& ref: assembly->annotation().externalReferences)
+					// So far any reference is enough, but we might want to actually analyze
-					if (auto variableDeclaration = dynamic_cast<VariableDeclaration const*>(ref.second.declaration))
+					// the control flow in the assembly at some point.
-						unassignedAtNode.erase(variableDeclaration);
+				case VariableOccurrence::Kind::Access:
+				case VariableOccurrence::Kind::Return:
+					if (unassignedVariables.count(&variableOccurrence.declaration()))
+					{
+						if (variableOccurrence.declaration().type()->dataStoredIn(DataLocation::Storage))
+							// Merely store the unassigned access. We do not generate an error right away, since this
+							// path might still always revert. It is only an error if this is propagated to the exit
+							// node of the function (i.e. there is a path with an uninitialized access).
+							nodeInfo.uninitializedVariableAccesses.insert(&variableOccurrence);
+					}
+					break;
+				case VariableOccurrence::Kind::Declaration:
+					unassignedVariables.insert(&variableOccurrence.declaration());
+					break;
+			}
 		}
+		nodeInfo.unassignedVariablesAtExit = std::move(unassignedVariables);
 
-		for (auto const& exit: node->exits)
+		// Propagate changes to all exits and queue them for traversal, if needed.
-		{
+		for (auto const& exit: currentNode->exits)
-			auto& unassignedAtExit = unassigned[exit];
+			if (nodeInfos[exit].propagateFrom(nodeInfo))
-			auto oldSize = unassignedAtExit.size();
+				nodesToTraverse.insert(exit);
-			unassignedAtExit.insert(unassignedAtNode.begin(), unassignedAtNode.end());
-			// (re)traverse an exit, if we are on a path with new unassigned return values to consider
-			// this will terminate, since there is only a finite number of unassigned return values
-			if (unassignedAtExit.size() > oldSize)
-				nodesToTraverse.push(exit);
-		}
 	}
 
-	if (!unassigned[_functionExit].empty())
+	auto const& exitInfo = nodeInfos[_exit];
+	if (!exitInfo.uninitializedVariableAccesses.empty())
 	{
-		vector<VariableDeclaration const*> unassignedOrdered(
+		vector<VariableOccurrence const*> uninitializedAccessesOrdered(
-			unassigned[_functionExit].begin(),
+			exitInfo.uninitializedVariableAccesses.begin(),
-			unassigned[_functionExit].end()
+			exitInfo.uninitializedVariableAccesses.end()
-		);
+		 );
-		sort(
+		boost::range::sort(
-			unassignedOrdered.begin(),
+			uninitializedAccessesOrdered,
-			unassignedOrdered.end(),
+			[](VariableOccurrence const* lhs, VariableOccurrence const* rhs) -> bool
-			[](VariableDeclaration const* lhs, VariableDeclaration const* rhs) -> bool {
+			{
-				return lhs->id() < rhs->id();
+				return *lhs < *rhs;
 			}
 		);
-		for (auto const* returnVal: unassignedOrdered)
+
+		for (auto const* variableOccurrence: uninitializedAccessesOrdered)
 		{
 			SecondarySourceLocation ssl;
-			for (CFGNode* lastNodeBeforeExit: _functionExit->entries)
+			if (variableOccurrence->occurrence())
-				if (unassigned[lastNodeBeforeExit].count(returnVal))
+				ssl.append("The variable was declared here.", variableOccurrence->declaration().location());
-				{
-					if (!!lastNodeBeforeExit->block.returnStatement)
-						ssl.append("Problematic return:", lastNodeBeforeExit->block.returnStatement->location());
-					else
-						ssl.append("Problematic end of function:", _function.location());
-				}
 
 			m_errorReporter.typeError(
-				returnVal->location(),
+				variableOccurrence->occurrence() ?
+					variableOccurrence->occurrence()->location() :
+					variableOccurrence->declaration().location(),
 				ssl,
-				"This variable is of storage pointer type and might be returned without assignment and "
+				string("This variable is of storage pointer type and can be ") +
-				"could be used uninitialized. Assign the variable (potentially from itself) "
+				(variableOccurrence->kind() == VariableOccurrence::Kind::Return ? "returned" : "accessed") +
-				"to fix this error."
+				" without prior assignment."
 			);
 		}
 	}

libsolidity/analysis/ControlFlowAnalyzer.h

@@ -37,12 +37,8 @@ public:
 	bool visit(FunctionDefinition const& _function) override;
 
 private:
-	static std::set<VariableDeclaration const*> variablesAssignedInNode(CFGNode const *node);
+	/// Checks for uninitialized variable accesses in the control flow between @param _entry and @param _exit.
-	void checkUnassignedStorageReturnValues(
+	void checkUninitializedAccess(CFGNode const* _entry, CFGNode const* _exit) const;
-		FunctionDefinition const& _function,
-		CFGNode const* _functionEntry,
-		CFGNode const* _functionExit
-	) const;
 
 	CFG const& m_cfg;
 	langutil::ErrorReporter& m_errorReporter;

libsolidity/analysis/ControlFlowBuilder.cpp

@@ -22,7 +22,10 @@ using namespace solidity;
 using namespace std;
 
 ControlFlowBuilder::ControlFlowBuilder(CFG::NodeContainer& _nodeContainer, FunctionFlow const& _functionFlow):
-	m_nodeContainer(_nodeContainer), m_currentFunctionFlow(_functionFlow), m_currentNode(_functionFlow.entry)
+	m_nodeContainer(_nodeContainer),
+	m_currentNode(_functionFlow.entry),
+	m_returnNode(_functionFlow.exit),
+	m_revertNode(_functionFlow.revert)
 {
 }
 
@@ -37,28 +40,10 @@ unique_ptr<FunctionFlow> ControlFlowBuilder::createFunctionFlow(
 	functionFlow->revert = _nodeContainer.newNode();
 	ControlFlowBuilder builder(_nodeContainer, *functionFlow);
 	builder.appendControlFlow(_function);
-	connect(builder.m_currentNode, functionFlow->exit);
+
 	return functionFlow;
 }
 
-
-unique_ptr<ModifierFlow> ControlFlowBuilder::createModifierFlow(
-	CFG::NodeContainer& _nodeContainer,
-	ModifierDefinition const& _modifier
-)
-{
-	auto modifierFlow = unique_ptr<ModifierFlow>(new ModifierFlow());
-	modifierFlow->entry = _nodeContainer.newNode();
-	modifierFlow->exit = _nodeContainer.newNode();
-	modifierFlow->revert = _nodeContainer.newNode();
-	modifierFlow->placeholderEntry = _nodeContainer.newNode();
-	modifierFlow->placeholderExit = _nodeContainer.newNode();
-	ControlFlowBuilder builder(_nodeContainer, *modifierFlow);
-	builder.appendControlFlow(_modifier);
-	connect(builder.m_currentNode, modifierFlow->exit);
-	return modifierFlow;
-}
-
 bool ControlFlowBuilder::visit(BinaryOperation const& _operation)
 {
 	solAssert(!!m_currentNode, "");
@@ -219,64 +204,24 @@ bool ControlFlowBuilder::visit(Continue const&)
 bool ControlFlowBuilder::visit(Throw const&)
 {
 	solAssert(!!m_currentNode, "");
-	solAssert(!!m_currentFunctionFlow.revert, "");
+	solAssert(!!m_revertNode, "");
-	connect(m_currentNode, m_currentFunctionFlow.revert);
+	connect(m_currentNode, m_revertNode);
 	m_currentNode = newLabel();
 	return false;
 }
 
-bool ControlFlowBuilder::visit(Block const&)
-{
-	solAssert(!!m_currentNode, "");
-	createLabelHere();
-	return true;
-}
-
-void ControlFlowBuilder::endVisit(Block const&)
-{
-	solAssert(!!m_currentNode, "");
-	createLabelHere();
-}
-
-bool ControlFlowBuilder::visit(Return const& _return)
-{
-	solAssert(!!m_currentNode, "");
-	solAssert(!!m_currentFunctionFlow.exit, "");
-	solAssert(!m_currentNode->block.returnStatement, "");
-	m_currentNode->block.returnStatement = &_return;
-	connect(m_currentNode, m_currentFunctionFlow.exit);
-	m_currentNode = newLabel();
-	return true;
-}
-
-
 bool ControlFlowBuilder::visit(PlaceholderStatement const&)
 {
 	solAssert(!!m_currentNode, "");
-	auto modifierFlow = dynamic_cast<ModifierFlow const*>(&m_currentFunctionFlow);
+	solAssert(!!m_placeholderEntry, "");
-	solAssert(!!modifierFlow, "");
+	solAssert(!!m_placeholderExit, "");
-
-	connect(m_currentNode, modifierFlow->placeholderEntry);
 
+	connect(m_currentNode, m_placeholderEntry);
 	m_currentNode = newLabel();
-
+	connect(m_placeholderExit, m_currentNode);
-	connect(modifierFlow->placeholderExit, m_currentNode);
 	return false;
 }
 
-bool ControlFlowBuilder::visitNode(ASTNode const& node)
-{
-	solAssert(!!m_currentNode, "");
-	if (auto const* expression = dynamic_cast<Expression const*>(&node))
-		m_currentNode->block.expressions.emplace_back(expression);
-	else if (auto const* variableDeclaration = dynamic_cast<VariableDeclaration const*>(&node))
-		m_currentNode->block.variableDeclarations.emplace_back(variableDeclaration);
-	else if (auto const* assembly = dynamic_cast<InlineAssembly const*>(&node))
-		m_currentNode->block.inlineAssemblyStatements.emplace_back(assembly);
-
-	return true;
-}
-
 bool ControlFlowBuilder::visit(FunctionCall const& _functionCall)
 {
 	solAssert(!!m_currentNode, "");
@@ -286,19 +231,19 @@ bool ControlFlowBuilder::visit(FunctionCall const& _functionCall)
 		switch (functionType->kind())
 		{
 			case FunctionType::Kind::Revert:
-				solAssert(!!m_currentFunctionFlow.revert, "");
+				solAssert(!!m_revertNode, "");
 				_functionCall.expression().accept(*this);
 				ASTNode::listAccept(_functionCall.arguments(), *this);
-				connect(m_currentNode, m_currentFunctionFlow.revert);
+				connect(m_currentNode, m_revertNode);
 				m_currentNode = newLabel();
 				return false;
 			case FunctionType::Kind::Require:
 			case FunctionType::Kind::Assert:
 			{
-				solAssert(!!m_currentFunctionFlow.revert, "");
+				solAssert(!!m_revertNode, "");
 				_functionCall.expression().accept(*this);
 				ASTNode::listAccept(_functionCall.arguments(), *this);
-				connect(m_currentNode, m_currentFunctionFlow.revert);
+				connect(m_currentNode, m_revertNode);
 				auto nextNode = newLabel();
 				connect(m_currentNode, nextNode);
 				m_currentNode = nextNode;
@@ -310,6 +255,183 @@ bool ControlFlowBuilder::visit(FunctionCall const& _functionCall)
 	return ASTConstVisitor::visit(_functionCall);
 }
 
+bool ControlFlowBuilder::visit(ModifierInvocation const& _modifierInvocation)
+{
+	if (auto arguments = _modifierInvocation.arguments())
+		for (auto& argument: *arguments)
+			appendControlFlow(*argument);
+
+	auto modifierDefinition = dynamic_cast<ModifierDefinition const*>(
+		_modifierInvocation.name()->annotation().referencedDeclaration
+	);
+	if (!modifierDefinition) return false;
+	solAssert(!!modifierDefinition, "");
+	solAssert(!!m_returnNode, "");
+
+	m_placeholderEntry = newLabel();
+	m_placeholderExit = newLabel();
+
+	appendControlFlow(*modifierDefinition);
+	connect(m_currentNode, m_returnNode);
+
+	m_currentNode = m_placeholderEntry;
+	m_returnNode = m_placeholderExit;
+
+	m_placeholderEntry = nullptr;
+	m_placeholderExit = nullptr;
+
+	return false;
+}
+
+bool ControlFlowBuilder::visit(FunctionDefinition const& _functionDefinition)
+{
+	for (auto const& parameter: _functionDefinition.parameters())
+		appendControlFlow(*parameter);
+
+	for (auto const& returnParameter: _functionDefinition.returnParameters())
+	{
+		appendControlFlow(*returnParameter);
+		m_returnNode->variableOccurrences.emplace_back(
+			*returnParameter,
+			VariableOccurrence::Kind::Return,
+			nullptr
+		);
+
+	}
+
+	for (auto const& modifier: _functionDefinition.modifiers())
+		appendControlFlow(*modifier);
+
+	appendControlFlow(_functionDefinition.body());
+
+	connect(m_currentNode, m_returnNode);
+	m_currentNode = nullptr;
+
+	return false;
+}
+
+bool ControlFlowBuilder::visit(Return const& _return)
+{
+	solAssert(!!m_currentNode, "");
+	solAssert(!!m_returnNode, "");
+	if (_return.expression())
+	{
+		appendControlFlow(*_return.expression());
+		// Returns with return expression are considered to be assignments to the return parameters.
+		for (auto returnParameter: _return.annotation().functionReturnParameters->parameters())
+			m_currentNode->variableOccurrences.emplace_back(
+				*returnParameter,
+				VariableOccurrence::Kind::Assignment,
+				&_return
+			);
+	}
+	connect(m_currentNode, m_returnNode);
+	m_currentNode = newLabel();
+	return true;
+}
+
+bool ControlFlowBuilder::visit(FunctionTypeName const&)
+{
+	// Do not visit the parameters and return values of a function type name.
+	// We do not want to consider them as variable declarations for the control flow graph.
+	return false;
+}
+
+bool ControlFlowBuilder::visit(InlineAssembly const& _inlineAssembly)
+{
+	solAssert(!!m_currentNode, "");
+	for (auto const& ref: _inlineAssembly.annotation().externalReferences)
+	{
+		if (auto variableDeclaration = dynamic_cast<VariableDeclaration const*>(ref.second.declaration))
+			m_currentNode->variableOccurrences.emplace_back(
+				*variableDeclaration,
+				VariableOccurrence::Kind::InlineAssembly,
+				&_inlineAssembly
+			);
+	}
+	return true;
+}
+
+bool ControlFlowBuilder::visit(VariableDeclaration const& _variableDeclaration)
+{
+	solAssert(!!m_currentNode, "");
+
+	m_currentNode->variableOccurrences.emplace_back(
+		_variableDeclaration,
+		VariableOccurrence::Kind::Declaration,
+		nullptr
+	);
+
+	// Handle declaration with immediate assignment.
+	if (_variableDeclaration.value())
+		m_currentNode->variableOccurrences.emplace_back(
+			_variableDeclaration,
+			VariableOccurrence::Kind::Assignment,
+			_variableDeclaration.value().get()
+		);
+	// Function arguments are considered to be immediately assigned as well (they are "externally assigned").
+	else if (_variableDeclaration.isCallableParameter() && !_variableDeclaration.isReturnParameter())
+		m_currentNode->variableOccurrences.emplace_back(
+			_variableDeclaration,
+			VariableOccurrence::Kind::Assignment,
+			nullptr
+		);
+	return true;
+}
+
+bool ControlFlowBuilder::visit(VariableDeclarationStatement const& _variableDeclarationStatement)
+{
+	solAssert(!!m_currentNode, "");
+
+	for (auto const& var: _variableDeclarationStatement.declarations())
+		if (var)
+			var->accept(*this);
+	if (_variableDeclarationStatement.initialValue())
+	{
+		_variableDeclarationStatement.initialValue()->accept(*this);
+		for (size_t i = 0; i < _variableDeclarationStatement.declarations().size(); i++)
+			if (auto const& var = _variableDeclarationStatement.declarations()[i])
+			{
+				auto expression = _variableDeclarationStatement.initialValue();
+				if (auto tupleExpression = dynamic_cast<TupleExpression const*>(expression))
+					if (tupleExpression->components().size() > 1)
+					{
+						solAssert(tupleExpression->components().size() > i, "");
+						expression = tupleExpression->components()[i].get();
+					}
+				while (auto tupleExpression = dynamic_cast<TupleExpression const*>(expression))
+					if (tupleExpression->components().size() == 1)
+						expression = tupleExpression->components().front().get();
+					else
+						break;
+				m_currentNode->variableOccurrences.emplace_back(
+					*var,
+					VariableOccurrence::Kind::Assignment,
+					expression
+				);
+			}
+	}
+	return false;
+}
+
+bool ControlFlowBuilder::visit(Identifier const& _identifier)
+{
+	solAssert(!!m_currentNode, "");
+
+	if (auto const* variableDeclaration = dynamic_cast<VariableDeclaration const*>(_identifier.annotation().referencedDeclaration))
+		m_currentNode->variableOccurrences.emplace_back(
+			*variableDeclaration,
+			static_cast<Expression const&>(_identifier).annotation().lValueRequested ?
+			VariableOccurrence::Kind::Assignment :
+			VariableOccurrence::Kind::Access,
+			&_identifier
+		);
+
+	return true;
+}
+
+
+
 void ControlFlowBuilder::appendControlFlow(ASTNode const& _node)
 {
 	_node.accept(*this);

libsolidity/analysis/ControlFlowBuilder.h

@@ -38,14 +38,11 @@ public:
 		CFG::NodeContainer& _nodeContainer,
 		FunctionDefinition const& _function
 	);
-	static std::unique_ptr<ModifierFlow> createModifierFlow(
-		CFG::NodeContainer& _nodeContainer,
-		ModifierDefinition const& _modifier
-	);
 
 private:
 	explicit ControlFlowBuilder(CFG::NodeContainer& _nodeContainer, FunctionFlow const& _functionFlow);
 
+	// Visits for constructing the control flow.
 	bool visit(BinaryOperation const& _operation) override;
 	bool visit(Conditional const& _conditional) override;
 	bool visit(IfStatement const& _ifStatement) override;
@@ -54,12 +51,20 @@ private:
 	bool visit(Break const&) override;
 	bool visit(Continue const&) override;
 	bool visit(Throw const&) override;
-	bool visit(Block const&) override;
-	void endVisit(Block const&) override;
-	bool visit(Return const& _return) override;
 	bool visit(PlaceholderStatement const&) override;
 	bool visit(FunctionCall const& _functionCall) override;
+	bool visit(ModifierInvocation const& _modifierInvocation) override;
 
+	// Visits for constructing the control flow as well as filling variable occurrences.
+	bool visit(FunctionDefinition const& _functionDefinition) override;
+	bool visit(Return const& _return) override;
+
+	// Visits for filling variable occurrences.
+	bool visit(FunctionTypeName const& _functionTypeName) override;
+	bool visit(InlineAssembly const& _inlineAssembly) override;
+	bool visit(VariableDeclaration const& _variableDeclaration) override;
+	bool visit(VariableDeclarationStatement const& _variableDeclarationStatement) override;
+	bool visit(Identifier const& _identifier) override;
 
 	/// Appends the control flow of @a _node to the current control flow.
 	void appendControlFlow(ASTNode const& _node);
@@ -73,9 +78,6 @@ private:
 	static void connect(CFGNode* _from, CFGNode* _to);
 
 
-protected:
-	bool visitNode(ASTNode const& node) override;
-
 private:
 
 	/// Splits the control flow starting at the current node into n paths.
@@ -114,17 +116,18 @@ private:
 
 	CFG::NodeContainer& m_nodeContainer;
 
-	/// The control flow of the function that is currently parsed.
-	/// Note: this can also be a ModifierFlow
-	FunctionFlow const& m_currentFunctionFlow;
-
 	CFGNode* m_currentNode = nullptr;
+	CFGNode* m_returnNode = nullptr;
+	CFGNode* m_revertNode = nullptr;
 
 	/// The current jump destination of break Statements.
 	CFGNode* m_breakJump = nullptr;
 	/// The current jump destination of continue Statements.
 	CFGNode* m_continueJump = nullptr;
 
+	CFGNode* m_placeholderEntry = nullptr;
+	CFGNode* m_placeholderExit = nullptr;
+
 	/// Helper class that replaces the break and continue jump destinations for the
 	/// current scope and restores the originals at the end of the scope.
 	class BreakContinueScope

libsolidity/analysis/ControlFlowGraph.cpp

@@ -29,20 +29,14 @@ using namespace dev::solidity;
 bool CFG::constructFlow(ASTNode const& _astRoot)
 {
 	_astRoot.accept(*this);
-	applyModifiers();
 	return Error::containsOnlyWarnings(m_errorReporter.errors());
 }
 
 
-bool CFG::visit(ModifierDefinition const& _modifier)
-{
-	m_modifierControlFlow[&_modifier] = ControlFlowBuilder::createModifierFlow(m_nodeContainer, _modifier);
-	return false;
-}
-
 bool CFG::visit(FunctionDefinition const& _function)
 {
-	m_functionControlFlow[&_function] = ControlFlowBuilder::createFunctionFlow(m_nodeContainer, _function);
+	if (_function.isImplemented())
+		m_functionControlFlow[&_function] = ControlFlowBuilder::createFunctionFlow(m_nodeContainer, _function);
 	return false;
 }
 
@@ -57,81 +51,3 @@ CFGNode* CFG::NodeContainer::newNode()
 	m_nodes.emplace_back(new CFGNode());
 	return m_nodes.back().get();
 }
-
-void CFG::applyModifiers()
-{
-	for (auto const& function: m_functionControlFlow)
-	{
-		for (auto const& modifierInvocation: boost::adaptors::reverse(function.first->modifiers()))
-		{
-			if (auto modifierDefinition = dynamic_cast<ModifierDefinition const*>(
-				modifierInvocation->name()->annotation().referencedDeclaration
-			))
-			{
-				solAssert(m_modifierControlFlow.count(modifierDefinition), "");
-				applyModifierFlowToFunctionFlow(*m_modifierControlFlow[modifierDefinition], function.second.get());
-			}
-		}
-	}
-}
-
-void CFG::applyModifierFlowToFunctionFlow(
-	ModifierFlow const& _modifierFlow,
-	FunctionFlow* _functionFlow
-)
-{
-	solAssert(!!_functionFlow, "");
-
-	map<CFGNode*, CFGNode*> copySrcToCopyDst;
-
-	// inherit the revert node of the function
-	copySrcToCopyDst[_modifierFlow.revert] = _functionFlow->revert;
-
-	// replace the placeholder nodes by the function entry and exit
-	copySrcToCopyDst[_modifierFlow.placeholderEntry] = _functionFlow->entry;
-	copySrcToCopyDst[_modifierFlow.placeholderExit] = _functionFlow->exit;
-
-	stack<CFGNode*> nodesToCopy;
-	nodesToCopy.push(_modifierFlow.entry);
-
-	// map the modifier entry to a new node that will become the new function entry
-	copySrcToCopyDst[_modifierFlow.entry] = m_nodeContainer.newNode();
-
-	while (!nodesToCopy.empty())
-	{
-		CFGNode* copySrcNode = nodesToCopy.top();
-		nodesToCopy.pop();
-
-		solAssert(copySrcToCopyDst.count(copySrcNode), "");
-
-		CFGNode* copyDstNode = copySrcToCopyDst[copySrcNode];
-
-		copyDstNode->block = copySrcNode->block;
-		for (auto const& entry: copySrcNode->entries)
-		{
-			if (!copySrcToCopyDst.count(entry))
-			{
-				copySrcToCopyDst[entry] = m_nodeContainer.newNode();
-				nodesToCopy.push(entry);
-			}
-			copyDstNode->entries.emplace_back(copySrcToCopyDst[entry]);
-		}
-		for (auto const& exit: copySrcNode->exits)
-		{
-			if (!copySrcToCopyDst.count(exit))
-			{
-				copySrcToCopyDst[exit] = m_nodeContainer.newNode();
-				nodesToCopy.push(exit);
-			}
-			copyDstNode->exits.emplace_back(copySrcToCopyDst[exit]);
-		}
-	}
-
-	// if the modifier control flow never reached its exit node,
-	// we need to create a new (disconnected) exit node now
-	if (!copySrcToCopyDst.count(_modifierFlow.exit))
-		copySrcToCopyDst[_modifierFlow.exit] = m_nodeContainer.newNode();
-
-	_functionFlow->entry = copySrcToCopyDst[_modifierFlow.entry];
-	_functionFlow->exit = copySrcToCopyDst[_modifierFlow.exit];
-}

libsolidity/analysis/ControlFlowGraph.h

@@ -31,25 +31,57 @@ namespace dev
 namespace solidity
 {
 
-/** Basic Control Flow Block.
+/** Occurrence of a variable in a block of control flow.
- * Basic block of control flow. Consists of a set of (unordered) AST nodes
+  * Stores the declaration of the referenced variable, the
- * for which control flow is always linear. A basic control flow block
+  * kind of the occurrence and possibly the node at which
- * encompasses at most one scope. Reverts are considered to break the control
+  * it occurred.
- * flow.
+  */
- * @todo Handle function calls correctly. So far function calls are not considered
+class VariableOccurrence
- * to change the control flow.
- */
-struct ControlFlowBlock
 {
-	/// All variable declarations inside this control flow block.
+public:
-	std::vector<VariableDeclaration const*> variableDeclarations;
+	enum class Kind
-	/// All expressions inside this control flow block (this includes all subexpressions!).
+	{
-	std::vector<Expression const*> expressions;
+		Declaration,
-	/// All inline assembly statements inside in this control flow block.
+		Access,
-	std::vector<InlineAssembly const*> inlineAssemblyStatements;
+		Return,
-	/// If control flow returns in this node, the return statement is stored in returnStatement,
+		Assignment,
-	/// otherwise returnStatement is nullptr.
+		InlineAssembly
-	Return const* returnStatement = nullptr;
+	};
+	VariableOccurrence(VariableDeclaration const& _declaration, Kind _kind, ASTNode const* _occurrence):
+		m_declaration(_declaration), m_occurrenceKind(_kind), m_occurrence(_occurrence)
+	{
+	}
+
+	/// Defines a deterministic order on variable occurrences.
+	bool operator<(VariableOccurrence const& _rhs) const
+	{
+		if (m_occurrence && _rhs.m_occurrence)
+		{
+			if (m_occurrence->id() < _rhs.m_occurrence->id()) return true;
+			if (_rhs.m_occurrence->id() < m_occurrence->id()) return false;
+		}
+		else if (_rhs.m_occurrence)
+			return true;
+		else if (m_occurrence)
+			return false;
+
+		using KindCompareType = std::underlying_type<VariableOccurrence::Kind>::type;
+		return
+			std::make_pair(m_declaration.id(), static_cast<KindCompareType>(m_occurrenceKind)) <
+			std::make_pair(_rhs.m_declaration.id(), static_cast<KindCompareType>(_rhs.m_occurrenceKind))
+		;
+	}
+
+	VariableDeclaration const& declaration() const { return m_declaration; }
+	Kind kind() const { return m_occurrenceKind; };
+	ASTNode const* occurrence() const { return m_occurrence; }
+private:
+	/// Declaration of the occurring variable.
+	VariableDeclaration const& m_declaration;
+	/// Kind of occurrence.
+	Kind m_occurrenceKind = Kind::Access;
+	/// AST node at which the variable occurred, if available (may be nullptr).
+	ASTNode const* m_occurrence = nullptr;
 };
 
 /** Node of the Control Flow Graph.
@@ -64,8 +96,8 @@ struct CFGNode
 	/// Exit nodes. All CFG nodes to which control flow may continue after this node.
 	std::vector<CFGNode*> exits;
 
-	/// Control flow in the node.
+	/// Variable occurrences in the node.
-	ControlFlowBlock block;
+	std::vector<VariableOccurrence> variableOccurrences;
 };
 
 /** Describes the control flow of a function. */
@@ -85,19 +117,6 @@ struct FunctionFlow
 	CFGNode* revert = nullptr;
 };
 
-/** Describes the control flow of a modifier.
- * Every placeholder breaks the control flow. The node preceding the
- * placeholder is assigned placeholderEntry as exit and the node
- * following the placeholder is assigned placeholderExit as entry.
- */
-struct ModifierFlow: FunctionFlow
-{
-	/// Control flow leading towards a placeholder exit in placeholderEntry.
-	CFGNode* placeholderEntry = nullptr;
-	/// Control flow coming from a placeholder enter from placeholderExit.
-	CFGNode* placeholderExit = nullptr;
-};
-
 class CFG: private ASTConstVisitor
 {
 public:
@@ -105,7 +124,6 @@ public:
 
 	bool constructFlow(ASTNode const& _astRoot);
 
-	bool visit(ModifierDefinition const& _modifier) override;
 	bool visit(FunctionDefinition const& _function) override;
 
 	FunctionFlow const& functionFlow(FunctionDefinition const& _function) const;
@@ -118,20 +136,6 @@ public:
 		std::vector<std::unique_ptr<CFGNode>> m_nodes;
 	};
 private:
-	/// Initially the control flow for all functions *ignoring* modifiers and for
-	/// all modifiers is constructed. Afterwards the control flow of functions
-	/// is adjusted by applying all modifiers.
-	void applyModifiers();
-
-	/// Creates a copy of the modifier flow @a _modifierFlow, while replacing the
-	/// placeholder entry and exit with the function entry and exit, as well as
-	/// replacing the modifier revert node with the function's revert node.
-	/// The resulting control flow is the new function flow with the modifier applied.
-	/// @a _functionFlow is updated in-place.
-	void applyModifierFlowToFunctionFlow(
-		ModifierFlow const& _modifierFlow,
-		FunctionFlow* _functionFlow
-	);
 
 	langutil::ErrorReporter& m_errorReporter;
 
@@ -141,7 +145,6 @@ private:
 	NodeContainer m_nodeContainer;
 
 	std::map<FunctionDefinition const*, std::unique_ptr<FunctionFlow>> m_functionControlFlow;
-	std::map<ModifierDefinition const*, std::unique_ptr<ModifierFlow>> m_modifierControlFlow;
 };
 
 }

test/libsolidity/syntaxTests/controlFlow/mappingReturn/named_err.sol

@@ -2,4 +2,4 @@ contract C {
     function f() internal pure returns (mapping(uint=>uint) storage r) { }
 }
 // ----
-// TypeError: (53-82): This variable is of storage pointer type and might be returned without assignment and could be used uninitialized. Assign the variable (potentially from itself) to fix this error.
+// TypeError: (53-82): This variable is of storage pointer type and can be returned without prior assignment.

test/libsolidity/syntaxTests/controlFlow/mappingReturn/unnamed_err.sol

@@ -2,4 +2,4 @@ contract C {
     function f() internal pure returns (mapping(uint=>uint) storage) {}
 }
 // ----
-// TypeError: (53-80): This variable is of storage pointer type and might be returned without assignment and could be used uninitialized. Assign the variable (potentially from itself) to fix this error.
+// TypeError: (53-80): This variable is of storage pointer type and can be returned without prior assignment.

test/libsolidity/syntaxTests/controlFlow/storageReturn/assembly_err.sol

@@ -7,4 +7,4 @@ contract C {
     }
 }
 // ----
-// TypeError: (87-96): This variable is of storage pointer type and might be returned without assignment and could be used uninitialized. Assign the variable (potentially from itself) to fix this error.
+// TypeError: (87-96): This variable is of storage pointer type and can be returned without prior assignment.

test/libsolidity/syntaxTests/controlFlow/storageReturn/assembly_fine.sol

@@ -1,26 +0,0 @@
-contract C {
-    struct S { bool f; }
-    S s;
-    function f() internal returns (S storage c) {
-        assembly {
-            sstore(c_slot, sload(s_slot))
-        }
-    }
-    function g(bool flag) internal returns (S storage c) {
-        // control flow in assembly will not be analyzed for now,
-        // so this will not issue an error
-        assembly {
-            if flag {
-                sstore(c_slot, sload(s_slot))
-            }
-        }
-    }
-    function h() internal returns (S storage c) {
-        // any reference from assembly will be sufficient for now,
-        // so this will not issue an error
-        assembly {
-            sstore(s_slot, sload(c_slot))
-        }
-    }
-}
-// ----

test/libsolidity/syntaxTests/controlFlow/storageReturn/dowhile_err.sol

@@ -45,8 +45,8 @@ contract C {
     }
 }
 // ----
-// TypeError: (87-98): This variable is of storage pointer type and might be returned without assignment and could be used uninitialized. Assign the variable (potentially from itself) to fix this error.
+// TypeError: (87-98): This variable is of storage pointer type and can be returned without prior assignment.
-// TypeError: (223-234): This variable is of storage pointer type and might be returned without assignment and could be used uninitialized. Assign the variable (potentially from itself) to fix this error.
+// TypeError: (223-234): This variable is of storage pointer type and can be returned without prior assignment.
-// TypeError: (440-451): This variable is of storage pointer type and might be returned without assignment and could be used uninitialized. Assign the variable (potentially from itself) to fix this error.
+// TypeError: (440-451): This variable is of storage pointer type and can be returned without prior assignment.
-// TypeError: (654-665): This variable is of storage pointer type and might be returned without assignment and could be used uninitialized. Assign the variable (potentially from itself) to fix this error.
+// TypeError: (654-665): This variable is of storage pointer type and can be returned without prior assignment.
-// TypeError: (871-882): This variable is of storage pointer type and might be returned without assignment and could be used uninitialized. Assign the variable (potentially from itself) to fix this error.
+// TypeError: (871-882): This variable is of storage pointer type and can be returned without prior assignment.

test/libsolidity/syntaxTests/controlFlow/storageReturn/for_err.sol

@@ -12,5 +12,5 @@ contract C {
     }
 }
 // ----
-// TypeError: (87-98): This variable is of storage pointer type and might be returned without assignment and could be used uninitialized. Assign the variable (potentially from itself) to fix this error.
+// TypeError: (87-98): This variable is of storage pointer type and can be returned without prior assignment.
-// TypeError: (182-193): This variable is of storage pointer type and might be returned without assignment and could be used uninitialized. Assign the variable (potentially from itself) to fix this error.
+// TypeError: (182-193): This variable is of storage pointer type and can be returned without prior assignment.

test/libsolidity/syntaxTests/controlFlow/storageReturn/if_err.sol

@@ -14,5 +14,5 @@ contract C {
     }
 }
 // ----
-// TypeError: (96-107): This variable is of storage pointer type and might be returned without assignment and could be used uninitialized. Assign the variable (potentially from itself) to fix this error.
+// TypeError: (96-107): This variable is of storage pointer type and can be returned without prior assignment.
-// TypeError: (186-197): This variable is of storage pointer type and might be returned without assignment and could be used uninitialized. Assign the variable (potentially from itself) to fix this error.
+// TypeError: (186-197): This variable is of storage pointer type and can be returned without prior assignment.

test/libsolidity/syntaxTests/controlFlow/storageReturn/modifier_err.sol

@@ -18,5 +18,5 @@ contract C {
     }
 }
 // ----
-// TypeError: (249-258): This variable is of storage pointer type and might be returned without assignment and could be used uninitialized. Assign the variable (potentially from itself) to fix this error.
+// TypeError: (249-258): This variable is of storage pointer type and can be returned without prior assignment.
-// TypeError: (367-376): This variable is of storage pointer type and might be returned without assignment and could be used uninitialized. Assign the variable (potentially from itself) to fix this error.
+// TypeError: (367-376): This variable is of storage pointer type and can be returned without prior assignment.

test/libsolidity/syntaxTests/controlFlow/storageReturn/short_circuit_err.sol

@@ -13,6 +13,6 @@ contract C {
     }
 }
 // ----
-// TypeError: (87-98): This variable is of storage pointer type and might be returned without assignment and could be used uninitialized. Assign the variable (potentially from itself) to fix this error.
+// TypeError: (87-98): This variable is of storage pointer type and can be returned without prior assignment.
-// TypeError: (176-187): This variable is of storage pointer type and might be returned without assignment and could be used uninitialized. Assign the variable (potentially from itself) to fix this error.
+// TypeError: (176-187): This variable is of storage pointer type and can be returned without prior assignment.
-// TypeError: (264-275): This variable is of storage pointer type and might be returned without assignment and could be used uninitialized. Assign the variable (potentially from itself) to fix this error.
+// TypeError: (264-275): This variable is of storage pointer type and can be returned without prior assignment.

test/libsolidity/syntaxTests/controlFlow/storageReturn/ternary_err.sol

@@ -9,5 +9,5 @@ contract C {
     }
 }
 // ----
-// TypeError: (96-107): This variable is of storage pointer type and might be returned without assignment and could be used uninitialized. Assign the variable (potentially from itself) to fix this error.
+// TypeError: (96-107): This variable is of storage pointer type and can be returned without prior assignment.
-// TypeError: (200-211): This variable is of storage pointer type and might be returned without assignment and could be used uninitialized. Assign the variable (potentially from itself) to fix this error.
+// TypeError: (200-211): This variable is of storage pointer type and can be returned without prior assignment.

test/libsolidity/syntaxTests/controlFlow/storageReturn/while_err.sol

@@ -8,4 +8,4 @@ contract C {
     }
 }
 // ----
-// TypeError: (87-98): This variable is of storage pointer type and might be returned without assignment and could be used uninitialized. Assign the variable (potentially from itself) to fix this error.
+// TypeError: (87-98): This variable is of storage pointer type and can be returned without prior assignment.

test/libsolidity/syntaxTests/controlFlow/uninitializedAccess/always_revert.sol

@@ -0,0 +1,8 @@
+contract C {
+    function f() internal view returns(uint[] storage a)
+    {
+        uint b = a[0];
+        revert();
+        b;
+    }
+}

test/libsolidity/syntaxTests/controlFlow/uninitializedAccess/assembly.sol

@@ -0,0 +1,9 @@
+contract C {
+	uint[] r;
+    function f() internal view returns (uint[] storage s) {
+        assembly { pop(s_slot) }
+        s = r;
+    }
+}
+// ----
+// TypeError: (92-126): This variable is of storage pointer type and can be accessed without prior assignment.

test/libsolidity/syntaxTests/controlFlow/uninitializedAccess/functionType.sol

@@ -0,0 +1,8 @@
+contract C {
+    // Make sure function parameters and return values are not considered
+    // for uninitialized return detection in the control flow analysis.
+    function f(function(uint[] storage) internal returns (uint[] storage)) internal pure
+    returns (function(uint[] storage) internal returns (uint[] storage))
+    {
+    }
+}

test/libsolidity/syntaxTests/controlFlow/uninitializedAccess/modifier_order_fail.sol

@@ -0,0 +1,8 @@
+contract C {
+    modifier m1(uint[] storage a) { _; }
+    modifier m2(uint[] storage a) { _; }
+    uint[] s;
+    function f() m1(b) m2(b = s) internal view returns (uint[] storage b) {}
+}
+// ----
+// TypeError: (129-130): This variable is of storage pointer type and can be accessed without prior assignment.

test/libsolidity/syntaxTests/controlFlow/uninitializedAccess/modifier_order_fine.sol

@@ -0,0 +1,6 @@
+contract C {
+    modifier m1(uint[] storage a) { _; }
+    modifier m2(uint[] storage a) { _; }
+    uint[] s;
+    function f() m1(b = s) m2(b) internal view returns (uint[] storage b) {}
+}

test/libsolidity/syntaxTests/controlFlow/uninitializedAccess/modifier_post_access.sol

@@ -0,0 +1,13 @@
+contract C {
+    uint[] s;
+    modifier mod(uint[] storage b) {
+        _;
+        b[0] = 0;
+    }
+    function f() mod(a) internal returns (uint[] storage a)
+    {
+		a = s;
+    }
+}
+// ----
+// TypeError: (120-121): This variable is of storage pointer type and can be accessed without prior assignment.

test/libsolidity/syntaxTests/controlFlow/uninitializedAccess/modifier_pre_access.sol

@@ -0,0 +1,13 @@
+contract C {
+    uint[] s;
+    modifier mod(uint[] storage b) {
+        b[0] = 0;
+        _;
+    }
+    function f() mod(a) internal returns (uint[] storage a)
+    {
+		a = s;
+    }
+}
+// ----
+// TypeError: (120-121): This variable is of storage pointer type and can be accessed without prior assignment.

test/libsolidity/syntaxTests/controlFlow/uninitializedAccess/smoke.sol

@@ -0,0 +1,10 @@
+contract C {
+    uint[] s;
+    function f() internal returns (uint[] storage a)
+    {
+        a[0] = 0;
+        a = s;
+    }
+}
+// ----
+// TypeError: (94-95): This variable is of storage pointer type and can be accessed without prior assignment.

test/libsolidity/syntaxTests/controlFlow/uninitializedAccess/struct.sol

@@ -0,0 +1,11 @@
+contract C {
+    struct S { uint a; }
+    S s;
+    function f() internal returns (S storage r)
+    {
+        r.a = 0;
+        r = s;
+    }
+}
+// ----
+// TypeError: (109-110): This variable is of storage pointer type and can be accessed without prior assignment.

test/libsolidity/syntaxTests/controlFlow/uninitializedAccess/unreachable.sol

@@ -0,0 +1,10 @@
+contract C {
+    uint[] s;
+    function f() internal returns (uint[] storage a)
+    {
+        revert();
+        a[0] = 0;
+        a = s;
+    }
+}
+// ----

test/libsolidity/syntaxTests/dataLocations/libraries/library_function_with_data_location_fine.sol

@@ -8,3 +8,9 @@ library L {
     function i(uint[] calldata, uint[] storage) external pure returns (S storage x) {return x; }
 }
 // ----
+// TypeError: (197-198): This variable is of storage pointer type and can be accessed without prior assignment.
+// TypeError: (203-204): This variable is of storage pointer type and can be accessed without prior assignment.
+// TypeError: (359-360): This variable is of storage pointer type and can be accessed without prior assignment.
+// TypeError: (365-366): This variable is of storage pointer type and can be accessed without prior assignment.
+// TypeError: (460-461): This variable is of storage pointer type and can be accessed without prior assignment.
+// TypeError: (557-558): This variable is of storage pointer type and can be accessed without prior assignment.