Change from ToImplicit to AsDesugared (carbon-language#5591)

This changes `ToImplicit` to `AsDesugared`, and adds a
`GetLocIdForDesugaring` to `InstStore`.

In particular, I'm motivated by the latter, to make it clearer what the
intended call convention is.

Author: jonmeow

toolchain/check/dump.cpp

@@ -134,7 +134,7 @@ LLVM_DUMP_METHOD static auto Dump(const Context& context, SemIR::LocId loc_id)
       auto token = context.parse_tree().node_token(loc_id.node_id());
       auto line = context.tokens().GetLineNumber(token);
       auto col = context.tokens().GetColumnNumber(token);
-      const char* implicit = loc_id.is_implicit() ? " implicit" : "";
+      const char* implicit = loc_id.is_desugared() ? " implicit" : "";
       out << "LocId(" << FormatEscaped(context.sem_ir().filename()) << ":"
           << line << ":" << col << implicit << ")";
       break;

toolchain/check/impl_lookup.cpp

@@ -375,7 +375,7 @@ static auto GetOrAddLookupImplWitness(Context& context, SemIR::LocId loc_id,
                                       SemIR::SpecificInterface interface)
     -> SemIR::InstId {
   auto witness_const_id = EvalOrAddInst(
-      context, context.insts().GetCanonicalLocId(loc_id).ToImplicit(),
+      context, context.insts().GetLocIdForDesugaring(loc_id),
       SemIR::LookupImplWitness{
           .type_id = GetSingletonType(context, SemIR::WitnessType::TypeInstId),
           .query_self_inst_id =

toolchain/check/inst.cpp

@@ -124,9 +124,9 @@ auto GetOrAddInst(Context& context, SemIR::LocIdAndInst loc_id_and_inst)
     return context.constant_values().GetInstId(const_id);
   };
 
-  // If the instruction is implicit, produce its constant value instead if
-  // possible.
-  if (loc_id_and_inst.loc_id.is_implicit()) {
+  // If the instruction is from desugaring, produce its constant value instead
+  // if possible.
+  if (loc_id_and_inst.loc_id.is_desugared()) {
     switch (loc_id_and_inst.inst.kind().constant_needs_inst_id()) {
       case SemIR::InstConstantNeedsInstIdKind::No: {
         // Evaluation doesn't need an InstId. Just do it.

toolchain/check/operator.cpp

@@ -17,7 +17,7 @@ namespace Carbon::Check {
 // Returns the `Op` function for the specified operator.
 static auto GetOperatorOpFunction(Context& context, SemIR::LocId loc_id,
                                   Operator op) -> SemIR::InstId {
-  auto implicit_loc_id = context.insts().GetCanonicalLocId(loc_id).ToImplicit();
+  auto implicit_loc_id = context.insts().GetLocIdForDesugaring(loc_id);
 
   // Look up the interface, and pass it any generic arguments.
   auto interface_id =

toolchain/check/subst.cpp

@@ -393,8 +393,7 @@ class SubstConstantCallbacks final : public SubstInstCallbacks {
     auto const_id = EvalOrAddInst(
         context(),
         SemIR::LocIdAndInst::UncheckedLoc(
-            context().insts().GetCanonicalLocId(old_inst_id).ToImplicit(),
-            new_inst));
+            context().insts().GetLocIdForDesugaring(old_inst_id), new_inst));
     CARBON_CHECK(const_id.has_value(),
                  "Substitution into constant produced non-constant");
     CARBON_CHECK(const_id.is_constant(),

toolchain/sem_ir/ids.cpp

@@ -178,6 +178,9 @@ auto LocId::Print(llvm::raw_ostream& out) const -> void {
       break;
     case Kind::NodeId:
       out << Label << "_" << node_id();
+      if (is_desugared()) {
+        out << "_desugared";
+      }
       break;
   }
 }

toolchain/sem_ir/ids.h

@@ -860,14 +860,10 @@ struct ImportIRInstId : public IdBase<ImportIRInstId> {
 //   values to 30 bits.
 //   - [-2 - (1 << 24), -(1 << 30))
 //
-// In addition, one bit is used for flags: `ImplicitBit`.
+// In addition, one bit is used for flags: `DesugaredBit`.
 // Note that this can only be used with negative, non-`InstId` values.
 //
-// Use `InstStore::GetCanonicalLocId()` to get a canonical `LocId` which will
-// not be backed by an `InstId`. Note that the canonical `LocId` may be `None`
-// even when the original `LocId` was not, so this operation needs to be done
-// before checking `has_value()`. Only canonical locations can be converted with
-// `ToImplicit()`.
+// For desugaring, use `InstStore::GetLocIdForDesugaring()`.
 struct LocId : public IdBase<LocId> {
   // The contained index kind.
   enum class Kind {
@@ -896,16 +892,15 @@ struct LocId : public IdBase<LocId> {
   constexpr LocId(Parse::NodeId node_id)
       : IdBase(FirstNodeId - node_id.index) {}
 
-  // Forms an equivalent LocId for a desugared location. Requires a
-  // canonical location. See `InstStore::GetCanonicalLocId()`.
-  //
-  // TODO: Rename to something like `ToDesugared`.
-  auto ToImplicit() const -> LocId {
+  // Forms an equivalent LocId for a desugared location. Prefer calling
+  // `InstStore::GetLocIdForDesugaring`.
+  auto AsDesugared() const -> LocId {
     // This should only be called for NodeId or ImportIRInstId (i.e. canonical
     // locations), but we only set the flag for NodeId.
-    CARBON_CHECK(kind() != Kind::InstId);
+    CARBON_CHECK(kind() != Kind::InstId,
+                 "Use InstStore::GetLocIdForDesugaring");
     if (kind() == Kind::NodeId) {
-      return LocId(index & ~ImplicitBit);
+      return LocId(index & ~DesugaredBit);
     }
     return *this;
   }
@@ -926,8 +921,8 @@ struct LocId : public IdBase<LocId> {
 
   // Returns true if the location corresponds to desugared instructions.
   // Requires a non-`InstId` location.
-  auto is_implicit() const -> bool {
-    return (kind() == Kind::NodeId) && (index & ImplicitBit) == 0;
+  auto is_desugared() const -> bool {
+    return (kind() == Kind::NodeId) && (index & DesugaredBit) == 0;
   }
 
   // Returns the equivalent `ImportIRInstId` when `kind()` matches or is `None`.
@@ -962,7 +957,7 @@ struct LocId : public IdBase<LocId> {
  private:
   // Whether a location corresponds to desugared instructions. This only applies
   // for `NodeId`.
-  static constexpr int32_t ImplicitBit = 1 << 30;
+  static constexpr int32_t DesugaredBit = 1 << 30;
 
   // The value of the 0 index for each of `NodeId` and `ImportIRInstId`.
   static constexpr int32_t FirstNodeId = NoneIndex - 1;
@@ -971,7 +966,7 @@ struct LocId : public IdBase<LocId> {
 
   auto index_without_flags() const -> int32_t {
     CARBON_DCHECK(index < NoneIndex, "Only for NodeId and ImportIRInstId");
-    return index | ImplicitBit;
+    return index | DesugaredBit;
   }
 };
 

toolchain/sem_ir/ids_test.cpp

@@ -33,29 +33,29 @@ TEST(IdsTest, LocIdValues) {
               Eq(-(1 << 30) + 1));
 }
 
-// A standard parameterized test for (implicit, index).
+// A standard parameterized test for (is_desugared, index).
 class IdsTestWithParam
     : public testing::TestWithParam<std::tuple<bool, int32_t>> {
  public:
   explicit IdsTestWithParam() {
-    llvm::errs() << "implicit=" << is_implicit()
-                 << ", index=" << std::get<1>(GetParam()) << "\n";
+    llvm::errs() << "is_desugared=" << is_desugared() << ", index=" << index()
+                 << "\n";
   }
 
   // Returns IdT with its matching LocId form. Sets flags based on test
   // parameters.
   template <typename IdT>
   auto BuildIdAndLocId() -> std::pair<IdT, LocId> {
-    auto [implicit, index] = GetParam();
-    IdT id(index);
-    LocId loc_id(id);
-    if (implicit) {
-      loc_id = loc_id.ToImplicit();
+    IdT id(index());
+    if (is_desugared()) {
+      return {id, LocId(id).AsDesugared()};
+    } else {
+      return {id, LocId(id)};
     }
-    return {id, loc_id};
   }
 
-  auto is_implicit() -> bool { return std::get<0>(GetParam()); }
+  auto is_desugared() -> bool { return std::get<0>(GetParam()); }
+  auto index() -> int32_t { return std::get<1>(GetParam()); }
 };
 
 // Returns a test case generator for edge-case values.
@@ -78,7 +78,7 @@ TEST_P(LocIdAsNoneTestWithParam, Test) {
   auto [_, loc_id] = BuildIdAndLocId<Parse::NodeId>();
   EXPECT_FALSE(loc_id.has_value());
   EXPECT_THAT(loc_id.kind(), Eq(LocId::Kind::None));
-  EXPECT_FALSE(loc_id.is_implicit());
+  EXPECT_FALSE(loc_id.is_desugared());
   EXPECT_THAT(loc_id.import_ir_inst_id(), Eq(ImportIRInstId::None));
   EXPECT_THAT(loc_id.inst_id(), Eq(InstId::None));
   EXPECT_THAT(loc_id.node_id(),
@@ -96,7 +96,7 @@ TEST_P(LocIdAsImportIRInstIdTest, Test) {
   EXPECT_TRUE(loc_id.has_value());
   ASSERT_THAT(loc_id.kind(), Eq(LocId::Kind::ImportIRInstId));
   EXPECT_THAT(loc_id.import_ir_inst_id(), import_ir_inst_id);
-  EXPECT_FALSE(loc_id.is_implicit());
+  EXPECT_FALSE(loc_id.is_desugared());
 }
 
 class LocIdAsInstIdTest : public IdsTestWithParam {};
@@ -111,7 +111,7 @@ TEST_P(LocIdAsInstIdTest, Test) {
   EXPECT_TRUE(loc_id.has_value());
   ASSERT_THAT(loc_id.kind(), Eq(LocId::Kind::InstId));
   EXPECT_THAT(loc_id.inst_id(), inst_id);
-  // Note that `is_implicit` is invalid to use with `InstId`.
+  // Note that `is_desugared` is invalid to use with `InstId`.
 }
 
 class LocIdAsNodeIdTest : public IdsTestWithParam {};
@@ -124,7 +124,7 @@ TEST_P(LocIdAsNodeIdTest, Test) {
   EXPECT_TRUE(loc_id.has_value());
   ASSERT_THAT(loc_id.kind(), Eq(LocId::Kind::NodeId));
   EXPECT_THAT(loc_id.node_id(), node_id);
-  EXPECT_THAT(loc_id.is_implicit(), Eq(is_implicit()));
+  EXPECT_THAT(loc_id.is_desugared(), Eq(is_desugared()));
 }
 
 }  // namespace

toolchain/sem_ir/inst.h

@@ -493,6 +493,15 @@ class InstStore {
     return GetCanonicalLocId(GetNonCanonicalLocId(inst_id));
   }
 
+  // Returns a virtual location to use for the desugaring of the code at the
+  // specified location.
+  auto GetLocIdForDesugaring(LocId loc_id) const -> LocId {
+    return GetCanonicalLocId(loc_id).AsDesugared();
+  }
+  auto GetLocIdForDesugaring(InstId inst_id) const -> LocId {
+    return GetCanonicalLocId(inst_id).AsDesugared();
+  }
+
   // Returns the instruction that this instruction was imported from, or
   // ImportIRInstId::None if this instruction was not generated by importing
   // another instruction.