From 1911569794ca2e5f18ce0783c21495c9359b3f1a Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Tue, 5 Nov 2024 19:31:52 +0000 Subject: [PATCH] Fix crash lowering an imported impl method. While we don't need a lookup table for an imported impl from a different library, we do still need to import the name scope so we can compute the parent scope for mangling purposes. --- toolchain/check/import_ref.cpp | 32 ++++---- toolchain/lower/testdata/impl/import.carbon | 81 +++++++++++++++++++++ 2 files changed, 99 insertions(+), 14 deletions(-) create mode 100644 toolchain/lower/testdata/impl/import.carbon diff --git a/toolchain/check/import_ref.cpp b/toolchain/check/import_ref.cpp index 8cb0f91d73957..eb44bf1030785 100644 --- a/toolchain/check/import_ref.cpp +++ b/toolchain/check/import_ref.cpp @@ -1803,23 +1803,27 @@ class ImportRefResolver { new_impl.witness_id = witness_id; - // Import the definition scope, if we might need it. Name lookup is never - // performed into this scope by a user of the impl, so this is only - // necessary in the same library that defined the impl, in order to support - // defining members of the impl out of line in the impl file when the impl - // is defined in the API file. - if (import_ir_id_ == SemIR::ImportIRId::ApiForImpl) { + if (import_impl.scope_id.is_valid()) { new_impl.scope_id = context_.name_scopes().Add( new_impl.first_owning_decl_id, SemIR::NameId::Invalid, new_impl.parent_scope_id); - auto& new_scope = context_.name_scopes().Get(new_impl.scope_id); - const auto& import_scope = - import_ir_.name_scopes().Get(import_impl.scope_id); - - // Push a block so that we can add scoped instructions to it. - context_.inst_block_stack().Push(); - AddNameScopeImportRefs(import_scope, new_scope); - new_impl.body_block_id = context_.inst_block_stack().Pop(); + // Import the contents of the definition scope, if we might need it. Name + // lookup is never performed into this scope by a user of the impl, so + // this is only necessary in the same library that defined the impl, in + // order to support defining members of the impl out of line in the impl + // file when the impl is defined in the API file. + // TODO: Check to see if this impl is owned by the API file, rather than + // merely being imported into it. + if (import_ir_id_ == SemIR::ImportIRId::ApiForImpl) { + auto& new_scope = context_.name_scopes().Get(new_impl.scope_id); + const auto& import_scope = + import_ir_.name_scopes().Get(import_impl.scope_id); + + // Push a block so that we can add scoped instructions to it. + context_.inst_block_stack().Push(); + AddNameScopeImportRefs(import_scope, new_scope); + new_impl.body_block_id = context_.inst_block_stack().Pop(); + } } } diff --git a/toolchain/lower/testdata/impl/import.carbon b/toolchain/lower/testdata/impl/import.carbon new file mode 100644 index 0000000000000..1bcefd58fc8fe --- /dev/null +++ b/toolchain/lower/testdata/impl/import.carbon @@ -0,0 +1,81 @@ +// 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/lower/testdata/impl/import.carbon +// TIP: To dump output, run: +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/lower/testdata/impl/import.carbon + +// --- impl_def.carbon + +library "[[@TEST_NAME]]"; + +interface I { + fn F[self: Self]() -> i32; +} + +class A { + var n: i32; +} + +impl A as I { + fn F[self: A]() -> i32 { + return self.n; + } +} + +// --- impl_use.carbon + +import library "impl_def"; + +fn Call(a: A) -> i32 { + return a.(I.F)(); +} + +// CHECK:STDOUT: ; ModuleID = 'impl_def.carbon' +// CHECK:STDOUT: source_filename = "impl_def.carbon" +// CHECK:STDOUT: +// CHECK:STDOUT: define i32 @"_CF.A.Main:I.Main"(ptr %self) !dbg !4 { +// CHECK:STDOUT: entry: +// CHECK:STDOUT: %.loc14_16.1.n = getelementptr inbounds nuw { i32 }, ptr %self, i32 0, i32 0, !dbg !7 +// CHECK:STDOUT: %.loc14_16.2 = load i32, ptr %.loc14_16.1.n, align 4, !dbg !7 +// CHECK:STDOUT: ret i32 %.loc14_16.2, !dbg !8 +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: !llvm.module.flags = !{!0, !1} +// CHECK:STDOUT: !llvm.dbg.cu = !{!2} +// CHECK:STDOUT: +// CHECK:STDOUT: !0 = !{i32 7, !"Dwarf Version", i32 5} +// CHECK:STDOUT: !1 = !{i32 2, !"Debug Info Version", i32 3} +// CHECK:STDOUT: !2 = distinct !DICompileUnit(language: DW_LANG_C, file: !3, producer: "carbon", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug) +// CHECK:STDOUT: !3 = !DIFile(filename: "impl_def.carbon", directory: "") +// CHECK:STDOUT: !4 = distinct !DISubprogram(name: "F", linkageName: "_CF.A.Main:I.Main", scope: null, file: !3, line: 13, type: !5, spFlags: DISPFlagDefinition, unit: !2) +// CHECK:STDOUT: !5 = !DISubroutineType(types: !6) +// CHECK:STDOUT: !6 = !{} +// CHECK:STDOUT: !7 = !DILocation(line: 14, column: 12, scope: !4) +// CHECK:STDOUT: !8 = !DILocation(line: 14, column: 5, scope: !4) +// CHECK:STDOUT: ; ModuleID = 'impl_use.carbon' +// CHECK:STDOUT: source_filename = "impl_use.carbon" +// CHECK:STDOUT: +// CHECK:STDOUT: define i32 @_CCall.Main(ptr %a) !dbg !4 { +// CHECK:STDOUT: entry: +// CHECK:STDOUT: %F.call = call i32 @"_CF.A.Main:I.Main"(ptr %a), !dbg !7 +// CHECK:STDOUT: ret i32 %F.call, !dbg !8 +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: declare i32 @"_CF.A.Main:I.Main"(ptr) +// CHECK:STDOUT: +// CHECK:STDOUT: !llvm.module.flags = !{!0, !1} +// CHECK:STDOUT: !llvm.dbg.cu = !{!2} +// CHECK:STDOUT: +// CHECK:STDOUT: !0 = !{i32 7, !"Dwarf Version", i32 5} +// CHECK:STDOUT: !1 = !{i32 2, !"Debug Info Version", i32 3} +// CHECK:STDOUT: !2 = distinct !DICompileUnit(language: DW_LANG_C, file: !3, producer: "carbon", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug) +// CHECK:STDOUT: !3 = !DIFile(filename: "impl_use.carbon", directory: "") +// CHECK:STDOUT: !4 = distinct !DISubprogram(name: "Call", linkageName: "_CCall.Main", scope: null, file: !3, line: 12, type: !5, spFlags: DISPFlagDefinition, unit: !2) +// CHECK:STDOUT: !5 = !DISubroutineType(types: !6) +// CHECK:STDOUT: !6 = !{} +// CHECK:STDOUT: !7 = !DILocation(line: 16, column: 10, scope: !4) +// CHECK:STDOUT: !8 = !DILocation(line: 16, column: 3, scope: !4)