Move TokenOnly to LocIdForDiagnostics (carbon-language#5590)

This reclaims a bit inside `LocId`. I'm hopeful we don't actually need
to store the token-only state.

Note I'm looking at this in the context of desugaring; I was thinking
about changing `ToImplicit` logic a little to push more towards
`GetCanonicalLocId`, and the "desugaring" TODO there. Removing
`ToTokenOnly` makes me feel a little more free to rename `ToImplicit`,
since it eliminates consistency as a question.

Author: jonmeow

toolchain/check/diagnostic_emitter.cpp

@@ -19,7 +19,7 @@ auto DiagnosticEmitter::ConvertLoc(LocIdForDiagnostics loc_id,
                                    ContextFnT context_fn) const
     -> Diagnostics::ConvertedLoc {
   auto converted =
-      ConvertLocImpl(static_cast<SemIR::LocId>(loc_id), context_fn);
+      ConvertLocImpl(loc_id.loc_id(), loc_id.is_token_only(), context_fn);
 
   // Use the token when possible, but -1 is the default value.
   auto last_offset = -1;
@@ -39,13 +39,11 @@ auto DiagnosticEmitter::ConvertLoc(LocIdForDiagnostics loc_id,
   return converted;
 }
 
-auto DiagnosticEmitter::ConvertLocImpl(SemIR::LocId loc_id,
+auto DiagnosticEmitter::ConvertLocImpl(SemIR::LocId loc_id, bool is_token_only,
                                        ContextFnT context_fn) const
     -> Diagnostics::ConvertedLoc {
   llvm::SmallVector<SemIR::AbsoluteNodeId> absolute_node_ids =
       SemIR::GetAbsoluteNodeId(sem_ir_, loc_id);
-  bool token_only =
-      loc_id.kind() != SemIR::LocId::Kind::InstId && loc_id.is_token_only();
 
   auto final_node_id = absolute_node_ids.pop_back_val();
   for (const auto& absolute_node_id : absolute_node_ids) {
@@ -55,12 +53,13 @@ auto DiagnosticEmitter::ConvertLocImpl(SemIR::LocId loc_id,
       continue;
     }
     // TODO: Include the name of the imported library in the diagnostic.
-    auto diag_loc = ConvertLocInFile(absolute_node_id, token_only, context_fn);
+    auto diag_loc =
+        ConvertLocInFile(absolute_node_id, is_token_only, context_fn);
     CARBON_DIAGNOSTIC(InImport, LocationInfo, "in import");
     context_fn(diag_loc.loc, InImport);
   }
 
-  return ConvertLocInFile(final_node_id, token_only, context_fn);
+  return ConvertLocInFile(final_node_id, is_token_only, context_fn);
 }
 
 auto DiagnosticEmitter::ConvertLocInFile(SemIR::AbsoluteNodeId absolute_node_id,

toolchain/check/diagnostic_emitter.h

@@ -48,8 +48,8 @@ class DiagnosticEmitter : public DiagnosticEmitterBase {
 
  private:
   // Implements `ConvertLoc`, but without `last_token_` applied.
-  auto ConvertLocImpl(SemIR::LocId loc_id, ContextFnT context_fn) const
-      -> Diagnostics::ConvertedLoc;
+  auto ConvertLocImpl(SemIR::LocId loc_id, bool is_token_only,
+                      ContextFnT context_fn) const -> Diagnostics::ConvertedLoc;
 
   // Converts a node_id corresponding to a specific sem_ir to a diagnostic
   // location.

toolchain/check/diagnostic_helpers.h

@@ -18,15 +18,30 @@ namespace Carbon::Check {
 // explicitly construct a `LocId` from it first.
 class LocIdForDiagnostics {
  public:
+  // Constructs a token-only location for a diagnostic.
+  //
+  // This means the displayed location will include only the location's specific
+  // parse node, instead of also including its descendants.
+  static auto TokenOnly(Parse::NodeId node_id) -> LocIdForDiagnostics {
+    return LocIdForDiagnostics(SemIR::LocId(node_id), true);
+  }
+
   template <class LocT>
     requires std::constructible_from<SemIR::LocId, LocT>
   // NOLINTNEXTLINE(google-explicit-constructor)
-  LocIdForDiagnostics(LocT loc_id) : loc_id_(SemIR::LocId(loc_id)) {}
+  LocIdForDiagnostics(LocT loc_id)
+      : LocIdForDiagnostics(SemIR::LocId(loc_id), false) {}
+
+  auto loc_id() const -> SemIR::LocId { return loc_id_; }
 
-  explicit operator SemIR::LocId() const { return loc_id_; }
+  auto is_token_only() const -> bool { return is_token_only_; }
 
  private:
+  explicit LocIdForDiagnostics(SemIR::LocId loc_id, bool is_token_only)
+      : loc_id_(loc_id), is_token_only_(is_token_only) {}
+
   SemIR::LocId loc_id_;
+  bool is_token_only_;
 };
 
 // We define the emitter separately for dependencies, so only provide a base

toolchain/check/handle_binding_pattern.cpp

@@ -369,7 +369,7 @@ auto HandleParseNode(Context& context, Parse::AddrId node_id) -> bool {
   } else {
     CARBON_DIAGNOSTIC(AddrOnNonSelfParam, Error,
                       "`addr` can only be applied to a `self` parameter");
-    context.emitter().Emit(SemIR::LocId(node_id).ToTokenOnly(),
+    context.emitter().Emit(LocIdForDiagnostics::TokenOnly(node_id),
                            AddrOnNonSelfParam);
     context.node_stack().Push(node_id, param_pattern_id);
   }

toolchain/check/handle_function.cpp

@@ -541,7 +541,7 @@ static auto BuildFunctionDecl(Context& context,
           SemIR::Function::VirtualModifier::Abstract) {
     CARBON_DIAGNOSTIC(DefinedAbstractFunction, Error,
                       "definition of `abstract` function");
-    context.emitter().Emit(SemIR::LocId(node_id).ToTokenOnly(),
+    context.emitter().Emit(LocIdForDiagnostics::TokenOnly(node_id),
                            DefinedAbstractFunction);
   }
 
@@ -626,7 +626,7 @@ auto HandleParseNode(Context& context, Parse::FunctionDefinitionId node_id)
       CARBON_DIAGNOSTIC(
           MissingReturnStatement, Error,
           "missing `return` at end of function with declared return type");
-      context.emitter().Emit(SemIR::LocId(node_id).ToTokenOnly(),
+      context.emitter().Emit(LocIdForDiagnostics::TokenOnly(node_id),
                              MissingReturnStatement);
     } else {
       AddReturnCleanupBlock(context, node_id);

toolchain/check/handle_let_and_var.cpp

@@ -369,7 +369,7 @@ auto HandleParseNode(Context& context, Parse::LetDeclId node_id) -> bool {
     CARBON_DIAGNOSTIC(
         ExpectedInitializerAfterLet, Error,
         "expected `=`; `let` declaration must have an initializer");
-    context.emitter().Emit(SemIR::LocId(node_id).ToTokenOnly(),
+    context.emitter().Emit(LocIdForDiagnostics::TokenOnly(node_id),
                            ExpectedInitializerAfterLet);
   }
   return true;

toolchain/check/handle_name.cpp

@@ -55,7 +55,7 @@ auto HandleParseNode(Context& context, Parse::PointerMemberAccessExprId node_id)
                       SemIR::TypeId);
 
     auto builder =
-        context.emitter().Build(SemIR::LocId(node_id).ToTokenOnly(),
+        context.emitter().Build(LocIdForDiagnostics::TokenOnly(node_id),
                                 ArrowOperatorOfNonPointer, not_pointer_type_id);
     builder.Emit();
   };

toolchain/check/handle_operator.cpp

@@ -237,14 +237,15 @@ auto HandleParseNode(Context& context, Parse::PrefixOperatorAmpId node_id)
     case SemIR::ExprCategory::EphemeralRef:
       CARBON_DIAGNOSTIC(AddrOfEphemeralRef, Error,
                         "cannot take the address of a temporary object");
-      context.emitter().Emit(SemIR::LocId(node_id).ToTokenOnly(),
+      context.emitter().Emit(LocIdForDiagnostics::TokenOnly(node_id),
                              AddrOfEphemeralRef);
       value_id = SemIR::ErrorInst::InstId;
       break;
     default:
       CARBON_DIAGNOSTIC(AddrOfNonRef, Error,
                         "cannot take the address of non-reference expression");
-      context.emitter().Emit(SemIR::LocId(node_id).ToTokenOnly(), AddrOfNonRef);
+      context.emitter().Emit(LocIdForDiagnostics::TokenOnly(node_id),
+                             AddrOfNonRef);
       value_id = SemIR::ErrorInst::InstId;
       break;
   }
@@ -327,15 +328,15 @@ auto HandleParseNode(Context& context, Parse::PrefixOperatorStarId node_id)
                           SemIR::TypeId);
 
         auto builder =
-            context.emitter().Build(SemIR::LocId(node_id).ToTokenOnly(),
+            context.emitter().Build(LocIdForDiagnostics::TokenOnly(node_id),
                                     DerefOfNonPointer, not_pointer_type_id);
 
         // TODO: Check for any facet here, rather than only a type.
         if (not_pointer_type_id == SemIR::TypeType::TypeId) {
           CARBON_DIAGNOSTIC(
               DerefOfType, Note,
               "to form a pointer type, write the `*` after the pointee type");
-          builder.Note(SemIR::LocId(node_id).ToTokenOnly(), DerefOfType);
+          builder.Note(LocIdForDiagnostics::TokenOnly(node_id), DerefOfType);
         }
 
         builder.Emit();

toolchain/sem_ir/ids.h

@@ -835,7 +835,7 @@ struct ImportIRInstId : public IdBase<ImportIRInstId> {
   using ValueType = ImportIRInst;
 
   // ImportIRInstId is restricted so that it can fit into LocId.
-  static constexpr int32_t BitsWithNodeId = 29;
+  static constexpr int BitsWithNodeId = 30;
 
   // The maximum ID, non-inclusive.
   static constexpr int Max = (1 << BitsWithNodeId) - Parse::NodeId::Max - 2;
@@ -857,17 +857,17 @@ struct ImportIRInstId : public IdBase<ImportIRInstId> {
 // - NodeId: negative values starting after None; the 24 bit NodeId range.
 //   - [-2, -2 - (1 << 24))
 // - ImportIRInstId: remaining negative values; after NodeId, fills out negative
-//   values to 29 bits.
-//   - [-2 - (1 << 24), -(1 << 29))
+//   values to 30 bits.
+//   - [-2 - (1 << 24), -(1 << 30))
 //
-// In addition, two bits are used for flags: `ImplicitBit` and `TokenOnlyBit`.
-// Note that these can only be used with negative, non-`InstId` values.
+// In addition, one bit is used for flags: `ImplicitBit`.
+// 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()` or `ToTokenOnly()`.
+// `ToImplicit()`.
 struct LocId : public IdBase<LocId> {
   // The contained index kind.
   enum class Kind {
@@ -910,20 +910,6 @@ struct LocId : public IdBase<LocId> {
     return *this;
   }
 
-  // Forms an equivalent `LocId` for a token-only diagnostic location. Requires
-  // a canonical location. See `InstStore::GetCanonicalLocId()`.
-  //
-  // TODO: Consider making this a part of check/ diagnostics instead, as a free
-  // function operation on `LocIdForDiagnostics`?
-  // https://github.com/carbon-language/carbon-lang/pull/5355#discussion_r2064113186
-  auto ToTokenOnly() const -> LocId {
-    CARBON_CHECK(kind() != Kind::InstId);
-    if (has_value()) {
-      return LocId(index & ~TokenOnlyBit);
-    }
-    return *this;
-  }
-
   // Returns the kind of the `LocId`.
   auto kind() const -> Kind {
     if (!has_value()) {
@@ -944,15 +930,6 @@ struct LocId : public IdBase<LocId> {
     return (kind() == Kind::NodeId) && (index & ImplicitBit) == 0;
   }
 
-  // Returns true if the location is token-only for diagnostics.
-  //
-  // This means the displayed location will include only the location's specific
-  // parse node, instead of also including its descendants. As such, this can
-  // only be true for locations backed by a `NodeId`.
-  auto is_token_only() const -> bool {
-    return kind() != Kind::InstId && (index & TokenOnlyBit) == 0;
-  }
-
   // Returns the equivalent `ImportIRInstId` when `kind()` matches or is `None`.
   // Note that the returned `ImportIRInstId` only identifies a location; it is
   // not correct to interpret it as the instruction from which another
@@ -987,18 +964,14 @@ struct LocId : public IdBase<LocId> {
   // for `NodeId`.
   static constexpr int32_t ImplicitBit = 1 << 30;
 
-  // See `is_token_only` for the use. This only applies for canonical locations
-  // (i.e. those containing `NodeId` or `ImportIRInstId`).
-  static constexpr int32_t TokenOnlyBit = 1 << 29;
-
   // The value of the 0 index for each of `NodeId` and `ImportIRInstId`.
   static constexpr int32_t FirstNodeId = NoneIndex - 1;
   static constexpr int32_t FirstImportIRInstId =
       FirstNodeId - Parse::NodeId::Max;
 
   auto index_without_flags() const -> int32_t {
     CARBON_DCHECK(index < NoneIndex, "Only for NodeId and ImportIRInstId");
-    return index | ImplicitBit | TokenOnlyBit;
+    return index | ImplicitBit;
   }
 };
 

toolchain/sem_ir/ids_test.cpp

@@ -30,37 +30,32 @@ TEST(IdsTest, LocIdValues) {
 
   EXPECT_THAT(static_cast<LocId>(ImportIRInstId(0)).index, Eq(-2 - (1 << 24)));
   EXPECT_THAT(static_cast<LocId>(ImportIRInstId(ImportIRInstId::Max - 1)).index,
-              Eq(-(1 << 29) + 1));
+              Eq(-(1 << 30) + 1));
 }
 
-// A standard parameterized test for (implicit, token_only, index).
+// A standard parameterized test for (implicit, index).
 class IdsTestWithParam
-    : public testing::TestWithParam<std::tuple<bool, bool, int32_t>> {
+    : public testing::TestWithParam<std::tuple<bool, int32_t>> {
  public:
   explicit IdsTestWithParam() {
     llvm::errs() << "implicit=" << is_implicit()
-                 << ", token_only=" << is_token_only()
-                 << ", index=" << std::get<2>(GetParam()) << "\n";
+                 << ", index=" << std::get<1>(GetParam()) << "\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, token_only, index] = GetParam();
+    auto [implicit, index] = GetParam();
     IdT id(index);
     LocId loc_id(id);
     if (implicit) {
       loc_id = loc_id.ToImplicit();
     }
-    if (token_only) {
-      loc_id = loc_id.ToTokenOnly();
-    }
     return {id, loc_id};
   }
 
   auto is_implicit() -> bool { return std::get<0>(GetParam()); }
-  auto is_token_only() -> bool { return std::get<1>(GetParam()); }
 };
 
 // Returns a test case generator for edge-case values.
@@ -70,7 +65,7 @@ static auto GetValueRange(int32_t max) -> auto {
 
 // Returns a test case generator for `IdsTestWithParam` uses.
 static auto CombineWithFlags(auto value_range) -> auto {
-  return testing::Combine(testing::Bool(), testing::Bool(), value_range);
+  return testing::Combine(testing::Bool(), value_range);
 }
 
 class LocIdAsNoneTestWithParam : public IdsTestWithParam {};
@@ -102,23 +97,21 @@ TEST_P(LocIdAsImportIRInstIdTest, Test) {
   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_THAT(loc_id.is_token_only(), Eq(is_token_only()));
 }
 
 class LocIdAsInstIdTest : public IdsTestWithParam {};
 
 INSTANTIATE_TEST_SUITE_P(
     Test, LocIdAsInstIdTest,
-    testing::Combine(testing::Values(false), testing::Values(false),
+    testing::Combine(testing::Values(false),
                      GetValueRange(std::numeric_limits<int32_t>::max())));
 
 TEST_P(LocIdAsInstIdTest, Test) {
   auto [inst_id, loc_id] = BuildIdAndLocId<InstId>();
   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` and `is_token_only` are invalid to use with
-  // `InstId`.
+  // Note that `is_implicit` is invalid to use with `InstId`.
 }
 
 class LocIdAsNodeIdTest : public IdsTestWithParam {};
@@ -132,7 +125,6 @@ TEST_P(LocIdAsNodeIdTest, Test) {
   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_token_only(), Eq(is_token_only()));
 }
 
 }  // namespace