Disallow compile time bindings where they aren't clearly supported. (#…

…4338)

This is resolving a fuzz-discovered crash related to function suspends
and compile time bind indices. Although the crash originally came from
clearly invalid syntax (missing the `=` inside a `class` decl), the
syntax with a value should also be valid but has the same crash.

This approach disallows compile-time bindings in contexts that can
create ambiguous results, particularly class declarations. These are an
issue because a suspended function can have let declarations after it.
I'm allowing them in function bodies and interface scopes.

---------

Co-authored-by: Richard Smith <[email protected]>

toolchain/check/handle_binding_pattern.cpp

@@ -32,6 +32,8 @@ static auto HandleAnyBindingPattern(Context& context, Parse::NodeId node_id,
         inst_id.is_valid() && context.insts().Is<SemIR::InterfaceDecl>(inst_id);
   }
 
+  bool needs_compile_time_binding = is_generic && !is_associated_constant;
+
   // Create the appropriate kind of binding for this pattern.
   auto make_bind_name = [&](SemIR::TypeId type_id,
                             SemIR::InstId value_id) -> SemIR::LocIdAndInst {
@@ -42,7 +44,7 @@ static auto HandleAnyBindingPattern(Context& context, Parse::NodeId node_id,
          .parent_scope_id = context.scope_stack().PeekNameScopeId(),
          // TODO: Don't allocate a compile-time binding index for an associated
          // constant declaration.
-         .bind_index = is_generic && !is_associated_constant
+         .bind_index = needs_compile_time_binding
                            ? context.scope_stack().AddCompileTimeBinding()
                            : SemIR::CompileTimeBindIndex::Invalid});
     if (is_generic) {
@@ -63,7 +65,7 @@ static auto HandleAnyBindingPattern(Context& context, Parse::NodeId node_id,
   // stack.
   auto push_bind_name = [&](SemIR::InstId bind_id) {
     context.node_stack().Push(node_id, bind_id);
-    if (is_generic && !is_associated_constant) {
+    if (needs_compile_time_binding) {
       context.scope_stack().PushCompileTimeBinding(bind_id);
     }
   };
@@ -200,7 +202,19 @@ auto HandleParseNode(Context& context, Parse::BindingPatternId node_id)
 
 auto HandleParseNode(Context& context,
                      Parse::CompileTimeBindingPatternId node_id) -> bool {
-  return HandleAnyBindingPattern(context, node_id, /*is_generic=*/true);
+  bool is_generic = true;
+  if (context.decl_introducer_state_stack().innermost().kind ==
+      Lex::TokenKind::Let) {
+    auto scope_inst = context.insts().Get(context.scope_stack().PeekInstId());
+    if (!scope_inst.Is<SemIR::InterfaceDecl>() &&
+        !scope_inst.Is<SemIR::FunctionDecl>()) {
+      context.TODO(node_id,
+                   "`let` compile time binding outside function or interface");
+      is_generic = false;
+    }
+  }
+
+  return HandleAnyBindingPattern(context, node_id, is_generic);
 }
 
 auto HandleParseNode(Context& context, Parse::AddrId node_id) -> bool {

toolchain/check/scope_stack.cpp

@@ -13,6 +13,18 @@ auto ScopeStack::VerifyOnFinish() -> void {
   CARBON_CHECK(scope_stack_.empty(), "{0}", scope_stack_.size());
 }
 
+auto ScopeStack::VerifyNextCompileTimeBindIndex(llvm::StringLiteral label,
+                                                const ScopeStackEntry& scope)
+    -> void {
+  CARBON_CHECK(
+      static_cast<int32_t>(compile_time_binding_stack_.all_values_size()) ==
+          scope.next_compile_time_bind_index.index,
+      "Wrong number of entries in compile-time binding stack after {0}: have "
+      "{1}, expected {2}",
+      label, compile_time_binding_stack_.all_values_size(),
+      scope.next_compile_time_bind_index.index);
+}
+
 auto ScopeStack::Push(SemIR::InstId scope_inst_id, SemIR::NameScopeId scope_id,
                       SemIR::SpecificId specific_id,
                       bool lexical_lookup_has_load_error) -> void {
@@ -49,6 +61,8 @@ auto ScopeStack::Push(SemIR::InstId scope_inst_id, SemIR::NameScopeId scope_id,
   CARBON_CHECK(next_scope_index_.index != std::numeric_limits<int32_t>::max(),
                "Ran out of scopes");
   ++next_scope_index_.index;
+
+  VerifyNextCompileTimeBindIndex("Push", scope_stack_.back());
 }
 
 auto ScopeStack::Pop() -> void {
@@ -72,13 +86,7 @@ auto ScopeStack::Pop() -> void {
     return_scope_stack_.back().returned_var = SemIR::InstId::Invalid;
   }
 
-  CARBON_CHECK(
-      scope.next_compile_time_bind_index.index ==
-          static_cast<int32_t>(compile_time_binding_stack_.all_values_size()),
-      "Wrong number of entries in compile-time binding stack, have {0}, "
-      "expected {1}",
-      compile_time_binding_stack_.all_values_size(),
-      scope.next_compile_time_bind_index.index);
+  VerifyNextCompileTimeBindIndex("Pop", scope);
   compile_time_binding_stack_.PopArray();
 }
 
@@ -216,13 +224,7 @@ auto ScopeStack::Restore(SuspendedScope scope) -> void {
     }
   }
 
-  CARBON_CHECK(
-      scope.entry.next_compile_time_bind_index.index ==
-          static_cast<int32_t>(compile_time_binding_stack_.all_values_size()),
-      "Wrong number of entries in compile-time binding stack when restoring, "
-      "have {0}, expected {1}",
-      compile_time_binding_stack_.all_values_size(),
-      scope.entry.next_compile_time_bind_index.index);
+  VerifyNextCompileTimeBindIndex("Restore", scope.entry);
 
   if (scope.entry.scope_id.is_valid()) {
     non_lexical_scope_stack_.push_back(

toolchain/check/scope_stack.h

@@ -212,6 +212,13 @@ class ScopeStack {
            scope_stack_.back().lexical_lookup_has_load_error;
   }
 
+  // Checks that the provided scope's `next_compile_time_bind_index` matches the
+  // full size of the current `compile_time_binding_stack_`. The values should
+  // always match, and this is used to validate the correspondence during
+  // significant changes.
+  auto VerifyNextCompileTimeBindIndex(llvm::StringLiteral label,
+                                      const ScopeStackEntry& scope) -> void;
+
   // A stack of scopes from which we can `return`.
   llvm::SmallVector<ReturnScope> return_scope_stack_;