Merge remote-tracking branch 'origin/topic/awelzel/set-val-on-ids-for-enums'

* origin/topic/awelzel/set-val-on-ids-for-enums:
  broker/main: Adapt enum values to agree with comm.bif
  btest/plugin: Add plugin testing enum identifiers
  Var/add_type: Do not clone EnumType when declared for the first time
  type/id/zeekygen: Add EnumVal to enum identifiers

CHANGES

@@ -1,3 +1,31 @@
+7.2.0-dev.478 | 2025-04-04 18:36:37 +0200
+
+  * broker/main: Adapt enum values to agree with comm.bif (Arne Welzel, Corelight)
+
+    Logic to detect this error already existed, but due to enum identifiers
+    not having a value set, it never triggered before.
+
+  * btest/plugin: Add plugin testing enum identifiers (Arne Welzel, Corelight)
+
+    This plugin has a generic name, but for now just tests the API around
+    enum types and enum vals at InitPostScript() time.
+
+  * Var/add_type: Do not clone EnumType when declared for the first time (Arne Welzel, Corelight)
+
+    EnumType receives the name into its constructor. Even for the  first declaration
+    the name is not empty and instead the same as the identifier's name. Due to that,
+    add_type() previously took the else path and created a shallow clone of the
+    initial type instead of using it. This lead to buggy behavior where enum value
+    identifiers declared within an enum's first body have a different TypePtr
+    associated than the one that is found via `zeek::id::find_type()`. It also
+    means that enum identifiers added via redef later would have a different
+    TypePtr than those in the initial pointer.
+
+  * type/id/zeekygen: Add EnumVal to enum identifiers (Arne Welzel, Corelight)
+
+    Provide a direct way to go from a zeek:id::ID value to EnumVal without
+    needing to go through the type.
+
 7.2.0-dev.473 | 2025-04-03 13:10:43 +0200
 
   * GH-4323: Traverse: Do not short-circuit traverse_all() if stmts is nullptr (Arne Welzel, Corelight)

NEWS

@@ -120,6 +120,10 @@ Changed Functionality
 - The RDP analyzer now also parses connections that do not contain the cookie
   field, which were previously rejected.
 
+- An enum's zeek::detail::ID instance now holds its ``EnumVal``. For example,
+  looking up the "Conn::LOG" identifier allows to directly query the ``EnumVal``
+  using ``ID::GetVal()``.
+
 Removed Functionality
 ---------------------
 

VERSION

@@ -1 +1 @@
-7.2.0-dev.473
+7.2.0-dev.478

scripts/base/frameworks/broker/main.zeek

@@ -207,24 +207,26 @@ export {
 		PEER_INVALID = 3,
 		## Remote peer not listening.
 		PEER_UNAVAILABLE = 4,
+		## Remote peer disconnected during the handshake.
+		PEER_DISCONNECT_DURING_HANDSHAKE = 5,
 		## A peering request timed out.
-		PEER_TIMEOUT = 5,
+		PEER_TIMEOUT = 6,
 		## Master with given name already exists.
-		MASTER_EXISTS = 6,
+		MASTER_EXISTS = 7,
 		## Master with given name does not exist.
-		NO_SUCH_MASTER = 7,
+		NO_SUCH_MASTER = 8,
 		## The given data store key does not exist.
-		NO_SUCH_KEY = 8,
+		NO_SUCH_KEY = 9,
 		## The store operation timed out.
-		REQUEST_TIMEOUT = 9,
+		REQUEST_TIMEOUT = 10,
 		## The operation expected a different type than provided.
-		TYPE_CLASH = 10,
+		TYPE_CLASH = 11,
 		## The data value cannot be used to carry out the desired operation.
-		INVALID_DATA = 11,
+		INVALID_DATA = 12,
 		## The storage backend failed to execute the operation.
-		BACKEND_FAILURE = 12,
+		BACKEND_FAILURE = 13,
 		## The storage backend failed to execute the operation.
-		STALE_DATA = 13,
+		STALE_DATA = 14,
 		## Catch-all for a CAF-level problem.
 		CAF_ERROR = 100
 	};

