Visiting type class bodies in experimental analysis #14578

cameel · 2023-09-28T14:33:27Z

~~Depends on #14566.~~

This adds "pass 3" as described in Experimental Type System Notes, which is responsible for collecting types of type class member functions.

I also moved builtin class registration to TypeClassRegistration, though builtin classes are going to be removed soon (we'll instead have specific class members marked as builtin).

cameel · 2023-09-28T14:34:28Z

libsolidity/experimental/analysis/TypeInference.cpp

@@ -216,8 +216,7 @@ bool TypeInference::visit(TypeClassDefinition const& _typeClassDefinition)
 		subNode->accept(*this);
 		auto const* functionDefinition = dynamic_cast<FunctionDefinition const*>(subNode.get());
 		solAssert(functionDefinition);
-		// TODO: need polymorphicInstance?
-		auto functionType = polymorphicInstance(typeAnnotation(*functionDefinition));
+		auto functionType = typeAnnotation(*functionDefinition);


I still need to confirm that removing this does not break things and makes sense.

cameel · 2023-09-28T14:38:41Z

libsolidity/experimental/analysis/TypeInference.cpp

@@ -628,8 +557,6 @@ bool TypeInference::visit(TypeClassInstantiation const& _typeClassInstantiation)
 		[&](ASTPointer<IdentifierPath> _typeClassName) -> std::optional<TypeClass> {
 			if (auto const* typeClassDefinition = dynamic_cast<TypeClassDefinition const*>(_typeClassName->annotation().referencedDeclaration))
 			{
-				// visiting the type class will re-visit this instantiation
-				typeClassDefinition->accept(*this);


I'm not 100% I can really remove this.

Initially I assumed that we're doing the separate pass partially to get rid of the necessity to do such a jump. But I see that visit(TypeClassDefinition) then recursively goes back to here and I wonder if this has other consequences in the type system. We do not have any tests that would reveal potential problems though so it's hard to tell.

Looks like it's still needed. Otherwise type inference breaks if the instantiation is visited before the class:

pragma experimental solidity; type word = __builtin("word"); type T = word; instantiation T: D { function f(self: T) -> word {} } class Self: D { function f(self: Self) -> word; }

TypeError 7428: Type mismatch for function f T -> word != 'x:type -> 'w:type

We don't need it, if we already construct the full type of the class functions in the previous pass - but yeah, that depends on already constructing types from type expressions, which we still need to disentangle from inference (see also https://github.com/ethereum/solidity/pull/14578/files#r1343054472)

cameel · 2023-09-28T14:52:55Z

libsolidity/experimental/analysis/TypeInference.cpp

+		// For type class members the type is assigned by TypeClassMemberRegistration.
+		// Fill in the `type` annotation to avoid `visit(FunctionDefinition)` giving it a fresh one.
+		Type functionType = functionTypes.at(functionDefinition->name());
+		solAssert(!annotation(*functionDefinition).type.has_value());
+		annotation(*functionDefinition).type = functionType;


This is a bit awkward and I'm starting to doubt if this is really what I was supposed to do. What I did here is to create the function type for every type class member in TypeClassMemberRegistration and then copy it into this annotation here during TypeInference.

I wonder if it would make more sense to have a separate pass that initializes type annotations with fresh variables instead so that other passes before type inference can refer to them.

I also expected to be able to move more of the stuff from visit(TypeClassDefinition) but it seems to be mostly tied to unification so most of it had to stay and I still have iterate over FunctionDefinitions.

The notes say this:

Pass 3: visit type class bodies, collecting function signatures (can involve defined types, so depends on 2!)

The thing is, we're not really collecting signatures - we don't know these until after inference - before it we only have two fresh variables, one for the whole tuple of arguments and one for a tuple of returns. And doing this does not require knowing anything about defined types since those are just two opaque type variables before inference. So I could just as well do this already in TypeClassRegistration.

I must be missing something because I don't really see why exactly this needs to be a separate pass.

How do the signatures of type class functions depend on type inference? (They don't...) But yeah, I'll need to look at the rest of the comments and the PR to see how to clarify things :-).

But I see the problem now. The signatures of type class functions don't depend on type inference, really - but what type inference also does is to construct types from expressions in type context. That we may need to move to a separate pass actually, since that's what you need to get function signatures before inference.

Can you clarify what exactly you understand as "function signatures"? Technically they don't depend on inference but before inference all you know is that it's some under-specified function type - its inputs and outputs are just fresh variables. I was assuming that signature is the fully inferred type.

Actually, initially I assumed that it's what stated in the declaration in the source file (however incomplete it may be), but this does not seem like collecting that would be of any use.

cameel · 2023-09-28T15:19:32Z

libsolidity/experimental/analysis/TypeClassMemberRegistration.cpp

+	defineConversion(BuiltinClass::Integer, PrimitiveType::Integer, "fromInteger");
+
+	defineBinaryMonoidalOperator(BuiltinClass::Mul, Token::Mul, "mul");
+	defineBinaryMonoidalOperator(BuiltinClass::Add, Token::Add, "add");
+
+	defineBinaryCompareOperator(BuiltinClass::Equal, Token::Equal, "eq");
+	defineBinaryCompareOperator(BuiltinClass::Less, Token::LessThan, "lt");
+	defineBinaryCompareOperator(BuiltinClass::LessOrEqual, Token::LessThanOrEqual, "leq");
+	defineBinaryCompareOperator(BuiltinClass::Greater, Token::GreaterThan, "gt");
+	defineBinaryCompareOperator(BuiltinClass::GreaterOrEqual, Token::GreaterThanOrEqual, "geq");


@matheusaaguiar The task for you now, on top of this PR, would be to remove the whole concept of BuiltinClass and instead make it possible to register these members using __builtin(). This is the remaining part of pass 3 from Daniel's note.

To do this, you'd remove this hard-coded initialization and only fill in the typeClassFunctions/operators annotations when visiting Builtin AST node. Of course only if the usage is correct - only one definition, only those predefined operators/conversions, etc. After this change we'll also no longer need the ability to use a token as type class name, so I'd remove that too (otherwise it will just be dead code).

You'll also need a small parser/AST modification to allow more than one argument in __builtin() (in #14566 I made it expect exactly one).

When you do this, the code that currently looks like this:

pragma experimental solidity; type word = __builtin("word"); type uint256 = word; instantiation uint256: + { function add(x: uint256, y: uint256) -> uint256 {} } function f(x: uint256, y: uint256) -> uint256 { return x + y; }

should look like this:

pragma experimental solidity; type word = __builtin("word"); type uint256 = word; __builtin("+", Add.add); class uint256: Add { function add(x: uint256, y: uint256) -> uint256; } instantiation uint256: Add { function add(x: uint256, y: uint256) -> uint256 {} } function f(x: uint256, y: uint256) -> uint256 { return x + y; }

Without __builtin() most of this should still work, but the x + y bit should report an error due to the operator + not being defined.

cameel · 2023-09-29T19:41:02Z

test/libsolidity/syntaxTests/experimental/inference/class_and_instantiation_order.sol

+instantiation T: D {
+    function f(self: T) -> word {}
+}
+
+class Self: D {
+    function f(self: Self) -> word;
+}


This is broken after my changes:

/solidity/test/soltest.cpp(120): error: in "syntaxTests/experimental/inference/infinite_recursion_two_functions": Exception during extracted test: /solidity/libsolidity/experimental/analysis/TypeInference.cpp(319): Throw in function virtual bool solidity::frontend::experimental::TypeInference::visit(const solidity::frontend::VariableDeclaration &) Dynamic exception type: boost::wrapexcept<solidity::langutil::InternalCompilerError> std::exception::what: Solidity assertion failed [solidity::util::tag_comment*] = Solidity assertion failed

Looks like I need a check against multiple visitation in TypeInference::visit(FunctionDefinition) after all. The problem is that I can no longer rely on the type annotation for that...

This shouldn't be an issue... this pass shouldn't look at instantiations at all, but only at the class definitions - but at all of those - so the state after this pass should be that all function definitions in class definitions have types from this pass, but none of the ones within intantiations (for types of those we need to rely on inference first) But yeah, not yet sure I understand the problem correctly :-).

The broken bit is in the type inference pass. I can solve it by adding some extra flag, but I don't like that too much and looks like we'll have more things moved to the extra pass so maybe this problem will just solve itself.

ekpyron · 2023-10-02T19:02:57Z

libsolidity/experimental/analysis/TypeInference.cpp

-
-	m_currentFunctionType = functionType;
-	functionAnnotation.type = functionType;
+	// For type class members the type annotation is filled by visit(TypeClassDefinition)


Just don't visit the bodies of type classes again here. (EDIT: that also only works if we actually fully determine the function types earlier)

cameel · 2023-10-04T15:18:44Z

Based on our discussions on the call and on the chat today, here's how I understand what needs to be done in this PR next:

Keep it as a separate pass.
Create a helper for a specific use case pattern of unification for typing function applications

Unification is pretty much the most basic operation on types there is - so we should have more helpers for specific patterns involving unification, in particular for the "function application" pattern...
- E.g. a unification helper that gives you the return type of a function given its generic type and concrete argument types. Currently that usually done ad-hoc inline.
- If there are any other uses for types, they could also be separate helpers needed only in the new pass.
- The if (!m_activeInstantiations.empty()) block in TypeInference::unify() should become unnecessary after the PR and can be removed.
Visit function definitions, including the whole parameter list and return type.
- The new pass should have a copy of the type annotation (maybe use a more specific name to keep things clear).
- Anything needed to fill the type annotation of a function definition inside a type class should be moved to the new pass. For unify() use the new helpers.
- Basically everything from TypeInference::visit(TypeClassDefinition) should be moved, including the unification.
- The unification should give us a complete type, with no type variables other than the class variable (validate that).
- The back and forth visitation between type classes and the instantiation is no longer necessary.
- Also keep the current logic of visiting non-class functions in type inference.
Move all the visitation in type and sort context to the new pass
- We don't really need the distinction between type and sort context. Do it inline and remove the context mechanism.

Exclude list for AST JSON tests Fix type inference shellcheck failure Disable shellcheck SC2086

…tration-pass Separate type class registration pass in experimental analysis

- Using the same name causes syntax ambiguities. It's also not allowed for user-defined classes and types.

github-actions · 2024-01-02T12:04:30Z