Refactor diagnostics out of parser_context.h (#2834)

This is addressing an issue left behind by the context switch, removing a few diagnostics that had been in the header rather than figuring out proper homes. I'm splitting one for semis a little further, sharing one, and then the other two are actually able to be moved into more specific homes as-is (one is only used in one place, clearly an oversight that it wasn't there already).

toolchain/diagnostics/diagnostic_kind.def

@@ -58,6 +58,7 @@ CARBON_DIAGNOSTIC_KIND(ExpectedParameterName)
 CARBON_DIAGNOSTIC_KIND(ExpectedParenAfter)
 CARBON_DIAGNOSTIC_KIND(ExpectedSemiAfter)
 CARBON_DIAGNOSTIC_KIND(ExpectedSemiAfterExpression)
+CARBON_DIAGNOSTIC_KIND(ExpectedSemiAfterVar)
 CARBON_DIAGNOSTIC_KIND(ExpectedStructLiteralField)
 CARBON_DIAGNOSTIC_KIND(ExpectedVariableDeclaration)
 CARBON_DIAGNOSTIC_KIND(ExpectedVariableName)

toolchain/lowering/lowering_context.cpp

@@ -57,14 +57,12 @@ auto LoweringContext::BuildLoweredNodeAsType(SemanticsNodeId node_id)
     -> llvm::Type* {
   switch (node_id.index) {
     case SemanticsBuiltinKind::EmptyStructType.AsInt():
-    case SemanticsBuiltinKind::EmptyTuple.AsInt():
     case SemanticsBuiltinKind::EmptyTupleType.AsInt():
       // Represent empty types as empty structs.
       // TODO: Investigate special-casing handling of these so that they can be
       // collectively replaced with LLVM's void, particularly around function
       // returns. LLVM doesn't allow declaring variables with a void type, so
       // that may require significant special casing.
-      // TODO: Work to remove EmptyTuple here.
       return llvm::StructType::create(
           *llvm_context_, llvm::ArrayRef<llvm::Type*>(),
           SemanticsBuiltinKind::FromInt(node_id.index).name());

toolchain/lowering/lowering_handle.cpp

@@ -77,8 +77,9 @@ auto LoweringHandleFunctionDeclaration(LoweringContext& context,
   }
 
   llvm::Type* return_type = context.GetLoweredNodeAsType(
-      callable.return_type_id.is_valid() ? callable.return_type_id
-                                         : SemanticsNodeId::BuiltinEmptyTuple);
+      callable.return_type_id.is_valid()
+          ? callable.return_type_id
+          : SemanticsNodeId::BuiltinEmptyTupleType);
   llvm::FunctionType* function_type =
       llvm::FunctionType::get(return_type, args, /*isVarArg=*/false);
   auto* function = llvm::Function::Create(

toolchain/lowering/testdata/function/definition/params_one.carbon

@@ -6,9 +6,9 @@
 // CHECK:STDOUT: ; ModuleID = 'params_one.carbon'
 // CHECK:STDOUT: source_filename = "params_one.carbon"
 // CHECK:STDOUT:
-// CHECK:STDOUT: %EmptyTuple = type {}
+// CHECK:STDOUT: %EmptyTupleType = type {}
 // CHECK:STDOUT:
-// CHECK:STDOUT: define %EmptyTuple @Foo(i32 %a) {
+// CHECK:STDOUT: define %EmptyTupleType @Foo(i32 %a) {
 // CHECK:STDOUT: entry:
 // CHECK:STDOUT: }
 

toolchain/lowering/testdata/function/definition/params_two.carbon

@@ -6,9 +6,9 @@
 // CHECK:STDOUT: ; ModuleID = 'params_two.carbon'
 // CHECK:STDOUT: source_filename = "params_two.carbon"
 // CHECK:STDOUT:
-// CHECK:STDOUT: %EmptyTuple = type {}
+// CHECK:STDOUT: %EmptyTupleType = type {}
 // CHECK:STDOUT:
-// CHECK:STDOUT: define %EmptyTuple @Foo(i32 %a, i32 %b) {
+// CHECK:STDOUT: define %EmptyTupleType @Foo(i32 %a, i32 %b) {
 // CHECK:STDOUT: entry:
 // CHECK:STDOUT: }
 

toolchain/lowering/testdata/function/definition/params_zero.carbon

@@ -6,9 +6,9 @@
 // CHECK:STDOUT: ; ModuleID = 'params_zero.carbon'
 // CHECK:STDOUT: source_filename = "params_zero.carbon"
 // CHECK:STDOUT:
-// CHECK:STDOUT: %EmptyTuple = type {}
+// CHECK:STDOUT: %EmptyTupleType = type {}
 // CHECK:STDOUT:
-// CHECK:STDOUT: define %EmptyTuple @Foo() {
+// CHECK:STDOUT: define %EmptyTupleType @Foo() {
 // CHECK:STDOUT: entry:
 // CHECK:STDOUT: }
 

toolchain/lowering/testdata/return/no_value.carbon

@@ -6,9 +6,9 @@
 // CHECK:STDOUT: ; ModuleID = 'no_value.carbon'
 // CHECK:STDOUT: source_filename = "no_value.carbon"
 // CHECK:STDOUT:
-// CHECK:STDOUT: %EmptyTuple = type {}
+// CHECK:STDOUT: %EmptyTupleType = type {}
 // CHECK:STDOUT:
-// CHECK:STDOUT: define %EmptyTuple @Main() {
+// CHECK:STDOUT: define %EmptyTupleType @Main() {
 // CHECK:STDOUT: entry:
 // CHECK:STDOUT:   ret void
 // CHECK:STDOUT: }

toolchain/parser/parser_context.cpp

@@ -422,4 +422,14 @@ auto ParserContext::RecoverFromDeclarationError(StateStackEntry state,
           /*has_error=*/true);
 }
 
+auto ParserContext::EmitExpectedDeclarationSemiOrDefinition(
+    TokenKind expected_kind) -> void {
+  CARBON_DIAGNOSTIC(ExpectedDeclarationSemiOrDefinition, Error,
+                    "`{0}` should either end with a `;` for a declaration or "
+                    "have a `{{ ... }` block for a definition.",
+                    TokenKind);
+  emitter().Emit(*position(), ExpectedDeclarationSemiOrDefinition,
+                 expected_kind);
+}
+
 }  // namespace Carbon

