Merge remote-tracking branch 'origin/topic/etyp/attribute-consistent-validation'

(Merge commit includes some small additional tweaks to style and code placement.)

* origin/topic/etyp/attribute-consistent-validation:
  Add consistent validation for attributes.

Author: rsmmr

CHANGES

@@ -1,3 +1,16 @@
+1.12.0-dev.242 | 2024-12-13 09:26:36 +0100
+
+  * GH-1901: Add consistent validation for attributes. (Evan Typanski, Corelight)
+
+    This updates attribute validation to see if attributes are in the
+    right places. The goal here is to have one place (ie a big set) to
+    answer the question "Can I use X attribute on Y node?"
+
+    There are a lot more moving parts with attribute validation, but those
+    generally have to do with behavior. A lot of that requires extra context
+    in the validator, which is exactly what the validator is meant to do.
+    Much of that is pretty ad-hoc, so it could get cleaned up as well.
+
 1.12.0-dev.240 | 2024-12-12 13:09:05 +0100
 
   * Make sure autogen-docs pre-commit hook can always run in CI. (Benjamin Bannier, Corelight)

VERSION

@@ -1 +1 @@
-1.12.0-dev.240
+1.12.0-dev.242

hilti/toolchain/include/ast/node.h

@@ -240,7 +240,15 @@ class Node {
     virtual ~Node();
 
     /** Returns the node tag associated with the instance's class. */
-    auto nodeTag() const { return _node_tags.back(); }
+    node::Tag nodeTag() const {
+        // Get the last non-zero tag. The last element(s) may be unset
+        for ( auto it = _node_tags.rbegin(); it != _node_tags.rend(); it++ ) {
+            if ( *it != 0 )
+                return *it;
+        }
+
+        return 0;
+    }
 
     /** Returns true if the node has a parent (i.e., it's part of an AST). */
     bool hasParent() const { return _parent; }

hilti/toolchain/src/compiler/validator.cc

@@ -60,6 +60,18 @@
 using namespace hilti;
 using util::fmt;
 
+/**
+ * A mapping of node tags to any attributes that node allows. When a new
+ * attribute is added, this map must be updated to accept that attribute on any
+ * nodes it applies to.
+ */
+static std::unordered_map<node::Tag, std::unordered_set<Attribute::Kind>> allowed_attributes{
+    {node::tag::Function,
+     {Attribute::Kind::Cxxname, Attribute::Kind::HavePrototype, Attribute::Kind::Priority, Attribute::Kind::Static,
+      Attribute::Kind::NeededByFeature, Attribute::Kind::Debug, Attribute::Kind::Foreach, Attribute::Kind::Error}},
+    {node::tag::declaration::Parameter, {Attribute::Kind::RequiresTypeFeature}},
+};
+
 void hilti::validator::VisitorMixIn::deprecated(const std::string& msg, const Location& l) const {
     hilti::logger().deprecated(msg, l);
 }
@@ -124,6 +136,30 @@ struct VisitorPre : visitor::PreOrder, public validator::VisitorMixIn {
 struct VisitorPost : visitor::PreOrder, public validator::VisitorMixIn {
     using hilti::validator::VisitorMixIn::VisitorMixIn;
 
+    // Ensures that the node represented by tag is allowed to have all of the
+    // provided attributes. This does not use any context, if more information
+    // is needed, then do the check elsewhere.
+    void checkNodeAttributes(node::Tag tag, AttributeSet* attributes, const std::string_view& where) {
+        if ( ! attributes )
+            return;
+
+        auto it = allowed_attributes.find(tag);
+
+        if ( it == allowed_attributes.end() ) {
+            if ( ! attributes->attributes().empty() )
+                error(hilti::util::fmt("No attributes expected in %s", where), attributes);
+
+            return;
+        }
+
+        auto allowed = it->second;
+
+        for ( const auto& attr : attributes->attributes() ) {
+            if ( allowed.find(attr->kind()) == allowed.end() )
+                error(hilti::util::fmt("invalid attribute '%s' in %s", attr->attributeName(), where), attr);
+        }
+    }
+
     // Returns an error if the given type cannot be used for ordering at
     // runtime.
     Result<Nothing> isSortable(QualifiedType* t) {
@@ -176,6 +212,8 @@ struct VisitorPost : visitor::PreOrder, public validator::VisitorMixIn {
     }
 
     void operator()(Function* n) final {
+        checkNodeAttributes(n->nodeTag(), n->attributes(), "function");
+
         if ( auto attrs = n->attributes() ) {
             if ( auto prio = attrs->find(hilti::Attribute::Kind::Priority) ) {
                 if ( n->ftype()->flavor() != type::function::Flavor::Hook )
@@ -260,6 +298,8 @@ struct VisitorPost : visitor::PreOrder, public validator::VisitorMixIn {
     }
 
     void operator()(declaration::Parameter* n) final {
+        checkNodeAttributes(n->nodeTag(), n->attributes(), n->displayName());
+
         if ( ! n->type()->type()->isA<type::Auto>() ) {
             if ( ! n->type()->type()->isAllocable() && ! n->type()->type()->isA<type::Any>() )
                 error(fmt("type '%s' cannot be used for function parameter", *n->type()), n);
@@ -285,10 +325,7 @@ struct VisitorPost : visitor::PreOrder, public validator::VisitorMixIn {
 
         if ( auto attrs = n->attributes() )
             for ( const auto& attr : attrs->attributes() ) {
-                if ( attr->kind() != hilti::Attribute::Kind::RequiresTypeFeature )
-                    error(fmt("invalid attribute '%s' for function parameter", attr->attributeName()), n);
-
-                else {
+                if ( attr->kind() == hilti::Attribute::Kind::RequiresTypeFeature ) {
                     if ( auto x = attr->valueAsString(); ! x )
                         error(x.error(), n);
                 }