Clean up Context API (#4969)

- Add a better class comment.
- Remove the obsolete `type_ids_for_type_constants_` and `TypeNode`.
- For `bind_name_map`, fix declaration order and move comment to be
consistent with other members.
- Reorder accessors to better match order of data members.
  - This is how I noticed `type_ids_for_type_constants_`
- Put a bigger notice so that `sem_ir` helper functions don't end up in
the middle of other members again.

toolchain/check/context.cpp

@@ -62,15 +62,6 @@ Context::Context(DiagnosticEmitter* emitter,
   import_ir_constant_values_.reserve(imported_ir_count);
   check_ir_map_.resize(total_ir_count, SemIR::ImportIRId::None);
 
-  // Map the builtin `<error>` and `type` type constants to their corresponding
-  // special `TypeId` values.
-  type_ids_for_type_constants_.Insert(
-      SemIR::ConstantId::ForConcreteConstant(SemIR::ErrorInst::SingletonInstId),
-      SemIR::ErrorInst::SingletonTypeId);
-  type_ids_for_type_constants_.Insert(
-      SemIR::ConstantId::ForConcreteConstant(SemIR::TypeType::SingletonInstId),
-      SemIR::TypeType::SingletonTypeId);
-
   // TODO: Remove this and add a `VerifyOnFinish` once we properly push and pop
   // in the right places.
   generic_region_stack().Push();

toolchain/check/context.h

@@ -32,7 +32,19 @@
 
 namespace Carbon::Check {
 
-// Context and shared functionality for semantics handlers.
+// Context stored during check.
+//
+// This file stores state, and members objects may provide an API. Other files
+// may also have helpers that operate on Context. To keep this file manageable,
+// please put logic into other files.
+//
+// For example, consider the API for functions:
+// - `context.functions()`: Exposes storage of `SemIR::Function` objects.
+// - `toolchain/check/function.h`: Contains helper functions which use
+//   `Check::Context`.
+// - `toolchain/sem_ir/function.h`: Contains helper functions which only need
+//   `SemIR` objects, for which it's helpful not to depend on `Check::Context`
+//   (for example, shared with lowering).
 class Context {
  public:
   using DiagnosticEmitter = Carbon::DiagnosticEmitter<SemIRLoc>;
@@ -43,6 +55,14 @@ class Context {
   // add contextual notes as appropriate.
   using BuildDiagnosticFn = llvm::function_ref<auto()->DiagnosticBuilder>;
 
+  // Pre-computed parts of a binding pattern.
+  struct BindingPatternInfo {
+    // The corresponding AnyBindName inst.
+    SemIR::InstId bind_name_id;
+    // The region of insts that computes the type of the binding.
+    SemIR::ExprRegionId type_expr_region_id;
+  };
+
   // An ongoing impl lookup, used to ensure termination.
   struct ImplLookupStackEntry {
     SemIR::ConstantId type_const_id;
@@ -86,10 +106,10 @@ class Context {
   auto sem_ir() -> SemIR::File& { return *sem_ir_; }
   auto sem_ir() const -> const SemIR::File& { return *sem_ir_; }
 
+  // Convenience functions for major phase data.
   auto parse_tree() const -> const Parse::Tree& {
     return sem_ir_->parse_tree();
   }
-
   auto tokens() const -> const Lex::TokenizedBuffer& {
     return parse_tree().tokens();
   }
@@ -125,14 +145,17 @@ class Context {
 
   auto scope_stack() -> ScopeStack& { return scope_stack_; }
 
+  // Conveneicne functions for frequently-used `scope_stack` members.
   auto return_scope_stack() -> llvm::SmallVector<ScopeStack::ReturnScope>& {
     return scope_stack().return_scope_stack();
   }
-
   auto break_continue_stack()
       -> llvm::SmallVector<ScopeStack::BreakContinueScope>& {
     return scope_stack().break_continue_stack();
   }
+  auto full_pattern_stack() -> FullPatternStack& {
+    return scope_stack_.full_pattern_stack();
+  }
 
   auto generic_region_stack() -> GenericRegionStack& {
     return generic_region_stack_;
@@ -149,7 +172,35 @@ class Context {
     return import_ir_constant_values_;
   }
 
+  auto definitions_required() -> llvm::SmallVector<SemIR::InstId>& {
+    return definitions_required_;
+  }
+
+  auto global_init() -> GlobalInit& { return global_init_; }
+
+  auto import_ref_ids() -> llvm::SmallVector<SemIR::InstId>& {
+    return import_ref_ids_;
+  }
+
+  // TODO: Consider putting this behind a narrower API to guard against emitting
+  // multiple times.
+  auto bind_name_map() -> Map<SemIR::InstId, BindingPatternInfo>& {
+    return bind_name_map_;
+  }
+
+  auto var_storage_map() -> Map<SemIR::InstId, SemIR::InstId>& {
+    return var_storage_map_;
+  }
+
+  auto region_stack() -> RegionStack& { return region_stack_; }
+
+  auto impl_lookup_stack() -> llvm::SmallVector<ImplLookupStackEntry>& {
+    return impl_lookup_stack_;
+  }
+
+  // --------------------------------------------------------------------------
   // Directly expose SemIR::File data accessors for brevity in calls.
+  // --------------------------------------------------------------------------
 
   auto identifiers() -> SharedValueStores::IdentifierStore& {
     return sem_ir().identifiers();
@@ -207,59 +258,11 @@ class Context {
   }
   auto constants() -> SemIR::ConstantStore& { return sem_ir().constants(); }
 
-  auto definitions_required() -> llvm::SmallVector<SemIR::InstId>& {
-    return definitions_required_;
-  }
-
-  auto global_init() -> GlobalInit& { return global_init_; }
-
-  auto import_ref_ids() -> llvm::SmallVector<SemIR::InstId>& {
-    return import_ref_ids_;
-  }
-
-  // Map from an AnyBindingPattern inst to precomputed parts of the
-  // pattern-match SemIR for it.
-  //
-  // TODO: Consider putting this behind a narrower API to guard against emitting
-  // multiple times.
-  struct BindingPatternInfo {
-    // The corresponding AnyBindName inst.
-    SemIR::InstId bind_name_id;
-    // The region of insts that computes the type of the binding.
-    SemIR::ExprRegionId type_expr_region_id;
-  };
-  auto bind_name_map() -> Map<SemIR::InstId, BindingPatternInfo>& {
-    return bind_name_map_;
-  }
-
-  auto var_storage_map() -> Map<SemIR::InstId, SemIR::InstId>& {
-    return var_storage_map_;
-  }
-
-  auto region_stack() -> RegionStack& { return region_stack_; }
-
-  auto full_pattern_stack() -> FullPatternStack& {
-    return scope_stack_.full_pattern_stack();
-  }
-
-  auto impl_lookup_stack() -> llvm::SmallVector<ImplLookupStackEntry>& {
-    return impl_lookup_stack_;
-  }
+  // --------------------------------------------------------------------------
+  // End of SemIR::File members.
+  // --------------------------------------------------------------------------
 
  private:
-  // A FoldingSet node for a type.
-  class TypeNode : public llvm::FastFoldingSetNode {
-   public:
-    explicit TypeNode(const llvm::FoldingSetNodeID& node_id,
-                      SemIR::TypeId type_id)
-        : llvm::FastFoldingSetNode(node_id), type_id_(type_id) {}
-
-    auto type_id() -> SemIR::TypeId { return type_id_; }
-
-   private:
-    SemIR::TypeId type_id_;
-  };
-
   // Handles diagnostics.
   DiagnosticEmitter* emitter_;
 
@@ -313,16 +316,6 @@ class Context {
   // defined, regardless of whether the class can have virtual functions.
   InstBlockStack vtable_stack_;
 
-  // Cache of reverse mapping from type constants to types.
-  //
-  // TODO: Instead of mapping to a dense `TypeId` space, we could make `TypeId`
-  // be a thin wrapper around `ConstantId` and only perform the lookup only when
-  // we want to access the completeness and value representation of a type. It's
-  // not clear whether that would result in more or fewer lookups.
-  //
-  // TODO: Should this be part of the `TypeStore`?
-  Map<SemIR::ConstantId, SemIR::TypeId> type_ids_for_type_constants_;
-
   // The list which will form NodeBlockId::Exports.
   llvm::SmallVector<SemIR::InstId> exports_;
 
@@ -351,6 +344,8 @@ class Context {
   // FinalizeImportRefBlock() will produce an inst block for them.
   llvm::SmallVector<SemIR::InstId> import_ref_ids_;
 
+  // Map from an AnyBindingPattern inst to precomputed parts of the
+  // pattern-match SemIR for it.
   Map<SemIR::InstId, BindingPatternInfo> bind_name_map_;
 
   // Map from VarPattern insts to the corresponding VarStorage insts. The