Merge pull request #3722 from ethereum/fixMods

Fix modifier lookup in libraries.

6 files changed, 83 insertions, 5 deletions

diff --git a/Changelog.md b/Changelog.md
index a0ad6242..1b1d86b1 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -4,6 +4,7 @@ Features:
 
 Bugfixes:
  * Code Generator: Properly skip unneeded storgae array cleanup when not reducing length.
+ * Code Generator: Bugfix in modifier lookup in libraries.
  * Commandline interface: Support ``--evm-version constantinople`` properly.
  * Standard JSON: Support ``constantinople`` as ``evmVersion`` properly.
  * DocString Parser: Fix error message for empty descriptions.
diff --git a/libsolidity/codegen/CompilerContext.cpp b/libsolidity/codegen/CompilerContext.cpp
index 0bf88267..47333046 100644
--- a/libsolidity/codegen/CompilerContext.cpp
+++ b/libsolidity/codegen/CompilerContext.cpp
@@ -193,14 +193,22 @@ Declaration const* CompilerContext::nextFunctionToCompile() const
 	return m_functionCompilationQueue.nextFunctionToCompile();
 }
 
-ModifierDefinition const& CompilerContext::functionModifier(string const& _name) const
+ModifierDefinition const& CompilerContext::resolveVirtualFunctionModifier(
+	ModifierDefinition const& _modifier
+) const
 {
+	// Libraries do not allow inheritance and their functions can be inlined, so we should not
+	// search the inheritance hierarchy (which will be the wrong one in case the function
+	// is inlined).
+	if (auto scope = dynamic_cast<ContractDefinition const*>(_modifier.scope()))
+		if (scope->isLibrary())
+			return _modifier;
 	solAssert(!m_inheritanceHierarchy.empty(), "No inheritance hierarchy set.");
 	for (ContractDefinition const* contract: m_inheritanceHierarchy)
 		for (ModifierDefinition const* modifier: contract->functionModifiers())
-			if (modifier->name() == _name)
+			if (modifier->name() == _modifier.name())
 				return *modifier;
-	solAssert(false, "Function modifier " + _name + " not found.");
+	solAssert(false, "Function modifier " + _modifier.name() + " not found in inheritance hierarchy.");
 }
 
 unsigned CompilerContext::baseStackOffsetOfVariable(Declaration const& _declaration) const
diff --git a/libsolidity/codegen/CompilerContext.h b/libsolidity/codegen/CompilerContext.h
index cf626683..7b663277 100644
--- a/libsolidity/codegen/CompilerContext.h
+++ b/libsolidity/codegen/CompilerContext.h
@@ -130,7 +130,7 @@ public:
 	void appendMissingLowLevelFunctions();
 	ABIFunctions& abiFunctions() { return m_abiFunctions; }
 
-	ModifierDefinition const& functionModifier(std::string const& _name) const;
+	ModifierDefinition const& resolveVirtualFunctionModifier(ModifierDefinition const& _modifier) const;
 	/// Returns the distance of the given local variable from the bottom of the stack (of the current function).
 	unsigned baseStackOffsetOfVariable(Declaration const& _declaration) const;
 	/// If supplied by a value returned by @ref baseStackOffsetOfVariable(variable), returns
diff --git a/libsolidity/codegen/ContractCompiler.cpp b/libsolidity/codegen/ContractCompiler.cpp
index 5a9498f0..95d6c8b5 100644
--- a/libsolidity/codegen/ContractCompiler.cpp
+++ b/libsolidity/codegen/ContractCompiler.cpp
@@ -1002,7 +1002,10 @@ void ContractCompiler::appendModifierOrFunctionCode()
 			appendModifierOrFunctionCode();
 		else
 		{
-			ModifierDefinition const& modifier = m_context.functionModifier(modifierInvocation->name()->name());
+			ModifierDefinition const& nonVirtualModifier = dynamic_cast<ModifierDefinition const&>(
+				*modifierInvocation->name()->annotation().referencedDeclaration
+			);
+			ModifierDefinition const& modifier = m_context.resolveVirtualFunctionModifier(nonVirtualModifier);
 			CompilerContext::LocationSetter locationSetter(m_context, modifier);
 			solAssert(modifier.parameters().size() == modifierInvocation->arguments().size(), "");
 			for (unsigned i = 0; i < modifier.parameters().size(); ++i)
diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp
index 33cd1419..282136f7 100644
--- a/test/libsolidity/SolidityEndToEndTest.cpp
+++ b/test/libsolidity/SolidityEndToEndTest.cpp
@@ -2879,6 +2879,58 @@ BOOST_AUTO_TEST_CASE(function_modifier_multiple_times_local_vars)
 	ABI_CHECK(callContractFunction("a()"), encodeArgs(0));
 }
 
+BOOST_AUTO_TEST_CASE(function_modifier_library)
+{
+	char const* sourceCode = R"(
+		library L {
+			struct S { uint v; }
+			modifier mod(S storage s) { s.v++; _; }
+			function libFun(S storage s) mod(s) internal { s.v += 0x100; }
+		}
+
+		contract Test {
+			using L for *;
+			L.S s;
+
+			function f() public returns (uint) {
+				s.libFun();
+				L.libFun(s);
+				return s.v;
+			}
+		}
+	)";
+	compileAndRun(sourceCode);
+	ABI_CHECK(callContractFunction("f()"), encodeArgs(0x202));
+}
+
+BOOST_AUTO_TEST_CASE(function_modifier_library_inheritance)
+{
+	// Tests that virtual lookup for modifiers in libraries does not consider
+	// the current inheritance hierarchy.
+
+	char const* sourceCode = R"(
+		library L {
+			struct S { uint v; }
+			modifier mod(S storage s) { s.v++; _; }
+			function libFun(S storage s) mod(s) internal { s.v += 0x100; }
+		}
+
+		contract Test {
+			using L for *;
+			L.S s;
+			modifier mod(L.S storage) { revert(); _; }
+
+			function f() public returns (uint) {
+				s.libFun();
+				L.libFun(s);
+				return s.v;
+			}
+		}
+	)";
+	compileAndRun(sourceCode);
+	ABI_CHECK(callContractFunction("f()"), encodeArgs(0x202));
+}
+
 BOOST_AUTO_TEST_CASE(crazy_elementary_typenames_on_stack)
 {
 	char const* sourceCode = R"(
diff --git a/test/libsolidity/syntaxTests/virtualLookup/modifiers_in_libraries.sol b/test/libsolidity/syntaxTests/virtualLookup/modifiers_in_libraries.sol
new file mode 100644
index 00000000..b033fd0c
--- /dev/null
+++ b/test/libsolidity/syntaxTests/virtualLookup/modifiers_in_libraries.sol
@@ -0,0 +1,14 @@
+library WithModifier {
+    modifier mod() { require(msg.value > 10 ether); _; }
+    function withMod(uint self) mod() internal view { require(self > 0); }
+}
+
+contract Test {
+    using WithModifier for *;
+
+    function f(uint _value) public payable {
+        _value.withMod();
+        WithModifier.withMod(_value);
+    }
+}
+// ----