Make SemIRLoc data private to diagnostics (#4867)

I think the name of `SemIRLoc` might be leading to a few suggestions to
use it for non-diagnostic purposes that I've been responding to. At the
same time, a short name seems desirable given its frequency of use for
diagnostics. I did also clean up misuse in #4857, and eval.cpp (noted
below) might be similar. We don't typically use `friend` to emphasize
relationships, but given the diagnostic-specific intent and ways it's
been used, perhaps it's reasonable to close the API of this type using
`friend`?

eval.cpp inspects contents, but it's not clear that's needed because
changing logic doesn't affect tests, so I'm just removing it. Note that
SemIRLoc could've also been a loc_id, and it seems like the current
approach would mishandle that (i.e., print for `LocId::None` when that
seems not to be the intent). I figure we can add a `has_value` or
`AddNoteRequiringLoc` or something like that if needed.

Author: jonmeow

toolchain/check/diagnostic_helpers.h

@@ -12,29 +12,37 @@
 
 namespace Carbon::Check {
 
-// Diagnostic locations produced by checking may be either a parse node
-// directly, or an inst ID which is later translated to a parse node.
-struct SemIRLoc {
+// Tracks a location for diagnostic use, which is either a parse node or an inst
+// ID which can be translated to a parse node. Used when code needs to support
+// multiple possible ways of reporting a diagnostic location.
+class SemIRLoc {
+ public:
   // NOLINTNEXTLINE(google-explicit-constructor)
   SemIRLoc(SemIR::InstId inst_id)
-      : inst_id(inst_id), is_inst_id(true), token_only(false) {}
+      : inst_id_(inst_id), is_inst_id_(true), token_only_(false) {}
 
   // NOLINTNEXTLINE(google-explicit-constructor)
   SemIRLoc(Parse::NodeId node_id) : SemIRLoc(node_id, false) {}
 
   // NOLINTNEXTLINE(google-explicit-constructor)
   SemIRLoc(SemIR::LocId loc_id) : SemIRLoc(loc_id, false) {}
 
+  // If `token_only` is true, refers to the specific node; otherwise, refers to
+  // the node and its children.
   explicit SemIRLoc(SemIR::LocId loc_id, bool token_only)
-      : loc_id(loc_id), is_inst_id(false), token_only(token_only) {}
+      : loc_id_(loc_id), is_inst_id_(false), token_only_(token_only) {}
+
+ private:
+  // Only allow member access for diagnostics.
+  friend class SemIRDiagnosticConverter;
 
   union {
-    SemIR::InstId inst_id;
-    SemIR::LocId loc_id;
+    SemIR::InstId inst_id_;
+    SemIR::LocId loc_id_;
   };
 
-  bool is_inst_id;
-  bool token_only;
+  bool is_inst_id_;
+  bool token_only_;
 };
 
 inline auto TokenOnly(SemIR::LocId loc_id) -> SemIRLoc {

toolchain/check/eval.cpp

@@ -2126,9 +2126,6 @@ auto TryEvalBlockForSpecific(Context& context, SemIRLoc loc,
       &context.emitter(), [&](auto& builder) {
         CARBON_DIAGNOSTIC(ResolvingSpecificHere, Note, "in {0} used here",
                           InstIdAsType);
-        if (loc.is_inst_id && !loc.inst_id.has_value()) {
-          return;
-        }
         builder.Note(loc, ResolvingSpecificHere,
                      GetInstForSpecific(context, specific_id));
       });

toolchain/check/sem_ir_diagnostic_converter.cpp

@@ -53,7 +53,7 @@ auto SemIRDiagnosticConverter::ConvertLocImpl(SemIRLoc loc,
     if (import_loc_id.is_node_id()) {
       // For imports in the current file, the location is simple.
       in_import_loc = ConvertLocInFile(cursor_ir, import_loc_id.node_id(),
-                                       loc.token_only, context_fn);
+                                       loc.token_only_, context_fn);
     } else if (import_loc_id.is_import_ir_inst_id()) {
       // For implicit imports, we need to unravel the location a little
       // further.
@@ -67,7 +67,7 @@ auto SemIRDiagnosticConverter::ConvertLocImpl(SemIRLoc loc,
                    "Should only be one layer of implicit imports");
       in_import_loc =
           ConvertLocInFile(implicit_ir.sem_ir, implicit_loc_id.node_id(),
-                           loc.token_only, context_fn);
+                           loc.token_only_, context_fn);
     }
 
     // TODO: Add an "In implicit import of prelude." note for the case where we
@@ -91,16 +91,16 @@ auto SemIRDiagnosticConverter::ConvertLocImpl(SemIRLoc loc,
       return std::nullopt;
     } else {
       // Parse nodes always refer to the current IR.
-      return ConvertLocInFile(cursor_ir, loc_id.node_id(), loc.token_only,
+      return ConvertLocInFile(cursor_ir, loc_id.node_id(), loc.token_only_,
                               context_fn);
     }
   };
 
   // Handle the base location.
-  if (loc.is_inst_id) {
-    cursor_inst_id = loc.inst_id;
+  if (loc.is_inst_id_) {
+    cursor_inst_id = loc.inst_id_;
   } else {
-    if (auto diag_loc = handle_loc(loc.loc_id)) {
+    if (auto diag_loc = handle_loc(loc.loc_id_)) {
       return *diag_loc;
     }
     CARBON_CHECK(cursor_inst_id.has_value(), "Should have been set");
@@ -135,7 +135,7 @@ auto SemIRDiagnosticConverter::ConvertLocImpl(SemIRLoc loc,
     }
 
     // `None` parse node but not an import; just nothing to point at.
-    return ConvertLocInFile(cursor_ir, Parse::NodeId::None, loc.token_only,
+    return ConvertLocInFile(cursor_ir, Parse::NodeId::None, loc.token_only_,
                             context_fn);
   }
 }