Change how diagnostics are ordered (#4778)

This change deliberately breaks away from the line/column ordering, and
instead focuses on a last byte offset corresponding to the final token
processed as part of producing the message. Where that's equal, this
maintains stable ordering in order to reflect the order that diagnostics
were produced.

The intent of this approach is that lex, parse, and check diagnostics
are interleaved based on where they are produced, but that
subexpressions still have diagnostics emitted prior to containing
expressions. In particular, the prior line/column sort essentially
sorted on the _start_ of where a diagnostic was associated, and this is
closer to sorting based on the _end_. As a consequence, something like
`F(1 2)` will have the error for `1 2` emitted _before_ a diagnostic for
`F(1 2)` not matching parameters, instead of _after_.

In check, we track the last handled node. This provides a
last_byte_offset _separate_ from where a diagnostic is associated. The
intent is that this creates an ordering of diagnostics which may be
associated with earlier code, to cause the diagnostics to be emitted
later. An example consequence of this is the change in ordering of
modifier diagnostics: we are diagnosing those from the same place, but
they have the same last_byte_offset, so we print them out in the order
produced.

I've added similar tracking to parse, but cannot identify any test which
is affected by it (note the separate commit, I thought about this late).
I'm not sure whether we have good out-of-order errors we could produce
for this.

A significant number of tests have reordered diagnostics as a
consequence of this change, so this change does not add further testing.

toolchain/check/BUILD

@@ -276,6 +276,7 @@ cc_library(
     deps = [
         ":context",
         "//toolchain/diagnostics:diagnostic_emitter",
+        "//toolchain/lex:token_index",
         "//toolchain/parse:tree_node_diagnostic_converter",
         "//toolchain/sem_ir:file",
         "//toolchain/sem_ir:stringify_type",

toolchain/check/check_unit.cpp

@@ -335,17 +335,19 @@ auto CheckUnit::ProcessNodeIds() -> bool {
 
   // On crash, report which token we were handling.
   PrettyStackTraceFunction node_dumper([&](llvm::raw_ostream& output) {
-    auto loc = unit_and_imports_->unit->node_converter->ConvertLoc(
+    auto converted = unit_and_imports_->unit->node_converter->ConvertLoc(
         node_id, [](DiagnosticLoc, const DiagnosticBase<>&) {});
-    loc.FormatLocation(output);
+    converted.loc.FormatLocation(output);
     output << ": checking " << context_.parse_tree().node_kind(node_id) << "\n";
     // Crash output has a tab indent; try to indent slightly past that.
-    loc.FormatSnippet(output, /*indent=*/10);
+    converted.loc.FormatSnippet(output, /*indent=*/10);
   });
 
   while (auto maybe_node_id = traversal.Next()) {
     node_id = *maybe_node_id;
-    auto parse_kind = context_.parse_tree().node_kind(node_id);
+
+    unit_and_imports_->unit->sem_ir_converter->AdvanceToken(
+        context_.parse_tree().node_token(node_id));
 
     if (context_.parse_tree().node_has_error(node_id)) {
       context_.TODO(node_id, "handle invalid parse trees in `check`");
@@ -353,6 +355,7 @@ auto CheckUnit::ProcessNodeIds() -> bool {
     }
 
     bool result;
+    auto parse_kind = context_.parse_tree().node_kind(node_id);
     switch (parse_kind) {
 #define CARBON_PARSE_NODE_KIND(Name)                              \
   case Parse::NodeKind::Name: {                                   \

toolchain/check/sem_ir_diagnostic_converter.cpp

@@ -9,7 +9,30 @@ namespace Carbon::Check {
 
 auto SemIRDiagnosticConverter::ConvertLoc(SemIRLoc loc,
                                           ContextFnT context_fn) const
-    -> DiagnosticLoc {
+    -> ConvertedDiagnosticLoc {
+  auto converted = ConvertLocImpl(loc, context_fn);
+
+  // Use the token when possible, but -1 is the default value.
+  auto last_offset = -1;
+  if (last_token_.is_valid()) {
+    last_offset = sem_ir_->parse_tree().tokens().GetByteOffset(last_token_);
+  }
+
+  // When the diagnostic is in the same file, we use the last possible offset;
+  // otherwise, we ignore the offset because it's probably in that file.
+  if (converted.loc.filename == sem_ir_->filename()) {
+    converted.last_byte_offset =
+        std::max(converted.last_byte_offset, last_offset);
+  } else {
+    converted.last_byte_offset = last_offset;
+  }
+
+  return converted;
+}
+
+auto SemIRDiagnosticConverter::ConvertLocImpl(SemIRLoc loc,
+                                              ContextFnT context_fn) const
+    -> ConvertedDiagnosticLoc {
   // Cursors for the current IR and instruction in that IR.
   const auto* cursor_ir = sem_ir_;
   auto cursor_inst_id = SemIR::InstId::Invalid;
@@ -23,7 +46,7 @@ auto SemIRDiagnosticConverter::ConvertLoc(SemIRLoc loc,
                  "If we get invalid locations here, we may need to more "
                  "thoroughly track ImportDecls.");
 
-    DiagnosticLoc in_import_loc;
+    ConvertedDiagnosticLoc in_import_loc;
     auto import_loc_id = cursor_ir->insts().GetLocId(import_ir.decl_id);
     if (import_loc_id.is_node_id()) {
       // For imports in the current file, the location is simple.
@@ -50,7 +73,7 @@ auto SemIRDiagnosticConverter::ConvertLoc(SemIRLoc loc,
     if (import_loc_id.is_valid()) {
       // TODO: Include the name of the imported library in the diagnostic.
       CARBON_DIAGNOSTIC(InImport, LocationInfo, "in import");
-      context_fn(in_import_loc, InImport);
+      context_fn(in_import_loc.loc, InImport);
     }
 
     cursor_ir = import_ir.sem_ir;
@@ -59,7 +82,8 @@ auto SemIRDiagnosticConverter::ConvertLoc(SemIRLoc loc,
 
   // If the location is is an import, follows it and returns nullopt.
   // Otherwise, it's a parse node, so return the final location.
-  auto handle_loc = [&](SemIR::LocId loc_id) -> std::optional<DiagnosticLoc> {
+  auto handle_loc =
+      [&](SemIR::LocId loc_id) -> std::optional<ConvertedDiagnosticLoc> {
     if (loc_id.is_import_ir_inst_id()) {
       follow_import_ref(loc_id.import_ir_inst_id());
       return std::nullopt;
@@ -172,7 +196,7 @@ auto SemIRDiagnosticConverter::ConvertLocInFile(const SemIR::File* sem_ir,
                                                 Parse::NodeId node_id,
                                                 bool token_only,
                                                 ContextFnT context_fn) const
-    -> DiagnosticLoc {
+    -> ConvertedDiagnosticLoc {
   return node_converters_[sem_ir->check_ir_id().index]->ConvertLoc(
       Parse::NodeLoc(node_id, token_only), context_fn);
 }

toolchain/check/sem_ir_diagnostic_converter.h

@@ -8,6 +8,7 @@
 #include "llvm/ADT/ArrayRef.h"
 #include "toolchain/check/diagnostic_helpers.h"
 #include "toolchain/diagnostics/diagnostic_converter.h"
+#include "toolchain/lex/token_index.h"
 #include "toolchain/parse/tree_node_diagnostic_converter.h"
 #include "toolchain/sem_ir/file.h"
 
@@ -21,27 +22,43 @@ class SemIRDiagnosticConverter : public DiagnosticConverter<SemIRLoc> {
       const SemIR::File* sem_ir)
       : node_converters_(node_converters), sem_ir_(sem_ir) {}
 
-  // Converts an instruction's location to a diagnostic location, which will be
-  // the underlying line of code. Adds context for any imports used in the
-  // current SemIR to get to the underlying code.
+  // Implements `DiagnosticConverter::ConvertLoc`. Adds context for any imports
+  // used in the current SemIR to get to the underlying code.
+  //
+  // For the last byte offset, this uses `last_token_` exclusively for imported
+  // locations, or `loc` if it's in the same file and (for whatever reason)
+  // later.
   auto ConvertLoc(SemIRLoc loc, ContextFnT context_fn) const
-      -> DiagnosticLoc override;
+      -> ConvertedDiagnosticLoc override;
 
   // Implements argument conversions for supported check-phase arguments.
   auto ConvertArg(llvm::Any arg) const -> llvm::Any override;
 
+  // If a byte offset is past the current last byte offset, advances forward.
+  // Earlier offsets are ignored.
+  auto AdvanceToken(Lex::TokenIndex token) -> void {
+    last_token_ = std::max(last_token_, token);
+  }
+
  private:
+  // Implements `ConvertLoc`, but without `last_token_` applied.
+  auto ConvertLocImpl(SemIRLoc loc, ContextFnT context_fn) const
+      -> ConvertedDiagnosticLoc;
+
   // Converts a node_id corresponding to a specific sem_ir to a diagnostic
   // location.
   auto ConvertLocInFile(const SemIR::File* sem_ir, Parse::NodeId node_id,
                         bool token_only, ContextFnT context_fn) const
-      -> DiagnosticLoc;
+      -> ConvertedDiagnosticLoc;
 
   // Converters for each SemIR.
   llvm::ArrayRef<Parse::NodeLocConverter*> node_converters_;
 
   // The current SemIR being processed.
   const SemIR::File* sem_ir_;
+
+  // The last token encountered during processing.
+  Lex::TokenIndex last_token_ = Lex::TokenIndex::Invalid;
 };
 
 }  // namespace Carbon::Check

toolchain/check/testdata/alias/no_prelude/fail_modifiers.carbon

@@ -10,28 +10,28 @@
 
 class Class {}
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+25]]:1: error: `abstract` not allowed on `alias` declaration [ModifierNotAllowedOnDeclaration]
-// CHECK:STDERR: abstract base default final alias A = Class;
-// CHECK:STDERR: ^~~~~~~~
-// CHECK:STDERR:
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+21]]:10: error: `base` not allowed on declaration with `abstract` [ModifierNotAllowedWith]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+25]]:10: error: `base` not allowed on declaration with `abstract` [ModifierNotAllowedWith]
 // CHECK:STDERR: abstract base default final alias A = Class;
 // CHECK:STDERR:          ^~~~
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+18]]:1: note: `abstract` previously appeared here [ModifierPrevious]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+22]]:1: note: `abstract` previously appeared here [ModifierPrevious]
 // CHECK:STDERR: abstract base default final alias A = Class;
 // CHECK:STDERR: ^~~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+14]]:15: error: `default` not allowed on declaration with `abstract` [ModifierNotAllowedWith]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+18]]:15: error: `default` not allowed on declaration with `abstract` [ModifierNotAllowedWith]
 // CHECK:STDERR: abstract base default final alias A = Class;
 // CHECK:STDERR:               ^~~~~~~
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:1: note: `abstract` previously appeared here [ModifierPrevious]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+15]]:1: note: `abstract` previously appeared here [ModifierPrevious]
 // CHECK:STDERR: abstract base default final alias A = Class;
 // CHECK:STDERR: ^~~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+7]]:23: error: `final` not allowed on declaration with `abstract` [ModifierNotAllowedWith]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:23: error: `final` not allowed on declaration with `abstract` [ModifierNotAllowedWith]
 // CHECK:STDERR: abstract base default final alias A = Class;
 // CHECK:STDERR:                       ^~~~~
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: note: `abstract` previously appeared here [ModifierPrevious]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+8]]:1: note: `abstract` previously appeared here [ModifierPrevious]
+// CHECK:STDERR: abstract base default final alias A = Class;
+// CHECK:STDERR: ^~~~~~~~
+// CHECK:STDERR:
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: error: `abstract` not allowed on `alias` declaration [ModifierNotAllowedOnDeclaration]
 // CHECK:STDERR: abstract base default final alias A = Class;
 // CHECK:STDERR: ^~~~~~~~
 // CHECK:STDERR:

toolchain/check/testdata/class/fail_modifiers.carbon

@@ -47,18 +47,18 @@ base class BaseDecl;
 // CHECK:STDERR:
 abstract abstract class TwoAbstract { }
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+15]]:1: error: `protected` not allowed; requires class scope [ModifierProtectedNotAllowed]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+15]]:19: error: `base` not allowed on declaration with `virtual` [ModifierNotAllowedWith]
 // CHECK:STDERR: protected virtual base class Virtual {}
