From d1b9bd20e6df349d5d26c8a4671fb71519ee0cde Mon Sep 17 00:00:00 2001
From: Robin Sommer <robin@corelight.com>
Date: Wed, 10 Aug 2022 14:26:30 +0200
Subject: [PATCH] Add coercion on assignment for optionals that only differ in
 constness of their inner types.

Also adding a coercion for ternary expressions that coerces the 2dn
expression into the type of the 1st (which we already assume provides
the result type).

Closes #1143.
Closes #1220.
---
 .../include/ast/expressions/ternary.h          |  2 ++
 hilti/toolchain/src/compiler/coercion.cc       | 10 ++++++++++
 .../toolchain/src/compiler/visitors/coercer.cc | 13 +++++++++++++
 .../src/compiler/visitors/validator.cc         |  3 ++-
 tests/spicy/types/optional/coercion.spicy      | 18 ++++++++++++++++++
 5 files changed, 45 insertions(+), 1 deletion(-)
 create mode 100644 tests/spicy/types/optional/coercion.spicy

diff --git a/hilti/toolchain/include/ast/expressions/ternary.h b/hilti/toolchain/include/ast/expressions/ternary.h
index 7077a1b69..a9a736eab 100644
--- a/hilti/toolchain/include/ast/expressions/ternary.h
+++ b/hilti/toolchain/include/ast/expressions/ternary.h
@@ -22,6 +22,8 @@ class Ternary : public NodeBase, public trait::isExpression {
         return condition() == other.condition() && true_() == other.true_() && false_() == other.false_();
     }
 
+    void setFalse(const Expression& expr) { children()[2] = expr; }
+
     /** Implements `Expression` interface. */
     bool isLhs() const { return false; }
     /** Implements `Expression` interface. */
diff --git a/hilti/toolchain/src/compiler/coercion.cc b/hilti/toolchain/src/compiler/coercion.cc
index 47b7ab80f..5efed874b 100644
--- a/hilti/toolchain/src/compiler/coercion.cc
+++ b/hilti/toolchain/src/compiler/coercion.cc
@@ -424,6 +424,16 @@ struct VisitorType : public visitor::PreOrder<std::optional<Type>, VisitorType>
     }
 
     result_t operator()(const type::Optional& r) {
+        if ( auto t = dst.tryAs<type::Optional>() ) {
+            const auto& s = r.dereferencedType();
+            const auto& d = t->dereferencedType();
+
+            if ( type::sameExceptForConstness(s, d) && (style & CoercionStyle::Assignment) )
+                // Assignments copy, so it's safe to turn  into the
+                // destination without considering constness.
+                return dst;
+        }
+
         if ( auto t = dst.tryAs<type::Bool>(); (style & CoercionStyle::ContextualConversion) && t )
             return dst;
 
diff --git a/hilti/toolchain/src/compiler/visitors/coercer.cc b/hilti/toolchain/src/compiler/visitors/coercer.cc
index cd4aaef52..b7fa0090d 100644
--- a/hilti/toolchain/src/compiler/visitors/coercer.cc
+++ b/hilti/toolchain/src/compiler/visitors/coercer.cc
@@ -308,6 +308,19 @@ struct Visitor : public visitor::PreOrder<void, Visitor> {
         }
     }
 
+    void operator()(const expression::Ternary& n, position_t p) {
+        if ( ! (type::isResolved(n.true_().type()) && type::isResolved(n.false_().type())) )
+            return;
+
+        // Coerce the second branch to the type of the first. This isn't quite
+        // ideal, but as good as we can do right now.
+        if ( auto coerced = coerceExpression(n.false_(), n.true_().type()); coerced && coerced.nexpr ) {
+            logChange(p.node, *coerced.nexpr, "ternary");
+            p.node.as<expression::Ternary>().setFalse(*coerced.nexpr);
+            modified = true;
+        }
+    }
+
     void operator()(const operator_::generic::New& n, position_t p) {
         auto etype = n.op0().tryAs<expression::Type_>();
         if ( ! etype )
diff --git a/hilti/toolchain/src/compiler/visitors/validator.cc b/hilti/toolchain/src/compiler/visitors/validator.cc
index 1d2e528b7..dbed41bb5 100644
--- a/hilti/toolchain/src/compiler/visitors/validator.cc
+++ b/hilti/toolchain/src/compiler/visitors/validator.cc
@@ -276,10 +276,11 @@ struct VisitorPost : public hilti::visitor::PreOrder<void, VisitorPost>, public
     }
 
     void operator()(const expression::Ternary& n, position_t p) {
-        if ( ! hilti::type::sameExceptForConstness(n.true_().type(), n.false_().type()) )
+        if ( ! hilti::type::sameExceptForConstness(n.true_().type(), n.false_().type()) ) {
             error(fmt("types of alternatives do not match in ternary expression (%s vs. %s)", n.true_().type(),
                       n.false_().type()),
                   p);
+        }
     }
 
     void operator()(const expression::UnresolvedID& n, position_t p) {
diff --git a/tests/spicy/types/optional/coercion.spicy b/tests/spicy/types/optional/coercion.spicy
new file mode 100644
index 000000000..668082dab
--- /dev/null
+++ b/tests/spicy/types/optional/coercion.spicy
@@ -0,0 +1,18 @@
+# @TEST-EXEC: ${SPICYC} -j %INPUT >output
+#
+# @TEST-DOC: Regression test for #1143.
+
+module Foo;
+
+type Bar = struct {
+  wutang: bytes &optional;
+};
+
+type BarTuple = tuple<
+  wutang: optional<bytes>
+>;
+
+public function make_bar_tuple(bar: Bar): BarTuple {
+  local nope: optional<bytes>;
+  return tuple(bar?.wutang ? optional(bar.wutang) : nope);
+}