Remove default constructor from IndexBase (#2598)

There were some notes about default constructors being required in parse_tree.h, but they don't seem to be. Removing the default constructor forces the explicit `::Invalid` where there's no value immediately being assigned, which works fine for existing code. I think this actually reduces the chance of accidents more than the prior default-construct-as-invalid approach.

toolchain/common/index_base.h

@@ -24,7 +24,7 @@ class DataIterator;
 struct IndexBase {
   static constexpr int32_t InvalidIndex = -1;
 
-  constexpr IndexBase() : index(InvalidIndex) {}
+  IndexBase() = delete;
   constexpr explicit IndexBase(int index) : index(index) {}
 
   auto Print(llvm::raw_ostream& output) const -> void {

toolchain/lexer/tokenized_buffer.h

@@ -81,6 +81,8 @@ class TokenizedBuffer {
   // All other APIs to query a `Identifier` are on the `TokenizedBuffer`.
   struct Identifier : public IndexBase {
     using IndexBase::IndexBase;
+
+    static const Identifier Invalid;
   };
 
   // Random-access iterator over tokens within the buffer.
@@ -88,7 +90,7 @@ class TokenizedBuffer {
       : public llvm::iterator_facade_base<
             TokenIterator, std::random_access_iterator_tag, const Token, int> {
    public:
-    TokenIterator() = default;
+    TokenIterator() = delete;
 
     explicit TokenIterator(Token token) : token_(token) {}
 
@@ -343,7 +345,7 @@ class TokenizedBuffer {
           sizeof(Token) <= sizeof(int32_t),
           "Unable to pack token and identifier index into the same space!");
 
-      Identifier id;
+      Identifier id = Identifier::Invalid;
       int32_t literal_index;
       Token closing_token;
       Token opening_token;
@@ -404,6 +406,9 @@ class TokenizedBuffer {
   bool has_errors_ = false;
 };
 
+constexpr TokenizedBuffer::Identifier TokenizedBuffer::Identifier::Invalid =
+    TokenizedBuffer::Identifier(TokenizedBuffer::Identifier::InvalidIndex);
+
 // A diagnostic emitter that uses positions within a source buffer's text as
 // its source of location information.
 using LexerDiagnosticEmitter = DiagnosticEmitter<const char*>;

toolchain/parser/parse_tree.cpp

@@ -124,7 +124,7 @@ auto ParseTree::Print(llvm::raw_ostream& output) const -> void {
   }
 
   while (!node_stack.empty()) {
-    Node n;
+    Node n = Node::Invalid;
     int depth;
     std::tie(n, depth) = node_stack.pop_back_val();
     for (Node sibling_n : children(n)) {
@@ -161,7 +161,7 @@ auto ParseTree::Print(llvm::raw_ostream& output, bool preorder) const -> void {
   }
 
   while (!node_stack.empty()) {
-    Node n;
+    Node n = Node::Invalid;
     int depth;
     std::tie(n, depth) = node_stack.pop_back_val();
 

toolchain/parser/parse_tree.h

@@ -249,7 +249,8 @@ struct ParseTree::Node : public ComparableIndexBase {
   using ComparableIndexBase::ComparableIndexBase;
 };
 
-constexpr ParseTree::Node ParseTree::Node::Invalid;
+constexpr ParseTree::Node ParseTree::Node::Invalid =
+    ParseTree::Node(ParseTree::Node::InvalidIndex);
 
 // A random-access iterator to the depth-first postorder sequence of parse nodes
 // in the parse tree. It produces `ParseTree::Node` objects which are opaque
@@ -259,10 +260,7 @@ class ParseTree::PostorderIterator
                                         std::random_access_iterator_tag, Node,
                                         int, Node*, Node> {
  public:
-  // Default construction is only provided to satisfy iterator requirements. It
-  // produces an unusable iterator, and you must assign a valid iterator to it
-  // before performing any operations.
-  explicit PostorderIterator() = default;
+  PostorderIterator() = delete;
 
   auto operator==(const PostorderIterator& rhs) const -> bool {
     return node_ == rhs.node_;
@@ -312,7 +310,7 @@ class ParseTree::SiblingIterator
     : public llvm::iterator_facade_base<
           SiblingIterator, std::forward_iterator_tag, Node, int, Node*, Node> {
  public:
-  explicit SiblingIterator() = default;
+  explicit SiblingIterator() = delete;
 
   auto operator==(const SiblingIterator& rhs) const -> bool {
     return node_ == rhs.node_;

toolchain/parser/parse_tree_test.cpp

@@ -39,11 +39,6 @@ class ParseTreeTest : public ::testing::Test {
   DiagnosticConsumer& consumer = ConsoleDiagnosticConsumer();
 };
 
-TEST_F(ParseTreeTest, DefaultInvalid) {
-  ParseTree::Node node;
-  EXPECT_FALSE(node.is_valid());
-}
-
 TEST_F(ParseTreeTest, IsValid) {
   TokenizedBuffer tokens = GetTokenizedBuffer("");
   ParseTree tree = ParseTree::Parse(tokens, consumer, /*vlog_stream=*/nullptr);

toolchain/semantics/semantics_node.h

@@ -33,7 +33,8 @@ struct SemanticsNodeId : public IndexBase {
   }
 };
 
-constexpr SemanticsNodeId SemanticsNodeId::Invalid = SemanticsNodeId();
+constexpr SemanticsNodeId SemanticsNodeId::Invalid =
+    SemanticsNodeId(SemanticsNodeId::InvalidIndex);
 
 // Uses the cross-reference node ID for a builtin. This relies on SemanticsIR
 // guarantees for builtin cross-reference placement.
@@ -88,7 +89,7 @@ struct SemanticsNodeBlockId : public IndexBase {
 constexpr SemanticsNodeBlockId SemanticsNodeBlockId::Empty =
     SemanticsNodeBlockId(0);
 constexpr SemanticsNodeBlockId SemanticsNodeBlockId::Invalid =
-    SemanticsNodeBlockId();
+    SemanticsNodeBlockId(SemanticsNodeBlockId::InvalidIndex);
 
 // Type-safe storage of strings.
 struct SemanticsStringId : public IndexBase {
@@ -144,8 +145,8 @@ class SemanticsNode {
                           SemanticsNodeId type) -> SemanticsNode {
     // Builtins won't have a ParseTree node associated, so we provide the
     // default invalid one.
-    return SemanticsNode(ParseTree::Node(), SemanticsNodeKind::Builtin, type,
-                         builtin_kind.AsInt());
+    return SemanticsNode(ParseTree::Node::Invalid, SemanticsNodeKind::Builtin,
+                         type, builtin_kind.AsInt());
   }
   auto GetAsBuiltin() const -> SemanticsBuiltinKind {
     CARBON_CHECK(kind_ == SemanticsNodeKind::Builtin);
@@ -155,7 +156,7 @@ class SemanticsNode {
   static auto MakeCodeBlock(ParseTree::Node parse_node,
                             SemanticsNodeBlockId node_block) -> SemanticsNode {
     return SemanticsNode(parse_node, SemanticsNodeKind::CodeBlock,
-                         SemanticsNodeId(), node_block.index);
+                         SemanticsNodeId::Invalid, node_block.index);
   }
   auto GetAsCodeBlock() const -> SemanticsNodeBlockId {
     CARBON_CHECK(kind_ == SemanticsNodeKind::CodeBlock);
@@ -180,7 +181,7 @@ class SemanticsNode {
                                       SemanticsCallableId signature)
       -> SemanticsNode {
     return SemanticsNode(parse_node, SemanticsNodeKind::FunctionDeclaration,
-                         SemanticsNodeId(), signature.index);
+                         SemanticsNodeId::Invalid, signature.index);
   }
   auto GetAsFunctionDeclaration() const -> SemanticsCallableId {
     CARBON_CHECK(kind_ == SemanticsNodeKind::FunctionDeclaration);
@@ -192,7 +193,8 @@ class SemanticsNode {
                                      SemanticsNodeBlockId node_block)
       -> SemanticsNode {
     return SemanticsNode(parse_node, SemanticsNodeKind::FunctionDefinition,
-                         SemanticsNodeId(), decl.index, node_block.index);
+                         SemanticsNodeId::Invalid, decl.index,
+                         node_block.index);
   }
   auto GetAsFunctionDefinition() const
       -> std::pair<SemanticsNodeId, SemanticsNodeBlockId> {
@@ -225,7 +227,7 @@ class SemanticsNode {
     // understand the type without checking, so it's not necessary but could be
     // specified if needed.
     return SemanticsNode(parse_node, SemanticsNodeKind::Return,
-                         SemanticsNodeId());
+                         SemanticsNodeId::Invalid);
   }
   auto GetAsReturn() const -> NoArgs {
     CARBON_CHECK(kind_ == SemanticsNodeKind::Return);
@@ -253,8 +255,8 @@ class SemanticsNode {
   }
 
   SemanticsNode()
-      : SemanticsNode(ParseTree::Node(), SemanticsNodeKind::Invalid,
-                      SemanticsNodeId()) {}
+      : SemanticsNode(ParseTree::Node::Invalid, SemanticsNodeKind::Invalid,
+                      SemanticsNodeId::Invalid) {}
 
   auto parse_node() const -> ParseTree::Node { return parse_node_; }
   auto kind() const -> SemanticsNodeKind { return kind_; }

toolchain/semantics/semantics_parse_tree_handler.cpp

@@ -324,10 +324,8 @@ auto SemanticsParseTreeHandler::HandleFunctionDefinitionStart(
   auto fn_node =
       node_stack_.PopForSoloParseNode(ParseNodeKind::FunctionIntroducer);
 
-  SemanticsCallable callable;
-  callable.param_ir_id = param_ir_id;
-  callable.param_refs_id = param_refs_id;
-  auto callable_id = semantics_->AddCallable(callable);
+  auto callable_id = semantics_->AddCallable(
+      {.param_ir_id = param_ir_id, .param_refs_id = param_refs_id});
   auto decl_id =
       AddNode(SemanticsNode::MakeFunctionDeclaration(fn_node, callable_id));
   // TODO: Propagate the type of the function.