Switch `GetNodeBlock` and `GetTypeBlock` to return an `ArrayRef` (#3220)

This removes the risk of accidentally performing a vector copy when
calling these functions, and is a preparation step towards the new node
block allocation design.

This required changing how we build call expressions. Instead of
finishing the argument block and then later adding a return slot, we now
delay finishing the argument block and checking for conversions to
parameter types until after we've added the return slot to it.

toolchain/check/context.cpp

@@ -69,6 +69,11 @@ auto Context::AddNodeToBlock(SemIR::NodeBlockId block, SemIR::Node node)
   return semantics_ir_->AddNode(block, node);
 }
 
+auto Context::AddNodeIdToBlock(SemIR::NodeBlockId block, SemIR::NodeId node_id)
+    -> void {
+  semantics_ir_->AddNodeId(block, node_id);
+}
+
 auto Context::AddNodeAndPush(Parse::Node parse_node, SemIR::Node node) -> void {
   auto node_id = AddNode(node);
   node_stack_.Push(parse_node, node_id);
@@ -436,7 +441,8 @@ auto Context::HandleDiscardedExpression(SemIR::NodeId expr_id) -> void {
 auto Context::ImplicitAsForArgs(Parse::Node call_parse_node,
                                 SemIR::NodeBlockId arg_refs_id,
                                 Parse::Node param_parse_node,
-                                SemIR::NodeBlockId param_refs_id) -> bool {
+                                SemIR::NodeBlockId param_refs_id,
+                                bool has_return_slot) -> bool {
   // If both arguments and parameters are empty, return quickly. Otherwise,
   // we'll fetch both so that errors are consistent.
   if (arg_refs_id == SemIR::NodeBlockId::Empty &&
@@ -444,8 +450,16 @@ auto Context::ImplicitAsForArgs(Parse::Node call_parse_node,
     return true;
   }
 
-  auto& arg_refs = semantics_ir_->GetNodeBlock(arg_refs_id);
-  const auto& param_refs = semantics_ir_->GetNodeBlock(param_refs_id);
+  auto arg_refs = semantics_ir_->GetNodeBlock(arg_refs_id);
+  auto param_refs = semantics_ir_->GetNodeBlock(param_refs_id);
+
+  if (has_return_slot) {
+    // There's no entry in the parameter block for the return slot, so ignore
+    // the corresponding entry in the argument block.
+    // TODO: Consider adding the return slot to the parameter list.
+    CARBON_CHECK(!arg_refs.empty()) << "missing return slot";
+    arg_refs = arg_refs.drop_back();
+  }
 
   // If sizes mismatch, fail early.
   if (arg_refs.size() != param_refs.size()) {
@@ -581,14 +595,23 @@ auto Context::ParamOrArgComma(bool for_args) -> void {
   ParamOrArgSave(for_args);
 }
 
-auto Context::ParamOrArgEnd(bool for_args, Parse::NodeKind start_kind)
-    -> SemIR::NodeBlockId {
+auto Context::ParamOrArgEndNoPop(bool for_args, Parse::NodeKind start_kind)
+    -> void {
   if (parse_tree_->node_kind(node_stack_.PeekParseNode()) != start_kind) {
     ParamOrArgSave(for_args);
   }
+}
+
+auto Context::ParamOrArgPop() -> SemIR::NodeBlockId {
   return params_or_args_stack_.Pop();
 }
 
+auto Context::ParamOrArgEnd(bool for_args, Parse::NodeKind start_kind)
+    -> SemIR::NodeBlockId {
+  ParamOrArgEndNoPop(for_args, start_kind);
+  return ParamOrArgPop();
+}
+
 auto Context::ParamOrArgSave(bool for_args) -> void {
   auto [entry_parse_node, entry_node_id] =
       node_stack_.PopExpressionWithParseNode();
@@ -601,9 +624,7 @@ auto Context::ParamOrArgSave(bool for_args) -> void {
   }
 
   // Save the param or arg ID.
-  auto& params_or_args =
-      semantics_ir_->GetNodeBlock(params_or_args_stack_.PeekForAdd());
-  params_or_args.push_back(entry_node_id);
+  AddNodeIdToBlock(params_or_args_stack_.PeekForAdd(), entry_node_id);
 }
 
 auto Context::CanonicalizeTypeImpl(
@@ -641,9 +662,9 @@ auto Context::CanonicalizeTypeImpl(
 }
 
 // Compute a fingerprint for a tuple type, for use as a key in a folding set.
-static auto ProfileTupleType(const llvm::SmallVector<SemIR::TypeId>& type_ids,
+static auto ProfileTupleType(llvm::ArrayRef<SemIR::TypeId> type_ids,
                              llvm::FoldingSetNodeID& canonical_id) -> void {
-  for (const auto& type_id : type_ids) {
+  for (auto type_id : type_ids) {
     canonical_id.AddInteger(type_id.index);
   }
 }
@@ -739,18 +760,16 @@ auto Context::CanonicalizeStructType(Parse::Node parse_node,
 }
 
 auto Context::CanonicalizeTupleType(Parse::Node parse_node,
-                                    llvm::SmallVector<SemIR::TypeId>&& type_ids)
+                                    llvm::ArrayRef<SemIR::TypeId> type_ids)
     -> SemIR::TypeId {
   // Defer allocating a SemIR::TypeBlockId until we know this is a new type.
   auto profile_tuple = [&](llvm::FoldingSetNodeID& canonical_id) {
     ProfileTupleType(type_ids, canonical_id);
   };
   auto make_tuple_node = [&] {
-    auto type_block_id = semantics_ir_->AddTypeBlock();
-    auto& type_block = semantics_ir_->GetTypeBlock(type_block_id);
-    type_block = std::move(type_ids);
-    return AddNode(SemIR::Node::TupleType::Make(
-        parse_node, SemIR::TypeId::TypeType, type_block_id));
+    return AddNode(
+        SemIR::Node::TupleType::Make(parse_node, SemIR::TypeId::TypeType,
+                                     semantics_ir_->AddTypeBlock(type_ids)));
   };
   return CanonicalizeTypeImpl(SemIR::NodeKind::TupleType, profile_tuple,
                               make_tuple_node);

toolchain/check/context.h

@@ -40,6 +40,10 @@ class Context {
   auto AddNodeToBlock(SemIR::NodeBlockId block, SemIR::Node node)
       -> SemIR::NodeId;
 
+  // Adds the ID of an existing node to the given block.
+  auto AddNodeIdToBlock(SemIR::NodeBlockId block, SemIR::NodeId node_id)
+      -> void;
+
   // Pushes a parse tree node onto the stack, storing the SemIR::Node as the
   // result.
   auto AddNodeAndPush(Parse::Node parse_node, SemIR::Node node) -> void;
@@ -148,7 +152,8 @@ class Context {
   auto ImplicitAsForArgs(Parse::Node call_parse_node,
                          SemIR::NodeBlockId arg_refs_id,
                          Parse::Node param_parse_node,
-                         SemIR::NodeBlockId param_refs_id) -> bool;
+                         SemIR::NodeBlockId param_refs_id, bool has_return_slot)
+      -> bool;
 
   // Canonicalizes a type which is tracked as a single node.
   // TODO: This should eventually return a type ID.
@@ -167,7 +172,7 @@ class Context {
   // Handles canonicalization of tuple types. This may create a new tuple type
   // if the `type_ids` doesn't match an existing tuple type.
   auto CanonicalizeTupleType(Parse::Node parse_node,
-                             llvm::SmallVector<SemIR::TypeId>&& type_ids)
+                             llvm::ArrayRef<SemIR::TypeId> type_ids)
       -> SemIR::TypeId;
 
   // Returns a pointer type whose pointee type is `pointee_type_id`.
@@ -200,9 +205,19 @@ class Context {
   // start_kind.
   auto ParamOrArgComma(bool for_args) -> void;
 
-  // Detects whether there's an entry to push. On return, the top of
-  // node_stack_ will be start_kind, and the caller should do type-specific
-  // processing. Returns refs_id.
+  // Detects whether there's an entry to push from the end of a parameter or
+  // argument list, and if so, moves it to the current parameter or argument
+  // list. Does not pop the list. `start_kind` is the node kind at the start
+  // of the parameter or argument list, and will be at the top of the parse node
+  // stack when this function returns.
+  auto ParamOrArgEndNoPop(bool for_args, Parse::NodeKind start_kind) -> void;
+
+  // Pops the current parameter or argument list. Should only be called after
+  // `ParamOrArgEndNoPop`.
+  auto ParamOrArgPop() -> SemIR::NodeBlockId;
+
+  // Detects whether there's an entry to push. Pops and returns the argument
+  // list. This is the same as `ParamOrArgEndNoPop` followed by `ParamOrArgPop`.
   auto ParamOrArgEnd(bool for_args, Parse::NodeKind start_kind)
       -> SemIR::NodeBlockId;
 

toolchain/check/handle_call_expression.cpp

@@ -8,7 +8,9 @@
 namespace Carbon::Check {
 
 auto HandleCallExpression(Context& context, Parse::Node parse_node) -> bool {
-  auto refs_id = context.ParamOrArgEnd(
+  // Process the final explicit call argument, but leave the arguments block on
+  // the stack until we add the return slot argument.
+  context.ParamOrArgEndNoPop(
       /*for_args=*/true, Parse::NodeKind::CallExpressionStart);
 
   // TODO: Convert to call expression.
@@ -20,19 +22,13 @@ auto HandleCallExpression(Context& context, Parse::Node parse_node) -> bool {
     // TODO: Work on error.
     context.TODO(parse_node, "Not a callable name");
     context.node_stack().Push(parse_node, name_id);
+    context.ParamOrArgPop();
     return true;
   }
 
   auto function_id = name_node.GetAsFunctionDeclaration();
   const auto& callable = context.semantics_ir().GetFunction(function_id);
 
-  if (!context.ImplicitAsForArgs(call_expr_parse_node, refs_id,
-                                 name_node.parse_node(),
-                                 callable.param_refs_id)) {
-    context.node_stack().Push(parse_node, SemIR::NodeId::BuiltinError);
-    return true;
-  }
-
   // For functions with an implicit return type, the return type is the empty
   // tuple type.
   SemIR::TypeId type_id = callable.return_type_id;
@@ -42,15 +38,26 @@ auto HandleCallExpression(Context& context, Parse::Node parse_node) -> bool {
 
   // If there is a return slot, add a corresponding argument.
   if (callable.return_slot_id.is_valid()) {
-    if (refs_id == SemIR::NodeBlockId::Empty) {
-      refs_id = context.semantics_ir().AddNodeBlock();
-    }
     // Tentatively put storage for a temporary in the function's return slot.
     // This will be replaced if necessary when we perform initialization.
-    auto return_slot_id = context.AddNode(SemIR::Node::TemporaryStorage::Make(
-        call_expr_parse_node, callable.return_type_id));
-    context.semantics_ir().GetNodeBlock(refs_id).push_back(return_slot_id);
+    context.AddNodeAndPush(parse_node,
+                           SemIR::Node::TemporaryStorage::Make(
+                               call_expr_parse_node, callable.return_type_id));
+    // TODO: We pass for_args=false here because we don't want a StubReference.
+    // We should remove the StubReferences for arguments and the corresponding
+    // `for_args` parameter. They didn't turn out to be used for anything.
+    context.ParamOrArgSave(/*for_args=*/false);
   }
+
+  // Convert the arguments to match the parameters.
+  auto refs_id = context.ParamOrArgPop();
+  if (!context.ImplicitAsForArgs(call_expr_parse_node, refs_id,
+                                 name_node.parse_node(), callable.param_refs_id,
+                                 callable.return_slot_id.is_valid())) {
+    context.node_stack().Push(parse_node, SemIR::NodeId::BuiltinError);
+    return true;
+  }
+
   auto call_node_id = context.AddNode(SemIR::Node::Call::Make(
       call_expr_parse_node, type_id, refs_id, function_id));
 

toolchain/check/testdata/function/call/fail_not_callable.carbon

@@ -0,0 +1,27 @@
+// 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
+//
+// AUTOUPDATE
+
+fn Run() {
+  // CHECK:STDERR: fail_not_callable.carbon:[[@LINE+6]]:24: Semantics TODO: `Not a callable name`.
+  // CHECK:STDERR:   var x: i32 = "hello"();
+  // CHECK:STDERR:                        ^
+  // CHECK:STDERR: fail_not_callable.carbon:[[@LINE+3]]:25: Cannot implicitly convert from `String` to `i32`.
+  // CHECK:STDERR:   var x: i32 = "hello"();
+  // CHECK:STDERR:                         ^
+  var x: i32 = "hello"();
+}
+
+// CHECK:STDOUT: file "fail_not_callable.carbon" {
+// CHECK:STDOUT:   %.loc7 = fn_decl @Run
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: fn @Run() {
+// CHECK:STDOUT: !entry:
+// CHECK:STDOUT:   %x: ref i32 = var "x"
+// CHECK:STDOUT:   %.loc14: String = string_literal "hello"
+// CHECK:STDOUT:   assign %x, <error>
+// CHECK:STDOUT:   return
+// CHECK:STDOUT: }

toolchain/check/testdata/function/call/fail_param_count.carbon

@@ -81,6 +81,7 @@ fn Main() {
 // CHECK:STDOUT: !entry:
 // CHECK:STDOUT:   %.loc18_8.1: i32 = int_literal 1
 // CHECK:STDOUT:   %.loc18_8.2: i32 = stub_reference %.loc18_8.1
+// CHECK:STDOUT:   %.loc18_7: type = tuple_type ()
 // CHECK:STDOUT:   %.loc25_8.1: i32 = int_literal 0
 // CHECK:STDOUT:   %.loc25_8.2: i32 = stub_reference %.loc25_8.1
 // CHECK:STDOUT:   %.loc25_11.1: i32 = int_literal 1

toolchain/check/testdata/function/call/fail_param_type.carbon

@@ -30,5 +30,6 @@ fn Main() {
 // CHECK:STDOUT: !entry:
 // CHECK:STDOUT:   %.loc16_7.1: f64 = real_literal 10e-1
 // CHECK:STDOUT:   %.loc16_7.2: f64 = stub_reference %.loc16_7.1
+// CHECK:STDOUT:   %.loc16_6: type = tuple_type ()
 // CHECK:STDOUT:   return
 // CHECK:STDOUT: }

toolchain/lower/file_context.cpp

@@ -59,7 +59,7 @@ auto FileContext::BuildFunctionDeclaration(SemIR::FunctionId function_id)
     -> llvm::Function* {
   const auto& function = semantics_ir().GetFunction(function_id);
   const bool has_return_slot = function.return_slot_id.is_valid();
-  auto& param_refs = semantics_ir().GetNodeBlock(function.param_refs_id);
+  auto param_refs = semantics_ir().GetNodeBlock(function.param_refs_id);
 
   SemIR::InitializingRepresentation return_rep =
       function.return_type_id.is_valid()
@@ -147,7 +147,7 @@ auto FileContext::BuildFunctionDefinition(SemIR::FunctionId function_id)
   // TODO: This duplicates the mapping between semantics nodes and LLVM
   // function parameters that was already computed in BuildFunctionDeclaration.
   // We should only do that once.
-  auto& param_refs = semantics_ir().GetNodeBlock(function.param_refs_id);
+  auto param_refs = semantics_ir().GetNodeBlock(function.param_refs_id);
   int param_index = 0;
   if (has_return_slot) {
     function_lowering.SetLocal(function.return_slot_id,
@@ -219,7 +219,7 @@ auto FileContext::BuildType(SemIR::NodeId node_id) -> llvm::Type* {
     case SemIR::NodeKind::PointerType:
       return llvm::PointerType::get(*llvm_context_, /*AddressSpace=*/0);
     case SemIR::NodeKind::StructType: {
-      auto& refs = semantics_ir_->GetNodeBlock(node.GetAsStructType());
+      auto refs = semantics_ir_->GetNodeBlock(node.GetAsStructType());
       llvm::SmallVector<llvm::Type*> subtypes;
       subtypes.reserve(refs.size());
       for (auto ref_id : refs) {
@@ -238,7 +238,7 @@ auto FileContext::BuildType(SemIR::NodeId node_id) -> llvm::Type* {
       // can be collectively replaced with LLVM's void, particularly around
       // function returns. LLVM doesn't allow declaring variables with a void
       // type, so that may require significant special casing.
-      auto& refs = semantics_ir_->GetTypeBlock(node.GetAsTupleType());
+      auto refs = semantics_ir_->GetTypeBlock(node.GetAsTupleType());
       llvm::SmallVector<llvm::Type*> subtypes;
       subtypes.reserve(refs.size());
       for (auto ref_id : refs) {

toolchain/sem_ir/file.h

@@ -152,10 +152,15 @@ class File : public Printable<File> {
   auto AddNode(NodeBlockId block_id, Node node) -> NodeId {
     NodeId node_id(nodes_.size());
     nodes_.push_back(node);
+    AddNodeId(block_id, node_id);
+    return node_id;
+  }
+
+  // Adds the ID of an existing node to the specified block.
+  auto AddNodeId(NodeBlockId block_id, NodeId node_id) -> void {
     if (block_id != NodeBlockId::Unreachable) {
       node_blocks_[block_id.index].push_back(node_id);
     }
-    return node_id;
   }
 
   // Overwrites a given node with a new value.
@@ -173,15 +178,22 @@ class File : public Printable<File> {
     return id;
   }
 
+  // Adds a node block with the given content, returning an ID to reference it.
+  auto AddNodeBlock(llvm::ArrayRef<NodeId> content) -> NodeBlockId {
+    NodeBlockId id(node_blocks_.size());
+    node_blocks_.push_back({});
+    node_blocks_.back().assign(content.begin(), content.end());
+    return id;
+  }
+
   // Returns the requested node block.
-  auto GetNodeBlock(NodeBlockId block_id) const
-      -> const llvm::SmallVector<NodeId>& {
+  auto GetNodeBlock(NodeBlockId block_id) const -> llvm::ArrayRef<NodeId> {
     CARBON_CHECK(block_id != NodeBlockId::Unreachable);
     return node_blocks_[block_id.index];
   }
 
   // Returns the requested node block.
-  auto GetNodeBlock(NodeBlockId block_id) -> llvm::SmallVector<NodeId>& {
+  auto GetNodeBlock(NodeBlockId block_id) -> llvm::MutableArrayRef<NodeId> {
     CARBON_CHECK(block_id != NodeBlockId::Unreachable);
     return node_blocks_[block_id.index];
   }
@@ -245,21 +257,21 @@ class File : public Printable<File> {
     }
   }
 
-  // Adds an empty type block, returning an ID to reference it.
-  auto AddTypeBlock() -> TypeBlockId {
+  // Adds a type block with the given content, returning an ID to reference it.
+  auto AddTypeBlock(llvm::ArrayRef<TypeId> content) -> TypeBlockId {
     TypeBlockId id(type_blocks_.size());
     type_blocks_.push_back({});
+    type_blocks_.back().assign(content.begin(), content.end());
     return id;
   }
 
   // Returns the requested type block.
-  auto GetTypeBlock(TypeBlockId block_id) const
-      -> const llvm::SmallVector<TypeId>& {
+  auto GetTypeBlock(TypeBlockId block_id) const -> llvm::ArrayRef<TypeId> {
     return type_blocks_[block_id.index];
   }
 
   // Returns the requested type block.
-  auto GetTypeBlock(TypeBlockId block_id) -> llvm::SmallVector<TypeId>& {
+  auto GetTypeBlock(TypeBlockId block_id) -> llvm::MutableArrayRef<TypeId> {
     return type_blocks_[block_id.index];
   }