From 20972ec74892c11dcf59a3aa3d070e6feca9efb2 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Fri, 5 Jun 2026 16:30:09 -0700 Subject: [PATCH] Don't perform access-control checks on namespace-scope entities. (#7310) Instead of silently producing an `ErrorInst::InstId` when looking up a private qualified name in the current package, bypass the access check. We don't need it -- private names from other libraries are filtered out by the import logic. Also fix `DiagnoseInvalidQualifiedNameAccess` to actually always produce a diagnostic, instead of silently ignoring access control failures in non-class types. This is a no-op after the fix to the access logic, since we only allow access control at class and namespace scope currently, but should avoid this issue from recurring when that changes. Assisted-by: Gemini via Antigravity --- toolchain/check/name_lookup.cpp | 42 +++++----- .../package_expr/fail_not_found.carbon | 84 ------------------- .../testdata/package_expr/not_found.carbon | 35 ++++++++ 3 files changed, 58 insertions(+), 103 deletions(-) delete mode 100644 toolchain/check/testdata/package_expr/fail_not_found.carbon create mode 100644 toolchain/check/testdata/package_expr/not_found.carbon diff --git a/toolchain/check/name_lookup.cpp b/toolchain/check/name_lookup.cpp index 278e0351b658..341070d6133a 100644 --- a/toolchain/check/name_lookup.cpp +++ b/toolchain/check/name_lookup.cpp @@ -209,26 +209,26 @@ static auto DiagnoseInvalidQualifiedNameAccess( Context& context, SemIR::LocId loc_id, SemIR::LocId member_loc_id, SemIR::NameId name_id, SemIR::AccessKind access_kind, bool is_parent_access, AccessInfo access_info) -> void { - auto class_type = context.constant_values().TryGetInstAs( - access_info.constant_id); - if (!class_type) { - return; - } - - // TODO: Support scoped entities other than just classes. - const auto& class_info = context.classes().Get(class_type->class_id); - - auto parent_type_id = class_info.self_type_id; + // TODO: Will an access scope always be a type? Should we support access + // within `impl` scopes? + auto scope_type_id = + context.types().GetTypeIdForTypeConstantId(access_info.constant_id); if (access_kind == SemIR::AccessKind::Private && is_parent_access) { + // TODO: Do we need to support parent access for entities other than + // classes? + auto class_type = context.constant_values().GetInstAs( + access_info.constant_id); + const auto& class_info = context.classes().Get(class_type.class_id); + if (auto base_type_id = - class_info.GetBaseType(context.sem_ir(), class_type->specific_id); + class_info.GetBaseType(context.sem_ir(), class_type.specific_id); base_type_id.has_value()) { - parent_type_id = base_type_id; + scope_type_id = base_type_id; } else if (auto adapted_type_id = class_info.GetAdaptedType( - context.sem_ir(), class_type->specific_id); + context.sem_ir(), class_type.specific_id); adapted_type_id.has_value()) { - parent_type_id = adapted_type_id; + scope_type_id = adapted_type_id; } else { CARBON_FATAL("Expected parent for parent access"); } @@ -241,16 +241,20 @@ static auto DiagnoseInvalidQualifiedNameAccess( CARBON_DIAGNOSTIC(ClassMemberDeclaration, Note, "declared here"); context.emitter() .Build(loc_id, ClassInvalidMemberAccess, - access_kind == SemIR::AccessKind::Private, name_id, parent_type_id) + access_kind == SemIR::AccessKind::Private, name_id, scope_type_id) .Note(member_loc_id, ClassMemberDeclaration) .Emit(); } // Returns whether the access is prohibited by the access modifiers. -static auto IsAccessProhibited(std::optional access_info, +static auto IsAccessProhibited(Context& context, + std::optional access_info, SemIR::AccessKind access_kind, bool is_parent_access) -> bool { - if (!access_info) { + // Namespace members are always available within the current library; + // filtering of private names happens in import, not here. + if (!access_info || context.constant_values().InstIs( + access_info->constant_id)) { return false; } @@ -272,7 +276,7 @@ auto CheckAccess(Context& context, SemIR::LocId loc_id, SemIR::LocId member_loc_id, SemIR::NameId name_id, SemIR::AccessKind access_kind, bool is_parent_access, AccessInfo access_info) -> void { - if (IsAccessProhibited(access_info, access_kind, is_parent_access)) { + if (IsAccessProhibited(context, access_info, access_kind, is_parent_access)) { DiagnoseInvalidQualifiedNameAccess(context, loc_id, member_loc_id, name_id, access_kind, is_parent_access, access_info); @@ -519,7 +523,7 @@ auto LookupQualifiedName(Context& context, SemIR::LocId loc_id, } auto is_access_prohibited = - IsAccessProhibited(access_info, access_kind, is_parent_access); + IsAccessProhibited(context, access_info, access_kind, is_parent_access); // Keep track of prohibited accesses, this will be useful for reporting // multiple prohibited accesses if we can't find a suitable lookup. diff --git a/toolchain/check/testdata/package_expr/fail_not_found.carbon b/toolchain/check/testdata/package_expr/fail_not_found.carbon deleted file mode 100644 index 51af46b903d3..000000000000 --- a/toolchain/check/testdata/package_expr/fail_not_found.carbon +++ /dev/null @@ -1,84 +0,0 @@ -// 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 -// -// INCLUDE-FILE: toolchain/testing/testdata/min_prelude/int.carbon -// TODO: Add ranges and switch to "--dump-sem-ir-ranges=only". -// EXTRA-ARGS: --dump-sem-ir-ranges=if-present -// -// AUTOUPDATE -// TIP: To test this file alone, run: -// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/package_expr/fail_not_found.carbon -// TIP: To dump output, run: -// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/package_expr/fail_not_found.carbon - -fn Main() { - // CHECK:STDERR: fail_not_found.carbon:[[@LINE+4]]:23: error: member name `x` not found in `Main` [MemberNameNotFoundInInstScope] - // CHECK:STDERR: var unused y: i32 = package.x; - // CHECK:STDERR: ^~~~~~~~~ - // CHECK:STDERR: - var unused y: i32 = package.x; -} - -// CHECK:STDOUT: --- fail_not_found.carbon -// CHECK:STDOUT: -// CHECK:STDOUT: constants { -// CHECK:STDOUT: %Main.type: type = fn_type @Main [concrete] -// CHECK:STDOUT: %empty_tuple.type: type = tuple_type () [concrete] -// CHECK:STDOUT: %Main: %Main.type = struct_value () [concrete] -// CHECK:STDOUT: %int_32: Core.IntLiteral = int_value 32 [concrete] -// CHECK:STDOUT: %Int.type: type = generic_class_type @Int [concrete] -// CHECK:STDOUT: %Int.generic: %Int.type = struct_value () [concrete] -// CHECK:STDOUT: %i32: type = class_type @Int, @Int(%int_32) [concrete] -// CHECK:STDOUT: %i32.builtin: type = int_type signed, %int_32 [concrete] -// CHECK:STDOUT: %pattern_type.6b6: type = pattern_type %i32 [concrete] -// CHECK:STDOUT: %Destroy.type: type = facet_type <@Destroy> [concrete] -// CHECK:STDOUT: %Destroy.Op.type.1d8f74.2: type = fn_type @Destroy.Op.loc20_3.2 [concrete] -// CHECK:STDOUT: %Destroy.Op.1a2547.2: %Destroy.Op.type.1d8f74.2 = struct_value () [concrete] -// CHECK:STDOUT: } -// CHECK:STDOUT: -// CHECK:STDOUT: imports { -// CHECK:STDOUT: %Core: = namespace file.%Core.import, [concrete] { -// CHECK:STDOUT: .Int = %Core.Int -// CHECK:STDOUT: .Destroy = %Core.Destroy -// CHECK:STDOUT: import Core//prelude -// CHECK:STDOUT: import Core//prelude/... -// CHECK:STDOUT: } -// CHECK:STDOUT: %Core.Int: %Int.type = import_ref Core//prelude/parts/int, Int, loaded [concrete = constants.%Int.generic] -// CHECK:STDOUT: %Core.Destroy: type = import_ref Core//prelude/parts/destroy, Destroy, loaded [concrete = constants.%Destroy.type] -// CHECK:STDOUT: } -// CHECK:STDOUT: -// CHECK:STDOUT: file { -// CHECK:STDOUT: package: = namespace [concrete] { -// CHECK:STDOUT: .Core = imports.%Core -// CHECK:STDOUT: .Main = %Main.decl -// CHECK:STDOUT: .x = -// CHECK:STDOUT: } -// CHECK:STDOUT: %Core.import = import Core -// CHECK:STDOUT: %Main.decl: %Main.type = fn_decl @Main [concrete = constants.%Main] {} {} -// CHECK:STDOUT: } -// CHECK:STDOUT: -// CHECK:STDOUT: fn @Main() { -// CHECK:STDOUT: !entry: -// CHECK:STDOUT: name_binding_decl { -// CHECK:STDOUT: %y.patt: %pattern_type.6b6 = ref_binding_pattern y [concrete] -// CHECK:STDOUT: %y.var_patt: %pattern_type.6b6 = var_pattern %y.patt [concrete] -// CHECK:STDOUT: } -// CHECK:STDOUT: %y.var: ref %i32 = var %y.var_patt -// CHECK:STDOUT: %package.ref: = name_ref package, package [concrete = package] -// CHECK:STDOUT: %x.ref: = name_ref x, [concrete = ] -// CHECK:STDOUT: assign %y.var, -// CHECK:STDOUT: %i32: type = type_literal constants.%i32 [concrete = constants.%i32] -// CHECK:STDOUT: %y: ref %i32 = ref_binding y, %y.var -// CHECK:STDOUT: %Destroy.Op.bound: = bound_method %y.var, constants.%Destroy.Op.1a2547.2 -// CHECK:STDOUT: %Destroy.Op.call: init %empty_tuple.type = call %Destroy.Op.bound(%y.var) -// CHECK:STDOUT: return -// CHECK:STDOUT: } -// CHECK:STDOUT: -// CHECK:STDOUT: fn @Destroy.Op.loc20_3.1(%self.param: ref %i32.builtin) = "no_op"; -// CHECK:STDOUT: -// CHECK:STDOUT: fn @Destroy.Op.loc20_3.2(%self.param: ref %i32) { -// CHECK:STDOUT: !entry: -// CHECK:STDOUT: return -// CHECK:STDOUT: } -// CHECK:STDOUT: diff --git a/toolchain/check/testdata/package_expr/not_found.carbon b/toolchain/check/testdata/package_expr/not_found.carbon new file mode 100644 index 000000000000..ca255115a6f4 --- /dev/null +++ b/toolchain/check/testdata/package_expr/not_found.carbon @@ -0,0 +1,35 @@ +// 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 +// +// INCLUDE-FILE: toolchain/testing/testdata/min_prelude/int.carbon +// +// AUTOUPDATE +// TIP: To test this file alone, run: +// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/package_expr/not_found.carbon +// TIP: To dump output, run: +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/package_expr/not_found.carbon + +// --- fail_direct.carbon +library "[[@TEST_NAME]]"; + +fn F() { + // CHECK:STDERR: fail_direct.carbon:[[@LINE+4]]:23: error: member name `x` not found in `Main` [MemberNameNotFoundInInstScope] + // CHECK:STDERR: var unused y: i32 = package.x; + // CHECK:STDERR: ^~~~~~~~~ + // CHECK:STDERR: + var unused y: i32 = package.x; +} + +// --- fail_nested.carbon +library "[[@TEST_NAME]]"; + +private interface Private {} + +fn F() { + // CHECK:STDERR: fail_nested.carbon:[[@LINE+4]]:3: error: member name `DoesNotExist` not found in `Private` [MemberNameNotFoundInSpecificScope] + // CHECK:STDERR: package.Private.DoesNotExist; + // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ + // CHECK:STDERR: + package.Private.DoesNotExist; +}