-// CHECK:STDERR: ^~~~~~~~~
-// CHECK:STDERR:
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:11: error: `virtual` not allowed on `class` declaration [ModifierNotAllowedOnDeclaration]
+// CHECK:STDERR:                   ^~~~
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+12]]:11: note: `virtual` previously appeared here [ModifierPrevious]
 // CHECK:STDERR: protected virtual base class Virtual {}
 // CHECK:STDERR:           ^~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+7]]:19: error: `base` not allowed on declaration with `virtual` [ModifierNotAllowedWith]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+8]]:1: error: `protected` not allowed; requires class scope [ModifierProtectedNotAllowed]
 // CHECK:STDERR: protected virtual base class Virtual {}
-// CHECK:STDERR:                   ^~~~
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:11: note: `virtual` previously appeared here [ModifierPrevious]
+// CHECK:STDERR: ^~~~~~~~~
+// CHECK:STDERR:
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:11: error: `virtual` not allowed on `class` declaration [ModifierNotAllowedOnDeclaration]
 // CHECK:STDERR: protected virtual base class Virtual {}
 // CHECK:STDERR:           ^~~~~~~
 // CHECK:STDERR:

toolchain/check/testdata/function/declaration/no_prelude/extern_library.carbon

@@ -102,16 +102,16 @@ extern library "extern_library_owner" fn F();
 
 library "[[@TEST_NAME]]";
 
-// CHECK:STDERR: fail_extern_library_mismatch_owner.carbon:[[@LINE+4]]:1: error: semantics TODO: `handle invalid parse trees in `check`` [SemanticsTodo]
-// CHECK:STDERR: import library "extern_library_mismatch"
-// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-// CHECK:STDERR:
 import library "extern_library_mismatch"
 
-// CHECK:STDERR: fail_extern_library_mismatch_owner.carbon:[[@LINE+4]]:1: error: `import` declarations must end with a `;` [ExpectedDeclSemi]
+// CHECK:STDERR: fail_extern_library_mismatch_owner.carbon:[[@LINE+8]]:1: error: `import` declarations must end with a `;` [ExpectedDeclSemi]
 // CHECK:STDERR: extern fn F();
 // CHECK:STDERR: ^~~~~~
 // CHECK:STDERR:
+// CHECK:STDERR: fail_extern_library_mismatch_owner.carbon:[[@LINE-6]]:1: error: semantics TODO: `handle invalid parse trees in `check`` [SemanticsTodo]
+// CHECK:STDERR: import library "extern_library_mismatch"
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
 extern fn F();
 
 // --- fail_extern_self_library.carbon

toolchain/check/testdata/function/declaration/no_prelude/fail_modifiers.carbon

@@ -8,27 +8,27 @@
 // TIP: To dump output, run:
 // TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/function/declaration/no_prelude/fail_modifiers.carbon
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:1: error: `default` not allowed; requires interface scope [ModifierRequiresInterface]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:9: error: `protected` must appear before `default` [ModifierMustAppearBefore]
+// CHECK:STDERR: default protected fn WrongOrder();
+// CHECK:STDERR:         ^~~~~~~~~
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+8]]:1: note: `default` previously appeared here [ModifierPrevious]
 // CHECK:STDERR: default protected fn WrongOrder();
 // CHECK:STDERR: ^~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+7]]:9: error: `protected` must appear before `default` [ModifierMustAppearBefore]
-// CHECK:STDERR: default protected fn WrongOrder();
-// CHECK:STDERR:         ^~~~~~~~~
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: note: `default` previously appeared here [ModifierPrevious]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: error: `default` not allowed; requires interface scope [ModifierRequiresInterface]
 // CHECK:STDERR: default protected fn WrongOrder();
 // CHECK:STDERR: ^~~~~~~
 // CHECK:STDERR:
 default protected fn WrongOrder();
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:1: error: `virtual` not allowed; requires class scope [ModifierRequiresClass]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:9: error: `virtual` repeated on declaration [ModifierRepeated]
+// CHECK:STDERR: virtual virtual fn DuplicateVirtual() {}
+// CHECK:STDERR:         ^~~~~~~
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+8]]:1: note: `virtual` previously appeared here [ModifierPrevious]
 // CHECK:STDERR: virtual virtual fn DuplicateVirtual() {}
 // CHECK:STDERR: ^~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+7]]:9: error: `virtual` repeated on declaration [ModifierRepeated]
-// CHECK:STDERR: virtual virtual fn DuplicateVirtual() {}
-// CHECK:STDERR:         ^~~~~~~
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: note: `virtual` previously appeared here [ModifierPrevious]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: error: `virtual` not allowed; requires class scope [ModifierRequiresClass]
 // CHECK:STDERR: virtual virtual fn DuplicateVirtual() {}
 // CHECK:STDERR: ^~~~~~~
 // CHECK:STDERR:
@@ -43,14 +43,14 @@ virtual virtual fn DuplicateVirtual() {}
 // CHECK:STDERR:
 private protected fn TwoAccess();
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:1: error: `abstract` not allowed; requires class scope [ModifierRequiresClass]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:10: error: `virtual` not allowed on declaration with `abstract` [ModifierNotAllowedWith]
+// CHECK:STDERR: abstract virtual fn ModifiersConflict() {}
+// CHECK:STDERR:          ^~~~~~~
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+8]]:1: note: `abstract` previously appeared here [ModifierPrevious]
 // CHECK:STDERR: abstract virtual fn ModifiersConflict() {}
 // CHECK:STDERR: ^~~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+7]]:10: error: `virtual` not allowed on declaration with `abstract` [ModifierNotAllowedWith]
-// CHECK:STDERR: abstract virtual fn ModifiersConflict() {}
-// CHECK:STDERR:          ^~~~~~~
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: note: `abstract` previously appeared here [ModifierPrevious]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: error: `abstract` not allowed; requires class scope [ModifierRequiresClass]
 // CHECK:STDERR: abstract virtual fn ModifiersConflict() {}
 // CHECK:STDERR: ^~~~~~~~
 // CHECK:STDERR:
@@ -62,21 +62,21 @@ abstract virtual fn ModifiersConflict() {}
 // CHECK:STDERR:
 base fn InvalidModifier();
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+18]]:1: error: `default` not allowed; requires interface scope [ModifierRequiresInterface]
-// CHECK:STDERR: default final virtual fn ModifiersConflict2() {}
-// CHECK:STDERR: ^~~~~~~
-// CHECK:STDERR:
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+14]]:9: error: `final` not allowed on declaration with `default` [ModifierNotAllowedWith]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+18]]:9: error: `final` not allowed on declaration with `default` [ModifierNotAllowedWith]
 // CHECK:STDERR: default final virtual fn ModifiersConflict2() {}
 // CHECK:STDERR:         ^~~~~
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:1: note: `default` previously appeared here [ModifierPrevious]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+15]]:1: note: `default` previously appeared here [ModifierPrevious]
 // CHECK:STDERR: default final virtual fn ModifiersConflict2() {}
 // CHECK:STDERR: ^~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+7]]:15: error: `virtual` not allowed on declaration with `default` [ModifierNotAllowedWith]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:15: error: `virtual` not allowed on declaration with `default` [ModifierNotAllowedWith]
 // CHECK:STDERR: default final virtual fn ModifiersConflict2() {}
 // CHECK:STDERR:               ^~~~~~~
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: note: `default` previously appeared here [ModifierPrevious]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+8]]:1: note: `default` previously appeared here [ModifierPrevious]
+// CHECK:STDERR: default final virtual fn ModifiersConflict2() {}
+// CHECK:STDERR: ^~~~~~~
+// CHECK:STDERR:
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: error: `default` not allowed; requires interface scope [ModifierRequiresInterface]
 // CHECK:STDERR: default final virtual fn ModifiersConflict2() {}
 // CHECK:STDERR: ^~~~~~~
 // CHECK:STDERR:

toolchain/check/testdata/function/declaration/no_prelude/fail_todo_no_params.carbon

@@ -31,13 +31,13 @@ fn A {
 
 // TODO: We don't have parsing support for this yet.
 library "[[@TEST_NAME]]";
-// CHECK:STDERR: fail_todo_arrow_body.carbon:[[@LINE+8]]:1: error: semantics TODO: `handle invalid parse trees in `check`` [SemanticsTodo]
+// CHECK:STDERR: fail_todo_arrow_body.carbon:[[@LINE+8]]:6: error: `fn` declarations must either end with a `;` or have a `{ ... }` block for a definition [ExpectedDeclSemiOrDefinition]
 // CHECK:STDERR: fn A => 0;
-// CHECK:STDERR: ^~~~~~~~~~
+// CHECK:STDERR:      ^~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_todo_arrow_body.carbon:[[@LINE+4]]:6: error: `fn` declarations must either end with a `;` or have a `{ ... }` block for a definition [ExpectedDeclSemiOrDefinition]
+// CHECK:STDERR: fail_todo_arrow_body.carbon:[[@LINE+4]]:1: error: semantics TODO: `handle invalid parse trees in `check`` [SemanticsTodo]
 // CHECK:STDERR: fn A => 0;
-// CHECK:STDERR:      ^~
+// CHECK:STDERR: ^~~~~~~~~~
 // CHECK:STDERR:
 fn A => 0;
 

toolchain/check/testdata/if_expr/fail_not_in_function.carbon

@@ -12,17 +12,17 @@
 // CHECK:STDERR: let x: i32 = if true then 1 else 0;
 // CHECK:STDERR:              ^~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_not_in_function.carbon:[[@LINE+12]]:14: error: semantics TODO: `Control flow expressions are currently only supported inside functions.` [SemanticsTodo]
+// CHECK:STDERR: fail_not_in_function.carbon:[[@LINE+12]]:22: error: semantics TODO: `Control flow expressions are currently only supported inside functions.` [SemanticsTodo]
 // CHECK:STDERR: let x: i32 = if true then 1 else 0;
-// CHECK:STDERR:              ^~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:                      ^~~~~~
 // CHECK:STDERR:
 // CHECK:STDERR: fail_not_in_function.carbon:[[@LINE+8]]:14: error: semantics TODO: `Control flow expressions are currently only supported inside functions.` [SemanticsTodo]
 // CHECK:STDERR: let x: i32 = if true then 1 else 0;
-// CHECK:STDERR:              ^~~~~~~
+// CHECK:STDERR:              ^~~~~~~~~~~~~~~~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_not_in_function.carbon:[[@LINE+4]]:22: error: semantics TODO: `Control flow expressions are currently only supported inside functions.` [SemanticsTodo]
+// CHECK:STDERR: fail_not_in_function.carbon:[[@LINE+4]]:14: error: semantics TODO: `Control flow expressions are currently only supported inside functions.` [SemanticsTodo]
 // CHECK:STDERR: let x: i32 = if true then 1 else 0;
-// CHECK:STDERR:                      ^~~~~~
+// CHECK:STDERR:              ^~~~~~~
 // CHECK:STDERR:
 let x: i32 = if true then 1 else 0;
 

toolchain/check/testdata/impl/fail_extend_impl_type_as.carbon

@@ -19,27 +19,27 @@ class C {
 }
 
 class D {
-  // CHECK:STDERR: fail_extend_impl_type_as.carbon:[[@LINE+11]]:3: error: cannot `extend` an `impl` with an explicit self type [ExtendImplSelfAs]
+  // CHECK:STDERR: fail_extend_impl_type_as.carbon:[[@LINE+7]]:3: error: cannot `extend` an `impl` with an explicit self type [ExtendImplSelfAs]
   // CHECK:STDERR:   extend impl D as I;
   // CHECK:STDERR:   ^~~~~~
-  // CHECK:STDERR: fail_extend_impl_type_as.carbon:[[@LINE+8]]:15: note: remove the explicit `Self` type here [ExtendImplSelfAsDefault]
+  // CHECK:STDERR: fail_extend_impl_type_as.carbon:[[@LINE+4]]:15: note: remove the explicit `Self` type here [ExtendImplSelfAsDefault]
   // CHECK:STDERR:   extend impl D as I;
   // CHECK:STDERR:               ^
   // CHECK:STDERR:
-  // CHECK:STDERR: fail_extend_impl_type_as.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
-  // CHECK:STDERR:   extend impl D as I;
-  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~
-  // CHECK:STDERR:
   extend impl D as I;
 }
 
 class E {
-  // CHECK:STDERR: fail_extend_impl_type_as.carbon:[[@LINE+6]]:3: error: cannot `extend` an `impl` with an explicit self type [ExtendImplSelfAs]
+  // CHECK:STDERR: fail_extend_impl_type_as.carbon:[[@LINE+10]]:3: error: cannot `extend` an `impl` with an explicit self type [ExtendImplSelfAs]
   // CHECK:STDERR:   extend impl Self as I {}
   // CHECK:STDERR:   ^~~~~~
-  // CHECK:STDERR: fail_extend_impl_type_as.carbon:[[@LINE+3]]:15: note: remove the explicit `Self` type here [ExtendImplSelfAsDefault]
+  // CHECK:STDERR: fail_extend_impl_type_as.carbon:[[@LINE+7]]:15: note: remove the explicit `Self` type here [ExtendImplSelfAsDefault]
   // CHECK:STDERR:   extend impl Self as I {}
   // CHECK:STDERR:               ^~~~
+  // CHECK:STDERR:
+  // CHECK:STDERR: fail_extend_impl_type_as.carbon:[[@LINE-11]]:3: error: impl declared but not defined [MissingImplDefinition]
+  // CHECK:STDERR:   extend impl D as I;
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~
   extend impl Self as I {}
 }
 

toolchain/check/testdata/impl/fail_extend_undefined_interface.carbon

@@ -11,17 +11,13 @@
 interface I;
 
 class C {
-  // CHECK:STDERR: fail_extend_undefined_interface.carbon:[[@LINE+11]]:3: error: implementation of undefined interface I [ImplOfUndefinedInterface]
+  // CHECK:STDERR: fail_extend_undefined_interface.carbon:[[@LINE+7]]:3: error: implementation of undefined interface I [ImplOfUndefinedInterface]
   // CHECK:STDERR:   extend impl as I;
   // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~
   // CHECK:STDERR: fail_extend_undefined_interface.carbon:[[@LINE-6]]:1: note: interface was forward declared here [InterfaceForwardDeclaredHere]
   // CHECK:STDERR: interface I;
   // CHECK:STDERR: ^~~~~~~~~~~~
   // CHECK:STDERR:
-  // CHECK:STDERR: fail_extend_undefined_interface.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
-  // CHECK:STDERR:   extend impl as I;
-  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~
-  // CHECK:STDERR:
   extend impl as I;
 }
 
@@ -29,7 +25,7 @@ fn F(c: C) {
   // CHECK:STDERR: fail_extend_undefined_interface.carbon:[[@LINE+10]]:3: error: member access into undefined interface `I` [QualifiedExprInUndefinedInterfaceScope]
   // CHECK:STDERR:   C.F();
   // CHECK:STDERR:   ^~~
-  // CHECK:STDERR: fail_extend_undefined_interface.carbon:[[@LINE-21]]:1: note: interface was forward declared here [InterfaceForwardDeclaredHere]
+  // CHECK:STDERR: fail_extend_undefined_interface.carbon:[[@LINE-17]]:1: note: interface was forward declared here [InterfaceForwardDeclaredHere]
   // CHECK:STDERR: interface I;
   // CHECK:STDERR: ^~~~~~~~~~~~
   // CHECK:STDERR: fail_extend_undefined_interface.carbon:[[@LINE-10]]:18: note: declared as an extended scope here [FromExtendHere]
@@ -37,15 +33,19 @@ fn F(c: C) {
   // CHECK:STDERR:                  ^
   // CHECK:STDERR:
   C.F();
-  // CHECK:STDERR: fail_extend_undefined_interface.carbon:[[@LINE+9]]:3: error: member access into undefined interface `I` [QualifiedExprInUndefinedInterfaceScope]
+  // CHECK:STDERR: fail_extend_undefined_interface.carbon:[[@LINE+13]]:3: error: member access into undefined interface `I` [QualifiedExprInUndefinedInterfaceScope]
   // CHECK:STDERR:   c.F();
   // CHECK:STDERR:   ^~~
-  // CHECK:STDERR: fail_extend_undefined_interface.carbon:[[@LINE-32]]:1: note: interface was forward declared here [InterfaceForwardDeclaredHere]
+  // CHECK:STDERR: fail_extend_undefined_interface.carbon:[[@LINE-28]]:1: note: interface was forward declared here [InterfaceForwardDeclaredHere]
   // CHECK:STDERR: interface I;
   // CHECK:STDERR: ^~~~~~~~~~~~
   // CHECK:STDERR: fail_extend_undefined_interface.carbon:[[@LINE-21]]:18: note: declared as an extended scope here [FromExtendHere]
   // CHECK:STDERR:   extend impl as I;
   // CHECK:STDERR:                  ^
+  // CHECK:STDERR:
+  // CHECK:STDERR: fail_extend_undefined_interface.carbon:[[@LINE-25]]:3: error: impl declared but not defined [MissingImplDefinition]
+  // CHECK:STDERR:   extend impl as I;
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~
   c.F();
 }
 
@@ -82,7 +82,7 @@ fn F(c: C) {
 // CHECK:STDOUT:     %c.param_patt: %C = value_param_pattern %c.patt, runtime_param0
 // CHECK:STDOUT:   } {
 // CHECK:STDOUT:     %c.param: %C = value_param runtime_param0
-// CHECK:STDOUT:     %C.ref.loc28: type = name_ref C, file.%C.decl [template = constants.%C]
+// CHECK:STDOUT:     %C.ref.loc24: type = name_ref C, file.%C.decl [template = constants.%C]
 // CHECK:STDOUT:     %c: %C = bind_name c, %c.param
 // CHECK:STDOUT:   }
 // CHECK:STDOUT: }
@@ -107,8 +107,8 @@ fn F(c: C) {
 // CHECK:STDOUT:
 // CHECK:STDOUT: fn @F(%c.param_patt: %C) {
 // CHECK:STDOUT: !entry:
-// CHECK:STDOUT:   %C.ref.loc39: type = name_ref C, file.%C.decl [template = constants.%C]
-// CHECK:STDOUT:   %F.ref.loc39: <error> = name_ref F, <error> [template = <error>]
+// CHECK:STDOUT:   %C.ref.loc35: type = name_ref C, file.%C.decl [template = constants.%C]
+// CHECK:STDOUT:   %F.ref.loc35: <error> = name_ref F, <error> [template = <error>]
 // CHECK:STDOUT:   %c.ref: %C = name_ref c, %c
 // CHECK:STDOUT:   %F.ref.loc49: <error> = name_ref F, <error> [template = <error>]
 // CHECK:STDOUT:   return

toolchain/check/testdata/impl/lookup/fail_todo_undefined_impl.carbon

@@ -15,17 +15,17 @@ interface I {
 class C {
   // This doesn't match `impl C as I` below under proposal #3763.
 
-  // CHECK:STDERR: fail_todo_undefined_impl.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
-  // CHECK:STDERR:   extend impl as I;
-  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~
-  // CHECK:STDERR:
   extend impl as I;
 }
 
 fn F() -> i32 {
-  // CHECK:STDERR: fail_todo_undefined_impl.carbon:[[@LINE+3]]:10: error: accessing member from impl before the end of its definition [ImplAccessMemberBeforeComplete]
+  // CHECK:STDERR: fail_todo_undefined_impl.carbon:[[@LINE+7]]:10: error: accessing member from impl before the end of its definition [ImplAccessMemberBeforeComplete]
   // CHECK:STDERR:   return C.F();
   // CHECK:STDERR:          ^~~
+  // CHECK:STDERR:
+  // CHECK:STDERR: fail_todo_undefined_impl.carbon:[[@LINE-8]]:3: error: impl declared but not defined [MissingImplDefinition]
+  // CHECK:STDERR:   extend impl as I;
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~
   return C.F();
 }
 

toolchain/check/testdata/interface/no_prelude/fail_assoc_const_not_constant.carbon

@@ -9,13 +9,13 @@
 // TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/interface/no_prelude/fail_assoc_const_not_constant.carbon
 
 interface I {
-  // CHECK:STDERR: fail_assoc_const_not_constant.carbon:[[@LINE+7]]:7: error: pattern in associated constant declaration must be a single `:!` binding [ExpectedSymbolicBindingInAssociatedConstant]
+  // CHECK:STDERR: fail_assoc_const_not_constant.carbon:[[@LINE+7]]:10: error: `Core.Int` implicitly referenced here, but package `Core` not found [CoreNotFound]
   // CHECK:STDERR:   let a: i32;
-  // CHECK:STDERR:       ^
+  // CHECK:STDERR:          ^~~
   // CHECK:STDERR:
-  // CHECK:STDERR: fail_assoc_const_not_constant.carbon:[[@LINE+3]]:10: error: `Core.Int` implicitly referenced here, but package `Core` not found [CoreNotFound]
+  // CHECK:STDERR: fail_assoc_const_not_constant.carbon:[[@LINE+3]]:7: error: pattern in associated constant declaration must be a single `:!` binding [ExpectedSymbolicBindingInAssociatedConstant]
   // CHECK:STDERR:   let a: i32;
-  // CHECK:STDERR:          ^~~
+  // CHECK:STDERR:       ^
   let a: i32;
 }
 

toolchain/check/testdata/let/fail_modifiers.carbon

@@ -32,53 +32,53 @@ final let d: i32 = 1;
 // CHECK:STDERR:
 virtual let e: i32 = 1;
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:1: error: `default` not allowed; requires interface scope [ModifierRequiresInterface]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:9: error: `final` not allowed on declaration with `default` [ModifierNotAllowedWith]
+// CHECK:STDERR: default final let f: i32 = 1;
+// CHECK:STDERR:         ^~~~~
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+8]]:1: note: `default` previously appeared here [ModifierPrevious]
 // CHECK:STDERR: default final let f: i32 = 1;
 // CHECK:STDERR: ^~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+7]]:9: error: `final` not allowed on declaration with `default` [ModifierNotAllowedWith]
-// CHECK:STDERR: default final let f: i32 = 1;
-// CHECK:STDERR:         ^~~~~
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: note: `default` previously appeared here [ModifierPrevious]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: error: `default` not allowed; requires interface scope [ModifierRequiresInterface]
 // CHECK:STDERR: default final let f: i32 = 1;
 // CHECK:STDERR: ^~~~~~~
 // CHECK:STDERR:
 default final let f: i32 = 1;
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:1: error: `default` not allowed; requires interface scope [ModifierRequiresInterface]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:9: error: `default` repeated on declaration [ModifierRepeated]
+// CHECK:STDERR: default default let g: i32 = 1;
+// CHECK:STDERR:         ^~~~~~~
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+8]]:1: note: `default` previously appeared here [ModifierPrevious]
 // CHECK:STDERR: default default let g: i32 = 1;
 // CHECK:STDERR: ^~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+7]]:9: error: `default` repeated on declaration [ModifierRepeated]
-// CHECK:STDERR: default default let g: i32 = 1;
-// CHECK:STDERR:         ^~~~~~~
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: note: `default` previously appeared here [ModifierPrevious]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: error: `default` not allowed; requires interface scope [ModifierRequiresInterface]
 // CHECK:STDERR: default default let g: i32 = 1;
 // CHECK:STDERR: ^~~~~~~
 // CHECK:STDERR:
 default default let g: i32 = 1;
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:1: error: `protected` not allowed; requires class scope [ModifierProtectedNotAllowed]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:11: error: `private` not allowed on declaration with `protected` [ModifierNotAllowedWith]
+// CHECK:STDERR: protected private let h: i32 = 1;
+// CHECK:STDERR:           ^~~~~~~
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+8]]:1: note: `protected` previously appeared here [ModifierPrevious]
 // CHECK:STDERR: protected private let h: i32 = 1;
 // CHECK:STDERR: ^~~~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+7]]:11: error: `private` not allowed on declaration with `protected` [ModifierNotAllowedWith]
