Underline the complete declaration in diagnostics (carbon-language#3508)

Builds upon @domisterwoozy 's excellent carbon-language#3442 . Removes the need to
store the first node of a declaration in the declaration state stack.

toolchain/check/decl_state.h

@@ -62,8 +62,7 @@ struct DeclState {
  Var
  };
 
- explicit DeclState(DeclKind decl_kind, Parse::NodeId parse_node)
- : kind(decl_kind), first_node(parse_node) {}
+ explicit DeclState(DeclKind decl_kind) : kind(decl_kind) {}
 
  DeclKind kind;
 
@@ -75,25 +74,17 @@ struct DeclState {
  // Invariant: contains just the modifiers represented by `saw_access_modifier`
  // and `saw_other_modifier`.
  KeywordModifierSet modifier_set = KeywordModifierSet::None;
-
- // Node corresponding to the first token of the declaration.
- Parse::NodeId first_node;
 };
 
 // Stack of `DeclState` values, representing all the declarations we are
 // currently nested within.
 // Invariant: Bottom of the stack always has a "DeclState::FileScope" entry.
 class DeclStateStack {
  public:
- DeclStateStack() {
- stack_.emplace_back(DeclState::FileScope, Parse::NodeId::Invalid);
- }
+ DeclStateStack() { stack_.emplace_back(DeclState::FileScope); }
 
- // Enters a declaration of kind `k`, with `parse_node` for the introducer
- // token.
- auto Push(DeclState::DeclKind k, Parse::NodeId parse_node) -> void {
- stack_.push_back(DeclState(k, parse_node));
- }
+ // Enters a declaration of kind `k`.
+ auto Push(DeclState::DeclKind k) -> void { stack_.emplace_back(k); }
 
  // Gets the state of declaration at the top of the stack -- the innermost
  // declaration currently being processed.

toolchain/check/handle_class.cpp

@@ -15,17 +15,16 @@ auto HandleClassIntroducer(Context& context, Parse::NodeId parse_node) -> bool {
  // Push the bracketing node.
  context.node_stack().Push(parse_node);
  // Optional modifiers and the name follow.
- context.decl_state_stack().Push(DeclState::Class, parse_node);
+ context.decl_state_stack().Push(DeclState::Class);
  context.decl_name_stack().PushScopeAndStartName();
  return true;
 }
 
-static auto BuildClassDecl(Context& context)
+static auto BuildClassDecl(Context& context, Parse::NodeId parse_node)
  -> std::tuple<SemIR::ClassId, SemIR::InstId> {
  auto name_context = context.decl_name_stack().FinishName();
  context.node_stack()
  .PopAndDiscardSoloParseNode<Parse::NodeKind::ClassIntroducer>();
- auto first_node = context.decl_state_stack().innermost().first_node;
 
  // Process modifiers.
  CheckAccessModifiersOnDecl(context, Lex::TokenKind::Class);
@@ -48,7 +47,7 @@ static auto BuildClassDecl(Context& context)
 
  // Add the class declaration.
  auto class_decl =
- SemIR::ClassDecl{first_node, SemIR::ClassId::Invalid, decl_block_id};
+ SemIR::ClassDecl{parse_node, SemIR::ClassId::Invalid, decl_block_id};
  auto class_decl_id = context.AddInst(class_decl);
 
  // Check whether this is a redeclaration.
@@ -69,7 +68,7 @@ static auto BuildClassDecl(Context& context)
  CARBON_DIAGNOSTIC(ClassRedeclarationDifferentIntroducerPrevious, Note,
  "Previously declared here.");
  context.emitter()
- .Build(first_node, ClassRedeclarationDifferentIntroducer)
+ .Build(parse_node, ClassRedeclarationDifferentIntroducer)
  .Note(existing_class_decl->parse_node,
  ClassRedeclarationDifferentIntroducerPrevious)
  .Emit();
@@ -102,7 +101,7 @@ static auto BuildClassDecl(Context& context)
  auto& class_info = context.classes().Get(class_decl.class_id);
  class_info.self_type_id =
  context.CanonicalizeType(context.AddInst(SemIR::ClassType{
- first_node, context.GetBuiltinType(SemIR::BuiltinKind::TypeType),
+ parse_node, context.GetBuiltinType(SemIR::BuiltinKind::TypeType),
  class_decl.class_id}));
  }
 
@@ -112,15 +111,15 @@ static auto BuildClassDecl(Context& context)
  return {class_decl.class_id, class_decl_id};
 }
 
-auto HandleClassDecl(Context& context, Parse::NodeId /*parse_node*/) -> bool {
- BuildClassDecl(context);
+auto HandleClassDecl(Context& context, Parse::NodeId parse_node) -> bool {
+ BuildClassDecl(context, parse_node);
  context.decl_name_stack().PopScope();
  return true;
 }
 
 auto HandleClassDefinitionStart(Context& context, Parse::NodeId parse_node)
  -> bool {
- auto [class_id, class_decl_id] = BuildClassDecl(context);
+ auto [class_id, class_decl_id] = BuildClassDecl(context, parse_node);
  auto& class_info = context.classes().Get(class_id);
 
  // Track that this declaration is the definition.
@@ -164,8 +163,9 @@ auto HandleClassDefinitionStart(Context& context, Parse::NodeId parse_node)
  return true;
 }
 
-auto HandleBaseIntroducer(Context& context, Parse::NodeId parse_node) -> bool {
- context.decl_state_stack().Push(DeclState::Base, parse_node);
+auto HandleBaseIntroducer(Context& context, Parse::NodeId /*parse_node*/)
+ -> bool {
+ context.decl_state_stack().Push(DeclState::Base);
  return true;
 }
 
@@ -184,8 +184,7 @@ auto HandleBaseDecl(Context& context, Parse::NodeId parse_node) -> bool {
  if (!(modifiers & KeywordModifierSet::Extend)) {
  CARBON_DIAGNOSTIC(BaseMissingExtend, Error,
  "Missing `extend` before `base` declaration in class.");
- context.emitter().Emit(context.decl_state_stack().innermost().first_node,
- BaseMissingExtend);
+ context.emitter().Emit(parse_node, BaseMissingExtend);
  }
  context.decl_state_stack().Pop(DeclState::Base);
 

toolchain/check/handle_function.cpp

@@ -43,7 +43,8 @@ static auto DiagnoseModifiers(Context& context) -> KeywordModifierSet {
 // Build a FunctionDecl describing the signature of a function. This
 // handles the common logic shared by function declaration syntax and function
 // definition syntax.
-static auto BuildFunctionDecl(Context& context, bool is_definition)
+static auto BuildFunctionDecl(Context& context, Parse::NodeId parse_node,
+ bool is_definition)
  -> std::pair<SemIR::FunctionId, SemIR::InstId> {
  // TODO: This contains the IR block for the parameters and return type. At
  // present, it's just loose, but it's not strictly required for parameter
@@ -88,8 +89,6 @@ static auto BuildFunctionDecl(Context& context, bool is_definition)
  context.node_stack()
  .PopAndDiscardSoloParseNode<Parse::NodeKind::FunctionIntroducer>();
 
- auto first_node = context.decl_state_stack().innermost().first_node;
-
  // Process modifiers.
  auto modifiers = DiagnoseModifiers(context);
  if (!!(modifiers & KeywordModifierSet::Access)) {
@@ -110,7 +109,7 @@ static auto BuildFunctionDecl(Context& context, bool is_definition)
 
  // Add the function declaration.
  auto function_decl = SemIR::FunctionDecl{
- first_node, context.GetBuiltinType(SemIR::BuiltinKind::FunctionType),
+ parse_node, context.GetBuiltinType(SemIR::BuiltinKind::FunctionType),
  SemIR::FunctionId::Invalid};
  auto function_decl_id = context.AddInst(function_decl);
 
@@ -165,20 +164,19 @@ static auto BuildFunctionDecl(Context& context, bool is_definition)
  (return_slot_id.is_valid() &&
  return_type_id !=
  context.GetBuiltinType(SemIR::BuiltinKind::BoolType) &&
- return_type_id != context.CanonicalizeTupleType(first_node, {}))) {
+ return_type_id != context.CanonicalizeTupleType(parse_node, {}))) {
  CARBON_DIAGNOSTIC(InvalidMainRunSignature, Error,
  "Invalid signature for `Main.Run` function. Expected "
  "`fn ()` or `fn () -> i32`.");
- context.emitter().Emit(first_node, InvalidMainRunSignature);
+ context.emitter().Emit(parse_node, InvalidMainRunSignature);
  }
  }
 
  return {function_decl.function_id, function_decl_id};
 }
 
-auto HandleFunctionDecl(Context& context, Parse::NodeId /*parse_node*/)
- -> bool {
- BuildFunctionDecl(context, /*is_definition=*/false);
+auto HandleFunctionDecl(Context& context, Parse::NodeId parse_node) -> bool {
+ BuildFunctionDecl(context, parse_node, /*is_definition=*/false);
  context.decl_name_stack().PopScope();
  return true;
 }
@@ -212,7 +210,7 @@ auto HandleFunctionDefinitionStart(Context& context, Parse::NodeId parse_node)
  -> bool {
  // Process the declaration portion of the function.
  auto [function_id, decl_id] =
- BuildFunctionDecl(context, /*is_definition=*/true);
+ BuildFunctionDecl(context, parse_node, /*is_definition=*/true);
  auto& function = context.functions().Get(function_id);
 
  // Track that this declaration is the definition.
@@ -278,7 +276,7 @@ auto HandleFunctionIntroducer(Context& context, Parse::NodeId parse_node)
  // Push the bracketing node.
  context.node_stack().Push(parse_node);
  // Optional modifiers and the name follow.
- context.decl_state_stack().Push(DeclState::Fn, parse_node);
+ context.decl_state_stack().Push(DeclState::Fn);
  context.decl_name_stack().PushScopeAndStartName();
  return true;
 }

toolchain/check/handle_interface.cpp

@@ -15,17 +15,16 @@ auto HandleInterfaceIntroducer(Context& context, Parse::NodeId parse_node)
  // Push the bracketing node.
  context.node_stack().Push(parse_node);
  // Optional modifiers and the name follow.
- context.decl_state_stack().Push(DeclState::Interface, parse_node);
+ context.decl_state_stack().Push(DeclState::Interface);
  context.decl_name_stack().PushScopeAndStartName();
  return true;
 }
 
-static auto BuildInterfaceDecl(Context& context)
+static auto BuildInterfaceDecl(Context& context, Parse::NodeId parse_node)
  -> std::tuple<SemIR::InterfaceId, SemIR::InstId> {
  auto name_context = context.decl_name_stack().FinishName();
  context.node_stack()
  .PopAndDiscardSoloParseNode<Parse::NodeKind::InterfaceIntroducer>();
- auto first_node = context.decl_state_stack().innermost().first_node;
 
  // Process modifiers.
  CheckAccessModifiersOnDecl(context, Lex::TokenKind::Interface);
@@ -43,7 +42,7 @@ static auto BuildInterfaceDecl(Context& context)
 
  // Add the interface declaration.
  auto interface_decl = SemIR::InterfaceDecl{
- first_node, SemIR::InterfaceId::Invalid, decl_block_id};
+ parse_node, SemIR::InterfaceId::Invalid, decl_block_id};
  auto interface_decl_id = context.AddInst(interface_decl);
 
  // Check whether this is a redeclaration.
@@ -84,16 +83,16 @@ static auto BuildInterfaceDecl(Context& context)
  return {interface_decl.interface_id, interface_decl_id};
 }
 
-auto HandleInterfaceDecl(Context& context, Parse::NodeId /*parse_node*/)
- -> bool {
- BuildInterfaceDecl(context);
+auto HandleInterfaceDecl(Context& context, Parse::NodeId parse_node) -> bool {
+ BuildInterfaceDecl(context, parse_node);
  context.decl_name_stack().PopScope();
  return true;
 }
 
 auto HandleInterfaceDefinitionStart(Context& context, Parse::NodeId parse_node)
  -> bool {
- auto [interface_id, interface_decl_id] = BuildInterfaceDecl(context);
+ auto [interface_id, interface_decl_id] =
+ BuildInterfaceDecl(context, parse_node);
  auto& interface_info = context.interfaces().Get(interface_id);
 
  // Track that this declaration is the definition.

toolchain/check/handle_let.cpp

@@ -57,7 +57,7 @@ auto HandleLetDecl(Context& context, Parse::NodeId parse_node) -> bool {
 }
 
 auto HandleLetIntroducer(Context& context, Parse::NodeId parse_node) -> bool {
- context.decl_state_stack().Push(DeclState::Let, parse_node);
+ context.decl_state_stack().Push(DeclState::Let);
  // Push a bracketing node to establish the pattern context.
  context.node_stack().Push(parse_node);
  return true;

toolchain/check/handle_modifier.cpp

@@ -57,9 +57,6 @@ static auto HandleModifier(Context& context, Parse::NodeId parse_node,
  } else {
  s.modifier_set |= keyword;
  saw_modifier = parse_node;
- if (is_access || !s.saw_access_modifier.is_valid()) {
- s.first_node = parse_node;
- }
  }
  return true;
 }

toolchain/check/handle_namespace.cpp

@@ -9,20 +9,20 @@
 
 namespace Carbon::Check {
 
-auto HandleNamespaceStart(Context& context, Parse::NodeId parse_node) -> bool {
+auto HandleNamespaceStart(Context& context, Parse::NodeId /*parse_node*/)
+ -> bool {
  // Optional modifiers and the name follow.
- context.decl_state_stack().Push(DeclState::Namespace, parse_node);
+ context.decl_state_stack().Push(DeclState::Namespace);
  context.decl_name_stack().PushScopeAndStartName();
  return true;
 }
 
-auto HandleNamespace(Context& context, Parse::NodeId /*parse_node*/) -> bool {
+auto HandleNamespace(Context& context, Parse::NodeId parse_node) -> bool {
  auto name_context = context.decl_name_stack().FinishName();
- auto first_node = context.decl_state_stack().innermost().first_node;
  LimitModifiersOnDecl(context, KeywordModifierSet::None,
  Lex::TokenKind::Namespace);
  auto namespace_id = context.AddInst(SemIR::Namespace{
- first_node, context.GetBuiltinType(SemIR::BuiltinKind::NamespaceType),
+ parse_node, context.GetBuiltinType(SemIR::BuiltinKind::NamespaceType),
  context.name_scopes().Add()});
  context.decl_name_stack().AddNameToLookup(name_context, namespace_id);
 

toolchain/check/handle_variable.cpp

@@ -13,7 +13,7 @@ auto HandleVariableIntroducer(Context& context, Parse::NodeId parse_node)
  -> bool {
  // No action, just a bracketing node.
  context.node_stack().Push(parse_node);
- context.decl_state_stack().Push(DeclState::Var, parse_node);
+ context.decl_state_stack().Push(DeclState::Var);
  return true;
 }
 

toolchain/check/testdata/array/fail_incomplete_element.carbon

@@ -11,7 +11,7 @@ class Incomplete;
 // CHECK:STDERR: ^~~~~~~~~~~~~~~
 // CHECK:STDERR: fail_incomplete_element.carbon:[[@LINE-5]]:1: Class was forward declared here.
 // CHECK:STDERR: class Incomplete;
-// CHECK:STDERR: ^~~~~
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~
 var a: [Incomplete; 1];
 
 // CHECK:STDERR: fail_incomplete_element.carbon:[[@LINE+3]]:1: ERROR: Cannot implicitly convert from `<error>*` to `Incomplete*`.

toolchain/check/testdata/basics/fail_bad_run.carbon

@@ -6,7 +6,7 @@
 
 // CHECK:STDERR: fail_bad_run.carbon:[[@LINE+6]]:1: ERROR: Invalid signature for `Main.Run` function. Expected `fn ()` or `fn () -> i32`.
 // CHECK:STDERR: fn Run() -> String {}
-// CHECK:STDERR: ^~
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~
 // CHECK:STDERR: fail_bad_run.carbon:[[@LINE+3]]:21: ERROR: Missing `return` at end of function with declared return type.
 // CHECK:STDERR: fn Run() -> String {}
 // CHECK:STDERR: ^

toolchain/check/testdata/basics/fail_bad_run_2.carbon

@@ -6,7 +6,7 @@
 
 // CHECK:STDERR: fail_bad_run_2.carbon:[[@LINE+3]]:1: ERROR: Invalid signature for `Main.Run` function. Expected `fn ()` or `fn () -> i32`.
 // CHECK:STDERR: fn Run(n: i32) {}
-// CHECK:STDERR: ^~
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~
 fn Run(n: i32) {}
 
 // CHECK:STDOUT: --- fail_bad_run_2.carbon

toolchain/check/testdata/class/fail_base_bad_type.carbon

@@ -53,7 +53,7 @@ class DeriveFromIncomplete {
  // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~
  // CHECK:STDERR: fail_base_bad_type.carbon:[[@LINE-6]]:1: Class was forward declared here.
  // CHECK:STDERR: base class Incomplete;
- // CHECK:STDERR: ^~~~
+ // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~
  extend base: Incomplete;
 }
 
@@ -81,7 +81,7 @@ fn ConvertToBadBaseFinal(p: DeriveFromFinal*) -> Final* { return p; }
 // CHECK:STDOUT: %.loc32_22: type = tuple_type (type)
 // CHECK:STDOUT: %.loc32_23.1: type = tuple_type (Base)
 // CHECK:STDOUT: %.loc7_18.2: type = tuple_type ()
-// CHECK:STDOUT: %.loc7_1: type = ptr_type {}
+// CHECK:STDOUT: %.loc7_17: type = ptr_type {}
 // CHECK:STDOUT: %.loc32_23.2: type = tuple_type ({}*)
 // CHECK:STDOUT: %.loc33_1.1: type = struct_type {.base: (Base,)}
 // CHECK:STDOUT: %.loc33_1.2: type = struct_type {.base: ({}*,)}

toolchain/check/testdata/class/fail_base_modifiers.carbon

@@ -19,7 +19,7 @@ class C2 {
  // CHECK:STDERR: ^~~~~~~~
  // CHECK:STDERR: fail_base_modifiers.carbon:[[@LINE+3]]:3: ERROR: Missing `extend` before `base` declaration in class.
  // CHECK:STDERR: abstract base: B;
- // CHECK:STDERR: ^~~~~~~~
+ // CHECK:STDERR: ^~~~~~~~~~~~~~~~~
  abstract base: B;
 }
 
@@ -48,7 +48,7 @@ class C4 {
 // CHECK:STDOUT: constants {
 // CHECK:STDOUT: %.loc7_15.1: type = struct_type {}
 // CHECK:STDOUT: %.loc7_15.2: type = tuple_type ()
-// CHECK:STDOUT: %.loc7_1: type = ptr_type {}
+// CHECK:STDOUT: %.loc7_14: type = ptr_type {}
 // CHECK:STDOUT: %.loc14: type = struct_type {.base: B}
 // CHECK:STDOUT: }
 // CHECK:STDOUT:

toolchain/check/testdata/class/fail_base_no_extend.carbon

@@ -9,7 +9,7 @@ base class B {}
 class C {
  // CHECK:STDERR: fail_base_no_extend.carbon:[[@LINE+3]]:3: ERROR: Missing `extend` before `base` declaration in class.
  // CHECK:STDERR: base: B;
- // CHECK:STDERR: ^~~~
+ // CHECK:STDERR: ^~~~~~~~
  base: B;
 }
 
@@ -18,7 +18,7 @@ class C {
 // CHECK:STDOUT: constants {
 // CHECK:STDOUT: %.loc7_15.1: type = struct_type {}
 // CHECK:STDOUT: %.loc7_15.2: type = tuple_type ()
-// CHECK:STDOUT: %.loc7_1: type = ptr_type {}
+// CHECK:STDOUT: %.loc7_14: type = ptr_type {}
 // CHECK:STDOUT: %.loc14: type = struct_type {.base: B}
 // CHECK:STDOUT: }
 // CHECK:STDOUT:

toolchain/check/testdata/class/fail_base_repeated.carbon

@@ -35,7 +35,7 @@ class D {
 // CHECK:STDOUT: constants {
 // CHECK:STDOUT: %.loc7_16.1: type = struct_type {}
 // CHECK:STDOUT: %.loc7_16.2: type = tuple_type ()
-// CHECK:STDOUT: %.loc7_1: type = ptr_type {}
+// CHECK:STDOUT: %.loc7_15: type = ptr_type {}
 // CHECK:STDOUT: %.loc19: type = struct_type {.base: B1}
 // CHECK:STDOUT: }
 // CHECK:STDOUT: