From 84bc8cc4bfec0cf5170f79854fc0376597007304 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Tue, 7 Nov 2023 14:14:15 -0800 Subject: [PATCH] Fix crash when if expressions aren't in a function. (#3373) Another issue found while trying to make `package` work, lurking in fuzzer inputs. This leaves TODOs because we probably do want to support this, it's just non-trivial to fix. --- toolchain/check/context.cpp | 12 ++++- toolchain/check/context.h | 6 ++- toolchain/check/handle_if_expression.cpp | 6 +-- .../if_expression/fail_not_in_function.carbon | 53 +++++++++++++++++++ 4 files changed, 71 insertions(+), 6 deletions(-) create mode 100644 toolchain/check/testdata/if_expression/fail_not_in_function.carbon diff --git a/toolchain/check/context.cpp b/toolchain/check/context.cpp index 4aa6d6c20d8d..a77027383f07 100644 --- a/toolchain/check/context.cpp +++ b/toolchain/check/context.cpp @@ -379,9 +379,17 @@ auto Context::AddConvergenceBlockWithArgAndPush( } // Add the current code block to the enclosing function. -auto Context::AddCurrentCodeBlockToFunction() -> void { +auto Context::AddCurrentCodeBlockToFunction(Parse::Node parse_node) -> void { CARBON_CHECK(!inst_block_stack().empty()) << "no current code block"; - CARBON_CHECK(!return_scope_stack().empty()) << "no current function"; + + if (return_scope_stack().empty()) { + CARBON_CHECK(parse_node.is_valid()) + << "No current function, but parse_node not provided"; + TODO(parse_node, + "Control flow expressions are currently only supported inside " + "functions."); + return; + } if (!inst_block_stack().is_current_block_reachable()) { // Don't include unreachable blocks in the function. diff --git a/toolchain/check/context.h b/toolchain/check/context.h index 851c2a5acee2..35c644e203e8 100644 --- a/toolchain/check/context.h +++ b/toolchain/check/context.h @@ -158,7 +158,11 @@ class Context { std::initializer_list blocks_and_args) -> SemIR::InstId; // Add the current code block to the enclosing function. - auto AddCurrentCodeBlockToFunction() -> void; + // TODO: The parse_node is taken for expressions, which can occur in + // non-function contexts. This should be refactored to support non-function + // contexts, and parse_node removed. + auto AddCurrentCodeBlockToFunction( + Parse::Node parse_node = Parse::Node::Invalid) -> void; // Returns whether the current position in the current block is reachable. auto is_current_position_reachable() -> bool; diff --git a/toolchain/check/handle_if_expression.cpp b/toolchain/check/handle_if_expression.cpp index dd4c1fafec99..18a25ba8b0f7 100644 --- a/toolchain/check/handle_if_expression.cpp +++ b/toolchain/check/handle_if_expression.cpp @@ -22,7 +22,7 @@ auto HandleIfExpressionIf(Context& context, Parse::Node parse_node) -> bool { // Start emitting the `then` block. context.inst_block_stack().Pop(); context.inst_block_stack().Push(then_block_id); - context.AddCurrentCodeBlockToFunction(); + context.AddCurrentCodeBlockToFunction(parse_node); context.node_stack().Push(if_node, else_block_id); return true; @@ -38,7 +38,7 @@ auto HandleIfExpressionThen(Context& context, Parse::Node parse_node) -> bool { // Start emitting the `else` block. context.inst_block_stack().Push(else_block_id); - context.AddCurrentCodeBlockToFunction(); + context.AddCurrentCodeBlockToFunction(parse_node); context.node_stack().Push(parse_node, then_value_id); return true; @@ -64,7 +64,7 @@ auto HandleIfExpressionElse(Context& context, Parse::Node parse_node) -> bool { // Create a resumption block and branches to it. auto chosen_value_id = context.AddConvergenceBlockWithArgAndPush( if_node, {else_value_id, then_value_id}); - context.AddCurrentCodeBlockToFunction(); + context.AddCurrentCodeBlockToFunction(parse_node); // Push the result value. context.node_stack().Push(else_node, chosen_value_id); diff --git a/toolchain/check/testdata/if_expression/fail_not_in_function.carbon b/toolchain/check/testdata/if_expression/fail_not_in_function.carbon new file mode 100644 index 000000000000..f4692fb7bb3d --- /dev/null +++ b/toolchain/check/testdata/if_expression/fail_not_in_function.carbon @@ -0,0 +1,53 @@ +// 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 + +// TODO: Should work with compile-time evaluation. +// CHECK:STDERR: fail_not_in_function.carbon:[[@LINE+9]]:14: ERROR: Semantics TODO: `Control flow expressions are currently only supported inside functions.`. +// CHECK:STDERR: let x: i32 = if true then 1 else 0; +// CHECK:STDERR: ^ +// CHECK:STDERR: fail_not_in_function.carbon:[[@LINE+6]]:22: ERROR: Semantics TODO: `Control flow expressions are currently only supported inside functions.`. +// CHECK:STDERR: let x: i32 = if true then 1 else 0; +// CHECK:STDERR: ^ +// CHECK:STDERR: fail_not_in_function.carbon:[[@LINE+3]]:29: ERROR: Semantics TODO: `Control flow expressions are currently only supported inside functions.`. +// CHECK:STDERR: let x: i32 = if true then 1 else 0; +// CHECK:STDERR: ^ +let x: i32 = if true then 1 else 0; + +class C { + // TODO: Should work with compile-time evaluation. + // CHECK:STDERR: fail_not_in_function.carbon:[[@LINE+12]]:10: ERROR: Semantics TODO: `Control flow expressions are currently only supported inside functions.`. + // CHECK:STDERR: var n: if true then i32 else f64; + // CHECK:STDERR: ^ + // CHECK:STDERR: fail_not_in_function.carbon:[[@LINE+9]]:18: ERROR: Semantics TODO: `Control flow expressions are currently only supported inside functions.`. + // CHECK:STDERR: var n: if true then i32 else f64; + // CHECK:STDERR: ^ + // CHECK:STDERR: fail_not_in_function.carbon:[[@LINE+6]]:27: ERROR: Semantics TODO: `Control flow expressions are currently only supported inside functions.`. + // CHECK:STDERR: var n: if true then i32 else f64; + // CHECK:STDERR: ^ + // CHECK:STDERR: fail_not_in_function.carbon:[[@LINE+3]]:27: ERROR: Cannot evaluate type expression. + // CHECK:STDERR: var n: if true then i32 else f64; + // CHECK:STDERR: ^ + var n: if true then i32 else f64; +} + +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %.loc34: type = struct_type {.n: } +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: file "fail_not_in_function.carbon" { +// CHECK:STDOUT: %.loc17: i32 = block_arg +// CHECK:STDOUT: %x: i32 = bind_name "x", %.loc17 +// CHECK:STDOUT: class_declaration @C, () +// CHECK:STDOUT: %C: type = class_type @C +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: class @C { +// CHECK:STDOUT: %.loc33: bool = bool_literal true +// CHECK:STDOUT: if %.loc33 br !if.expr.then else br !if.expr.else +// CHECK:STDOUT: +// CHECK:STDOUT: !members: +// CHECK:STDOUT: .n = +// CHECK:STDOUT: }