-// CHECK:STDERR: protected private let h: i32 = 1;
-// CHECK:STDERR:           ^~~~~~~
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: note: `protected` previously appeared here [ModifierPrevious]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: error: `protected` not allowed; requires class scope [ModifierProtectedNotAllowed]
 // CHECK:STDERR: protected private let h: i32 = 1;
 // CHECK:STDERR: ^~~~~~~~~
 // CHECK:STDERR:
 protected private let h: i32 = 1;
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+10]]:1: error: `protected` not allowed; requires class scope [ModifierProtectedNotAllowed]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+10]]:11: error: `protected` repeated on declaration [ModifierRepeated]
+// CHECK:STDERR: protected protected let i: i32 = 1;
+// CHECK:STDERR:           ^~~~~~~~~
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+7]]:1: note: `protected` previously appeared here [ModifierPrevious]
 // CHECK:STDERR: protected protected let i: i32 = 1;
 // CHECK:STDERR: ^~~~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+6]]:11: error: `protected` repeated on declaration [ModifierRepeated]
-// CHECK:STDERR: protected protected let i: i32 = 1;
-// CHECK:STDERR:           ^~~~~~~~~
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+3]]:1: note: `protected` previously appeared here [ModifierPrevious]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+3]]:1: error: `protected` not allowed; requires class scope [ModifierProtectedNotAllowed]
 // CHECK:STDERR: protected protected let i: i32 = 1;
 // CHECK:STDERR: ^~~~~~~~~
 protected protected let i: i32 = 1;

toolchain/check/testdata/namespace/fail_modifiers.carbon

@@ -8,36 +8,36 @@
 // TIP: To dump output, run:
 // TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/namespace/fail_modifiers.carbon
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+33]]:1: error: `private` not allowed on `namespace` declaration [ModifierNotAllowedOnDeclaration]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+33]]:25: error: `base` not allowed on declaration with `abstract` [ModifierNotAllowedWith]
 // CHECK:STDERR: private extern abstract base default final namespace A;
-// CHECK:STDERR: ^~~~~~~
-// CHECK:STDERR:
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+29]]:9: error: `extern` not allowed on `namespace` declaration [ModifierNotAllowedOnDeclaration]
-// CHECK:STDERR: private extern abstract base default final namespace A;
-// CHECK:STDERR:         ^~~~~~
-// CHECK:STDERR:
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+25]]:16: error: `abstract` not allowed on `namespace` declaration [ModifierNotAllowedOnDeclaration]
+// CHECK:STDERR:                         ^~~~
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+30]]:16: note: `abstract` previously appeared here [ModifierPrevious]
 // CHECK:STDERR: private extern abstract base default final namespace A;
 // CHECK:STDERR:                ^~~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+21]]:25: error: `base` not allowed on declaration with `abstract` [ModifierNotAllowedWith]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+26]]:30: error: `default` not allowed on declaration with `abstract` [ModifierNotAllowedWith]
 // CHECK:STDERR: private extern abstract base default final namespace A;
-// CHECK:STDERR:                         ^~~~
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+18]]:16: note: `abstract` previously appeared here [ModifierPrevious]
+// CHECK:STDERR:                              ^~~~~~~
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+23]]:16: note: `abstract` previously appeared here [ModifierPrevious]
 // CHECK:STDERR: private extern abstract base default final namespace A;
 // CHECK:STDERR:                ^~~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+14]]:30: error: `default` not allowed on declaration with `abstract` [ModifierNotAllowedWith]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+19]]:38: error: `final` not allowed on declaration with `abstract` [ModifierNotAllowedWith]
 // CHECK:STDERR: private extern abstract base default final namespace A;
-// CHECK:STDERR:                              ^~~~~~~
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:16: note: `abstract` previously appeared here [ModifierPrevious]
+// CHECK:STDERR:                                      ^~~~~
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+16]]:16: note: `abstract` previously appeared here [ModifierPrevious]
 // CHECK:STDERR: private extern abstract base default final namespace A;
 // CHECK:STDERR:                ^~~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+7]]:38: error: `final` not allowed on declaration with `abstract` [ModifierNotAllowedWith]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+12]]:1: error: `private` not allowed on `namespace` declaration [ModifierNotAllowedOnDeclaration]
 // CHECK:STDERR: private extern abstract base default final namespace A;
-// CHECK:STDERR:                                      ^~~~~
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:16: note: `abstract` previously appeared here [ModifierPrevious]
+// CHECK:STDERR: ^~~~~~~
+// CHECK:STDERR:
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+8]]:9: error: `extern` not allowed on `namespace` declaration [ModifierNotAllowedOnDeclaration]
+// CHECK:STDERR: private extern abstract base default final namespace A;
+// CHECK:STDERR:         ^~~~~~
+// CHECK:STDERR:
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:16: error: `abstract` not allowed on `namespace` declaration [ModifierNotAllowedOnDeclaration]
 // CHECK:STDERR: private extern abstract base default final namespace A;
 // CHECK:STDERR:                ^~~~~~~~
 // CHECK:STDERR:

toolchain/check/testdata/operators/builtin/fail_and_or_not_in_function.carbon

@@ -14,13 +14,13 @@ fn F(b: bool) -> type {
 }
 
 // TODO: Short-circuit operators should be permitted outside functions.
-// CHECK:STDERR: fail_and_or_not_in_function.carbon:[[@LINE+8]]:11: error: cannot evaluate type expression [TypeExprEvaluationFailure]
+// CHECK:STDERR: fail_and_or_not_in_function.carbon:[[@LINE+8]]:13: error: semantics TODO: `Control flow expressions are currently only supported inside functions.` [SemanticsTodo]
 // CHECK:STDERR: var and_: F(true and true);
-// CHECK:STDERR:           ^~~~~~~~~~~~~~~~
+// CHECK:STDERR:             ^~~~~~~~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_and_or_not_in_function.carbon:[[@LINE+4]]:13: error: semantics TODO: `Control flow expressions are currently only supported inside functions.` [SemanticsTodo]
+// CHECK:STDERR: fail_and_or_not_in_function.carbon:[[@LINE+4]]:11: error: cannot evaluate type expression [TypeExprEvaluationFailure]
 // CHECK:STDERR: var and_: F(true and true);
-// CHECK:STDERR:             ^~~~~~~~~~~~~
+// CHECK:STDERR:           ^~~~~~~~~~~~~~~~
 // CHECK:STDERR:
 var and_: F(true and true);
 
@@ -38,13 +38,13 @@ var and_: F(true and true);
 // CHECK:STDERR:
 var and_val: bool = true and true;
 
-// CHECK:STDERR: fail_and_or_not_in_function.carbon:[[@LINE+8]]:10: error: cannot evaluate type expression [TypeExprEvaluationFailure]
+// CHECK:STDERR: fail_and_or_not_in_function.carbon:[[@LINE+8]]:12: error: semantics TODO: `Control flow expressions are currently only supported inside functions.` [SemanticsTodo]
 // CHECK:STDERR: var or_: F(true or true);
-// CHECK:STDERR:          ^~~~~~~~~~~~~~~
+// CHECK:STDERR:            ^~~~~~~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_and_or_not_in_function.carbon:[[@LINE+4]]:12: error: semantics TODO: `Control flow expressions are currently only supported inside functions.` [SemanticsTodo]
+// CHECK:STDERR: fail_and_or_not_in_function.carbon:[[@LINE+4]]:10: error: cannot evaluate type expression [TypeExprEvaluationFailure]
 // CHECK:STDERR: var or_: F(true or true);