toolchain/parser/parser_context.h

@@ -255,6 +255,8 @@ class ParserContext {
                                ParserState keyword_state, int subtree_start)
       -> void;
 
+  auto EmitExpectedDeclarationSemiOrDefinition(TokenKind expected_kind) -> void;
+
   // Handles error recovery in a declaration, particularly before any possible
   // definition has started (although one could be present). Recover to a
   // semicolon when it makes sense as a possible end, otherwise use the
@@ -302,24 +304,6 @@ class ParserContext {
   auto ParserHandle##Name(ParserContext& context)->void;
 #include "toolchain/parser/parser_state.def"
 
-// The diagnostics below may be emitted a couple different ways as part of
-// operator parsing.
-// TODO: Clean these up, maybe as context functions?
-
-CARBON_DIAGNOSTIC(
-    OperatorRequiresParentheses, Error,
-    "Parentheses are required to disambiguate operator precedence.");
-
-CARBON_DIAGNOSTIC(ExpectedSemiAfterExpression, Error,
-                  "Expected `;` after expression.");
-
-CARBON_DIAGNOSTIC(ExpectedDeclarationName, Error,
-                  "`{0}` introducer should be followed by a name.", TokenKind);
-CARBON_DIAGNOSTIC(ExpectedDeclarationSemiOrDefinition, Error,
-                  "`{0}` should either end with a `;` for a declaration or "
-                  "have a `{{ ... }` block for a definition.",
-                  TokenKind);
-
 }  // namespace Carbon
 
 #endif  // CARBON_TOOLCHAIN_PARSER_PARSER_CONTEXT_H_

toolchain/parser/parser_handle_declaration_name_and_params.cpp

