From 0396bbf9df01791f7ab2396f82b2dc97917c46df Mon Sep 17 00:00:00 2001 From: czapiga Date: Wed, 12 Feb 2025 21:10:51 +0100 Subject: [PATCH] Improve `match` parsing diagnostics (#4934) Adds diagnostics for missing `case` guard parenthesis. Adds test for which `lex` parenthesis check succeeds but errors are reported in `parse` phase. --- toolchain/diagnostics/diagnostic_kind.def | 11 +-- toolchain/parse/handle_match.cpp | 21 +++--- .../fail_missing_guard_close_paren.carbon | 2 +- .../fail_missing_guard_open_paren.carbon | 2 +- ...sing_guard_parens_only_parse_errors.carbon | 72 +++++++++++++++++++ 5 files changed, 89 insertions(+), 19 deletions(-) create mode 100644 toolchain/parse/testdata/match/fail_missing_guard_parens_only_parse_errors.carbon diff --git a/toolchain/diagnostics/diagnostic_kind.def b/toolchain/diagnostics/diagnostic_kind.def index ecb4ecb25746..24a229f67585 100644 --- a/toolchain/diagnostics/diagnostic_kind.def +++ b/toolchain/diagnostics/diagnostic_kind.def @@ -90,7 +90,6 @@ CARBON_DIAGNOSTIC_KIND(ExpectedVarAfterReturned) CARBON_DIAGNOSTIC_KIND(ExpectedVariableDecl) CARBON_DIAGNOSTIC_KIND(ExpectedChoiceDefinition) CARBON_DIAGNOSTIC_KIND(ExpectedChoiceAlternativeName) -CARBON_DIAGNOSTIC_KIND(UnreachableMatchCase) CARBON_DIAGNOSTIC_KIND(OperatorRequiresParentheses) CARBON_DIAGNOSTIC_KIND(StatementOperatorAsSubExpr) CARBON_DIAGNOSTIC_KIND(UnaryOperatorRequiresParentheses) @@ -101,12 +100,14 @@ CARBON_DIAGNOSTIC_KIND(UnrecognizedDecl) CARBON_DIAGNOSTIC_KIND(UnexpectedTokenInCompoundMemberAccess) // Match diagnostics. -CARBON_DIAGNOSTIC_KIND(UnexpectedTokenInMatchCasesBlock) -CARBON_DIAGNOSTIC_KIND(ExpectedMatchCases) -CARBON_DIAGNOSTIC_KIND(ExpectedMatchCasesBlock) CARBON_DIAGNOSTIC_KIND(ExpectedMatchCaseArrow) CARBON_DIAGNOSTIC_KIND(ExpectedMatchCaseBlock) -CARBON_DIAGNOSTIC_KIND(MatchParseTODO) +CARBON_DIAGNOSTIC_KIND(ExpectedMatchCaseGuardCloseParen) +CARBON_DIAGNOSTIC_KIND(ExpectedMatchCaseGuardOpenParen) +CARBON_DIAGNOSTIC_KIND(ExpectedMatchCases) +CARBON_DIAGNOSTIC_KIND(ExpectedMatchCasesBlock) +CARBON_DIAGNOSTIC_KIND(UnexpectedTokenInMatchCasesBlock) +CARBON_DIAGNOSTIC_KIND(UnreachableMatchCase) // Package-related diagnostics. CARBON_DIAGNOSTIC_KIND(FirstDecl) diff --git a/toolchain/parse/handle_match.cpp b/toolchain/parse/handle_match.cpp index feb98912c74d..034ca2da2b76 100644 --- a/toolchain/parse/handle_match.cpp +++ b/toolchain/parse/handle_match.cpp @@ -7,15 +7,6 @@ namespace Carbon::Parse { -// TODO: Some handling is producing erroneous nodes, but wasn't diagnosing and -// needed to. A more specific diagnostic could be added, but we should probably -// have a larger look at this code and how it produces parse errors. This may be -// good to re-examine when someone is working on implementing match checking. -static auto DiagnoseMatchParseTODO(Context& context) -> void { - CARBON_DIAGNOSTIC(MatchParseTODO, Error, "TODO: improve match parsing"); - context.emitter().Emit(*context.position(), MatchParseTODO); -} - static auto HandleStatementsBlockStart(Context& context, State finish, NodeKind equal_greater, NodeKind starter, NodeKind complete) -> void { @@ -171,11 +162,14 @@ auto HandleMatchCaseAfterPattern(Context& context) -> void { context.PushState(State::Expr); } else { if (!state.has_error) { - DiagnoseMatchParseTODO(context); + CARBON_DIAGNOSTIC(ExpectedMatchCaseGuardOpenParen, Error, + "expected `(` after `if`"); + context.emitter().Emit(*context.position(), + ExpectedMatchCaseGuardOpenParen); } context.AddLeafNode(NodeKind::MatchCaseGuardStart, *context.position(), - true); + /*has_error=*/true); context.AddInvalidParse(*context.position()); state = context.PopState(); context.AddNode(NodeKind::MatchCaseGuard, *context.position(), @@ -199,7 +193,10 @@ auto HandleMatchCaseGuardFinish(Context& context) -> void { context.AddNode(NodeKind::MatchCaseGuard, *close_paren, state.has_error); } else { if (!state.has_error) { - DiagnoseMatchParseTODO(context); + CARBON_DIAGNOSTIC(ExpectedMatchCaseGuardCloseParen, Error, + "expected `)`"); + context.emitter().Emit(*context.position(), + ExpectedMatchCaseGuardCloseParen); } context.AddNode(NodeKind::MatchCaseGuard, *context.position(), diff --git a/toolchain/parse/testdata/match/fail_missing_guard_close_paren.carbon b/toolchain/parse/testdata/match/fail_missing_guard_close_paren.carbon index 3554ccfbe53f..1ae02bdcfbba 100644 --- a/toolchain/parse/testdata/match/fail_missing_guard_close_paren.carbon +++ b/toolchain/parse/testdata/match/fail_missing_guard_close_paren.carbon @@ -14,7 +14,7 @@ fn f() -> i32 { // CHECK:STDERR: case x: bool if (false => { return 1; } // CHECK:STDERR: ^ // CHECK:STDERR: - // CHECK:STDERR: fail_missing_guard_close_paren.carbon:[[@LINE+4]]:28: error: TODO: improve match parsing [MatchParseTODO] + // CHECK:STDERR: fail_missing_guard_close_paren.carbon:[[@LINE+4]]:28: error: expected `)` [ExpectedMatchCaseGuardCloseParen] // CHECK:STDERR: case x: bool if (false => { return 1; } // CHECK:STDERR: ^~ // CHECK:STDERR: diff --git a/toolchain/parse/testdata/match/fail_missing_guard_open_paren.carbon b/toolchain/parse/testdata/match/fail_missing_guard_open_paren.carbon index 2e44c0ce64af..f58ff601dd3d 100644 --- a/toolchain/parse/testdata/match/fail_missing_guard_open_paren.carbon +++ b/toolchain/parse/testdata/match/fail_missing_guard_open_paren.carbon @@ -10,7 +10,7 @@ fn f() -> i32 { match (true) { - // CHECK:STDERR: fail_missing_guard_open_paren.carbon:[[@LINE+8]]:21: error: TODO: improve match parsing [MatchParseTODO] + // CHECK:STDERR: fail_missing_guard_open_paren.carbon:[[@LINE+8]]:21: error: expected `(` after `if` [ExpectedMatchCaseGuardOpenParen] // CHECK:STDERR: case x: bool if false) => { return 1; } // CHECK:STDERR: ^~~~~ // CHECK:STDERR: diff --git a/toolchain/parse/testdata/match/fail_missing_guard_parens_only_parse_errors.carbon b/toolchain/parse/testdata/match/fail_missing_guard_parens_only_parse_errors.carbon new file mode 100644 index 000000000000..813788cfdbd8 --- /dev/null +++ b/toolchain/parse/testdata/match/fail_missing_guard_parens_only_parse_errors.carbon @@ -0,0 +1,72 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +// AUTOUPDATE +// TIP: To test this file alone, run: +// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/parse/testdata/match/fail_missing_guard_parens_only_parse_errors.carbon +// TIP: To dump output, run: +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/parse/testdata/match/fail_missing_guard_parens_only_parse_errors.carbon + +fn f() -> i32 { + match (true) { + // CHECK:STDERR: fail_missing_guard_parens_only_parse_errors.carbon:[[@LINE+4]]:33: error: expected `)` [ExpectedMatchCaseGuardCloseParen] + // CHECK:STDERR: case x: bool if (x == false => { return 1; } + // CHECK:STDERR: ^~ + // CHECK:STDERR: + case x: bool if (x == false => { return 1; } + case y: bool if (not y) => { return 2; } + // CHECK:STDERR: fail_missing_guard_parens_only_parse_errors.carbon:[[@LINE+4]]:21: error: expected `(` after `if` [ExpectedMatchCaseGuardOpenParen] + // CHECK:STDERR: case z: bool if true or false) => { return 3; } + // CHECK:STDERR: ^~~~ + // CHECK:STDERR: + case z: bool if true or false) => { return 3; } + } + return 0; +} + +// CHECK:STDOUT: - filename: fail_missing_guard_parens_only_parse_errors.carbon +// CHECK:STDOUT: parse_tree: [ +// CHECK:STDOUT: {kind: 'FileStart', text: ''}, +// CHECK:STDOUT: {kind: 'FunctionIntroducer', text: 'fn'}, +// CHECK:STDOUT: {kind: 'IdentifierNameBeforeParams', text: 'f'}, +// CHECK:STDOUT: {kind: 'TuplePatternStart', text: '('}, +// CHECK:STDOUT: {kind: 'TuplePattern', text: ')', subtree_size: 2}, +// CHECK:STDOUT: {kind: 'IntTypeLiteral', text: 'i32'}, +// CHECK:STDOUT: {kind: 'ReturnType', text: '->', subtree_size: 2}, +// CHECK:STDOUT: {kind: 'FunctionDefinitionStart', text: '{', subtree_size: 7}, +// CHECK:STDOUT: {kind: 'MatchIntroducer', text: 'match'}, +// CHECK:STDOUT: {kind: 'MatchConditionStart', text: '('}, +// CHECK:STDOUT: {kind: 'BoolLiteralTrue', text: 'true'}, +// CHECK:STDOUT: {kind: 'MatchCondition', text: ')', subtree_size: 3}, +// CHECK:STDOUT: {kind: 'MatchStatementStart', text: '{', subtree_size: 5}, +// CHECK:STDOUT: {kind: 'MatchCaseIntroducer', text: 'case'}, +// CHECK:STDOUT: {kind: 'IdentifierNameNotBeforeParams', text: 'x'}, +// CHECK:STDOUT: {kind: 'BoolTypeLiteral', text: 'bool'}, +// CHECK:STDOUT: {kind: 'LetBindingPattern', text: ':', subtree_size: 3}, +// CHECK:STDOUT: {kind: 'MatchCaseGuardIntroducer', text: 'if'}, +// CHECK:STDOUT: {kind: 'MatchCaseGuardStart', text: '('}, +// CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'x'}, +// CHECK:STDOUT: {kind: 'BoolLiteralFalse', text: 'false'}, +// CHECK:STDOUT: {kind: 'InfixOperatorEqualEqual', text: '==', subtree_size: 3}, +// CHECK:STDOUT: {kind: 'MatchCaseGuard', text: '=>', has_error: yes, subtree_size: 6}, +// CHECK:STDOUT: {kind: 'MatchCaseEqualGreater', text: 'case', has_error: yes}, +// CHECK:STDOUT: {kind: 'MatchCaseStart', text: 'case', has_error: yes, subtree_size: 12}, +// CHECK:STDOUT: {kind: 'MatchCase', text: 'case', has_error: yes, subtree_size: 13}, +// CHECK:STDOUT: {kind: 'MatchCaseIntroducer', text: 'case'}, +// CHECK:STDOUT: {kind: 'IdentifierNameNotBeforeParams', text: 'z'}, +// CHECK:STDOUT: {kind: 'BoolTypeLiteral', text: 'bool'}, +// CHECK:STDOUT: {kind: 'LetBindingPattern', text: ':', subtree_size: 3}, +// CHECK:STDOUT: {kind: 'MatchCaseGuardIntroducer', text: 'if'}, +// CHECK:STDOUT: {kind: 'MatchCaseGuardStart', text: 'true', has_error: yes}, +// CHECK:STDOUT: {kind: 'InvalidParse', text: 'true', has_error: yes}, +// CHECK:STDOUT: {kind: 'MatchCaseGuard', text: 'true', has_error: yes, subtree_size: 4}, +// CHECK:STDOUT: {kind: 'MatchCaseStart', text: 'true', has_error: yes, subtree_size: 9}, +// CHECK:STDOUT: {kind: 'MatchCase', text: 'true', has_error: yes, subtree_size: 10}, +// CHECK:STDOUT: {kind: 'MatchStatement', text: '}', subtree_size: 29}, +// CHECK:STDOUT: {kind: 'ReturnStatementStart', text: 'return'}, +// CHECK:STDOUT: {kind: 'IntLiteral', text: '0'}, +// CHECK:STDOUT: {kind: 'ReturnStatement', text: ';', subtree_size: 3}, +// CHECK:STDOUT: {kind: 'FunctionDefinition', text: '}', subtree_size: 40}, +// CHECK:STDOUT: {kind: 'FileEnd', text: ''}, +// CHECK:STDOUT: ]