Refactor KeywordModifierSet to provide a class API (#4003)

This is primarily to avoid the use of `!!` in code, trying not to create
too much code as a result (obviously still a net increase). Also
refactoring to its own file to make the enum easier to find.

Note, NodeCategory does similar, I might propose similar there if
everyone's good with the API. However, that's just two, so creating
something like enum_base felt like too much.

toolchain/check/BUILD

@@ -72,6 +72,7 @@ cc_library(
         "function.h",
         "import_ref.h",
         "inst_block_stack.h",
+        "keyword_modifier_set.h",
         "merge.h",
         "modifiers.h",
         "param_and_arg_refs_stack.h",

toolchain/check/decl_state.h

@@ -5,66 +5,11 @@
 #ifndef CARBON_TOOLCHAIN_CHECK_DECL_STATE_H_
 #define CARBON_TOOLCHAIN_CHECK_DECL_STATE_H_
 
-#include "llvm/ADT/BitmaskEnum.h"
+#include "toolchain/check/keyword_modifier_set.h"
 #include "toolchain/parse/node_ids.h"
 
 namespace Carbon::Check {
 
-LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE();
-
-// Represents a set of keyword modifiers, using a separate bit per modifier.
-//
-// We expect this to grow, so are using a bigger size than needed.
-// NOLINTNEXTLINE(performance-enum-size)
-enum class KeywordModifierSet : uint32_t {
-  // At most one of these access modifiers allowed for a given declaration,
-  // and if present it must be first:
-  Private = 1 << 0,
-  Protected = 1 << 1,
-
-  // Extern is standalone.
-  Extern = 1 << 2,
-
-  // At most one of these declaration modifiers allowed for a given
-  // declaration:
-  Abstract = 1 << 3,
-  Base = 1 << 4,
-  Default = 1 << 5,
-  Export = 1 << 6,
-  Extend = 1 << 7,
-  Final = 1 << 8,
-  Impl = 1 << 9,
-  Virtual = 1 << 10,
-
-  // Sets of modifiers:
-  Access = Private | Protected,
-  Class = Abstract | Base,
-  Method = Abstract | Impl | Virtual,
-  ImplDecl = Extend | Final,
-  Interface = Default | Final,
-  Decl = Class | Method | ImplDecl | Interface | Export,
-  None = 0,
-
-  LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/Virtual)
-};
-
-inline constexpr auto operator!(KeywordModifierSet k) -> bool {
-  return !static_cast<uint32_t>(k);
-}
-
-// The order of modifiers. Each of these corresponds to a group on
-// KeywordModifierSet, and can be used as an array index.
-enum class ModifierOrder : int8_t { Access, Extern, Decl, Last = Decl };
-
-static_assert(!(KeywordModifierSet::Access & KeywordModifierSet::Extern) &&
-                  !((KeywordModifierSet::Access | KeywordModifierSet::Extern) &
-                    KeywordModifierSet::Decl),
-              "Order-related sets must not overlap");
-static_assert(~KeywordModifierSet::None ==
-                  (KeywordModifierSet::Access | KeywordModifierSet::Extern |
-                   KeywordModifierSet::Decl),
-              "Modifier missing from all modifier sets");
-
 // State stored for each declaration we are currently in: the kind of
 // declaration and the keyword modifiers that apply to that declaration.
 struct DeclState {
@@ -107,7 +52,7 @@ struct DeclState {
            Parse::NodeId::Invalid};
 
   // Invariant: contains just the modifiers represented by `saw_*_modifier`.
-  KeywordModifierSet modifier_set = KeywordModifierSet::None;
+  KeywordModifierSet modifier_set;
 };
 
 // Stack of `DeclState` values, representing all the declarations we are

toolchain/check/handle_alias.cpp

@@ -30,7 +30,7 @@ auto HandleAlias(Context& context, Parse::AliasId /*node_id*/) -> bool {
   LimitModifiersOnDecl(context, KeywordModifierSet::Access,
                        Lex::TokenKind::Alias);
   auto modifiers = context.decl_state_stack().innermost().modifier_set;
-  if (!!(modifiers & KeywordModifierSet::Access)) {
+  if (modifiers.HasAnyOf(KeywordModifierSet::Access)) {
     context.TODO(context.decl_state_stack().innermost().modifier_node_id(
                      ModifierOrder::Access),
                  "access modifier");

toolchain/check/handle_class.cpp

@@ -198,17 +198,17 @@ static auto BuildClassDecl(Context& context, Parse::AnyClassDeclId node_id,
                                name_context.enclosing_scope_id, is_definition);
 
   auto modifiers = context.decl_state_stack().innermost().modifier_set;
-  if (!!(modifiers & KeywordModifierSet::Access)) {
+  if (modifiers.HasAnyOf(KeywordModifierSet::Access)) {
     context.TODO(context.decl_state_stack().innermost().modifier_node_id(
                      ModifierOrder::Access),
                  "access modifier");
   }
 
-  bool is_extern = !!(modifiers & KeywordModifierSet::Extern);
+  bool is_extern = modifiers.HasAnyOf(KeywordModifierSet::Extern);
   auto inheritance_kind =
-      !!(modifiers & KeywordModifierSet::Abstract) ? SemIR::Class::Abstract
-      : !!(modifiers & KeywordModifierSet::Base)   ? SemIR::Class::Base
-                                                   : SemIR::Class::Final;
+      modifiers.HasAnyOf(KeywordModifierSet::Abstract) ? SemIR::Class::Abstract
+      : modifiers.HasAnyOf(KeywordModifierSet::Base)   ? SemIR::Class::Base
+                                                       : SemIR::Class::Final;
 
   context.decl_state_stack().Pop(DeclState::Class);
   auto decl_block_id = context.inst_block_stack().Pop();
@@ -395,7 +395,7 @@ auto HandleAdaptDecl(Context& context, Parse::AdaptDeclId node_id) -> bool {
       context.AddInst({node_id, SemIR::AdaptDecl{adapted_type_id}});
 
   // Extend the class scope with the adapted type's scope if requested.
-  if (!!(modifiers & KeywordModifierSet::Extend)) {
+  if (modifiers.HasAnyOf(KeywordModifierSet::Extend)) {
     auto extended_scope_id = SemIR::NameScopeId::Invalid;
     if (adapted_type_id == SemIR::TypeId::Error) {
       // Recover by not extending any scope. We instead set has_error to true
@@ -497,7 +497,7 @@ auto HandleBaseDecl(Context& context, Parse::BaseDeclId node_id) -> bool {
   LimitModifiersOnDecl(context, KeywordModifierSet::Extend,
                        Lex::TokenKind::Base);
   auto modifiers = context.decl_state_stack().innermost().modifier_set;
-  if (!(modifiers & KeywordModifierSet::Extend)) {
+  if (!modifiers.HasAnyOf(KeywordModifierSet::Extend)) {
     CARBON_DIAGNOSTIC(BaseMissingExtend, Error,
                       "Missing `extend` before `base` declaration in class.");
     context.emitter().Emit(node_id, BaseMissingExtend);
@@ -544,7 +544,7 @@ auto HandleBaseDecl(Context& context, Parse::BaseDeclId node_id) -> bool {
       class_info.base_id);
 
   // Extend the class scope with the base class.
-  if (!!(modifiers & KeywordModifierSet::Extend)) {
+  if (modifiers.HasAnyOf(KeywordModifierSet::Extend)) {
     auto& class_scope = context.name_scopes().Get(class_info.scope_id);
     if (base_info.scope_id.is_valid()) {
       class_scope.extended_scopes.push_back(base_info.scope_id);

toolchain/check/handle_function.cpp

@@ -229,18 +229,18 @@ static auto BuildFunctionDecl(Context& context,
   // Process modifiers.
   auto modifiers = DiagnoseModifiers(context, is_definition,
                                      name_context.enclosing_scope_id);
-  if (!!(modifiers & KeywordModifierSet::Access)) {
+  if (modifiers.HasAnyOf(KeywordModifierSet::Access)) {
     context.TODO(context.decl_state_stack().innermost().modifier_node_id(
                      ModifierOrder::Access),
                  "access modifier");
   }
-  bool is_extern = !!(modifiers & KeywordModifierSet::Extern);
-  if (!!(modifiers & KeywordModifierSet::Method)) {
+  bool is_extern = modifiers.HasAnyOf(KeywordModifierSet::Extern);
+  if (modifiers.HasAnyOf(KeywordModifierSet::Method)) {
     context.TODO(context.decl_state_stack().innermost().modifier_node_id(
                      ModifierOrder::Decl),
                  "method modifier");
   }
-  if (!!(modifiers & KeywordModifierSet::Interface)) {
+  if (modifiers.HasAnyOf(KeywordModifierSet::Interface)) {
     // TODO: Once we are saving the modifiers for a function, add check that
     // the function may only be defined if it is marked `default` or `final`.
     context.TODO(context.decl_state_stack().innermost().modifier_node_id(

toolchain/check/handle_impl.cpp

@@ -212,8 +212,8 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id)
   auto impl_decl_id = context.AddInst({node_id, impl_decl});
 
   // For an `extend impl` declaration, mark the impl as extending this `impl`.
-  if (!!(context.decl_state_stack().innermost().modifier_set &
-         KeywordModifierSet::Extend)) {
+  if (context.decl_state_stack().innermost().modifier_set.HasAnyOf(
+          KeywordModifierSet::Extend)) {
     auto extend_node = context.decl_state_stack().innermost().modifier_node_id(
         ModifierOrder::Decl);
     ExtendImpl(context, extend_node, node_id, self_type_node, self_type_id,

toolchain/check/handle_interface.cpp

@@ -43,7 +43,7 @@ static auto BuildInterfaceDecl(Context& context,
                        Lex::TokenKind::Interface);
 
   auto modifiers = context.decl_state_stack().innermost().modifier_set;
-  if (!!(modifiers & KeywordModifierSet::Access)) {
+  if (modifiers.HasAnyOf(KeywordModifierSet::Access)) {
     context.TODO(context.decl_state_stack().innermost().modifier_node_id(
                      ModifierOrder::Access),
                  "access modifier");

toolchain/check/handle_let.cpp

@@ -86,12 +86,12 @@ auto HandleLetDecl(Context& context, Parse::LetDeclId node_id) -> bool {
       Lex::TokenKind::Let);
 
   auto modifiers = context.decl_state_stack().innermost().modifier_set;
-  if (!!(modifiers & KeywordModifierSet::Access)) {
+  if (modifiers.HasAnyOf(KeywordModifierSet::Access)) {
     context.TODO(context.decl_state_stack().innermost().modifier_node_id(
                      ModifierOrder::Access),
                  "access modifier");
   }
-  if (!!(modifiers & KeywordModifierSet::Interface)) {
+  if (modifiers.HasAnyOf(KeywordModifierSet::Interface)) {
     context.TODO(context.decl_state_stack().innermost().modifier_node_id(
                      ModifierOrder::Decl),
                  "interface modifier");

toolchain/check/handle_modifier.cpp

@@ -39,10 +39,10 @@ static auto HandleModifier(Context& context, Parse::NodeId node_id,
 
   ModifierOrder order;
   KeywordModifierSet later_modifiers;
-  if (!!(keyword & KeywordModifierSet::Access)) {
+  if (keyword.HasAnyOf(KeywordModifierSet::Access)) {
     order = ModifierOrder::Access;
     later_modifiers = KeywordModifierSet::Extern | KeywordModifierSet::Decl;
-  } else if (keyword == KeywordModifierSet::Extern) {
+  } else if (keyword.HasAnyOf(KeywordModifierSet::Extern)) {
     order = ModifierOrder::Extern;
     later_modifiers = KeywordModifierSet::Decl;
   } else {
@@ -51,12 +51,12 @@ static auto HandleModifier(Context& context, Parse::NodeId node_id,
   }
 
   auto current_modifier_node_id = s.modifier_node_id(order);
-  if (!!(s.modifier_set & keyword)) {
+  if (s.modifier_set.HasAnyOf(keyword)) {
     DiagnoseRepeated(context, current_modifier_node_id, node_id);
   } else if (current_modifier_node_id.is_valid()) {
     DiagnoseNotAllowedWith(context, current_modifier_node_id, node_id);
   } else if (auto later_modifier_set = s.modifier_set & later_modifiers;
-             !!later_modifier_set) {
+             !later_modifier_set.empty()) {
     // At least one later modifier is present. Diagnose using the closest.
     Parse::NodeId closest_later_modifier = Parse::NodeId::Invalid;
     for (auto later_order = static_cast<int8_t>(order) + 1;
@@ -79,7 +79,7 @@ static auto HandleModifier(Context& context, Parse::NodeId node_id,
               context.token_kind(closest_later_modifier))
         .Emit();
   } else {
-    s.modifier_set |= keyword;
+    s.modifier_set.Add(keyword);
     s.set_modifier_node_id(order, node_id);
   }
   return true;

toolchain/check/handle_variable.cpp

@@ -102,7 +102,7 @@ auto HandleVariableDecl(Context& context, Parse::VariableDeclId node_id)
   LimitModifiersOnDecl(context, KeywordModifierSet::Access,
                        Lex::TokenKind::Var);
   auto modifiers = context.decl_state_stack().innermost().modifier_set;
-  if (!!(modifiers & KeywordModifierSet::Access)) {
+  if (modifiers.HasAnyOf(KeywordModifierSet::Access)) {
     context.TODO(context.decl_state_stack().innermost().modifier_node_id(
                      ModifierOrder::Access),
                  "access modifier");

toolchain/check/keyword_modifier_set.h

@@ -0,0 +1,104 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#ifndef CARBON_TOOLCHAIN_CHECK_KEYWORD_MODIFIER_SET_H_
+#define CARBON_TOOLCHAIN_CHECK_KEYWORD_MODIFIER_SET_H_
+
+#include "llvm/ADT/BitmaskEnum.h"
+
+namespace Carbon::Check {
+
+LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE();
+
+// The order of modifiers. Each of these corresponds to a group on
+// KeywordModifierSet, and can be used as an array index.
+enum class ModifierOrder : int8_t { Access, Extern, Decl, Last = Decl };
+
+// Represents a set of keyword modifiers, using a separate bit per modifier.
+class KeywordModifierSet {
+ public:
+  // Provide values as an enum. This doesn't expose these as KeywordModifierSet
+  // instances just due to the duplication of declarations that would cause.
+  //
+  // We expect this to grow, so are using a bigger size than needed.
+  // NOLINTNEXTLINE(performance-enum-size)
+  enum Enum : uint32_t {
+    // At most one of these access modifiers allowed for a given declaration,
+    // and if present it must be first:
+    Private = 1 << 0,
+    Protected = 1 << 1,
+
+    // Extern is standalone.
+    Extern = 1 << 2,
+
+    // At most one of these declaration modifiers allowed for a given
+    // declaration:
+    Abstract = 1 << 3,
+    Base = 1 << 4,
+    Default = 1 << 5,
+    Export = 1 << 6,
+    Extend = 1 << 7,
+    Final = 1 << 8,
+    Impl = 1 << 9,
+    Virtual = 1 << 10,
+
+    // Sets of modifiers:
+    Access = Private | Protected,
+    Class = Abstract | Base,
+    Method = Abstract | Impl | Virtual,
+    ImplDecl = Extend | Final,
+    Interface = Default | Final,
+    Decl = Class | Method | ImplDecl | Interface | Export,
+    None = 0,
+
+    LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/Virtual)
+  };
+
+  // Default construct to empty.
+  explicit KeywordModifierSet() : set_(None) {}
+
+  // Support implicit conversion so that the difference with the member enum is
+  // opaque.
+  // NOLINTNEXTLINE(google-explicit-constructor)
+  constexpr KeywordModifierSet(Enum set) : set_(set) {}
+
+  // Adds entries to the set.
+  auto Add(KeywordModifierSet set) -> void { set_ |= set.set_; }
+  // Removes entries from the set.
+  auto Remove(KeywordModifierSet set) -> void { set_ &= ~set.set_; }
+
+  // Returns true if there's a non-empty set intersection.
+  constexpr auto HasAnyOf(KeywordModifierSet other) -> bool {
+    return !(*this & other).empty();
+  }
+
+  // Returns true if empty.
+  constexpr auto empty() -> bool { return set_ == Enum::None; }
+
+  // Returns the set intersection.
+  constexpr auto operator&(KeywordModifierSet other) -> KeywordModifierSet {
+    return set_ & other.set_;
+  }
+
+  // Returns the set inverse.
+  auto operator~() -> KeywordModifierSet { return ~set_; }
+
+ private:
+  Enum set_;
+};
+
+static_assert(!KeywordModifierSet(KeywordModifierSet::Access)
+                      .HasAnyOf(KeywordModifierSet::Extern) &&
+                  !KeywordModifierSet(KeywordModifierSet::Access |
+                                      KeywordModifierSet::Extern)
+                       .HasAnyOf(KeywordModifierSet::Decl),
+              "Order-related sets must not overlap");
+static_assert(~KeywordModifierSet::None ==
+                  (KeywordModifierSet::Access | KeywordModifierSet::Extern |
+                   KeywordModifierSet::Decl),
+              "Modifier missing from all modifier sets");
+
+}  // namespace Carbon::Check
+
+#endif  // CARBON_TOOLCHAIN_CHECK_KEYWORD_MODIFIER_SET_H_

toolchain/check/modifiers.cpp

@@ -44,21 +44,21 @@ auto ForbidModifiersOnDecl(Context& context, KeywordModifierSet forbidden,
                            SemIR::LocId context_loc_id) -> void {
   auto& s = context.decl_state_stack().innermost();
   auto not_allowed = s.modifier_set & forbidden;
-  if (!not_allowed) {
+  if (not_allowed.empty()) {
     return;
   }
 
   for (auto order_index = 0;
        order_index <= static_cast<int8_t>(ModifierOrder::Last); ++order_index) {
     auto order = static_cast<ModifierOrder>(order_index);
-    if (!!(not_allowed & ModifierOrderAsSet(order))) {
+    if (not_allowed.HasAnyOf(ModifierOrderAsSet(order))) {
       DiagnoseNotAllowed(context, s.modifier_node_id(order), decl_kind,
                          context_string, context_loc_id);
       s.set_modifier_node_id(order, Parse::NodeId::Invalid);
     }
   }
 
-  s.modifier_set &= ~forbidden;
+  s.modifier_set.Remove(forbidden);
 }
 
 // Returns the instruction that owns the given scope, or Invalid if the scope is