From d42c1a3a7c606eaee83d9481484c2b0974f14d39 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Fri, 27 Oct 2023 18:00:59 -0700 Subject: [PATCH] Diagnose attempts to copy a non-copyable type. (#3345) For now, we treat class types and `String` as non-copyable, because we don't know how to emit SemIR to copy them yet. This will change as we add support for copying those types when appropriate. --------- Co-authored-by: Jon Ross-Perkins --- toolchain/check/convert.cpp | 55 ++++++++++++++++--- .../testdata/basics/builtin_types.carbon | 6 +- .../testdata/var/fail_not_copyable.carbon | 52 ++++++++++++++++++ toolchain/diagnostics/diagnostic_kind.def | 1 + toolchain/lower/testdata/class/basic.carbon | 9 +-- 5 files changed, 103 insertions(+), 20 deletions(-) create mode 100644 toolchain/check/testdata/var/fail_not_copyable.carbon diff --git a/toolchain/check/convert.cpp b/toolchain/check/convert.cpp index e67b38392c15..fe7439606ed4 100644 --- a/toolchain/check/convert.cpp +++ b/toolchain/check/convert.cpp @@ -632,6 +632,36 @@ static auto PerformBuiltinConversion(Context& context, Parse::Node parse_node, return value_id; } +// Given a value expression, form a corresponding initializer that copies from +// that value, if it is possible to do so. +static auto PerformCopy(Context& context, SemIR::NodeId expr_id) + -> SemIR::NodeId { + auto expr = context.nodes().Get(expr_id); + auto type_id = expr.type_id(); + if (type_id == SemIR::TypeId::Error) { + return SemIR::NodeId::BuiltinError; + } + + // TODO: Directly track on the value representation whether it's a copy of + // the object representation. + auto value_rep = SemIR::GetValueRepresentation(context.sem_ir(), type_id); + if (value_rep.kind == SemIR::ValueRepresentation::Copy && + value_rep.aggregate_kind == SemIR::ValueRepresentation::NotAggregate && + value_rep.type_id == type_id) { + // For by-value scalar types, no explicit action is required. Initializing + // from a value expression is treated as copying the value. + return expr_id; + } + + // TODO: We don't yet have rules for whether and when a class type is + // copyable, or how to perform the copy. + CARBON_DIAGNOSTIC(CopyOfUncopyableType, Error, + "Cannot copy value of type `{0}`.", std::string); + context.emitter().Emit(expr.parse_node(), CopyOfUncopyableType, + context.sem_ir().StringifyType(type_id)); + return SemIR::NodeId::BuiltinError; +} + auto Convert(Context& context, Parse::Node parse_node, SemIR::NodeId expr_id, ConversionTarget target) -> SemIR::NodeId { auto& sem_ir = context.sem_ir(); @@ -730,18 +760,25 @@ auto Convert(Context& context, Parse::Node parse_node, SemIR::NodeId expr_id, [[fallthrough]]; case SemIR::ExpressionCategory::DurableReference: - case SemIR::ExpressionCategory::EphemeralReference: { - // If we have a reference and don't want one, form a value binding. - if (target.kind != ConversionTarget::ValueOrReference && - target.kind != ConversionTarget::Discarded) { - // TODO: Support types with custom value representations. - expr_id = context.AddNode( - SemIR::BindValue{expr.parse_node(), expr.type_id(), expr_id}); + case SemIR::ExpressionCategory::EphemeralReference: + // If a reference expression is an acceptable result, we're done. + if (target.kind == ConversionTarget::ValueOrReference || + target.kind == ConversionTarget::Discarded) { + break; } - break; - } + + // If we have a reference and don't want one, form a value binding. + // TODO: Support types with custom value representations. + expr_id = context.AddNode( + SemIR::BindValue{expr.parse_node(), expr.type_id(), expr_id}); + // We now have a value expression. + [[fallthrough]]; case SemIR::ExpressionCategory::Value: + // When initializing from a value, perform a copy. + if (target.is_initializer()) { + expr_id = PerformCopy(context, expr_id); + } break; } diff --git a/toolchain/check/testdata/basics/builtin_types.carbon b/toolchain/check/testdata/basics/builtin_types.carbon index bccbbfa0e7b8..9cd4fbbd8f9d 100644 --- a/toolchain/check/testdata/basics/builtin_types.carbon +++ b/toolchain/check/testdata/basics/builtin_types.carbon @@ -6,7 +6,7 @@ var test_i32: i32 = 0; var test_f64: f64 = 0.1; -var test_str: String = "Test"; +let test_str: String = "Test"; var test_type: type = i32; // CHECK:STDOUT: file "builtin_types.carbon" { @@ -19,10 +19,8 @@ var test_type: type = i32; // CHECK:STDOUT: %.loc8: f64 = real_literal 1e-1 // CHECK:STDOUT: assign %test_f64.var, %.loc8 // CHECK:STDOUT: %.1: type = ptr_type String -// CHECK:STDOUT: %test_str.var: ref String = var "test_str" -// CHECK:STDOUT: %test_str: ref String = bind_name "test_str", %test_str.var // CHECK:STDOUT: %.loc9: String = string_literal "Test" -// CHECK:STDOUT: assign %test_str.var, %.loc9 +// CHECK:STDOUT: %test_str: String = bind_name "test_str", %.loc9 // CHECK:STDOUT: %test_type.var: ref type = var "test_type" // CHECK:STDOUT: %test_type: ref type = bind_name "test_type", %test_type.var // CHECK:STDOUT: assign %test_type.var, i32 diff --git a/toolchain/check/testdata/var/fail_not_copyable.carbon b/toolchain/check/testdata/var/fail_not_copyable.carbon new file mode 100644 index 000000000000..2d8479e6e17a --- /dev/null +++ b/toolchain/check/testdata/var/fail_not_copyable.carbon @@ -0,0 +1,52 @@ +// 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 + +class X { +} + +fn F(x: X) { + // TODO: Strings should eventually be copyable, once we decide how to + // represent them. + // CHECK:STDERR: fail_not_copyable.carbon:[[@LINE+3]]:19: ERROR: Cannot copy value of type `String`. + // CHECK:STDERR: var s: String = "hello"; + // CHECK:STDERR: ^ + var s: String = "hello"; + + // TODO: Decide on rules for when classes are copyable. + // CHECK:STDERR: fail_not_copyable.carbon:[[@LINE+3]]:14: ERROR: Cannot copy value of type `X`. + // CHECK:STDERR: var y: X = x; + // CHECK:STDERR: ^ + var y: X = x; +} + +// CHECK:STDOUT: file "fail_not_copyable.carbon" { +// CHECK:STDOUT: class_declaration @X, () +// CHECK:STDOUT: %X: type = class_type @X +// CHECK:STDOUT: %.loc8: type = struct_type {} +// CHECK:STDOUT: %F: = fn_decl @F +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: class @X { +// CHECK:STDOUT: +// CHECK:STDOUT: !members: +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: fn @F(%x: X) { +// CHECK:STDOUT: !entry: +// CHECK:STDOUT: %.loc8: type = tuple_type () +// CHECK:STDOUT: %.loc7: type = ptr_type {} +// CHECK:STDOUT: %.1: type = ptr_type String +// CHECK:STDOUT: %s.var: ref String = var "s" +// CHECK:STDOUT: %s: ref String = bind_name "s", %s.var +// CHECK:STDOUT: %.loc16: String = string_literal "hello" +// CHECK:STDOUT: assign %s.var, +// CHECK:STDOUT: %X.ref: type = name_reference "X", file.%X +// CHECK:STDOUT: %y.var: ref X = var "y" +// CHECK:STDOUT: %y: ref X = bind_name "y", %y.var +// CHECK:STDOUT: %x.ref: X = name_reference "x", %x +// CHECK:STDOUT: assign %y.var, +// CHECK:STDOUT: return +// CHECK:STDOUT: } diff --git a/toolchain/diagnostics/diagnostic_kind.def b/toolchain/diagnostics/diagnostic_kind.def index 3d995b949b74..fa71d14724d2 100644 --- a/toolchain/diagnostics/diagnostic_kind.def +++ b/toolchain/diagnostics/diagnostic_kind.def @@ -129,6 +129,7 @@ CARBON_DIAGNOSTIC_KIND(ClassPreviousDefinition) CARBON_DIAGNOSTIC_KIND(ClassRedefinition) CARBON_DIAGNOSTIC_KIND(ClassIncompleteWithinDefinition) CARBON_DIAGNOSTIC_KIND(ContinueOutsideLoop) +CARBON_DIAGNOSTIC_KIND(CopyOfUncopyableType) CARBON_DIAGNOSTIC_KIND(DereferenceOfNonPointer) CARBON_DIAGNOSTIC_KIND(DereferenceOfType) CARBON_DIAGNOSTIC_KIND(FunctionPreviousDefinition) diff --git a/toolchain/lower/testdata/class/basic.carbon b/toolchain/lower/testdata/class/basic.carbon index 8b9d5865dbcd..929aa434d61f 100644 --- a/toolchain/lower/testdata/class/basic.carbon +++ b/toolchain/lower/testdata/class/basic.carbon @@ -9,10 +9,7 @@ class C { var b: C*; } -fn F(c: C) -> C { - // TODO: We should copy `c` into the return slot here. - return c; -} +fn F(c: C) -> C; fn Run() { var c: C; @@ -22,9 +19,7 @@ fn Run() { // CHECK:STDOUT: ; ModuleID = 'basic.carbon' // CHECK:STDOUT: source_filename = "basic.carbon" // CHECK:STDOUT: -// CHECK:STDOUT: define void @F(ptr sret({ i32, ptr }) %return, ptr %c) { -// CHECK:STDOUT: ret void -// CHECK:STDOUT: } +// CHECK:STDOUT: declare void @F(ptr sret({ i32, ptr }), ptr) // CHECK:STDOUT: // CHECK:STDOUT: define void @main() { // CHECK:STDOUT: %c = alloca { i32, ptr }, align 8