Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Abort checking when encountering an invalid parse node #4700

Merged
merged 2 commits into from
Dec 18, 2024
Merged

Abort checking when encountering an invalid parse node #4700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
55afe8d
Abort checking when encountering an invalid parse node
dwblaikie Dec 17, 2024
9f5bb2c
CARBON_FATAL/CARBON_CHECK for cases that are (for now) unreachable
dwblaikie Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions toolchain/check/check_unit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,11 @@ auto CheckUnit::ProcessNodeIds() -> bool {
node_id = *maybe_node_id;
auto parse_kind = context_.parse_tree().node_kind(node_id);

if (context_.parse_tree().node_has_error(node_id)) {
context_.TODO(node_id, "handle invalid parse trees in `check`");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where this causes code to no longer be reached, would it be worth switching to CARBON_FATAL?

e.g.:

  • HandleInvalidParse (and variants)
  • The pre-existing TODOs for "Error recovery from keyword name"

(for contrast, valid code encounters "function with positional parameters", you're only removing 1 case of 4)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yep - done.
I did change the "Error recovery from keyword name" to CARBON_CHECK since otherwise they'd be branch-to-unreachable (which is disfavorable in the LLVM style, at least) - happy to switch back to CARBON_FATAL if it makes more sense in this particular case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to CHECK where it fits.

return false;
}

bool result;
switch (parse_kind) {
#define CARBON_PARSE_NODE_KIND(Name) \
Expand Down
10 changes: 4 additions & 6 deletions toolchain/check/handle_name.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,19 +127,17 @@ auto HandleParseNode(Context& context, Parse::IdentifierNameId node_id)
-> bool {
// The parent is responsible for binding the name.
auto name_id = GetIdentifierAsName(context, node_id);
if (!name_id) {
return context.TODO(node_id, "Error recovery from keyword name.");
}
CARBON_CHECK(name_id,
"Unreachable until we support checking error parse nodes");
context.node_stack().Push(node_id, *name_id);
return true;
}

auto HandleParseNode(Context& context, Parse::IdentifierNameExprId node_id)
-> bool {
auto name_id = GetIdentifierAsName(context, node_id);
if (!name_id) {
return context.TODO(node_id, "Error recovery from keyword name.");
}
CARBON_CHECK(name_id,
"Unreachable until we support checking error parse nodes");
context.node_stack().Push(node_id,
HandleNameAsExpr(context, node_id, *name_id));
return true;
Expand Down
17 changes: 9 additions & 8 deletions toolchain/check/handle_noop.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,19 @@ auto HandleParseNode(Context& /*context*/, Parse::EmptyDeclId /*node_id*/)
return true;
}

auto HandleParseNode(Context& context, Parse::InvalidParseId node_id) -> bool {
return context.TODO(node_id, "HandleInvalidParse");
auto HandleParseNode(Context& /*context*/, Parse::InvalidParseId /*node_id*/)
-> bool {
CARBON_FATAL("Unreachable until we support checking error parse nodes");
}

auto HandleParseNode(Context& context, Parse::InvalidParseStartId node_id)
-> bool {
return context.TODO(node_id, "HandleInvalidParseStart");
auto HandleParseNode(Context& /*context*/,
Parse::InvalidParseStartId /*node_id*/) -> bool {
CARBON_FATAL("Unreachable until we support checking error parse nodes");
}

auto HandleParseNode(Context& context, Parse::InvalidParseSubtreeId node_id)
-> bool {
return context.TODO(node_id, "HandleInvalidParseSubtree");
auto HandleParseNode(Context& /*context*/,
Parse::InvalidParseSubtreeId /*node_id*/) -> bool {
CARBON_FATAL("Unreachable until we support checking error parse nodes");
}

auto HandleParseNode(Context& /*context*/, Parse::PlaceholderId /*node_id*/)
Expand Down
4 changes: 2 additions & 2 deletions toolchain/check/testdata/basics/type_literals.carbon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ var test_i15: i15;
// CHECK:STDERR:
var test_i1000000000: i1000000000;
// TODO: This diagnostic is not very good.
// CHECK:STDERR: fail_iN_bad_width.carbon:[[@LINE+8]]:33: error: semantics TODO: `HandleInvalidParse` [SemanticsTodo]
// CHECK:STDERR: fail_iN_bad_width.carbon:[[@LINE+8]]:33: error: semantics TODO: `handle invalid parse trees in `check`` [SemanticsTodo]
// CHECK:STDERR: var test_i10000000000000000000: i10000000000000000000;
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~
// CHECK:STDERR:
Expand Down Expand Up @@ -86,7 +86,7 @@ var test_u15: u15;
// CHECK:STDERR:
var test_u1000000000: u1000000000;
// TODO: This diagnostic is not very good.
// CHECK:STDERR: fail_uN_bad_width.carbon:[[@LINE+8]]:33: error: semantics TODO: `HandleInvalidParse` [SemanticsTodo]
// CHECK:STDERR: fail_uN_bad_width.carbon:[[@LINE+8]]:33: error: semantics TODO: `handle invalid parse trees in `check`` [SemanticsTodo]
// CHECK:STDERR: var test_u10000000000000000000: u10000000000000000000;
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~
// CHECK:STDERR:
Expand Down
2 changes: 1 addition & 1 deletion toolchain/check/testdata/class/fail_base_as_declared_name.carbon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ namespace N;
// CHECK:STDERR: fn N.base() {}
// CHECK:STDERR: ^~~~
// CHECK:STDERR:
// CHECK:STDERR: fail_base_as_declared_name.carbon:[[@LINE+3]]:6: error: semantics TODO: `HandleInvalidParse` [SemanticsTodo]
// CHECK:STDERR: fail_base_as_declared_name.carbon:[[@LINE+3]]:6: error: semantics TODO: `handle invalid parse trees in `check`` [SemanticsTodo]
// CHECK:STDERR: fn N.base() {}
// CHECK:STDERR: ^~~~
fn N.base() {}
Expand Down
2 changes: 1 addition & 1 deletion toolchain/check/testdata/class/fail_base_misplaced.carbon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ fn F() {
// CHECK:STDERR: extend base: B;
// CHECK:STDERR: ^~~~~~
// CHECK:STDERR:
// CHECK:STDERR: fail_base_misplaced.carbon:[[@LINE+3]]:3: error: semantics TODO: `HandleInvalidParse` [SemanticsTodo]
// CHECK:STDERR: fail_base_misplaced.carbon:[[@LINE+3]]:3: error: semantics TODO: `handle invalid parse trees in `check`` [SemanticsTodo]
// CHECK:STDERR: extend base: B;
// CHECK:STDERR: ^~~~~~
extend base: B;
Expand Down
2 changes: 1 addition & 1 deletion toolchain/check/testdata/class/fail_self_type_member.carbon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ fn F() -> bool {
// CHECK:STDERR: var c2: Class.Self = c1;
// CHECK:STDERR: ^~~~
// CHECK:STDERR:
// CHECK:STDERR: fail_self_type_member.carbon:[[@LINE+3]]:17: error: semantics TODO: `Error recovery from keyword name.` [SemanticsTodo]
// CHECK:STDERR: fail_self_type_member.carbon:[[@LINE+3]]:17: error: semantics TODO: `handle invalid parse trees in `check`` [SemanticsTodo]
// CHECK:STDERR: var c2: Class.Self = c1;
// CHECK:STDERR: ^~~~
var c2: Class.Self = c1;
Expand Down
2 changes: 1 addition & 1 deletion toolchain/check/testdata/class/fail_todo_local_class.carbon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ class A {
// CHECK:STDERR: class B {
// CHECK:STDERR: ^~~~~
// CHECK:STDERR:
// CHECK:STDERR: fail_todo_local_class.carbon:[[@LINE+3]]:5: error: semantics TODO: `HandleInvalidParse` [SemanticsTodo]
// CHECK:STDERR: fail_todo_local_class.carbon:[[@LINE+3]]:5: error: semantics TODO: `handle invalid parse trees in `check`` [SemanticsTodo]
// CHECK:STDERR: class B {
// CHECK:STDERR: ^~~~~
class B {
Expand Down
18 changes: 5 additions & 13 deletions toolchain/check/testdata/function/declaration/no_prelude/extern_library.carbon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,10 @@ 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]
Expand Down Expand Up @@ -294,19 +298,7 @@ extern library "extern_library_owner" fn F() {}
// CHECK:STDOUT:
// CHECK:STDOUT: --- fail_extern_library_mismatch_owner.carbon
// CHECK:STDOUT:
// CHECK:STDOUT: constants {
// CHECK:STDOUT: %F.type: type = fn_type @F [template]
// CHECK:STDOUT: %F: %F.type = struct_value () [template]
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: file {
// CHECK:STDOUT: package: <namespace> = namespace [template] {
// CHECK:STDOUT: .F = %F.decl
// CHECK:STDOUT: }
// CHECK:STDOUT: %F.decl: %F.type = fn_decl @F [template = constants.%F] {} {}
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: fn @F();
// CHECK:STDOUT: file {}
// CHECK:STDOUT:
// CHECK:STDOUT: --- fail_extern_self_library.carbon
// CHECK:STDOUT:
Expand Down
2 changes: 1 addition & 1 deletion toolchain/check/testdata/function/declaration/no_prelude/fail_pattern_in_signature.carbon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
// CHECK:STDERR: fn F((a: {}, b: {}), c: {});
// CHECK:STDERR: ^
// CHECK:STDERR:
// CHECK:STDERR: fail_pattern_in_signature.carbon:[[@LINE+3]]:6: error: semantics TODO: `Error recovery from keyword name.` [SemanticsTodo]
// CHECK:STDERR: fail_pattern_in_signature.carbon:[[@LINE+3]]:6: error: semantics TODO: `handle invalid parse trees in `check`` [SemanticsTodo]
// CHECK:STDERR: fn F((a: {}, b: {}), c: {});
// CHECK:STDERR: ^
fn F((a: {}, b: {}), c: {});
Expand Down
16 changes: 2 additions & 14 deletions toolchain/check/testdata/function/declaration/no_prelude/fail_todo_no_params.carbon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ 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: `function with positional parameters` [SemanticsTodo]
// CHECK:STDERR: fail_todo_arrow_body.carbon:[[@LINE+8]]:1: error: semantics TODO: `handle invalid parse trees in `check`` [SemanticsTodo]
// CHECK:STDERR: fn A => 0;
// CHECK:STDERR: ^~~~~~~~~~
// CHECK:STDERR:
Expand Down Expand Up @@ -95,19 +95,7 @@ fn A {
// CHECK:STDOUT:
// CHECK:STDOUT: --- fail_todo_arrow_body.carbon
// CHECK:STDOUT:
// CHECK:STDOUT: constants {
// CHECK:STDOUT: %A.type: type = fn_type @A [template]
// CHECK:STDOUT: %A: %A.type = struct_value () [template]
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: file {
// CHECK:STDOUT: package: <namespace> = namespace [template] {
// CHECK:STDOUT: .A = %A.decl
// CHECK:STDOUT: }
// CHECK:STDOUT: %A.decl: %A.type = fn_decl @A [template = constants.%A] {} {}
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: fn @A();
// CHECK:STDOUT: file {}
// CHECK:STDOUT:
// CHECK:STDOUT: --- fail_invalid_file_generic_regression_test.carbon
// CHECK:STDOUT:
Expand Down
54 changes: 6 additions & 48 deletions toolchain/check/testdata/packages/no_prelude/export_import.carbon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,14 @@ var c: C = {.x = ()};

impl library "[[@TEST_NAME]]";

// CHECK:STDERR: fail_export_in_impl.impl.carbon:[[@LINE+4]]:1: error: `export` is only allowed in API files [ExportFromImpl]
// CHECK:STDERR: fail_export_in_impl.impl.carbon:[[@LINE+8]]:1: error: `export` is only allowed in API files [ExportFromImpl]
// CHECK:STDERR: export import library "base";
// CHECK:STDERR: ^~~~~~
// CHECK:STDERR:
// CHECK:STDERR: fail_export_in_impl.impl.carbon:[[@LINE+4]]:1: error: semantics TODO: `handle invalid parse trees in `check`` [SemanticsTodo]
// CHECK:STDERR: export import library "base";
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// CHECK:STDERR:
export import library "base";

// Note the import still occurs.
Expand Down Expand Up @@ -317,53 +321,7 @@ var indirect_c: C = {.x = ()};
// CHECK:STDOUT:
// CHECK:STDOUT: --- fail_export_in_impl.impl.carbon
// CHECK:STDOUT:
// CHECK:STDOUT: constants {
// CHECK:STDOUT: %C: type = class_type @C [template]
// CHECK:STDOUT: %empty_tuple.type: type = tuple_type () [template]
// CHECK:STDOUT: %struct_type.x: type = struct_type {.x: %empty_tuple.type} [template]
// CHECK:STDOUT: %complete_type: <witness> = complete_type_witness %struct_type.x [template]
// CHECK:STDOUT: %empty_tuple: %empty_tuple.type = tuple_value () [template]
// CHECK:STDOUT: %C.val: %C = struct_value (%empty_tuple) [template]
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: imports {
// CHECK:STDOUT: %import_ref.1: type = import_ref Main//base, C, loaded [template = constants.%C]
// CHECK:STDOUT: %import_ref.2: <witness> = import_ref Main//base, loc6_1, loaded [template = constants.%complete_type]
// CHECK:STDOUT: %import_ref.3 = import_ref Main//base, inst14 [no loc], unloaded
// CHECK:STDOUT: %import_ref.4 = import_ref Main//base, loc5_8, unloaded
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: file {
// CHECK:STDOUT: package: <namespace> = namespace [template] {
// CHECK:STDOUT: .C = imports.%import_ref.1
// CHECK:STDOUT: .c = %c
// CHECK:STDOUT: }
// CHECK:STDOUT: %default.import.loc2_6.1 = import <invalid>
// CHECK:STDOUT: %default.import.loc2_6.2 = import <invalid>
// CHECK:STDOUT: %C.ref: type = name_ref C, imports.%import_ref.1 [template = constants.%C]
// CHECK:STDOUT: %c.var: ref %C = var c
// CHECK:STDOUT: %c: ref %C = bind_name c, %c.var
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: class @C [from "base.carbon"] {
// CHECK:STDOUT: !members:
// CHECK:STDOUT: .Self = imports.%import_ref.3
// CHECK:STDOUT: .x = imports.%import_ref.4
// CHECK:STDOUT: complete_type_witness = imports.%import_ref.2
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: fn @__global_init() {
// CHECK:STDOUT: !entry:
// CHECK:STDOUT: %.loc11_19.1: %empty_tuple.type = tuple_literal ()
// CHECK:STDOUT: %.loc11_20.1: %struct_type.x = struct_literal (%.loc11_19.1)
// CHECK:STDOUT: %.loc11_20.2: ref %empty_tuple.type = class_element_access file.%c.var, element0
// CHECK:STDOUT: %.loc11_19.2: init %empty_tuple.type = tuple_init () to %.loc11_20.2 [template = constants.%empty_tuple]
// CHECK:STDOUT: %.loc11_20.3: init %empty_tuple.type = converted %.loc11_19.1, %.loc11_19.2 [template = constants.%empty_tuple]
// CHECK:STDOUT: %.loc11_20.4: init %C = class_init (%.loc11_20.3), file.%c.var [template = constants.%C.val]
// CHECK:STDOUT: %.loc11_21: init %C = converted %.loc11_20.1, %.loc11_20.4 [template = constants.%C.val]
// CHECK:STDOUT: assign file.%c.var, %.loc11_21
// CHECK:STDOUT: return
// CHECK:STDOUT: }
// CHECK:STDOUT: file {}
// CHECK:STDOUT:
// CHECK:STDOUT: --- export_export.impl.carbon
// CHECK:STDOUT:
Expand Down
2 changes: 1 addition & 1 deletion toolchain/check/testdata/struct/fail_keyword_name.carbon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
// CHECK:STDERR: fn F() -> {.class: i32};
// CHECK:STDERR: ^~~~~
// CHECK:STDERR:
// CHECK:STDERR: fail_keyword_name.carbon:[[@LINE+4]]:13: error: semantics TODO: `Error recovery from keyword name.` [SemanticsTodo]
// CHECK:STDERR: fail_keyword_name.carbon:[[@LINE+4]]:13: error: semantics TODO: `handle invalid parse trees in `check`` [SemanticsTodo]
// CHECK:STDERR: fn F() -> {.class: i32};
// CHECK:STDERR: ^~~~~
// CHECK:STDERR:
Expand Down
2 changes: 1 addition & 1 deletion toolchain/lower/testdata/basics/fail_before_lowering.carbon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
// CHECK:STDERR: a;
// CHECK:STDERR: ^
// CHECK:STDERR:
// CHECK:STDERR: fail_before_lowering.carbon:[[@LINE+3]]:1: error: semantics TODO: `HandleInvalidParseStart` [SemanticsTodo]
// CHECK:STDERR: fail_before_lowering.carbon:[[@LINE+3]]:1: error: semantics TODO: `handle invalid parse trees in `check`` [SemanticsTodo]
// CHECK:STDERR: a;
// CHECK:STDERR: ^
a;
Loading