src/ID.cc

@@ -481,6 +481,10 @@ void ID::DescribeReST(ODesc* d, bool roles_only) const {
     }
 
     if ( val && type && type->Tag() != TYPE_FUNC && type->InternalType() != TYPE_INTERNAL_VOID &&
+         // Do not include a default value for enum const identifiers,
+         // as their value can't be changed.
+         ! IsEnumConst() &&
+
          // Values within Version module are likely to include a
          // constantly-changing version number and be a frequent
          // source of error/desynchronization, so don't include them.

src/Type.cc

@@ -1567,7 +1567,7 @@ void EnumType::CheckAndAddName(const string& module_name, const char* name, zeek
         if ( deprecation )
             id->MakeDeprecated({NewRef{}, deprecation});
 
-        detail::zeekygen_mgr->Identifier(std::move(id), from_redef);
+        detail::zeekygen_mgr->Identifier(id, from_redef);
     }
     else {
         // We allow double-definitions if matching exactly. This is so that
@@ -1591,6 +1591,12 @@ void EnumType::CheckAndAddName(const string& module_name, const char* name, zeek
     if ( vals.find(val) == vals.end() )
         vals[val] = make_intrusive<EnumVal>(IntrusivePtr{NewRef{}, this}, val);
 
+    if ( ! id->HasVal() )
+        id->SetVal(vals[val]);
+    else if ( id->GetVal()->AsEnum() != val )
+        reporter->InternalError("inconsistent enum integer value for '%s' (old %" PRId64 " new %" PRId64 ")",
+                                fullname.c_str(), id->GetVal()->AsEnum(), val);
+
     const auto& types = Type::Aliases(GetName());
 
     for ( const auto& t : types )

src/Var.cc

@@ -369,9 +369,18 @@ void add_type(ID* id, TypePtr t, std::unique_ptr<std::vector<AttrPtr>> attr) {
 
     TypePtr tnew;
 
-    if ( (t->Tag() == TYPE_RECORD || t->Tag() == TYPE_ENUM) && old_type_name.empty() )
+    if ( (t->Tag() == TYPE_RECORD || t->Tag() == TYPE_ENUM) &&
+         (old_type_name.empty() || old_type_name == new_type_name) ) {
         // An extensible type (record/enum) being declared for first time.
+        //
+        // Enum types are initialized with the same name as their identifier
+        // when declared for the first time, double check that here.
+        if ( t->Tag() == TYPE_ENUM && new_type_name != old_type_name )
+            reporter->InternalError("enum type has unexpected names: '%s' and '%s'", old_type_name.c_str(),
+                                    new_type_name.c_str());
+
         tnew = std::move(t);
+    }
     else {
         // If the old type is an error or the old type doesn't exist, then return
         // an error instead of trying to clone it.

testing/btest/Baseline/plugins.api-plugin/output

@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+TestEnumIdentifiers successful

testing/btest/plugins/api-plugin.zeek

@@ -0,0 +1,26 @@
+# @TEST-DOC: A plugin testing some parts Zeek's C++ API
+# @TEST-EXEC: ${DIST}/auxil/zeek-aux/plugin-support/init-plugin -u . Demo API
+# @TEST-EXEC: cp -r %DIR/api-plugin/* .
+#
+# @TEST-EXEC: ./configure --zeek-dist=${DIST} && make
+#
+# @TEST-EXEC: ZEEK_PLUGIN_PATH=`pwd` zeek %INPUT >output
+#
+# @TEST-EXEC: TEST_DIFF_CANONIFIER= btest-diff output
+
+module DemoAPI;
+
+export {
+	type Severity: enum {
+		CRITICAL = 1,
+		ERROR = 2,
+		WARNING = 3,
+		INFO = 4,
+	};
+}
+
+module User;
+
+redef enum DemoAPI::Severity += {
+	USER_DEBUG = 50,
+};

testing/btest/plugins/api-plugin/src/Plugin.cc

@@ -0,0 +1,88 @@
+
+#include "Plugin.h"
+
+#include <zeek/ID.h>
+#include <zeek/Reporter.h>
+#include <zeek/Type.h>
+#include <zeek/Val.h>
+#include <iostream>
+
+namespace btest::plugin::Demo_API {
+Plugin plugin;
+}
+
+using namespace btest::plugin::Demo_API;
+
+zeek::plugin::Configuration Plugin::Configure() {
+    zeek::plugin::Configuration config;
+    config.name = "Demo::API";
+    config.description = "Use some of Zeek's API for testing";
+    config.version.major = 1;
+    config.version.minor = 0;
+    config.version.patch = 0;
+    return config;
+}
+
+namespace {
+
+// Test zeek::id::find for enums an their types.
+void TestEnumIdentifiers() {
+    auto severity_type = zeek::id::find_type<zeek::EnumType>("DemoAPI::Severity");
+    if ( ! severity_type )
+        zeek::reporter->FatalError("DemoAPI::Severity not found");
+
+    // Expect 5 entries!
+    if ( severity_type->Names().size() != 5 )
+        zeek::reporter->FatalError("Wrong number of severities %" PRId64, severity_type->Names().size());
+
+    // Ensure CRITICAL and USER_DEBUG identifiers have the same enum type as severity_type.
+    auto critical_id = zeek::id::find("DemoAPI::CRITICAL");
+    auto critical_type = critical_id->GetType();
+    auto user_debug_id = zeek::id::find("User::USER_DEBUG");
+    auto user_debug_type = user_debug_id->GetType();
+
+    if ( critical_type != user_debug_type )
+        zeek::reporter->FatalError("CRITICAL and USER_DEBUG have different types (%p and %p)", critical_type.get(),
+                                   user_debug_type.get());
+
+    // Ensure the critical_id and user_debug_type IDs have an EnumVal attached
+    // and that the value is the same as in script land.
+    auto critical_val = critical_id->GetVal();
+    auto user_debug_val = user_debug_id->GetVal();
+
+    if ( ! critical_val || ! user_debug_val )
+        zeek::reporter->FatalError("Missing values on enum value identifiers %p %p", critical_val.get(),
+                                   user_debug_val.get());
+
+    if ( critical_val->AsEnum() != 1 )
+        zeek::reporter->FatalError("Wrong value for CRITICAL: %" PRId64, critical_val->AsEnum());
+
+    if ( user_debug_val->AsEnum() != 50 )
+        zeek::reporter->FatalError("Wrong value for USER_DEBUG: %" PRId64, user_debug_val->AsEnum());
+
+    // Ensure all the types (identifiers and values) agree with severity_type.
+    if ( critical_type != severity_type )
+        zeek::reporter->FatalError("CRITICAL identifier has the wrong enum type %p vs %p", critical_type.get(),
+                                   severity_type.get());
+
+    if ( user_debug_type != severity_type )
+        zeek::reporter->FatalError("USER_DEBUG identifier has the wrong enum type %p vs %p", user_debug_type.get(),
+                                   severity_type.get());
+
+    if ( critical_val->GetType() != severity_type )
+        zeek::reporter->FatalError("CRITICAL value has the wrong enum type %p vs %p", critical_val->GetType().get(),
+                                   severity_type.get());
+
+    if ( user_debug_val->GetType() != severity_type )
+        zeek::reporter->FatalError("USER_DEBUG value has the wrong enum type %p vs %p", user_debug_val->GetType().get(),
+                                   severity_type.get());
+
+    std::cout << "TestEnumIdentifiers successful" << std::endl;
+}
+
+} // namespace
+
+void Plugin::InitPostScript() {
+    // Other API tests if wanted.
+    TestEnumIdentifiers();
+}

testing/btest/plugins/api-plugin/src/Plugin.h

@@ -0,0 +1,18 @@
+
+#pragma once
+
+#include <zeek/plugin/Plugin.h>
+
+namespace btest::plugin::Demo_API {
+
+class Plugin : public zeek::plugin::Plugin {
+protected:
+    // Overridden from zeek::plugin::Plugin.
+    zeek::plugin::Configuration Configure() override;
+
+    void InitPostScript() override;
+};
+
+extern Plugin plugin;
+
+} // namespace btest::plugin::Demo_API