@@ -13,6 +13,9 @@ static auto ParserHandleDeclarationNameAndParams(ParserContext& context,
 
   if (!context.ConsumeAndAddLeafNodeIf(TokenKind::Identifier,
                                        ParseNodeKind::DeclaredName)) {
+    CARBON_DIAGNOSTIC(ExpectedDeclarationName, Error,
+                      "`{0}` introducer should be followed by a name.",
+                      TokenKind);
     context.emitter().Emit(*context.position(), ExpectedDeclarationName,
                            context.tokens().GetKind(state.token));
     context.ReturnErrorOnState();

toolchain/parser/parser_handle_expression.cpp

@@ -6,6 +6,10 @@
 
 namespace Carbon {
 
+CARBON_DIAGNOSTIC(
+    OperatorRequiresParentheses, Error,
+    "Parentheses are required to disambiguate operator precedence.");
+
 auto ParserHandleExpression(ParserContext& context) -> void {
   auto state = context.PopState();
 
@@ -209,6 +213,8 @@ auto ParserHandleExpressionStatementFinish(ParserContext& context) -> void {
   }
 
   if (!state.has_error) {
+    CARBON_DIAGNOSTIC(ExpectedSemiAfterExpression, Error,
+                      "Expected `;` after expression.");
     context.emitter().Emit(*context.position(), ExpectedSemiAfterExpression);
   }
 

toolchain/parser/parser_handle_function.cpp

@@ -74,9 +74,7 @@ auto ParserHandleFunctionSignatureFinish(ParserContext& context) -> void {
     }
     default: {
       if (!state.has_error) {
-        context.emitter().Emit(*context.position(),
-                               ExpectedDeclarationSemiOrDefinition,
-                               TokenKind::Fn);
+        context.EmitExpectedDeclarationSemiOrDefinition(TokenKind::Fn);
       }
       // Only need to skip if we've not already found a new line.
       bool skip_past_likely_end =

toolchain/parser/parser_handle_type.cpp

@@ -58,9 +58,8 @@ static auto ParserHandleTypeAfterParams(ParserContext& context,
   }
 
   if (!context.PositionIs(TokenKind::OpenCurlyBrace)) {
-    context.emitter().Emit(*context.position(),
-                           ExpectedDeclarationSemiOrDefinition,
-                           context.tokens().GetKind(state.token));
+    context.EmitExpectedDeclarationSemiOrDefinition(
+        context.tokens().GetKind(state.token));
     context.RecoverFromDeclarationError(state, declaration_kind,
                                         /*skip_past_likely_end=*/true);
     return;

toolchain/parser/parser_handle_var.cpp

@@ -52,7 +52,10 @@ auto ParserHandleVarFinishAsSemicolon(ParserContext& context) -> void {
   if (context.PositionIs(TokenKind::Semi)) {
     end_token = context.Consume();
   } else {
-    context.emitter().Emit(*context.position(), ExpectedSemiAfterExpression);
+    // TODO: Disambiguate between statement and member declaration.
+    CARBON_DIAGNOSTIC(ExpectedSemiAfterVar, Error,
+                      "Expected `;` to terminate `var` declaration.");
+    context.emitter().Emit(*context.position(), ExpectedSemiAfterVar);
     state.has_error = true;
     if (auto semi_token = context.SkipPastLikelyEnd(state.token)) {
       end_token = *semi_token;

toolchain/parser/testdata/basics/fail_paren_match_regression.carbon

@@ -18,5 +18,5 @@
 
 // CHECK:STDERR: fail_paren_match_regression.carbon:[[@LINE+3]]:5: Expected pattern in `var` declaration.
 // CHECK:STDERR: fail_paren_match_regression.carbon:[[@LINE+2]]:12: Expected `,` or `)`.
-// CHECK:STDERR: fail_paren_match_regression.carbon:[[@LINE+1]]:15: Expected `;` after expression.
+// CHECK:STDERR: fail_paren_match_regression.carbon:[[@LINE+1]]:15: Expected `;` to terminate `var` declaration.
 var = (foo {})

toolchain/parser/testdata/operators/fail_infix_uneven_space_after.carbon

@@ -17,5 +17,5 @@
 
 // TODO: We could figure out that this first Failed example is infix
 // with one-token lookahead.
-// CHECK:STDERR: fail_infix_uneven_space_after.carbon:[[@LINE+1]]:16: Expected `;` after expression.
+// CHECK:STDERR: fail_infix_uneven_space_after.carbon:[[@LINE+1]]:16: Expected `;` to terminate `var` declaration.
 var n: i8 = n* n;

toolchain/parser/testdata/operators/fail_star_star_no_space.carbon

@@ -20,5 +20,5 @@
 // before we notice the missing whitespace around the second `*`.
 // It'd be better to (somehow) form n*(*p) and reject due to the missing
 // whitespace around the first `*`.
-// CHECK:STDERR: fail_star_star_no_space.carbon:[[@LINE+1]]:16: Expected `;` after expression.
+// CHECK:STDERR: fail_star_star_no_space.carbon:[[@LINE+1]]:16: Expected `;` to terminate `var` declaration.
 var n: i8 = n**p;