From 4e27d778f0eb380b32c32fe12ad30acde4e32b06 Mon Sep 17 00:00:00 2001
From: Sam Vervaeck <samvv@pm.me>
Date: Sun, 21 May 2023 20:33:06 +0200
Subject: [PATCH] Add some comments and lookup type class let declaration with
 Scope::lookupDirect()

---
 include/bolt/CST.hpp | 16 ++++++++++++++++
 src/CST.cc           | 15 ++++++++++++---
 src/Checker.cc       | 30 +++++++++++++++++++++++++-----
 3 files changed, 53 insertions(+), 8 deletions(-)

diff --git a/include/bolt/CST.hpp b/include/bolt/CST.hpp
index 2ca134e8f..00ca31a8d 100644
--- a/include/bolt/CST.hpp
+++ b/include/bolt/CST.hpp
@@ -187,6 +187,22 @@ namespace bolt {
 
     Scope(Node* Source);
 
+    /**
+     * Performs a direct lookup in this scope for the given symbol.
+     *
+     * This method will never traverse to parent scopes and will always return a
+     * symbol that belongs to this scope, if any is found.
+     *
+     * \returns nullptr when no such symbol could be found in this scope.
+     */
+    Node* lookupDirect(SymbolPath Path, SymbolKind Kind = SymbolKind::Var);
+
+    /**
+     * Find the symbol with the given name, either in this scope or in any of
+     * the parent ones.
+     *
+     * \returns nullptr when no such symbol could be found in any of the scopes.
+     */
     Node* lookup(SymbolPath Path, SymbolKind Kind = SymbolKind::Var);
 
     Scope* getParentScope();
diff --git a/src/CST.cc b/src/CST.cc
index 962ee5ab4..e933c43bb 100644
--- a/src/CST.cc
+++ b/src/CST.cc
@@ -63,13 +63,22 @@ namespace bolt {
     }
   }
 
+  Node* Scope::lookupDirect(SymbolPath Path, SymbolKind Kind) {
+    ZEN_ASSERT(Path.Modules.empty());
+    auto Match = Mapping.find(Path.Name);
+    if (Match != Mapping.end() && std::get<1>(Match->second) == Kind) {
+      return std::get<0>(Match->second);
+    }
+    return nullptr;
+  }
+
   Node* Scope::lookup(SymbolPath Path, SymbolKind Kind) {
     ZEN_ASSERT(Path.Modules.empty());
     auto Curr = this;
     do {
-      auto Match = Curr->Mapping.find(Path.Name);
-      if (Match != Curr->Mapping.end() && std::get<1>(Match->second) == Kind) {
-        return std::get<0>(Match->second);
+      auto Found= Curr->lookupDirect(Path, Kind);
+      if (Found) {
+        return Found;
       }
       Curr = Curr->getParentScope();
     } while (Curr != nullptr);
diff --git a/src/Checker.cc b/src/Checker.cc
index 9a0b0c95a..212390b34 100644
--- a/src/Checker.cc
+++ b/src/Checker.cc
@@ -390,7 +390,11 @@ namespace bolt {
           }
         }
 
-        Type* Ty;
+        // Here we infer the primary type of the let declaration. If there's a
+        // type assert, that assert should be authoritative so we use that.
+        // Otherwise, the type is not further specified and we create a new
+        // unification variable.
+        Type *Ty;
         if (Let->TypeAssert) {
           Ty = inferTypeExpression(Let->TypeAssert->TypeExpression);
         } else {
@@ -398,21 +402,37 @@ namespace bolt {
         }
         Let->Ty = Ty;
 
+        // If declaring a let-declaration inside a type instance declaration,
+        // we need to perform some work to make sure the type asserts of the corresponding let-declaration in the type class declaration are accounted for.
         if (llvm::isa<InstanceDeclaration>(Let->Parent)) {
           auto Instance = static_cast<InstanceDeclaration*>(Let->Parent);
           auto Class = llvm::cast<ClassDeclaration>(Instance->getScope()->lookup({ {}, Instance->Name->getCanonicalText() }, SymbolKind::Class));
-          std::vector<TVar*> Params;
+
+          // The type asserts in the type class declaration might make use of
+          // the type parameters of the type class declaration, so it is
+          // important to make them available in the type environment. Moreover,
+          // we will be unifying them with the actual types declared in the
+          // instance declaration, so we keep track of them.
+          std::vector<TVar *> Params;
           for (auto TE: Class->TypeVars) {
             auto TV = createTypeVar();
             NewCtx->Env.emplace(TE->Name->getCanonicalText(), new Forall(TV));
             Params.push_back(TV);
           }
-          // FIXME lookup should not go over parent envs
-          auto Let2 = llvm::cast<LetDeclaration>(Class->getScope()->lookup({ {}, llvm::cast<BindPattern>(Let->Pattern)->Name->getCanonicalText() }, SymbolKind::Var));
+
+          auto Let2 = llvm::cast<LetDeclaration>(Class->getScope()->lookupDirect({ {}, llvm::cast<BindPattern>(Let->Pattern)->Name->getCanonicalText() }, SymbolKind::Var));
+
+          // It would be very strange if there was no type assert in the type
+          // class let-declaration but we rather not let the compiler crash if that happens.
           if (Let2->TypeAssert) {
             addConstraint(new CEqual(Ty, inferTypeExpression(Let2->TypeAssert->TypeExpression), Let));
           }
-          for (auto [Param, TE]: zen::zip(Params, Instance->TypeExps)) {
+
+          // Here we do the actual unification of e.g. Eq a with Eq Bool. The
+          // unification variables we created previously will be unified with
+          // e.g. Bool, which causes the type assert to also collapse to e.g.
+          // Bool -> Bool -> Bool.
+          for (auto [Param, TE] : zen::zip(Params, Instance->TypeExps)) {
             addConstraint(new CEqual(Param, TE->getType()));
           }
         }