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

ekpyron · 2020-01-07T13:29:17Z

This is probably easiest to review commit by commit.
I first remove the defaulted _isInternal bool used to construct function types and replace it by an explicit kind argument, while reproducing the exact same behaviour as before.

Then I add the new Definition Kind and make it the default instead. So far if function types were needed without an actual calling context, the kind was "Internal" - now it is "Definition", which I'd say makes more sense.

Then finally I added all externally visible functions with kind "definition" as members to the contract TypeType and added appropriate assertions and type checks. Those members themselves for now can only be used for selector lookup with their "selector" members.

This partly fixes #3506 and is related to #7987.
What is missing is proper handling of base contracts, which is what #7987 tried to do, but which will be easier on top of this (as discussed with @Marenz).

ekpyron · 2020-01-07T13:31:47Z

Should have made this a draft... there's a few questions remaining, e.g. how to name those new kinds of function types - and generally, I need feedback about whether this seems like a reasonable direction to go.

leonardoalt · 2020-01-07T14:56:48Z

libsolidity/analysis/ContractLevelChecker.cpp

@@ -40,10 +40,16 @@ namespace
 template <class T, class B>
 bool hasEqualNameAndParameters(T const& _a, B const& _b)
 {
+	auto makeFunctionType = [](auto const& _decl) {
+		if constexpr (std::is_same_v<std::decay_t<decltype(_decl)>, FunctionDefinition>)


It's temporary :-).

As in gone again in a subsequent commit.

libsolidity/ast/AST.cpp

leonardoalt · 2020-01-07T15:04:22Z

libsolidity/ast/Types.cpp

+	string name = "function ";
+	if (m_kind == Kind::Definition)
+	{
+		// TODO: is this a reasonable way to compute names for these?


libsolidity/ast/Types.h

libsolidity/codegen/ExpressionCompiler.cpp

...ity/syntaxTests/types/function_types/definition/assign_function_via_contract_name_to_var.sol

leonardoalt · 2020-01-07T15:19:04Z

@ekpyron did one pass, looks good at a first glance. There are a lot of changes though, I would like to look at it again.
Does it work to call A.f() is it is pure? Can you add tests for those?

ekpyron · 2020-01-07T15:23:24Z

Nope, calling A.f() for pure functions is something we could consider implementing in the future, but I don't plan to include it for this PR. It would only work, if we actually have an implementation for A.f and can inline, though... in any case: I'd consider that out of scope for this PR.

However, this is the main reason, why I think it's good to make the type of these things actually a FunctionType at all, instead of defining an entirely new kind of type for them :-).

ekpyron · 2020-01-07T15:24:42Z

(So calling A.f() isn't allowed in general and there's a test for that for a non-pure f, but pureness doesn't make a difference for now - I can explicitly add a copy of that test with A.f being pure, though)

leonardoalt · 2020-01-07T15:25:21Z

Yes that's what I meant

ekpyron · 2020-01-07T15:27:51Z

But yeah, I'm still not entirely sure whether distinguishing these things using a new FunctionType::Kind is the best way to go... we could also have a boolean flag instead... but the default of having a lot of stuff having m_kind being Internal, e.g. for a type solely used to compute an external signature, didn't seem very clean either...

ekpyron · 2020-01-07T16:00:17Z

Added some explicit tests for pure functions.

libsolidity/ast/TypeProvider.h

libsolidity/ast/Types.cpp

libsolidity/ast/Types.h

chriseth · 2020-01-09T14:14:30Z

libsolidity/codegen/ExpressionCompiler.cpp

 	if (
-		_memberAccess.expression().annotation().type->category() == Type::Category::Function &&
-		member == "selector"
+		auto const* functionType = dynamic_cast<FunctionType const*>(_memberAccess.expression().annotation().type);


I'm still not used to this. Maybe we could change the style such that the ; has to be on a line of its own in these cases?

Yeah, I hesitated a bit with this :-). Mainly did it here to avoid blowing up the diff with indentation-only changes below... which is probably not the best reason...

In general I like it, though - if it were up to me alone, I'd e.g. also merge the one a few lines down to

if ( auto const* exprInt = dynamic_cast<Identifier const*>(&expr->expression())); exprInt && exprInt->name() == "this" )

Less nesting always seems nicer to me... And less nesting in ifs is likely to avoid brackets arising from otherwise ambiguous elses... but yeah, I do see the point about potentially forgetting the explicit null check, so not sure...

Not sure I'd still like it, if I had to use a whole line for the ;, though :-)... Would that really help other than it probably being looked at twice, because it looks weirder :-)?

I can nest here for now, if you prefer and until we decided how to deal with this option in general...

It's fine! I would just like to have this very different kind of if statement stand out a bit more. The fact that you have to read it differently is only apparent towards the end. Would have preferred something like

if <statement> (condition) { ... }

chriseth

Ready for squsahing!

…atures via contract name.

ekpyron · 2020-01-09T14:41:31Z

Squashed it!

ekpyron force-pushed the functionTypeRefactor branch from 6698510 to dfccce2 Compare January 7, 2020 13:30

ekpyron force-pushed the functionTypeRefactor branch from dfccce2 to ac60395 Compare January 7, 2020 13:33

ethereum deleted a comment from stackenbotten Jan 7, 2020

ekpyron force-pushed the functionTypeRefactor branch 2 times, most recently from 0659cb8 to 0925bd4 Compare January 7, 2020 14:11

ekpyron requested a review from chriseth January 7, 2020 14:25

leonardoalt reviewed Jan 7, 2020

View reviewed changes

libsolidity/ast/AST.cpp Show resolved Hide resolved

leonardoalt reviewed Jan 7, 2020

View reviewed changes

libsolidity/ast/Types.h Outdated Show resolved Hide resolved

leonardoalt reviewed Jan 7, 2020

View reviewed changes

libsolidity/codegen/ExpressionCompiler.cpp Outdated Show resolved Hide resolved

leonardoalt reviewed Jan 7, 2020

View reviewed changes

...ity/syntaxTests/types/function_types/definition/assign_function_via_contract_name_to_var.sol Show resolved Hide resolved

ekpyron force-pushed the functionTypeRefactor branch 2 times, most recently from ad66225 to 53ddfa0 Compare January 7, 2020 15:55

ekpyron force-pushed the functionTypeRefactor branch 3 times, most recently from 8c23004 to 79b1903 Compare January 8, 2020 12:21

leonardoalt previously approved these changes Jan 9, 2020

View reviewed changes