Merge remote-tracking branch 'origin/topic/robin/gh-1759-switch-if'

* origin/topic/robin/gh-1759-switch-if:
  Fix `if`-condition with `switch` parsing.
  Fix clang-tidy.

.clang-tidy

@@ -56,7 +56,7 @@ Checks: '
   -readability-container-size-empty,
   -readability-convert-member-functions-to-static,
   -readability-else-after-return,
-  -readability-function-cognitive-complexity
+  -readability-function-cognitive-complexity,
   -readability-identifier-length,
   -readability-identifier-naming
   -readability-implicit-bool-conversion,

CHANGES

@@ -1,3 +1,15 @@
+1.11.0-dev.200 | 2024-06-14 10:56:04 +0200
+
+  * GH-1759: Fix `if`-condition with `switch` parsing. (Robin Sommer, Corelight)
+
+    The parser generator was ignoring `if` conditions attached to `switch`
+    constructs. While we actually had a test for this already, turns out
+    we had recorded a broken baseline. Plus, we were testing only one
+    variant of `switch` (expression-based, not look-ahead-based). This
+    implements and tests both variants now.
+
+  * Fix clang-tidy. (Robin Sommer, Corelight)
+
 1.11.0-dev.197 | 2024-06-13 12:43:33 +0200
 
   * GH-1750: Add `to_real` method to `bytes`. (Robin Sommer, Corelight)

VERSION

@@ -1 +1 @@
-1.11.0-dev.197
+1.11.0-dev.200

spicy/toolchain/include/compiler/detail/codegen/productions/look-ahead.h

@@ -3,7 +3,6 @@
 #pragma once
 
 #include <memory>
-#include <set>
 #include <string>
 #include <utility>
 #include <vector>
@@ -24,12 +23,16 @@ enum class Default { First, Second, None };
 class LookAhead : public Production {
 public:
     LookAhead(ASTContext* /* ctx */, const std::string& symbol, std::unique_ptr<Production> alt1,
-              std::unique_ptr<Production> alt2, look_ahead::Default def, const Location& l = location::None)
-        : Production(symbol, l), _alternatives(std::make_pair(std::move(alt1), std::move(alt2))), _default(def) {}
+              std::unique_ptr<Production> alt2, look_ahead::Default def, Expression* condition,
+              const Location& l = location::None)
+        : Production(symbol, l),
+          _alternatives(std::make_pair(std::move(alt1), std::move(alt2))),
+          _default(def),
+          _condition(condition) {}
 
     LookAhead(ASTContext* ctx, const std::string& symbol, std::unique_ptr<Production> alt1,
-              std::unique_ptr<Production> alt2, const Location& l = location::None)
-        : LookAhead(ctx, symbol, std::move(alt1), std::move(alt2), look_ahead::Default::None, l) {}
+              std::unique_ptr<Production> alt2, Expression* condition, const Location& l = location::None)
+        : LookAhead(ctx, symbol, std::move(alt1), std::move(alt2), look_ahead::Default::None, condition, l) {}
 
     /** Returns the two alternatives. */
     std::pair<Production*, Production*> alternatives() const {
@@ -39,6 +42,9 @@ class LookAhead : public Production {
     /** Returns what's the default alternative. */
     const auto& default_() const { return _default; }
 
+    /** Returns the boolean condition associated with the production, if any. */
+    auto condition() const { return _condition; }
+
     bool isAtomic() const final { return false; };
     bool isEodOk() const final { return isNullable(); };
     bool isLiteral() const final { return false; };
@@ -70,6 +76,7 @@ class LookAhead : public Production {
     std::pair<std::unique_ptr<Production>, std::unique_ptr<Production>> _alternatives;
 
     look_ahead::Default _default;
+    Expression* _condition = nullptr;
 
     std::pair<production::Set, production::Set> _lahs;
 };

spicy/toolchain/include/compiler/detail/codegen/productions/switch.h

@@ -22,13 +22,16 @@ class Switch : public Production {
     using Cases = std::vector<std::pair<std::vector<Expression*>, std::unique_ptr<Production>>>;
 
     Switch(ASTContext* /* ctx */, const std::string& symbol, Expression* expr, Cases cases,
-           std::unique_ptr<Production> default_, AttributeSet* attributes, const Location& l = location::None)
+           std::unique_ptr<Production> default_, AttributeSet* attributes, Expression* condition,
+           const Location& l = location::None)
         : Production(symbol, l),
           _expression(expr),
           _cases(std::move(cases)),
           _default(std::move(default_)),
-          _attributes(attributes) {}
+          _attributes(attributes),
+          _condition(condition) {}
 
+    const auto& condition() const { return _condition; }
     const auto& cases() const { return _cases; }
     const auto* default_() const { return _default.get(); }
     const auto& attributes() const { return _attributes; }
@@ -54,6 +57,7 @@ class Switch : public Production {
     Cases _cases;
     std::unique_ptr<Production> _default;
     AttributeSet* _attributes = nullptr;
+    Expression* _condition = nullptr;
 };
 
 } // namespace spicy::detail::codegen::production

spicy/toolchain/src/compiler/codegen/grammar-builder.cc

@@ -230,7 +230,8 @@ struct Visitor : public visitor::PreOrder {
             }
 
             result = std::make_unique<production::Switch>(context(), switch_sym, n->expression(), std::move(cases),
-                                                          std::move(default_), n->attributes(), n->meta().location());
+                                                          std::move(default_), n->attributes(), n->condition(),
+                                                          n->meta().location());
             return;
         }
 