-// CHECK:STDERR:            ^~~~~~~~~~~~
+// CHECK:STDERR:          ^~~~~~~~~~~~~~~
 // CHECK:STDERR:
 var or_: F(true or true);
 

toolchain/check/testdata/packages/no_prelude/fail_modifiers.carbon

@@ -50,53 +50,53 @@ export library "export_library";
 
 // --- fail_import_modifiers.carbon
 
-// CHECK:STDERR: fail_import_modifiers.carbon:[[@LINE+8]]:1: error: `impl` not allowed on `import` declaration [ModifierNotAllowedOnDeclaration]
+// CHECK:STDERR: fail_import_modifiers.carbon:[[@LINE+8]]:6: error: imported API 'ImplImport' not found [ImportNotFound]
 // CHECK:STDERR: impl import ImplImport;
-// CHECK:STDERR: ^~~~
+// CHECK:STDERR:      ^~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_import_modifiers.carbon:[[@LINE+4]]:6: error: imported API 'ImplImport' not found [ImportNotFound]
+// CHECK:STDERR: fail_import_modifiers.carbon:[[@LINE+4]]:1: error: `impl` not allowed on `import` declaration [ModifierNotAllowedOnDeclaration]
 // CHECK:STDERR: impl import ImplImport;
-// CHECK:STDERR:      ^~~~~~
+// CHECK:STDERR: ^~~~
 // CHECK:STDERR:
 impl import ImplImport;
 
-// CHECK:STDERR: fail_import_modifiers.carbon:[[@LINE+8]]:1: error: `extend` not allowed on `import` declaration [ModifierNotAllowedOnDeclaration]
+// CHECK:STDERR: fail_import_modifiers.carbon:[[@LINE+8]]:8: error: imported API 'ExtendImport' not found [ImportNotFound]
 // CHECK:STDERR: extend import ExtendImport;
-// CHECK:STDERR: ^~~~~~
+// CHECK:STDERR:        ^~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_import_modifiers.carbon:[[@LINE+4]]:8: error: imported API 'ExtendImport' not found [ImportNotFound]
+// CHECK:STDERR: fail_import_modifiers.carbon:[[@LINE+4]]:1: error: `extend` not allowed on `import` declaration [ModifierNotAllowedOnDeclaration]
 // CHECK:STDERR: extend import ExtendImport;
-// CHECK:STDERR:        ^~~~~~
+// CHECK:STDERR: ^~~~~~
 // CHECK:STDERR:
 extend import ExtendImport;
 
-// CHECK:STDERR: fail_import_modifiers.carbon:[[@LINE+8]]:1: error: `virtual` not allowed on `import` declaration [ModifierNotAllowedOnDeclaration]
+// CHECK:STDERR: fail_import_modifiers.carbon:[[@LINE+8]]:9: error: imported API 'VirtualImport' not found [ImportNotFound]
 // CHECK:STDERR: virtual import VirtualImport;
-// CHECK:STDERR: ^~~~~~~
+// CHECK:STDERR:         ^~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_import_modifiers.carbon:[[@LINE+4]]:9: error: imported API 'VirtualImport' not found [ImportNotFound]
+// CHECK:STDERR: fail_import_modifiers.carbon:[[@LINE+4]]:1: error: `virtual` not allowed on `import` declaration [ModifierNotAllowedOnDeclaration]
 // CHECK:STDERR: virtual import VirtualImport;
-// CHECK:STDERR:         ^~~~~~
+// CHECK:STDERR: ^~~~~~~
 // CHECK:STDERR:
 virtual import VirtualImport;
 
-// CHECK:STDERR: fail_import_modifiers.carbon:[[@LINE+8]]:1: error: `base` not allowed on `import` declaration [ModifierNotAllowedOnDeclaration]
+// CHECK:STDERR: fail_import_modifiers.carbon:[[@LINE+8]]:6: error: imported API 'BaseImport' not found [ImportNotFound]
 // CHECK:STDERR: base import BaseImport;
-// CHECK:STDERR: ^~~~
+// CHECK:STDERR:      ^~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_import_modifiers.carbon:[[@LINE+4]]:6: error: imported API 'BaseImport' not found [ImportNotFound]
+// CHECK:STDERR: fail_import_modifiers.carbon:[[@LINE+4]]:1: error: `base` not allowed on `import` declaration [ModifierNotAllowedOnDeclaration]
 // CHECK:STDERR: base import BaseImport;
-// CHECK:STDERR:      ^~~~~~
+// CHECK:STDERR: ^~~~
 // CHECK:STDERR:
 base import BaseImport;
 
-// CHECK:STDERR: fail_import_modifiers.carbon:[[@LINE+7]]:1: error: `private` not allowed on `import` declaration [ModifierNotAllowedOnDeclaration]
+// CHECK:STDERR: fail_import_modifiers.carbon:[[@LINE+7]]:9: error: imported API 'PrivateImport' not found [ImportNotFound]
 // CHECK:STDERR: private import PrivateImport;
-// CHECK:STDERR: ^~~~~~~
+// CHECK:STDERR:         ^~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_import_modifiers.carbon:[[@LINE+3]]:9: error: imported API 'PrivateImport' not found [ImportNotFound]
+// CHECK:STDERR: fail_import_modifiers.carbon:[[@LINE+3]]:1: error: `private` not allowed on `import` declaration [ModifierNotAllowedOnDeclaration]
 // CHECK:STDERR: private import PrivateImport;
-// CHECK:STDERR:         ^~~~~~
+// CHECK:STDERR: ^~~~~~~
 private import PrivateImport;
 
 // CHECK:STDOUT: --- fail_export_package.carbon

toolchain/check/testdata/pointer/fail_address_of_error.carbon

@@ -14,13 +14,13 @@ fn Test() {
   // CHECK:STDERR:    ^~~~~~~~~~
   // CHECK:STDERR:
   &undeclared;
-  // CHECK:STDERR: fail_address_of_error.carbon:[[@LINE+7]]:3: error: cannot take the address of non-reference expression [AddrOfNonRef]
+  // CHECK:STDERR: fail_address_of_error.carbon:[[@LINE+7]]:6: error: name `undeclared` not found [NameNotFound]
   // CHECK:STDERR:   &(&undeclared);
-  // CHECK:STDERR:   ^
+  // CHECK:STDERR:      ^~~~~~~~~~
   // CHECK:STDERR:
-  // CHECK:STDERR: fail_address_of_error.carbon:[[@LINE+3]]:6: error: name `undeclared` not found [NameNotFound]
+  // CHECK:STDERR: fail_address_of_error.carbon:[[@LINE+3]]:3: error: cannot take the address of non-reference expression [AddrOfNonRef]
   // CHECK:STDERR:   &(&undeclared);
-  // CHECK:STDERR:      ^~~~~~~~~~
+  // CHECK:STDERR:   ^
   &(&undeclared);
 }
 

toolchain/check/testdata/return/fail_let_in_type.carbon

@@ -28,13 +28,13 @@ let y:! type = i32;
 fn HalfDozen() -> y { return 6; }
 
 // TODO: This should work.
-// CHECK:STDERR: fail_let_in_type.carbon:[[@LINE+7]]:5: error: semantics TODO: `HandleTemplate` [SemanticsTodo]
+// CHECK:STDERR: fail_let_in_type.carbon:[[@LINE+7]]:14: error: semantics TODO: ``let` compile time binding outside function or interface` [SemanticsTodo]
 // CHECK:STDERR: let template z:! type = i32;
-// CHECK:STDERR:     ^~~~~~~~~~~~~~~~~
+// CHECK:STDERR:              ^~~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_let_in_type.carbon:[[@LINE+3]]:14: error: semantics TODO: ``let` compile time binding outside function or interface` [SemanticsTodo]
+// CHECK:STDERR: fail_let_in_type.carbon:[[@LINE+3]]:5: error: semantics TODO: `HandleTemplate` [SemanticsTodo]
 // CHECK:STDERR: let template z:! type = i32;
-// CHECK:STDERR:              ^~~~~~~~
+// CHECK:STDERR:     ^~~~~~~~~~~~~~~~~
 let template z:! type = i32;
 fn FirstPerfectNumber() -> z { return 6; }
 

toolchain/check/testdata/var/fail_todo_control_flow_init.carbon

@@ -12,17 +12,17 @@
 // CHECK:STDERR: var x: () = if true then () else ();
 // CHECK:STDERR:             ^~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_todo_control_flow_init.carbon:[[@LINE+12]]:13: error: semantics TODO: `Control flow expressions are currently only supported inside functions.` [SemanticsTodo]
+// CHECK:STDERR: fail_todo_control_flow_init.carbon:[[@LINE+12]]:21: error: semantics TODO: `Control flow expressions are currently only supported inside functions.` [SemanticsTodo]
 // CHECK:STDERR: var x: () = if true then () else ();
-// CHECK:STDERR:             ^~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:                     ^~~~~~~
 // CHECK:STDERR:
 // CHECK:STDERR: fail_todo_control_flow_init.carbon:[[@LINE+8]]:13: error: semantics TODO: `Control flow expressions are currently only supported inside functions.` [SemanticsTodo]
 // CHECK:STDERR: var x: () = if true then () else ();
-// CHECK:STDERR:             ^~~~~~~
+// CHECK:STDERR:             ^~~~~~~~~~~~~~~~~~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_todo_control_flow_init.carbon:[[@LINE+4]]:21: error: semantics TODO: `Control flow expressions are currently only supported inside functions.` [SemanticsTodo]
+// CHECK:STDERR: fail_todo_control_flow_init.carbon:[[@LINE+4]]:13: error: semantics TODO: `Control flow expressions are currently only supported inside functions.` [SemanticsTodo]
 // CHECK:STDERR: var x: () = if true then () else ();
-// CHECK:STDERR:                     ^~~~~~~
+// CHECK:STDERR:             ^~~~~~~
 // CHECK:STDERR:
 var x: () = if true then () else ();
 
@@ -30,17 +30,17 @@ var x: () = if true then () else ();
 // CHECK:STDERR: var x2: () = if false then () else ();
 // CHECK:STDERR:              ^~~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_todo_control_flow_init.carbon:[[@LINE+12]]:14: error: semantics TODO: `Control flow expressions are currently only supported inside functions.` [SemanticsTodo]
+// CHECK:STDERR: fail_todo_control_flow_init.carbon:[[@LINE+12]]:23: error: semantics TODO: `Control flow expressions are currently only supported inside functions.` [SemanticsTodo]
 // CHECK:STDERR: var x2: () = if false then () else ();
-// CHECK:STDERR:              ^~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:                       ^~~~~~~
 // CHECK:STDERR:
 // CHECK:STDERR: fail_todo_control_flow_init.carbon:[[@LINE+8]]:14: error: semantics TODO: `Control flow expressions are currently only supported inside functions.` [SemanticsTodo]
 // CHECK:STDERR: var x2: () = if false then () else ();
