Respond to reviewer comments

carbon-language · Mar 6, 2025 · f4732b8 · f4732b8
1 parent 281db83
commit f4732b8
Show file tree

Hide file tree

Showing 5 changed files with 84 additions and 40 deletions.
diff --git a/toolchain/check/handle_let_and_var.cpp b/toolchain/check/handle_let_and_var.cpp
@@ -103,10 +103,10 @@ auto HandleParseNode(Context& context, Parse::VariableIntroducerId node_id)
   return HandleIntroducer<Lex::TokenKind::Var>(context, node_id);
 }
 
-// Returns a VarStorage inst for the given pattern. If the pattern
+// Returns a VarStorage inst for the given `var` pattern. If the pattern
 // is the body of a returned var, this reuses the return slot, and otherwise it
 // adds a new inst.
-static auto GetOrAddStorage(Context& context, SemIR::InstId pattern_id)
+static auto GetOrAddStorage(Context& context, SemIR::InstId var_pattern_id)
     -> SemIR::InstId {
   if (context.decl_introducer_state_stack().innermost().modifier_set.HasAnyOf(
           KeywordModifierSet::Returned)) {
@@ -117,7 +117,7 @@ static auto GetOrAddStorage(Context& context, SemIR::InstId pattern_id)
       return GetCurrentReturnSlot(context);
     }
   }