@@ -263,7 +264,7 @@ struct Visitor : public visitor::PreOrder {
 
                 auto lah_sym = fmt("%s_lha_%d", switch_sym, i);
                 auto lah = std::make_unique<production::LookAhead>(context(), lah_sym, std::move(prev), std::move(prod),
-                                                                   d, c->meta().location());
+                                                                   d, n->condition(), c->meta().location());
                 prev = std::move(lah);
             }
 

spicy/toolchain/src/compiler/codegen/parser-builder.cc

@@ -1494,6 +1494,9 @@ struct ProductionVisitor : public production::Visitor {
     }
 
     void operator()(const production::Switch* p) final {
+        if ( auto c = p->condition() )
+            pushBuilder(builder()->addIf(c));
+
         builder()->addCall("hilti::debugIndent", {builder()->stringLiteral("spicy")});
 
         if ( const auto& a = p->attributes()->find("&parse-from") )
@@ -1545,6 +1548,9 @@ struct ProductionVisitor : public production::Visitor {
             popState();
 
         builder()->addCall("hilti::debugDedent", {builder()->stringLiteral("spicy")});
+
+        if ( p->condition() )
+            popBuilder();
     }
 
     void operator()(const production::Unit* p) final {
@@ -1702,6 +1708,9 @@ struct ProductionVisitor : public production::Visitor {
     auto parseLookAhead(const production::LookAhead& p) {
         assert(state().needs_look_ahead);
 
+        if ( auto c = p.condition() )
+            pushBuilder(builder()->addIf(c));
+
         // If we don't have a look-ahead symbol pending, get one.
         auto true_ = builder()->addIf(builder()->not_(state().lahead));
         pushBuilder(true_);
@@ -1761,6 +1770,9 @@ struct ProductionVisitor : public production::Visitor {
         pb->parseError("no expected look-ahead token found", p.location());
         popBuilder();
 
+        if ( p.condition() )
+            popBuilder();
+
         return std::make_pair(builder_alt1, builder_alt2);
     }
 

spicy/toolchain/src/compiler/codegen/productions/while.cc

@@ -42,7 +42,7 @@ void production::While::preprocessLookAhead(ASTContext* ctx, Grammar* grammar) {
 
     auto l1 = std::make_unique<production::LookAhead>(ctx, symbol() + "_l1",
                                                       std::make_unique<production::Epsilon>(ctx, location()),
-                                                      std::move(unresolved), location());
+                                                      std::move(unresolved), nullptr, location());
     auto l1_ref = std::make_unique<production::Reference>(ctx, l1.get());
 
     auto body_ref = std::make_unique<production::Reference>(ctx, _body.get());

spicy/toolchain/tests/grammar.cc

@@ -55,10 +55,11 @@ static auto reference(hilti::ASTContext* ctx, const std::unique_ptr<T>& p) {
 
 static auto lookAhead(hilti::ASTContext* ctx, const std::string& symbol,
                       std::unique_ptr<spicy::detail::codegen::Production> a1,
-                      std::unique_ptr<spicy::detail::codegen::Production> a2) {
+                      std::unique_ptr<spicy::detail::codegen::Production> a2, hilti::Expression* condition = nullptr) {
     return std::make_unique<
         spicy::detail::codegen::production::LookAhead>(ctx, symbol, std::move(a1), std::move(a2),
-                                                       spicy::detail::codegen::production::look_ahead::Default::None);
+                                                       spicy::detail::codegen::production::look_ahead::Default::None,
+                                                       condition);
 }
 
 static auto epsilon(hilti::ASTContext* ctx) {

tests/Baseline/spicy.types.unit.switch-if/output → tests/Baseline/spicy.types.unit.switch-if-expr/output

@@ -1,3 +1,8 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
-[$a=b"1", $b1=b"2345", $b2=(not set)]
-[$a=b"2", $b1=(not set), $b2=b"2345"]
+Mini::test {
+  a: 1
+  b1: 2345
+}
+Mini::test {
+  a: 2
+}

tests/Baseline/spicy.types.unit.switch-if-lahead/output

@@ -0,0 +1,8 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+Test::Foo {
+  x1: Test::X {
+    a: x1
+  }
+  x2: Test::X {}
+  y: Y
+}

tests/spicy/types/unit/switch-if.spicy → tests/spicy/types/unit/switch-if-expr.spicy

@@ -1,6 +1,6 @@
-# @TEST-EXEC: ${SPICYC} %INPUT -j -o %INPUT.hlto
-# @TEST-EXEC: echo 1234567890 | spicy-driver %INPUT.hlto >output
-# @TEST-EXEC: echo 2234567890 | spicy-driver %INPUT.hlto >>output
+# @TEST-EXEC: spicyc %INPUT -j -o %INPUT.hlto
+# @TEST-EXEC: echo 1234567890 | spicy-dump %INPUT.hlto >output
+# @TEST-EXEC: echo 2234567890 | spicy-dump %INPUT.hlto >>output
 # @TEST-EXEC: btest-diff output
 
 module Mini;
@@ -13,6 +13,4 @@ public type test = unit {
     b"1" -> b1: bytes &size=4;
     b"2" -> b2: bytes &size=4;
   } if ( self.a == b"1" );
-
-  on %done { print self; }
 };

tests/spicy/types/unit/switch-if-lahead.spicy

@@ -0,0 +1,19 @@
+# @TEST-EXEC: spicyc %INPUT -j -o %INPUT.hlto
+# @TEST-EXEC: echo x1Y | spicy-dump %INPUT.hlto >output
+# @TEST-EXEC: btest-diff output
+
+module Test;
+
+type X = unit(cond: bool) {
+    switch {
+        -> a: b"x1";
+        -> b: b"x2";
+        -> c: b"x3";
+    } if ( cond ) ;
+};
+
+public type Foo = unit {
+    x1: X(True);
+    x2: X(False);
+    y: b"Y";
+};