-// CHECK:STDERR:              ^~~~~~~~
+// CHECK:STDERR:              ^~~~~~~~~~~~~~~~~~~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_todo_control_flow_init.carbon:[[@LINE+4]]:23: error: semantics TODO: `Control flow expressions are currently only supported inside functions.` [SemanticsTodo]
+// CHECK:STDERR: fail_todo_control_flow_init.carbon:[[@LINE+4]]:14: error: semantics TODO: `Control flow expressions are currently only supported inside functions.` [SemanticsTodo]
 // CHECK:STDERR: var x2: () = if false then () else ();
-// CHECK:STDERR:                       ^~~~~~~
+// CHECK:STDERR:              ^~~~~~~~
 // CHECK:STDERR:
 var x2: () = if false then () else ();
 

toolchain/check/testdata/var/no_prelude/fail_modifiers.carbon

@@ -23,14 +23,14 @@ protected var b: ();
 // CHECK:STDERR:
 private protected var c: ();
 
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:1: error: `protected` not allowed; requires class scope [ModifierProtectedNotAllowed]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+11]]:11: error: `protected` repeated on declaration [ModifierRepeated]
+// CHECK:STDERR: protected protected var d: ();
+// CHECK:STDERR:           ^~~~~~~~~
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+8]]:1: note: `protected` previously appeared here [ModifierPrevious]
 // CHECK:STDERR: protected protected var d: ();
 // CHECK:STDERR: ^~~~~~~~~
 // CHECK:STDERR:
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+7]]:11: error: `protected` repeated on declaration [ModifierRepeated]
-// CHECK:STDERR: protected protected var d: ();
-// CHECK:STDERR:           ^~~~~~~~~
-// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: note: `protected` previously appeared here [ModifierPrevious]
+// CHECK:STDERR: fail_modifiers.carbon:[[@LINE+4]]:1: error: `protected` not allowed; requires class scope [ModifierProtectedNotAllowed]
 // CHECK:STDERR: protected protected var d: ();
 // CHECK:STDERR: ^~~~~~~~~
 // CHECK:STDERR:

toolchain/diagnostics/diagnostic.h

@@ -60,12 +60,16 @@ struct DiagnosticLoc {
 
   // Name of the file or buffer that this diagnostic refers to.
   llvm::StringRef filename;
+
   // A reference to the line of the error.
   llvm::StringRef line;
+
   // 1-based line number. -1 indicates unknown; other values are unused.
   int32_t line_number = -1;
+
   // 1-based column number. -1 indicates unknown; other values are unused.
   int32_t column_number = -1;
+
   // The number of characters corresponding to the location in the line,
   // starting at column_number. Should always be at least 1.
   int32_t length = 1;
@@ -105,6 +109,18 @@ struct Diagnostic {
   // The diagnostic's level.
   DiagnosticLevel level;
 
+  // The byte offset of the final token which is associated with the diagnostic.
+  // This is used by `SortingDiagnosticConsumer`. This is separate from the
+  // `DiagnosticLoc` because it must refer to a position in the primary file
+  // being processed by a consumer, and has no use cross-file or in notes.
+  //
+  // This will usually be the start position (not end) of the last lexed token
+  // processed before the diagnostic; it could also be `-1` when no source code
+  // needs to be processed for a diagnostic, or an appropriate byte offset when
+  // we specifically want a different diagnostic ordering than when a diagnostic
+  // is issued.
+  int32_t last_byte_offset = -1;
+
   // Messages related to the diagnostic. Only one should be a warning or error;
   // other messages provide context.
   llvm::SmallVector<DiagnosticMessage> messages;

toolchain/diagnostics/diagnostic_converter.h

@@ -10,6 +10,15 @@
 
 namespace Carbon {
 
+// The result of `DiagnosticConvert::ConvertLoc`. This is non-templated to allow
+// sharing across converters.
+struct ConvertedDiagnosticLoc {
+  // Becomes DiagnosticMessage::loc.
+  DiagnosticLoc loc;
+  // Becomes Diagnostic::last_byte_offset.
+  int32_t last_byte_offset;
+};
+
 // An interface that can convert some representation of a location into a
 // diagnostic location.
 template <typename LocT>
@@ -23,10 +32,11 @@ class DiagnosticConverter {
 
   virtual ~DiagnosticConverter() = default;
 
-  // Converts a LocT to a DiagnosticLoc. ConvertLoc may invoke
-  // context_fn to provide context messages.
+  // Converts a LocT to a DiagnosticLoc and its `last_byte_offset` (see
+  // `DiagnosticMessage`). ConvertLoc may invoke context_fn to provide context
+  // messages.
   virtual auto ConvertLoc(LocT loc, ContextFnT context_fn) const
-      -> DiagnosticLoc = 0;
+      -> ConvertedDiagnosticLoc = 0;
 
   // Converts arg types as needed. Not all uses require conversion, so the
   // default returns the argument unchanged.

toolchain/diagnostics/diagnostic_emitter.h

@@ -114,15 +114,17 @@ class DiagnosticEmitter {
       if (!emitter_) {
         return;
       }
-      AddMessageWithDiagnosticLoc(
-          emitter_->converter_->ConvertLoc(
-              loc,
-              [&](DiagnosticLoc context_loc,
-                  const DiagnosticBase<>& context_diagnostic_base) {
-                AddMessageWithDiagnosticLoc(context_loc,
-                                            context_diagnostic_base, {});
-              }),
-          diagnostic_base, args);
+      auto converted = emitter_->converter_->ConvertLoc(
+          loc, [&](DiagnosticLoc context_loc,
+                   const DiagnosticBase<>& context_diagnostic_base) {
+            AddMessageWithDiagnosticLoc(context_loc, context_diagnostic_base,
+                                        {});
+          });
+      // Use the last byte offset from the first message.
+      if (diagnostic_.messages.empty()) {
+        diagnostic_.last_byte_offset = converted.last_byte_offset;
+      }
+      AddMessageWithDiagnosticLoc(converted.loc, diagnostic_base, args);
     }
 
     // Adds a message to the diagnostic, handling conversion of the arguments. A

toolchain/diagnostics/diagnostic_emitter_test.cpp

@@ -19,8 +19,9 @@ using testing::ElementsAre;
 
 struct FakeDiagnosticConverter : DiagnosticConverter<int> {
   auto ConvertLoc(int n, ContextFnT /*context_fn*/) const
-      -> DiagnosticLoc override {
-    return {.line_number = 1, .column_number = n};
+      -> ConvertedDiagnosticLoc override {
+    return {.loc = {.line_number = 1, .column_number = n},
+            .last_byte_offset = -1};
   }
 };
 

toolchain/diagnostics/null_diagnostics.h

@@ -14,8 +14,8 @@ inline auto NullDiagnosticConverter() -> DiagnosticConverter<LocT>& {
   struct Converter : public DiagnosticConverter<LocT> {
     auto ConvertLoc(LocT /*loc*/,
                     DiagnosticConverter<LocT>::ContextFnT /*context_fn*/) const
-        -> DiagnosticLoc override {
-      return {};
+        -> ConvertedDiagnosticLoc override {
+      return {.loc = {}, .last_byte_offset = -1};
     }
   };
   static auto* converter = new Converter;

toolchain/diagnostics/sorting_diagnostic_consumer.h

@@ -12,6 +12,13 @@
 namespace Carbon {
 
 // Buffers incoming diagnostics for printing and sorting.
+//
+// Sorting is based on `last_byte_offset` without taking the filename into
+// account. When processing multiple files, it's expected that separate
+// consumers will be used in order to keep diagnostics distinct. Typically
+// `Diagnostic::messages[0]` will always be a location in the consumer's primary
+// file, but if it needs to correspond to a different file, the
+// `last_byte_offset` must still indicate an offset within the primary file.
 class SortingDiagnosticConsumer : public DiagnosticConsumer {
  public:
   explicit SortingDiagnosticConsumer(DiagnosticConsumer& next_consumer)
@@ -35,12 +42,7 @@ class SortingDiagnosticConsumer : public DiagnosticConsumer {
   void Flush() override {
     llvm::stable_sort(diagnostics_,
                       [](const Diagnostic& lhs, const Diagnostic& rhs) {
-                        const auto& lhs_loc = lhs.messages[0].loc;
-                        const auto& rhs_loc = rhs.messages[0].loc;
-                        return std::tie(lhs_loc.filename, lhs_loc.line_number,
-                                        lhs_loc.column_number) <
-                               std::tie(rhs_loc.filename, rhs_loc.line_number,
-                                        rhs_loc.column_number);
+                        return lhs.last_byte_offset < rhs.last_byte_offset;
                       });
     for (auto& diag : diagnostics_) {
       next_consumer_->HandleDiagnostic(std::move(diag));

toolchain/diagnostics/sorting_diagnostic_consumer_test.cpp

@@ -15,14 +15,15 @@ namespace Carbon {
 namespace {
 
 using ::Carbon::Testing::IsSingleDiagnostic;
+using ::testing::_;
 using ::testing::InSequence;
 
 CARBON_DIAGNOSTIC(TestDiagnostic, Error, "M{0}", int);
 
-struct FakeDiagnosticConverter : DiagnosticConverter<DiagnosticLoc> {
-  auto ConvertLoc(DiagnosticLoc loc, ContextFnT /*context_fn*/) const
-      -> DiagnosticLoc override {
-    return loc;
+struct FakeDiagnosticConverter : DiagnosticConverter<int32_t> {
+  auto ConvertLoc(int32_t last_byte_offset, ContextFnT /*context_fn*/) const
+      -> ConvertedDiagnosticLoc override {
+    return {.loc = {}, .last_byte_offset = last_byte_offset};
   }
 };
 
@@ -30,34 +31,34 @@ TEST(SortedDiagnosticEmitterTest, SortErrors) {
   FakeDiagnosticConverter converter;
   Testing::MockDiagnosticConsumer consumer;
   SortingDiagnosticConsumer sorting_consumer(consumer);
-  DiagnosticEmitter<DiagnosticLoc> emitter(converter, sorting_consumer);
+  DiagnosticEmitter<int32_t> emitter(converter, sorting_consumer);
 
-  emitter.Emit({"f", "line", 2, 1}, TestDiagnostic, 1);
-  emitter.Emit({"f", "line", 1, 1}, TestDiagnostic, 2);
-  emitter.Emit({"f", "line", 1, 3}, TestDiagnostic, 3);
-  emitter.Emit({"f", "line", 3, 4}, TestDiagnostic, 4);
-  emitter.Emit({"f", "line", 3, 2}, TestDiagnostic, 5);
-  emitter.Emit({"f", "line", 3, 2}, TestDiagnostic, 6);
+  emitter.Emit(1, TestDiagnostic, 1);
+  emitter.Emit(-1, TestDiagnostic, 2);
+  emitter.Emit(0, TestDiagnostic, 3);
+  emitter.Emit(4, TestDiagnostic, 4);
+  emitter.Emit(3, TestDiagnostic, 5);
+  emitter.Emit(3, TestDiagnostic, 6);
 
   InSequence s;
   EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic(
                             DiagnosticKind::TestDiagnostic,
-                            DiagnosticLevel::Error, 1, 1, "M2")));
+                            DiagnosticLevel::Error, _, _, "M2")));
   EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic(
                             DiagnosticKind::TestDiagnostic,
-                            DiagnosticLevel::Error, 1, 3, "M3")));
+                            DiagnosticLevel::Error, _, _, "M3")));
   EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic(
                             DiagnosticKind::TestDiagnostic,
-                            DiagnosticLevel::Error, 2, 1, "M1")));
+                            DiagnosticLevel::Error, _, _, "M1")));
   EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic(
                             DiagnosticKind::TestDiagnostic,
-                            DiagnosticLevel::Error, 3, 2, "M5")));
+                            DiagnosticLevel::Error, _, _, "M5")));
   EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic(
                             DiagnosticKind::TestDiagnostic,
-                            DiagnosticLevel::Error, 3, 2, "M6")));
+                            DiagnosticLevel::Error, _, _, "M6")));
   EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic(
                             DiagnosticKind::TestDiagnostic,
-                            DiagnosticLevel::Error, 3, 4, "M4")));
+                            DiagnosticLevel::Error, _, _, "M4")));
   sorting_consumer.Flush();
 }
 

