Fix ICE when calling unimplemented base/super function #7987

Marenz · 2019-12-12T14:09:18Z

I need to use the resolveDirectBaseContracts function from the override checker. I am currently not sure what the best way to do that is. I could pull it out into a different or extra file.. I could move my check into the override checker.. or something else?

Marenz · 2019-12-12T14:20:21Z

I see this is in the 0.6.0 project.. but it doesn't need to be, nothing here is breaking. So if this is blocking feel free to move it to 0.6.1

chriseth · 2019-12-12T14:20:27Z

libsolidity/analysis/TypeChecker.cpp

+		{
+			if (
+				!funcDef->isImplemented() &&
+				contains(resolveDirectBaseContracts(*m_scope), funcDef->annotation().contract) &&


Why is it important whether it is a direct base or an indirect base?

that's a good point. I had a wrong assumption here.

chriseth · 2019-12-12T14:23:45Z

test/libsolidity/syntaxTests/unimplemented_super_function.sol

 }
 // ----
-// TypeError: (110-117): Member "f" not found or not visible after argument-dependent lookup in contract super b.
+// TypeError: (110-119): Call to unimplemented function "f".


I don't think this should be an error in general. At this point, we cannot tell whether there is a super function or not.

Really? I mean, the check is very specifically checking for this situation? Can you think of a scenario where this message is wrong?

super performs virtual function lookup. This means the lookup is performed differently depending on the final derived contract that is being compiled. In the following example, super.f() should compile for D.

contract A { function f() public pure virtual; } contract B is A { function f() public pure override virtual { super.f(); } } contract C is A { function f() public pure override virtual { } } contract D is B, C { function f() public pure virtual override(B, C) { super.f(); } }

I think we need to discuss what to do if B is to be compiled on its own. I think @ekpyron is of the opinion to remove super altogether.

what does the super in contract B refer to then? (that's where it would throw an error with this PR)

I suppose I can either exclude super from the check, or I set a tag in the annotation and only trigger errors when the final contract was processed... though that could get messy

Are you sure about your example? I think this doesn't even work with the current develop version

Please only fix the non-super case. I said that super.f() should compile. It is a bug that it currently does not compile, but it is a different bug that #7314

chriseth · 2019-12-12T14:24:12Z

test/libsolidity/syntaxTests/unimplemented_super_function_derived.sol

 }
 // ----
-// TypeError: (118-125): Member "f" not found or not visible after argument-dependent lookup in contract super b.
+// TypeError: (118-127): Call to unimplemented function "f".


chriseth · 2019-12-12T14:24:46Z

Please do not change the behaviour of super, I think the two should be considered independent of each other.

Marenz · 2019-12-12T14:28:17Z

I was doing that in response to

The way we solved it with super in #5130 is sub-optimal because it just removes non-implemented functions from the member list. Instead, a specific error should be thrown.

Marenz · 2019-12-12T16:03:26Z

updated to exclude super

chriseth · 2019-12-12T16:08:36Z

libsolidity/analysis/TypeChecker.cpp

+		{
+			if (
+				!funcDef->isImplemented() &&
+				contains(m_scope->annotation().linearizedBaseContracts, funcDef->annotation().contract) &&


I don't think this is how we should do it. It does not catch things like

function() internal pure x = a.t; x();

Marenz · 2019-12-16T16:47:57Z

Updated. The check happens now as soon as we access the member.
I am not too happy with the deep if nesting. I could make it more linear by putting it in an extra function. Let me know if you prefer that.

bshastry

Some minor comments.

bshastry · 2019-12-16T16:55:08Z

Changelog.md



+Bugfixes:
+	* Fix compiler error when calling the base function from the derived


White space instead of tab?

bshastry · 2019-12-16T16:56:04Z

libsolidity/analysis/TypeChecker.cpp

+	struct SetScopeFunc
+	{
+		SetScopeFunc(TypeChecker& _checker, FunctionDefinition const* _func)
+			:m_checker( _checker)


Suggested change

:m_checker( _checker)

: m_checker(_checker)

bshastry · 2019-12-16T16:58:20Z

test/libsolidity/syntaxTests/unimplemented_base_function.sol

+    function f() public override { a.f(); }
+}
+// ----
+// TypeError: (110-113): Referencing unimplemented function "f".


Referencing unimplemented function "a.f" would be a nicer error message though.

bshastry · 2019-12-16T17:16:44Z

This (valid?) test case throws a different ICE? Perhaps unrelated but documenting it here in any case.

abstract contract a {
    function f() virtual public;
}
contract b is a {
    function f() public override {}
    function g() public { a.f(); }
}

throws this

https://github.com/ethereum/solidity/blob/7711631153a43b1290a2b98db73fb972b295dfd7/libsolidity/codegen/ContractCompiler.cpp#L1253

bshastry · 2019-12-16T17:17:55Z

libsolidity/analysis/TypeChecker.cpp

+								if (
+									!funcDef->isImplemented() &&
+									contains(m_scope->annotation().linearizedBaseContracts, funcDef->annotation().contract) &&
+									m_scopeFunction->name() == funcDef->name() &&


Is the assumption that the names are the same valid? Please see the test input that I added

bshastry

One more related comment.

bshastry · 2019-12-16T17:24:06Z

libsolidity/analysis/TypeChecker.cpp

+								)
+									m_errorReporter.typeError(
+										_memberAccess.location(),
+										"Referencing unimplemented function \"" + funcDef->name() + "\"."


Suggested change

"Referencing unimplemented function \"" + funcDef->name() + "\"."

"Referencing unimplemented function \"" + funcDef->name() + "\" in contract \"" + tt->actualType()->canonicalName() + "\"."

If you accept this change, test expectations need to be updated.

fixes #7314

Marenz · 2019-12-16T17:42:17Z

Good finds! Not sure about that extra test case you found. We certainly shouldn't get an ICE, but I want to hear what @chriseth has to say before changing my code

chriseth · 2019-12-16T19:36:59Z

libsolidity/analysis/TypeChecker.cpp

+	{
 		if (tt->actualType()->category() == Type::Category::Enum)
 			annotation.isPure = true;
+		else if (tt->actualType()->category() == Type::Category::Contract)


This again does not catch function() x = A.f. I think that information has to be added to the type. We should discuss if we want that.

Ah, sorry, that's wrong - there is a test about exactly that :)

chriseth · 2019-12-16T19:39:41Z

libsolidity/analysis/TypeChecker.h


 	ContractDefinition const* m_scope = nullptr;

+	/// Pointer to the function we're currently in


Why is it important that we are currently inside that function?

I think this stems from a misunderstanding from my side. The issue description says

This is a rather specific case - you have to call the non-implemented base function and override the function at the same time.

I thought this means it only happens when you call it from within the function that overrides which is what I am detecting here.

@bshastry newly found test case shows that the problem is more general than this.

chriseth · 2019-12-16T19:40:32Z

I think we have to discuss that at a more basic level. Do we want A.f.signature to work? Do we want A.f itself to generate an error? Or do we want A.f to result in a function type that is not callable?

ekpyron · 2020-01-06T11:42:58Z

I would say A.f.signature should work and thereby merely accessing A.f alone is fine, but A.f is only callable or convertible to an actual function pointer, if A.f is implemented, so both A.f(); or some_function_pointer = A.f; are errors.

ekpyron · 2020-01-06T12:51:32Z

Part of my reasoning would be that A.f.signature should even work in general, i.e. in a contract that does not even inherit from A. So stuff like this should work:

interface I {
  function f() external;
}
contract C {
  function g() public returns (bytes4) { return I.f.selector; }
}

And for that we'd need to have a specific type for I.f anyways... which is a special kind of function type that is not callable and not convertible to function pointers... and that's the very same type I'd use for unimplemented base functions as well.

ekpyron · 2020-01-06T14:42:19Z

Ah, yes: for reference: access to selectors via contract names in general is tracked as #3506 and staged for 0.6.2 as well.

ekpyron · 2020-01-09T17:38:16Z

@Marenz We now merged #8105 , which introduces a new kind of FunctionType::Kind::Declaration which can be used for a nicer solution to this - there will still be some subtleties involved, though - if you reference unimplemented public or external base functions by the name of the base, they will just get a function type of kind FunctionType::Kind::Declaration. Probably internal ones as well, but they won't get the selector native member. For implemented public base functions we have to check, if it's nicer to keep them as internal function type kinds, or whether we let the declaration kind be callable in this case (I suspect the former will be less work).

So: do you want to continue with this based on that, or should I take over this PR as well?

ekpyron · 2020-01-10T10:01:05Z

As discussed with @Marenz in chat: I'm taking this over now.

ekpyron · 2020-01-15T17:51:05Z

Closing this in favour of #7987

chriseth reviewed Dec 12, 2019

View reviewed changes

Marenz force-pushed the fix-bug-7314 branch from a7b2765 to 4051d4e Compare December 12, 2019 15:39

chriseth reviewed Dec 12, 2019

View reviewed changes

Fix wrong comment

468734c

Marenz force-pushed the fix-bug-7314 branch from 4051d4e to 7711631 Compare December 16, 2019 16:44

Marenz requested a review from chriseth December 16, 2019 16:48

bshastry reviewed Dec 16, 2019

View reviewed changes

Fix ICE when calling unimplemented base/super function

a44cf26

fixes #7314

Marenz force-pushed the fix-bug-7314 branch from 7711631 to a44cf26 Compare December 16, 2019 17:41

chriseth reviewed Dec 16, 2019

View reviewed changes

Marenz self-assigned this Jan 2, 2020

ekpyron mentioned this pull request Jan 7, 2020

Add a new Function Type referring to FunctionDefinition's without calling context and use it to allow selector lookup. #8105

Merged

ekpyron unassigned Marenz Jan 10, 2020

ekpyron self-assigned this Jan 10, 2020

ekpyron mentioned this pull request Jan 14, 2020

Clean up visibility via contract name and unimplemented base functions. #8137

Merged

ekpyron closed this Jan 15, 2020

axic deleted the fix-bug-7314 branch April 5, 2020 23:36



		Bugfixes:
		* Fix compiler error when calling the base function from the derived

	"Referencing unimplemented function \"" + funcDef->name() + "\"."
	"Referencing unimplemented function \"" + funcDef->name() + "\" in contract \"" + tt->actualType()->canonicalName() + "\"."


		ContractDefinition const* m_scope = nullptr;

		/// Pointer to the function we're currently in

Fix ICE when calling unimplemented base/super function #7987

Fix ICE when calling unimplemented base/super function #7987

Uh oh!

Conversation

Marenz commented Dec 12, 2019

Uh oh!

Marenz commented Dec 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth commented Dec 12, 2019

Uh oh!

Marenz commented Dec 12, 2019

Uh oh!

Marenz commented Dec 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marenz commented Dec 16, 2019

Uh oh!

bshastry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bshastry Dec 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bshastry commented Dec 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bshastry left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marenz commented Dec 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth commented Dec 16, 2019

Uh oh!

ekpyron commented Jan 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ekpyron commented Jan 6, 2020

Uh oh!

ekpyron commented Jan 6, 2020

Uh oh!

ekpyron commented Jan 9, 2020

Uh oh!

bshastry Dec 16, 2019 •

edited

Loading

bshastry left a comment •

edited

Loading

ekpyron commented Jan 6, 2020 •

edited

Loading