-  auto pattern = context.insts().GetWithLocId(pattern_id);
+  auto pattern = context.insts().GetWithLocId(var_pattern_id);
 
   return AddInst(
       context,
@@ -139,17 +139,13 @@ auto HandleParseNode(Context& context, Parse::VariablePatternId node_id)
   // even if it contains multiple binding patterns.
   switch (context.full_pattern_stack().CurrentKind()) {
     case FullPatternStack::Kind::ExplicitParamList:
+    case FullPatternStack::Kind::ImplicitParamList:
       subpattern_id = AddPatternInst<SemIR::RefParamPattern>(
           context, node_id,
           {.type_id = type_id,
            .subpattern_id = subpattern_id,
            .index = SemIR::CallParamIndex::None});
       break;
-    case FullPatternStack::Kind::ImplicitParamList:
-      CARBON_DIAGNOSTIC(VarInImplicitParams, Error,
-                        "found `var` pattern in implicit parameter list");
-      context.emitter().Emit(node_id, VarInImplicitParams);
-      break;
     case FullPatternStack::Kind::NameBindingDecl:
       break;
   }

diff --git a/toolchain/check/pattern_match.cpp b/toolchain/check/pattern_match.cpp
@@ -262,13 +262,12 @@ auto MatchContext::DoEmitPatternMatch(Context& context,
                                       SemIR::ValueParamPattern param_pattern,
                                       SemIR::LocId pattern_loc_id,
                                       WorkItem entry) -> void {
-  CARBON_CHECK(
-      param_pattern.index.index < 0 ||
-          static_cast<size_t>(param_pattern.index.index) == results_.size(),
-      "Parameters out of order; expecting {0} but got {1}", results_.size(),
-      param_pattern.index.index);
   switch (kind_) {
     case MatchKind::Caller: {
+      CARBON_CHECK(
+          static_cast<size_t>(param_pattern.index.index) == results_.size(),
+          "Parameters out of order; expecting {0} but got {1}", results_.size(),
+          param_pattern.index.index);
       CARBON_CHECK(entry.scrutinee_id.has_value());
       if (entry.scrutinee_id == SemIR::ErrorInst::SingletonInstId) {
         results_.push_back(SemIR::ErrorInst::SingletonInstId);
@@ -284,10 +283,9 @@ auto MatchContext::DoEmitPatternMatch(Context& context,
       break;
     }
     case MatchKind::Callee: {
-      if (!param_pattern.index.has_value()) {
-        param_pattern.index = NextRuntimeIndex();
-        ReplaceInstBeforeConstantUse(context, entry.pattern_id, param_pattern);
-      }
+      CARBON_CHECK(!param_pattern.index.has_value());
+      param_pattern.index = NextRuntimeIndex();
+      ReplaceInstBeforeConstantUse(context, entry.pattern_id, param_pattern);
       auto param_id = AddInst<SemIR::ValueParam>(
           context, pattern_loc_id,
           {.type_id = param_pattern.type_id,
@@ -309,13 +307,12 @@ auto MatchContext::DoEmitPatternMatch(Context& context,
                                       SemIR::RefParamPattern param_pattern,
                                       SemIR::LocId pattern_loc_id,
                                       WorkItem entry) -> void {
-  CARBON_CHECK(
-      param_pattern.index.index < 0 ||
-          static_cast<size_t>(param_pattern.index.index) == results_.size(),
-      "Parameters out of order; expecting {0} but got {1}", results_.size(),
-      param_pattern.index.index);
   switch (kind_) {
     case MatchKind::Caller: {
+      CARBON_CHECK(
+          static_cast<size_t>(param_pattern.index.index) == results_.size(),
+          "Parameters out of order; expecting {0} but got {1}", results_.size(),
+          param_pattern.index.index);
       CARBON_CHECK(entry.scrutinee_id.has_value());
       auto expr_category =
           SemIR::GetExprCategory(context.sem_ir(), entry.scrutinee_id);
@@ -327,10 +324,9 @@ auto MatchContext::DoEmitPatternMatch(Context& context,
       break;
     }
     case MatchKind::Callee: {
-      if (param_pattern.index == SemIR::CallParamIndex::None) {
-        param_pattern.index = NextRuntimeIndex();
-        ReplaceInstBeforeConstantUse(context, entry.pattern_id, param_pattern);
-      }
+      CARBON_CHECK(!param_pattern.index.has_value());
+      param_pattern.index = NextRuntimeIndex();
+      ReplaceInstBeforeConstantUse(context, entry.pattern_id, param_pattern);
       auto param_id = AddInst<SemIR::RefParam>(
           context, pattern_loc_id,
           {.type_id = param_pattern.type_id,
@@ -354,6 +350,10 @@ auto MatchContext::DoEmitPatternMatch(Context& context,
                                       WorkItem entry) -> void {
   switch (kind_) {
     case MatchKind::Caller: {
+      CARBON_CHECK(
+          static_cast<size_t>(param_pattern.index.index) == results_.size(),
+          "Parameters out of order; expecting {0} but got {1}", results_.size(),
+          param_pattern.index.index);
       CARBON_CHECK(entry.scrutinee_id.has_value());
       CARBON_CHECK(context.insts().Get(entry.scrutinee_id).type_id() ==
                    SemIR::GetTypeInSpecific(context.sem_ir(),
@@ -367,10 +367,9 @@ auto MatchContext::DoEmitPatternMatch(Context& context,
     case MatchKind::Callee: {
       // TODO: Consider ways to address near-duplication with the
       // other ParamPattern cases.
-      if (!param_pattern.index.has_value()) {
-        param_pattern.index = NextRuntimeIndex();
-        ReplaceInstBeforeConstantUse(context, entry.pattern_id, param_pattern);
-      }
+      CARBON_CHECK(!param_pattern.index.has_value());
+      param_pattern.index = NextRuntimeIndex();
+      ReplaceInstBeforeConstantUse(context, entry.pattern_id, param_pattern);
       auto param_id = AddInst<SemIR::OutParam>(
           context, pattern_loc_id,
           {.type_id = param_pattern.type_id,

diff --git a/toolchain/check/testdata/var/no_prelude/var_pattern.carbon b/toolchain/check/testdata/var/no_prelude/var_pattern.carbon
@@ -68,16 +68,20 @@ fn f() {
 
 library "[[@TEST_NAME]]";
 
-// CHECK:STDERR: fail_implicit.carbon:[[@LINE+8]]:10: error: implicit parameters of functions must be constant or `self` [ImplictParamMustBeConstant]
+// CHECK:STDERR: fail_implicit.carbon:[[@LINE+4]]:10: error: implicit parameters of functions must be constant or `self` [ImplictParamMustBeConstant]
 // CHECK:STDERR: fn F[var u: ()]();
 // CHECK:STDERR:          ^~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_implicit.carbon:[[@LINE+4]]:6: error: found `var` pattern in implicit parameter list [VarInImplicitParams]
-// CHECK:STDERR: fn F[var u: ()]();
-// CHECK:STDERR:      ^~~~~~~~~
-// CHECK:STDERR:
 fn F[var u: ()]();
 
+// --- var_self.carbon
+
+library "[[@TEST_NAME]]";
+
+class C {
+  fn F[var self: Self]();
+}
+
 // CHECK:STDOUT: --- basic.carbon
 // CHECK:STDOUT:
 // CHECK:STDOUT: constants {
@@ -189,7 +193,7 @@ fn F[var u: ()]();
 // CHECK:STDOUT:       %.loc4_10.3: type = converted %.loc4_10.2, constants.%empty_tuple.type [concrete = constants.%empty_tuple.type]
 // CHECK:STDOUT:     }
 // CHECK:STDOUT:     %x: %empty_tuple.type = bind_name x, %x.param
-// CHECK:STDOUT:     %y.param: ref %empty_tuple.type = ref_param runtime_param1, y
+// CHECK:STDOUT:     %y.param: ref %empty_tuple.type = ref_param runtime_param1
 // CHECK:STDOUT:     %.loc4_21.1: type = splice_block %.loc4_21.3 [concrete = constants.%empty_tuple.type] {
 // CHECK:STDOUT:       %.loc4_21.2: %empty_tuple.type = tuple_literal ()
 // CHECK:STDOUT:       %.loc4_21.3: type = converted %.loc4_21.2, constants.%empty_tuple.type [concrete = constants.%empty_tuple.type]
@@ -322,9 +326,49 @@ fn F[var u: ()]();
 // CHECK:STDOUT:     .F = %F.decl
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:   %F.decl: %F.type = fn_decl @F [concrete = constants.%F] {
-// CHECK:STDOUT:     %.loc12: <error> = var_pattern <error>
-// CHECK:STDOUT:   } {}
+// CHECK:STDOUT:     %.param_patt: <error> = ref_param_pattern <error>, runtime_param0
+// CHECK:STDOUT:     %.loc8: <error> = var_pattern %.param_patt
+// CHECK:STDOUT:   } {
+// CHECK:STDOUT:     %.param: ref <error> = ref_param runtime_param0
+// CHECK:STDOUT:   }
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: fn @F[%.loc8: <error>]();
+// CHECK:STDOUT:
+// CHECK:STDOUT: --- var_self.carbon
+// CHECK:STDOUT:
+// CHECK:STDOUT: constants {
+// CHECK:STDOUT:   %C: type = class_type @C [concrete]
+// CHECK:STDOUT:   %F.type: type = fn_type @F [concrete]
+// CHECK:STDOUT:   %F: %F.type = struct_value () [concrete]
+// CHECK:STDOUT:   %empty_struct_type: type = struct_type {} [concrete]
+// CHECK:STDOUT:   %complete_type: <witness> = complete_type_witness %empty_struct_type [concrete]
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: file {
+// CHECK:STDOUT:   package: <namespace> = namespace [concrete] {
+// CHECK:STDOUT:     .C = %C.decl
+// CHECK:STDOUT:   }
+// CHECK:STDOUT:   %C.decl: type = class_decl @C [concrete = constants.%C] {} {}
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: class @C {
+// CHECK:STDOUT:   %F.decl: %F.type = fn_decl @F [concrete = constants.%F] {
+// CHECK:STDOUT:     %self.patt: %C = binding_pattern self
+// CHECK:STDOUT:     %self.param_patt: %C = ref_param_pattern %self.patt, runtime_param0
+// CHECK:STDOUT:     %.loc5: %C = var_pattern %self.param_patt
+// CHECK:STDOUT:   } {
+// CHECK:STDOUT:     %self.param: ref %C = ref_param runtime_param0
+// CHECK:STDOUT:     %Self.ref: type = name_ref Self, constants.%C [concrete = constants.%C]
+// CHECK:STDOUT:     %self: ref %C = bind_name self, %self.param
+// CHECK:STDOUT:   }
+// CHECK:STDOUT:   %complete_type: <witness> = complete_type_witness %empty_struct_type [concrete = constants.%complete_type]
+// CHECK:STDOUT:   complete_type_witness = %complete_type
+// CHECK:STDOUT:
+// CHECK:STDOUT: !members:
+// CHECK:STDOUT:   .Self = constants.%C
+// CHECK:STDOUT:   .F = %F.decl
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: fn @F[%.loc12: <error>]();
+// CHECK:STDOUT: fn @F[%.loc5: %C]();
 // CHECK:STDOUT:
diff --git a/toolchain/diagnostics/diagnostic_kind.def b/toolchain/diagnostics/diagnostic_kind.def
@@ -95,7 +95,6 @@ CARBON_DIAGNOSTIC_KIND(ExpectedChoiceAlternativeName)
 CARBON_DIAGNOSTIC_KIND(NestedVar)
 CARBON_DIAGNOSTIC_KIND(OperatorRequiresParentheses)
 CARBON_DIAGNOSTIC_KIND(StatementOperatorAsSubExpr)
-CARBON_DIAGNOSTIC_KIND(VarInImplicitParams)
 CARBON_DIAGNOSTIC_KIND(UnaryOperatorRequiresParentheses)
 CARBON_DIAGNOSTIC_KIND(UnaryOperatorHasWhitespace)
 CARBON_DIAGNOSTIC_KIND(UnaryOperatorRequiresWhitespace)

diff --git a/toolchain/sem_ir/formatter.cpp b/toolchain/sem_ir/formatter.cpp
@@ -1013,6 +1013,12 @@ class FormatterImpl {
     // pretty-printing.
   }
 
+  auto FormatInstRhs(RefParam inst) -> void {
+    FormatArgs(inst.index);
+    // Omit pretty_name because it's an implementation detail of
+    // pretty-printing.
+  }
+
   auto FormatInstRhs(OutParam inst) -> void {
     FormatArgs(inst.index);
     // Omit pretty_name because it's an implementation detail of