toolchain/lex/test_helpers.h

@@ -24,23 +24,28 @@ class SingleTokenDiagnosticConverter : public DiagnosticConverter<const char*> {
   explicit SingleTokenDiagnosticConverter(llvm::StringRef token)
       : token_(token) {}
 
+  // Implements `DiagnosticConverter::ConvertLoc`.
   auto ConvertLoc(const char* pos, ContextFnT /*context_fn*/) const
-      -> DiagnosticLoc override {
+      -> ConvertedDiagnosticLoc override {
     CARBON_CHECK(StringRefContainsPointer(token_, pos),
                  "invalid diagnostic location");
     llvm::StringRef prefix = token_.take_front(pos - token_.begin());
     auto [before_last_newline, this_line] = prefix.rsplit('\n');
     if (before_last_newline.size() == prefix.size()) {
       // On first line.
-      return {.line_number = 1,
-              .column_number = static_cast<int32_t>(pos - token_.begin() + 1)};
+      return {.loc = {.line_number = 1,
+                      .column_number =
+                          static_cast<int32_t>(pos - token_.begin() + 1)},
+              .last_byte_offset = -1};
     } else {
       // On second or subsequent lines. Note that the line number here is 2
       // more than the number of newlines because `rsplit` removed one newline
       // and `line_number` is 1-based.
-      return {.line_number =
-                  static_cast<int32_t>(before_last_newline.count('\n') + 2),
-              .column_number = static_cast<int32_t>(this_line.size() + 1)};
+      return {
+          .loc = {.line_number =
+                      static_cast<int32_t>(before_last_newline.count('\n') + 2),
+                  .column_number = static_cast<int32_t>(this_line.size() + 1)},
+          .last_byte_offset = -1};
     }
   }
 

toolchain/lex/tokenized_buffer.cpp

@@ -376,28 +376,29 @@ auto TokenizedBuffer::CollectMemUsage(MemUsage& mem_usage,
 }
 
 auto TokenizedBuffer::SourceBufferDiagnosticConverter::ConvertLoc(
-    const char* loc, ContextFnT /*context_fn*/) const -> DiagnosticLoc {
-  CARBON_CHECK(StringRefContainsPointer(buffer_->source_->text(), loc),
+    const char* loc, ContextFnT /*context_fn*/) const
+    -> ConvertedDiagnosticLoc {
+  CARBON_CHECK(StringRefContainsPointer(tokens_->source_->text(), loc),
                "location not within buffer");
-  int32_t offset = loc - buffer_->source_->text().begin();
+  int32_t offset = loc - tokens_->source_->text().begin();
 
   // Find the first line starting after the given location.
   const auto* next_line_it = llvm::partition_point(
-      buffer_->line_infos_,
+      tokens_->line_infos_,
       [offset](const LineInfo& line) { return line.start <= offset; });
 
   // Step back one line to find the line containing the given position.
-  CARBON_CHECK(next_line_it != buffer_->line_infos_.begin(),
+  CARBON_CHECK(next_line_it != tokens_->line_infos_.begin(),
                "location precedes the start of the first line");
   const auto* line_it = std::prev(next_line_it);
-  int line_number = line_it - buffer_->line_infos_.begin();
+  int line_number = line_it - tokens_->line_infos_.begin();
   int column_number = offset - line_it->start;
 
   // Grab the line from the buffer by slicing from this line to the next
   // minus the newline. When on the last line, instead use the start to the end
   // of the buffer.
-  llvm::StringRef text = buffer_->source_->text();
-  llvm::StringRef line = next_line_it != buffer_->line_infos_.end()
+  llvm::StringRef text = tokens_->source_->text();
+  llvm::StringRef line = next_line_it != tokens_->line_infos_.end()
                              ? text.slice(line_it->start, next_line_it->start)
                              : text.substr(line_it->start);
 
@@ -406,28 +407,29 @@ auto TokenizedBuffer::SourceBufferDiagnosticConverter::ConvertLoc(
   // tail of the line such as CR+LF, etc.
   line.consume_back("\n");
 
-  return {.filename = buffer_->source_->filename(),
-          .line = line,
-          .line_number = line_number + 1,
-          .column_number = column_number + 1};
+  return {.loc = {.filename = tokens_->source_->filename(),
+                  .line = line,
+                  .line_number = line_number + 1,
+                  .column_number = column_number + 1},
+          .last_byte_offset = offset};
 }
 
 auto TokenDiagnosticConverter::ConvertLoc(TokenIndex token,
                                           ContextFnT context_fn) const
-    -> DiagnosticLoc {
+    -> ConvertedDiagnosticLoc {
   // Map the token location into a position within the source buffer.
-  const auto& token_info = buffer_->GetTokenInfo(token);
+  const auto& token_info = tokens_->GetTokenInfo(token);
   const char* token_start =
-      buffer_->source_->text().begin() + token_info.byte_offset();
+      tokens_->source_->text().begin() + token_info.byte_offset();
 
   // Find the corresponding file location.
   // TODO: Should we somehow indicate in the diagnostic location if this token
   // is a recovery token that doesn't correspond to the original source?
-  DiagnosticLoc loc =
-      TokenizedBuffer::SourceBufferDiagnosticConverter(buffer_).ConvertLoc(
+  auto converted =
+      TokenizedBuffer::SourceBufferDiagnosticConverter(tokens_).ConvertLoc(
           token_start, context_fn);
-  loc.length = buffer_->GetTokenText(token).size();
-  return loc;
+  converted.loc.length = tokens_->GetTokenText(token).size();
+  return converted;
 }
 
 }  // namespace Carbon::Lex

toolchain/lex/tokenized_buffer.h

@@ -64,15 +64,18 @@ using TokenIterator = IndexIterator<TokenIndex>;
 // buffer locations.
 class TokenDiagnosticConverter : public DiagnosticConverter<TokenIndex> {
  public:
-  explicit TokenDiagnosticConverter(const TokenizedBuffer* buffer)
-      : buffer_(buffer) {}
+  explicit TokenDiagnosticConverter(const TokenizedBuffer* tokens)
+      : tokens_(tokens) {}
 
-  // Map the given token into a diagnostic location.
+  // Implements `DiagnosticConverter::ConvertLoc`.
   auto ConvertLoc(TokenIndex token, ContextFnT context_fn) const
-      -> DiagnosticLoc override;
+      -> ConvertedDiagnosticLoc override;
+
+ protected:
+  auto tokens() const -> const TokenizedBuffer& { return *tokens_; }
 
  private:
-  const TokenizedBuffer* buffer_;
+  const TokenizedBuffer* tokens_;
 };
 
 // A buffer of tokenized Carbon source code.
@@ -161,6 +164,10 @@ class TokenizedBuffer : public Printable<TokenizedBuffer> {
   // Returns the previous line handle.
   auto GetPrevLine(LineIndex line) const -> LineIndex;
 
+  auto GetByteOffset(TokenIndex token) const -> int32_t {
+    return GetTokenInfo(token).byte_offset();
+  }
+
   // Returns true if the token comes after the comment.
   auto IsAfterComment(TokenIndex token, CommentIndex comment_index) const
       -> bool;
@@ -221,16 +228,15 @@ class TokenizedBuffer : public Printable<TokenizedBuffer> {
   class SourceBufferDiagnosticConverter
       : public DiagnosticConverter<const char*> {
    public:
-    explicit SourceBufferDiagnosticConverter(const TokenizedBuffer* buffer)
-        : buffer_(buffer) {}
+    explicit SourceBufferDiagnosticConverter(const TokenizedBuffer* tokens)
+        : tokens_(tokens) {}
 
-    // Map the given position within the source buffer into a diagnostic
-    // location.
+    // Implements `DiagnosticConverter::ConvertLoc`.
     auto ConvertLoc(const char* loc, ContextFnT context_fn) const
-        -> DiagnosticLoc override;
+        -> ConvertedDiagnosticLoc override;
 
    private:
-    const TokenizedBuffer* buffer_;
+    const TokenizedBuffer* tokens_;
   };
 
   // Specifies minimum widths to use when printing a token's fields via

toolchain/lower/file_context.cpp

@@ -616,13 +616,13 @@ auto FileContext::BuildGlobalVariableDecl(SemIR::VarStorage var_storage)
 }
 
 auto FileContext::GetLocForDI(SemIR::InstId inst_id) -> LocForDI {
-  auto diag_loc = converter_.ConvertLoc(
+  auto converted = converter_.ConvertLoc(
       inst_id, [&](DiagnosticLoc /*context_loc*/,
                    const DiagnosticBase<>& /*context_diagnostic_base*/) {});
-  return {.filename = diag_loc.filename,
-          .line_number = diag_loc.line_number == -1 ? 0 : diag_loc.line_number,
-          .column_number =
-              diag_loc.column_number == -1 ? 0 : diag_loc.column_number};
+  const auto& loc = converted.loc;
+  return {.filename = loc.filename,
+          .line_number = loc.line_number == -1 ? 0 : loc.line_number,
+          .column_number = loc.column_number == -1 ? 0 : loc.column_number};
 }
 
 }  // namespace Carbon::Lower

toolchain/parse/BUILD

@@ -68,6 +68,7 @@ cc_library(
         "//common:check",
         "//common:ostream",
         "//common:vlog",
+        "//toolchain/diagnostics:diagnostic_emitter",
         "//toolchain/diagnostics:format_providers",
         "//toolchain/lex:token_kind",
         "//toolchain/lex:tokenized_buffer",

toolchain/parse/context.cpp

@@ -20,12 +20,13 @@
 
 namespace Carbon::Parse {
 
-Context::Context(Tree& tree, Lex::TokenizedBuffer& tokens,
-                 Lex::TokenDiagnosticEmitter& emitter,
-                 llvm::raw_ostream* vlog_stream)
-    : tree_(&tree),
-      tokens_(&tokens),
-      emitter_(&emitter),
+Context::Context(Tree* tree, Lex::TokenizedBuffer* tokens,
+                 DiagnosticConsumer* consumer, llvm::raw_ostream* vlog_stream)
+    : tree_(tree),
+      tokens_(tokens),
+      converter_(tokens, &position_),
+      err_tracker_(*consumer),
+      emitter_(converter_, err_tracker_),
       vlog_stream_(vlog_stream),
       position_(tokens_->tokens().begin()),
       end_(tokens_->tokens().end()) {
@@ -58,8 +59,8 @@ auto Context::ConsumeAndAddOpenParen(Lex::TokenIndex default_token,
   } else {
     CARBON_DIAGNOSTIC(ExpectedParenAfter, Error, "expected `(` after `{0}`",
                       Lex::TokenKind);
-    emitter_->Emit(*position_, ExpectedParenAfter,
-                   tokens().GetKind(default_token));
+    emitter_.Emit(*position_, ExpectedParenAfter,
+                  tokens().GetKind(default_token));
     AddLeafNode(start_kind, default_token, /*has_error=*/true);
     return std::nullopt;
   }
@@ -79,8 +80,8 @@ auto Context::ConsumeAndAddCloseSymbol(Lex::TokenIndex expected_open,
     // diagnostic.
     CARBON_DIAGNOSTIC(ExpectedCloseSymbol, Error,
                       "unexpected tokens before `{0}`", Lex::TokenKind);
-    emitter_->Emit(*position_, ExpectedCloseSymbol,
-                   open_token_kind.closing_symbol());
+    emitter_.Emit(*position_, ExpectedCloseSymbol,
+                  open_token_kind.closing_symbol());
 
     SkipTo(tokens().GetMatchedClosingToken(expected_open));
     AddNode(close_kind, Consume(), /*has_error=*/true);
@@ -289,7 +290,7 @@ auto Context::DiagnoseOperatorFixity(OperatorFixity fixity) -> void {
       } else if (tokens().HasTrailingWhitespace(*position_)) {
         pos.value = -1;
       }
-      emitter_->Emit(*position_, BinaryOperatorRequiresWhitespace, pos);
+      emitter_.Emit(*position_, BinaryOperatorRequiresWhitespace, pos);
     }
   } else {
     bool prefix = fixity == OperatorFixity::Prefix;
@@ -302,14 +303,14 @@ auto Context::DiagnoseOperatorFixity(OperatorFixity fixity) -> void {
           UnaryOperatorHasWhitespace, Error,
           "whitespace is not allowed {0:after|before} this unary operator",
           BoolAsSelect);
-      emitter_->Emit(*position_, UnaryOperatorHasWhitespace, prefix);
+      emitter_.Emit(*position_, UnaryOperatorHasWhitespace, prefix);
     } else if (IsLexicallyValidInfixOperator()) {
       // Pre/postfix operators must not satisfy the infix operator rules.
       CARBON_DIAGNOSTIC(
           UnaryOperatorRequiresWhitespace, Error,
           "whitespace is required {0:before|after} this unary operator",
           BoolAsSelect);
-      emitter_->Emit(*position_, UnaryOperatorRequiresWhitespace, prefix);
+      emitter_.Emit(*position_, UnaryOperatorRequiresWhitespace, prefix);
     }
   }
 }
@@ -321,7 +322,7 @@ auto Context::ConsumeListToken(NodeKind comma_kind, Lex::TokenKind close_kind,
     if (!already_has_error) {
       CARBON_DIAGNOSTIC(UnexpectedTokenAfterListElement, Error,
                         "expected `,` or `{0}`", Lex::TokenKind);
-      emitter_->Emit(*position_, UnexpectedTokenAfterListElement, close_kind);
+      emitter_.Emit(*position_, UnexpectedTokenAfterListElement, close_kind);
       ReturnErrorOnState();
     }
 

toolchain/parse/context.h

@@ -9,6 +9,7 @@
 
 #include "common/check.h"
 #include "common/vlog.h"
+#include "toolchain/diagnostics/diagnostic_converter.h"
 #include "toolchain/lex/token_kind.h"
 #include "toolchain/lex/tokenized_buffer.h"
 #include "toolchain/parse/node_kind.h"
@@ -91,8 +92,8 @@ class Context {
   static_assert(sizeof(StateStackEntry) == 12,
                 "StateStackEntry has unexpected size!");
 
-  explicit Context(Tree& tree, Lex::TokenizedBuffer& tokens,
-                   Lex::TokenDiagnosticEmitter& emitter,
+  explicit Context(Tree* tree, Lex::TokenizedBuffer* tokens,
+                   DiagnosticConsumer* consumer,
                    llvm::raw_ostream* vlog_stream);
 
   // Adds a node to the parse tree that has no children (a leaf).
@@ -358,7 +359,9 @@ class Context {
 
   auto tokens() const -> const Lex::TokenizedBuffer& { return *tokens_; }
 
-  auto emitter() -> Lex::TokenDiagnosticEmitter& { return *emitter_; }
+  auto has_errors() const -> bool { return err_tracker_.seen_error(); }
+
+  auto emitter() -> Lex::TokenDiagnosticEmitter& { return emitter_; }
 
   auto position() -> Lex::TokenIterator& { return position_; }
   auto position() const -> Lex::TokenIterator { return position_; }
@@ -384,13 +387,38 @@ class Context {
   }
 
  private:
+  // Applies the `position_` to the `last_byte_offset` returned by `ConvertLoc`.
+  class TokenDiagnosticConverterForParse
+      : public Lex::TokenDiagnosticConverter {
+   public:
+    explicit TokenDiagnosticConverterForParse(Lex::TokenizedBuffer* tokens,
+                                              Lex::TokenIterator* position)
+        : Lex::TokenDiagnosticConverter(tokens), position_(position) {}
+
+    auto ConvertLoc(Lex::TokenIndex token, ContextFnT context_fn) const
+        -> ConvertedDiagnosticLoc override {
+      auto converted =
+          Lex::TokenDiagnosticConverter::ConvertLoc(token, context_fn);
+      converted.last_byte_offset = std::max(
+          converted.last_byte_offset, tokens().GetByteOffset(**position_));
+      return converted;
+    }
+
+   private:
+    // The position in `Parse()`.
+    Lex::TokenIterator* position_;
+  };
+
   // Prints a single token for a stack dump. Used by PrintForStackDump.
   auto PrintTokenForStackDump(llvm::raw_ostream& output,
                               Lex::TokenIndex token) const -> void;
 
   Tree* tree_;
   Lex::TokenizedBuffer* tokens_;
-  Lex::TokenDiagnosticEmitter* emitter_;
+
+  TokenDiagnosticConverterForParse converter_;
+  ErrorTrackingDiagnosticConsumer err_tracker_;
+  Lex::TokenDiagnosticEmitter emitter_;
 
   // Whether to print verbose output.
   llvm::raw_ostream* vlog_stream_;

toolchain/parse/parse.cpp

@@ -19,13 +19,9 @@ auto HandleInvalid(Context& context) -> void {
 
 auto Parse(Lex::TokenizedBuffer& tokens, DiagnosticConsumer& consumer,
            llvm::raw_ostream* vlog_stream) -> Tree {
-  Lex::TokenDiagnosticConverter converter(&tokens);
-  ErrorTrackingDiagnosticConsumer err_tracker(consumer);
-  Lex::TokenDiagnosticEmitter emitter(converter, err_tracker);
-
   // Delegate to the parser.
   Tree tree(tokens);
-  Context context(tree, tokens, emitter, vlog_stream);
+  Context context(&tree, &tokens, &consumer, vlog_stream);
   PrettyStackTraceFunction context_dumper(
       [&](llvm::raw_ostream& output) { context.PrintForStackDump(output); });
 
@@ -48,7 +44,7 @@ auto Parse(Lex::TokenizedBuffer& tokens, DiagnosticConsumer& consumer,
 
   // Mark the tree as potentially having errors if there were errors coming in
   // from the tokenized buffer or we diagnosed new errors.
-  tree.set_has_errors(tokens.has_errors() || err_tracker.seen_error());
+  tree.set_has_errors(tokens.has_errors() || context.has_errors());
 
   if (auto verify = tree.Verify(); !verify.ok()) {
     // TODO: This is temporarily printing to stderr directly during development.

toolchain/parse/tree_node_diagnostic_converter.h

@@ -42,13 +42,13 @@ class NodeLocConverter : public DiagnosticConverter<NodeLoc> {
         filename_(filename),
         get_tree_and_subtrees_(get_tree_and_subtrees) {}
 
-  // Map the given token into a diagnostic location.
+  // Implements `DiagnosticConverter::ConvertLoc`.
   auto ConvertLoc(NodeLoc node_loc, ContextFnT context_fn) const
-      -> DiagnosticLoc override {
+      -> ConvertedDiagnosticLoc override {
     // Support the invalid token as a way to emit only the filename, when there
     // is no line association.
     if (!node_loc.node_id().is_valid()) {
-      return {.filename = filename_};
+      return {{.filename = filename_}, -1};
     }
 
     const auto& tree = get_tree_and_subtrees_();
@@ -73,20 +73,21 @@ class NodeLocConverter : public DiagnosticConverter<NodeLoc> {
         end_token = desc_token;
       }
     }
-    DiagnosticLoc start_loc =
-        token_converter_.ConvertLoc(start_token, context_fn);
+    auto start_loc = token_converter_.ConvertLoc(start_token, context_fn);
     if (start_token == end_token) {
       return start_loc;
     }
-    DiagnosticLoc end_loc = token_converter_.ConvertLoc(end_token, context_fn);
+    auto end_loc = token_converter_.ConvertLoc(end_token, context_fn);
+    start_loc.last_byte_offset = end_loc.last_byte_offset;
     // For multiline locations we simply return the rest of the line for now
     // since true multiline locations are not yet supported.
-    if (start_loc.line_number != end_loc.line_number) {
-      start_loc.length = start_loc.line.size() - start_loc.column_number + 1;
+    if (start_loc.loc.line_number != end_loc.loc.line_number) {
+      start_loc.loc.length =
+          start_loc.loc.line.size() - start_loc.loc.column_number + 1;
     } else {
-      if (start_loc.column_number != end_loc.column_number) {
-        start_loc.length =
-            end_loc.column_number + end_loc.length - start_loc.column_number;
+      if (start_loc.loc.column_number != end_loc.loc.column_number) {
+        start_loc.loc.length = end_loc.loc.column_number + end_loc.loc.length -
+                               start_loc.loc.column_number;
       }
     }
     return start_loc;

toolchain/source/source_buffer.cpp

@@ -13,8 +13,8 @@ namespace Carbon {
 namespace {
 struct FilenameConverter : DiagnosticConverter<llvm::StringRef> {
   auto ConvertLoc(llvm::StringRef filename, ContextFnT /*context_fn*/) const
-      -> DiagnosticLoc override {
-    return {.filename = filename};
+      -> ConvertedDiagnosticLoc override {
+    return {.loc = {.filename = filename}, .last_byte_offset = -1};
   }
 };
 }  // namespace