From 8cac0c398a507f27186a7e79932ae336a4cd526a Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Tue, 7 Jan 2025 14:10:42 -0800 Subject: [PATCH 1/5] Enable as many bazel incompatible flags as possible. (#4761) Uses bazel 8 flags since there's the update in #4729. bazelisk was used to generate the list of flags (see bazelrc comment). The overall approach is trying to do our best to follow https://bazel.build/release/backward-compatibility, in particular since `--incompatible_strict_action_env` had come up (essentially just adopting as much as we can now). The default visibility changes in `carbon_rules/BUILD` and `cc_toolchains/BUILD` files are for `--incompatible_config_setting_private_default_visibility`. That's enough for fastbuild, but we use tcmalloc in opt, and it has an issue. In `clang_toolchain.BUILD` it's for `--incompatible_check_visibility_for_toolchains`, even though `rules_shell` then breaks on it. `manifest/defs.bzl` changes are for `--incompatible_disable_target_default_provider_fields` which `rules_pkg` breaks on. Even though these flags are off, I'm keeping the changes since we should eventually enable the flags. I'm still looking at the tree sitter rules due to the WORKSPACE issue, but I think that needs more substantial work. --- .bazelrc | 106 ++++++++++++++++++++-- bazel/carbon_rules/BUILD | 2 + bazel/cc_toolchains/BUILD | 2 + bazel/cc_toolchains/clang_toolchain.BUILD | 2 + bazel/manifest/defs.bzl | 7 +- 5 files changed, 108 insertions(+), 11 deletions(-) diff --git a/.bazelrc b/.bazelrc index 22d3f12c7b941..30d6309cf63fc 100644 --- a/.bazelrc +++ b/.bazelrc @@ -128,18 +128,106 @@ build --allow_unresolved_symlinks=false # RC file here if present. try-import %workspace%/user.bazelrc -# TODO: WORKSPACE will be removed in bazel 9, should finish moving off. +# Incompatible with `rules_tree_sitter`. +# TODO: WORKSPACE will be removed in bazel 9, and we need to move off. +# TODO: The registry has a different treesitter rule set, and we should +# investigate switching. See: +# https://registry.bazel.build/modules/tree-sitter-bazel common --enable_workspace - -# The version of treesitter rules we're using depends on empty globs. -# TODO: Look at the different rules in the registry. +# This is on by default in bazel 8. common --incompatible_disallow_empty_glob=false +# common --incompatible_auto_exec_groups +# common --incompatible_disable_starlark_host_transitions + +# This excludes things like rules_android to reduce warnings. +# TODO: There's a pending fix, so hopefully we can remove it soon. +# - Issue: https://github.com/bazelbuild/bazel/issues/23929 +# - 8.0.1 tracker: https://github.com/bazelbuild/bazel/issues/24649 +common --incompatible_autoload_externally=+@rules_java,+@rules_python,+@rules_shell + +# Query error in `@bazel_tools`. This reproduces with +# `bazel query 'deps(//...)'`. +# TODO: Enable the flag once compatibility issues are fixed. +# common --incompatible_disable_non_executable_java_binary + +# Incompatible with `rules_cc`. +# TODO: Enable the flag once compatibility issues are fixed. +# common --incompatible_no_rule_outputs_param +# common --incompatible_stop_exporting_language_modules -# TODO: Enable as many incompatible flags as we can, per +# Incompatible with `rules_pkg`. +# TODO: Enable the flag once compatibility issues are fixed. +# common --incompatible_disable_target_default_provider_fields + +# Incompatible with `rules_shell`. +# TODO: Enable the flag once compatibility issues are fixed. +# common --incompatible_check_visibility_for_toolchains + +# Incompatible with `tcmalloc`. +# TODO: Fix `bazel build -c opt //...`. +# common --incompatible_config_setting_private_default_visibility + +# Enable as many incompatible flags as we can, per # https://bazel.build/release/backward-compatibility. To get the latest list, # using `bazelisk --migrate build //...` will help. +common --incompatible_allow_tags_propagation +common --incompatible_always_check_depset_elements +common --incompatible_always_include_files_in_data +common --incompatible_bazel_test_exec_run_under +common --incompatible_check_sharding_support +common --incompatible_check_testonly_for_output_files +common --incompatible_default_to_explicit_init_py +common --incompatible_depset_for_java_output_source_jars +common --incompatible_depset_for_libraries_to_link_getter +common --incompatible_disable_native_android_rules +common --incompatible_disable_native_repo_rules +common --incompatible_disable_objc_library_transition +common --incompatible_disable_target_provider_fields +common --incompatible_disallow_ctx_resolve_tools +common --incompatible_disallow_legacy_py_provider +common --incompatible_disallow_sdk_frameworks_attributes +common --incompatible_disallow_struct_provider_syntax +common --incompatible_do_not_split_linking_cmdline +common --incompatible_dont_enable_host_nonhost_crosstool_features +common --incompatible_dont_use_javasourceinfoprovider +common --incompatible_enable_apple_toolchain_resolution +common --incompatible_enable_deprecated_label_apis +common --incompatible_enable_proto_toolchain_resolution +common --incompatible_enforce_config_setting_visibility +common --incompatible_exclusive_test_sandboxed +common --incompatible_fail_on_unknown_attributes +common --incompatible_fix_package_group_reporoot_syntax +common --incompatible_java_common_parameters +common --incompatible_legacy_local_fallback +common --incompatible_make_thinlto_command_lines_standalone +common --incompatible_merge_fixed_and_default_shell_env +common --incompatible_merge_genfiles_directory +common --incompatible_modify_execution_info_additive +common --incompatible_new_actions_api +common --incompatible_no_attr_license +common --incompatible_no_implicit_file_export +common --incompatible_no_implicit_watch_label +common --incompatible_objc_alwayslink_by_default +common --incompatible_package_group_has_public_syntax +common --incompatible_py2_outputs_are_suffixed +common --incompatible_py3_is_default +common --incompatible_python_disable_py2 +common --incompatible_python_disallow_native_rules +common --incompatible_remote_use_new_exit_code_for_lost_inputs +common --incompatible_remove_legacy_whole_archive +common --incompatible_require_ctx_in_configure_features +common --incompatible_require_linker_input_cc_api +common --incompatible_run_shell_command_string +common --incompatible_sandbox_hermetic_tmp +common --incompatible_simplify_unconditional_selects_in_rule_attrs +common --incompatible_stop_exporting_build_file_path common --incompatible_strict_action_env - -# This excludes things like rules_android to reduce warnings. -# TODO: Disable this completely. -common --incompatible_autoload_externally=+@rules_java,+@rules_python,+@rules_shell +common --incompatible_strip_executable_safely +common --incompatible_top_level_aspects_require_providers +common --incompatible_unambiguous_label_stringification +common --incompatible_use_cc_configure_from_rules_cc +common --incompatible_use_new_cgroup_implementation +common --incompatible_use_plus_in_repo_names +common --incompatible_use_python_toolchains +common --incompatible_validate_top_level_header_inclusions +common --incompatible_visibility_private_attributes_at_definition diff --git a/bazel/carbon_rules/BUILD b/bazel/carbon_rules/BUILD index 7aca964de584a..1250c691225e3 100644 --- a/bazel/carbon_rules/BUILD +++ b/bazel/carbon_rules/BUILD @@ -4,6 +4,8 @@ load("@bazel_skylib//rules:common_settings.bzl", "bool_flag") +package(default_visibility = ["//visibility:public"]) + # Flag controlling whether the target config is used for the `carbon_*` Bazel # rules. The default is to use the exec config as that is more correct in cases # where the target config is not compatible with the exec (cross compiling), and diff --git a/bazel/cc_toolchains/BUILD b/bazel/cc_toolchains/BUILD index 6cf693cca0292..2059ee1fb7ba7 100644 --- a/bazel/cc_toolchains/BUILD +++ b/bazel/cc_toolchains/BUILD @@ -4,6 +4,8 @@ load("@bazel_skylib//lib:selects.bzl", "selects") +package(default_visibility = ["//visibility:public"]) + # For use by rules.bzl. # Matches when asan is enabled on a macOS platform. selects.config_setting_group( diff --git a/bazel/cc_toolchains/clang_toolchain.BUILD b/bazel/cc_toolchains/clang_toolchain.BUILD index 0da0ac8935649..13bf032931f31 100644 --- a/bazel/cc_toolchains/clang_toolchain.BUILD +++ b/bazel/cc_toolchains/clang_toolchain.BUILD @@ -7,6 +7,8 @@ load(":cc_toolchain_config.bzl", "cc_local_toolchain_suite") +package(default_visibility = ["//visibility:public"]) + cc_local_toolchain_suite( name = "bazel_cc_toolchain", configs = [ diff --git a/bazel/manifest/defs.bzl b/bazel/manifest/defs.bzl index 9e9bbd65006f0..926ec1f514c3d 100644 --- a/bazel/manifest/defs.bzl +++ b/bazel/manifest/defs.bzl @@ -9,8 +9,11 @@ def _manifest(ctx): files = [] for src in ctx.attr.srcs: - files.extend([f.path for f in src.files.to_list()]) - files.extend([f.path for f in src.default_runfiles.files.to_list()]) + files.extend([f.path for f in src[DefaultInfo].files.to_list()]) + files.extend([ + f.path + for f in src[DefaultInfo].default_runfiles.files.to_list() + ]) if ctx.attr.strip_package_dir: package_dir = ctx.label.package + "/" From 1d379ff7f8c93aac29f0b4dbc116170d304bc5e4 Mon Sep 17 00:00:00 2001 From: josh11b <15258583+josh11b@users.noreply.github.com> Date: Tue, 7 Jan 2025 15:31:31 -0800 Subject: [PATCH 2/5] Syntactic `impl` declaration matching updates (#4762) * Implement ignoring the difference between `Self as` and `as`, as well as `where` clauses at the end of an `impl` declaration when checking whether `impl` declarations match, from #3763. * Allow impl declarations with different constraint ids to match, as long as the facet type of the constraint has the same interface_id and specific_id. * Add some TODOs reflecting future facet type resolution. --------- Co-authored-by: Josh L --- toolchain/check/handle_impl.cpp | 43 ++++- toolchain/check/merge.cpp | 53 ++++-- .../impl/no_prelude/impl_self_as.carbon | 180 ++++++++++++++++++ .../impl/no_prelude/impl_where_redecl.carbon | 47 +++++ toolchain/sem_ir/impl.cpp | 17 +- toolchain/sem_ir/impl.h | 3 +- 6 files changed, 321 insertions(+), 22 deletions(-) create mode 100644 toolchain/check/testdata/impl/no_prelude/impl_self_as.carbon create mode 100644 toolchain/check/testdata/impl/no_prelude/impl_where_redecl.carbon diff --git a/toolchain/check/handle_impl.cpp b/toolchain/check/handle_impl.cpp index 808aa89d44d4e..30367ceaa7e09 100644 --- a/toolchain/check/handle_impl.cpp +++ b/toolchain/check/handle_impl.cpp @@ -212,21 +212,45 @@ static auto PopImplIntroducerAndParamsAsNameComponent( Parse::NodeId first_param_node_id = context.node_stack().PopForSoloNodeId(); + // Subtracting 1 since we don't want to include the final `{` or `;` of the // declaration when performing syntactic match. - // TODO: Following proposal #3763, we should exclude any `where` clause, and - // add `Self` before `as` if needed, see: + auto end_node_kind = context.parse_tree().node_kind(end_of_decl_node_id); + CARBON_CHECK(end_node_kind == Parse::NodeKind::ImplDefinitionStart || + end_node_kind == Parse::NodeKind::ImplDecl); + Parse::Tree::PostorderIterator last_param_iter(end_of_decl_node_id); + --last_param_iter; + + // Following proposal #3763, exclude a final `where` clause, if present. See: // https://github.com/carbon-language/carbon-lang/blob/trunk/proposals/p3763.md#redeclarations - auto node_kind = context.parse_tree().node_kind(end_of_decl_node_id); - CARBON_CHECK(node_kind == Parse::NodeKind::ImplDefinitionStart || - node_kind == Parse::NodeKind::ImplDecl); - Parse::NodeId last_param_node_id(end_of_decl_node_id.index - 1); + + // Caches the NodeKind for the current value of *last_param_iter so + if (context.parse_tree().node_kind(*last_param_iter) == + Parse::NodeKind::WhereExpr) { + int where_operands_to_skip = 1; + --last_param_iter; + CARBON_CHECK(Parse::Tree::PostorderIterator(first_param_node_id) < + last_param_iter); + do { + auto node_kind = context.parse_tree().node_kind(*last_param_iter); + if (node_kind == Parse::NodeKind::WhereExpr) { + // If we have a nested `where`, we need to see another `WhereOperand` + // before we find the one that matches our original `WhereExpr` node. + ++where_operands_to_skip; + } else if (node_kind == Parse::NodeKind::WhereOperand) { + --where_operands_to_skip; + } + --last_param_iter; + CARBON_CHECK(Parse::Tree::PostorderIterator(first_param_node_id) < + last_param_iter); + } while (where_operands_to_skip > 0); + } return { .name_loc_id = Parse::NodeId::Invalid, .name_id = SemIR::NameId::Invalid, .first_param_node_id = first_param_node_id, - .last_param_node_id = last_param_node_id, + .last_param_node_id = *last_param_iter, .implicit_params_loc_id = implicit_params_loc_id, .implicit_param_patterns_id = implicit_param_patterns_id.value_or(SemIR::InstBlockId::Invalid), @@ -297,6 +321,11 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id, // TODO: Check that its constant value is a constraint. auto [constraint_inst_id, constraint_type_id] = ExprAsType(context, constraint_node, constraint_id); + // TODO: Do facet type resolution here. + // TODO: Determine `interface_id` and `specific_id` once and save it in the + // resolved facet type, instead of in multiple functions called below. + // TODO: Skip work below if facet type resolution fails, so we don't have a + // valid/non-error `interface_id` at all. // Process modifiers. // TODO: Should we somehow permit access specifiers on `impl`s? diff --git a/toolchain/check/merge.cpp b/toolchain/check/merge.cpp index b19eb0cc87ddd..20340a93cb07d 100644 --- a/toolchain/check/merge.cpp +++ b/toolchain/check/merge.cpp @@ -394,18 +394,38 @@ static auto CheckRedeclParamSyntax(Context& context, CARBON_CHECK(prev_last_param_node_id.is_valid(), "prev_last_param_node_id.is_valid should match " "prev_first_param_node_id.is_valid"); - - auto new_range = Parse::Tree::PostorderIterator::MakeRange( - new_first_param_node_id, new_last_param_node_id); - auto prev_range = Parse::Tree::PostorderIterator::MakeRange( - prev_first_param_node_id, prev_last_param_node_id); - - // zip is using the shortest range. If they differ in length, there should be - // some difference inside the range because the range includes parameter - // brackets. As a consequence, we don't explicitly handle different range - // sizes here. - for (auto [new_node_id, prev_node_id] : llvm::zip(new_range, prev_range)) { + Parse::Tree::PostorderIterator new_iter(new_first_param_node_id); + Parse::Tree::PostorderIterator new_end(new_last_param_node_id); + Parse::Tree::PostorderIterator prev_iter(prev_first_param_node_id); + Parse::Tree::PostorderIterator prev_end(prev_last_param_node_id); + // Done when one past the last node to check. + ++new_end; + ++prev_end; + + // Compare up to the shortest length. + for (; new_iter != new_end && prev_iter != prev_end; + ++new_iter, ++prev_iter) { + auto new_node_id = *new_iter; + auto prev_node_id = *prev_iter; if (!IsNodeSyntaxEqual(context, new_node_id, prev_node_id)) { + // Skip difference if it is `Self as` vs. `as` in an `impl` declaration. + // https://github.com/carbon-language/carbon-lang/blob/trunk/proposals/p3763.md#redeclarations + auto new_node_kind = context.parse_tree().node_kind(new_node_id); + auto prev_node_kind = context.parse_tree().node_kind(prev_node_id); + if (new_node_kind == Parse::NodeKind::DefaultSelfImplAs && + prev_node_kind == Parse::NodeKind::SelfTypeNameExpr && + context.parse_tree().node_kind(prev_iter[1]) == + Parse::NodeKind::TypeImplAs) { + ++prev_iter; + continue; + } + if (prev_node_kind == Parse::NodeKind::DefaultSelfImplAs && + new_node_kind == Parse::NodeKind::SelfTypeNameExpr && + context.parse_tree().node_kind(new_iter[1]) == + Parse::NodeKind::TypeImplAs) { + ++new_iter; + continue; + } if (!diagnose) { return false; } @@ -420,6 +440,17 @@ static auto CheckRedeclParamSyntax(Context& context, return false; } } + // The prefixes are the same, but the lengths may still be different. This is + // only relevant for `impl` declarations where the final bracketing node is + // not included in the range of nodes being compared, and in those cases + // `diagnose` is false. + if (new_iter != new_end) { + CARBON_CHECK(!diagnose); + return false; + } else if (prev_iter != prev_end) { + CARBON_CHECK(!diagnose); + return false; + } return true; } diff --git a/toolchain/check/testdata/impl/no_prelude/impl_self_as.carbon b/toolchain/check/testdata/impl/no_prelude/impl_self_as.carbon new file mode 100644 index 0000000000000..cdb16c10026be --- /dev/null +++ b/toolchain/check/testdata/impl/no_prelude/impl_self_as.carbon @@ -0,0 +1,180 @@ +// 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 +// +// EXTRA-ARGS: --no-dump-sem-ir +// +// AUTOUPDATE +// TIP: To test this file alone, run: +// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/impl/no_prelude/impl_self_as.carbon +// TIP: To dump output, run: +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/impl/no_prelude/impl_self_as.carbon + +// --- match.carbon +library "[[@TEST_NAME]]"; + +interface I1 {} +interface I2 {} +interface J1(T1:! type) {} +interface J2(T2:! type) {} + +// `impl Self as` should match `impl as`, so these should not trigger impl +// declaration without definition diagnostics. + +class C1 { + impl Self as I1; + impl as I1 {} + + impl as I2; + impl Self as I2 {} + + impl forall [U:! type] Self as J1(U); + impl forall [U:! type] as J1(U) {} + + impl forall [V:! type] as J2(V); + impl forall [V:! type] Self as J2(V) {} +} + +class C2(W:! type) { + impl Self as I1; + impl as I1 {} + + impl as I2; + impl Self as I2 {} + + impl forall [X:! type] Self as J1(X); + impl forall [X:! type] as J1(X) {} + + impl forall [Y:! type] as J2(Y); + impl forall [Y:! type] Self as J2(Y) {} +} + + +// --- fail_no_match.carbon +library "[[@TEST_NAME]]"; + +interface I3 {} +interface I4 {} +interface I5 {} +interface I6 {} +interface J3(T3:! type) {} +interface J4(T4:! type) {} +interface J5(T5:! type) {} +interface J6(T6:! type) {} + +// `impl C as` should not match `impl Self as` or `impl as`. + +class C3 { + // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition] + // CHECK:STDERR: impl C3 as I3; + // CHECK:STDERR: ^~~~~~~~~~~~~~ + // CHECK:STDERR: + impl C3 as I3; + impl as I3 {} + + // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition] + // CHECK:STDERR: impl C3 as I4; + // CHECK:STDERR: ^~~~~~~~~~~~~~ + // CHECK:STDERR: + impl C3 as I4; + impl Self as I4 {} + + // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition] + // CHECK:STDERR: impl as I5; + // CHECK:STDERR: ^~~~~~~~~~~ + // CHECK:STDERR: + impl as I5; + impl C3 as I5 {} + + // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition] + // CHECK:STDERR: impl Self as I6; + // CHECK:STDERR: ^~~~~~~~~~~~~~~~ + // CHECK:STDERR: + impl Self as I6; + impl C3 as I6 {} + + // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition] + // CHECK:STDERR: impl forall [Z3:! type] C3 as J3(Z3); + // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + // CHECK:STDERR: + impl forall [Z3:! type] C3 as J3(Z3); + impl forall [Z3:! type] as J3(Z3) {} + + // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition] + // CHECK:STDERR: impl forall [Z4:! type] C3 as J4(Z4); + // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + // CHECK:STDERR: + impl forall [Z4:! type] C3 as J4(Z4); + impl forall [Z4:! type] Self as J4(Z4) {} + + // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition] + // CHECK:STDERR: impl forall [Z5:! type] as J5(Z5); + // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + // CHECK:STDERR: + impl forall [Z5:! type] as J5(Z5); + impl forall [Z5:! type] C3 as J5(Z5) {} + + // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition] + // CHECK:STDERR: impl forall [Z6:! type] Self as J6(Z6); + // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + // CHECK:STDERR: + impl forall [Z6:! type] Self as J6(Z6); + impl forall [Z6:! type] C3 as J6(Z6) {} +} + +class C4(A:! type) { + // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition] + // CHECK:STDERR: impl C4(A) as I3; + // CHECK:STDERR: ^~~~~~~~~~~~~~~~~ + // CHECK:STDERR: + impl C4(A) as I3; + impl as I3 {} + + // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition] + // CHECK:STDERR: impl C4(A) as I4; + // CHECK:STDERR: ^~~~~~~~~~~~~~~~~ + // CHECK:STDERR: + impl C4(A) as I4; + impl Self as I4 {} + + // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition] + // CHECK:STDERR: impl as I5; + // CHECK:STDERR: ^~~~~~~~~~~ + // CHECK:STDERR: + impl as I5; + impl C4(A) as I5 {} + + // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition] + // CHECK:STDERR: impl Self as I6; + // CHECK:STDERR: ^~~~~~~~~~~~~~~~ + // CHECK:STDERR: + impl Self as I6; + impl C4(A) as I6 {} + + // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition] + // CHECK:STDERR: impl forall [B3:! type] C4(A) as J3(B3); + // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + // CHECK:STDERR: + impl forall [B3:! type] C4(A) as J3(B3); + impl forall [B3:! type] as J3(B3) {} + + // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition] + // CHECK:STDERR: impl forall [B4:! type] C4(A) as J4(B4); + // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + // CHECK:STDERR: + impl forall [B4:! type] C4(A) as J4(B4); + impl forall [B4:! type] Self as J4(B4) {} + + // CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition] + // CHECK:STDERR: impl forall [B5:! type] as J5(B5); + // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + // CHECK:STDERR: + impl forall [B5:! type] as J5(B5); + impl forall [B5:! type] C4(A) as J5(B5) {} + + // CHECK:STDERR: fail_no_match.carbon:[[@LINE+3]]:3: error: impl declared but not defined [MissingImplDefinition] + // CHECK:STDERR: impl forall [B6:! type] Self as J6(B6); + // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + impl forall [B6:! type] Self as J6(B6); + impl forall [B6:! type] C4(A) as J6(B6) {} +} diff --git a/toolchain/check/testdata/impl/no_prelude/impl_where_redecl.carbon b/toolchain/check/testdata/impl/no_prelude/impl_where_redecl.carbon new file mode 100644 index 0000000000000..c59c393f64714 --- /dev/null +++ b/toolchain/check/testdata/impl/no_prelude/impl_where_redecl.carbon @@ -0,0 +1,47 @@ +// 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 +// +// EXTRA-ARGS: --no-dump-sem-ir +// +// AUTOUPDATE +// TIP: To test this file alone, run: +// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/impl/no_prelude/impl_where_redecl.carbon +// TIP: To dump output, run: +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/impl/no_prelude/impl_where_redecl.carbon + +// --- match.carbon +library "[[@TEST_NAME]]"; + +interface J {} + +// `impl` matching ignores the `where` clause. It should not get confused by +// nested `where`, so the following should not trigger impl declaration without +// definition diagnostics. + +impl () as J; +impl () as J where .Self impls type and .Self impls (type where .Self impls type) {} + +impl {} as J where .Self impls type and .Self impls (type where .Self impls type); +impl {} as J {} + +// --- parens_other_nesting.carbon +library "[[@TEST_NAME]]"; + +interface K {} + +// `impl` matching only ignores a root-level `where` clause. + +impl {} as (K where .Self impls type) where .Self impls type; +impl {} as (K where .Self impls type) {} + +// --- fail_other_nesting.carbon +library "[[@TEST_NAME]]"; + +interface L {} + +// CHECK:STDERR: fail_other_nesting.carbon:[[@LINE+3]]:1: error: impl declared but not defined [MissingImplDefinition] +// CHECK:STDERR: impl () as (L where .Self impls type) where .Self impls type; +// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +impl () as (L where .Self impls type) where .Self impls type; +impl () as L {} diff --git a/toolchain/sem_ir/impl.cpp b/toolchain/sem_ir/impl.cpp index 9bb5cb0b47632..89d369cef7b89 100644 --- a/toolchain/sem_ir/impl.cpp +++ b/toolchain/sem_ir/impl.cpp @@ -10,11 +10,22 @@ namespace Carbon::SemIR { auto ImplStore::GetOrAddLookupBucket(const Impl& impl) -> LookupBucketRef { auto self_id = sem_ir_.constant_values().GetConstantInstId(impl.self_id); - auto constraint_id = - sem_ir_.constant_values().GetConstantInstId(impl.constraint_id); + InterfaceId interface_id = InterfaceId::Invalid; + SpecificId specific_id = SpecificId::Invalid; + auto facet_type_id = TypeId::ForTypeConstant( + sem_ir_.constant_values().Get(impl.constraint_id)); + if (auto facet_type = + sem_ir_.types().TryGetAs(facet_type_id)) { + const SemIR::FacetTypeInfo& facet_type_info = + sem_ir_.facet_types().Get(facet_type->facet_type_id); + if (auto interface_type = facet_type_info.TryAsSingleInterface()) { + interface_id = interface_type->interface_id; + specific_id = interface_type->specific_id; + } + } return LookupBucketRef( *this, lookup_ - .Insert(std::pair{self_id, constraint_id}, + .Insert(std::tuple{self_id, interface_id, specific_id}, [] { return ImplOrLookupBucketId::Invalid; }) .value()); } diff --git a/toolchain/sem_ir/impl.h b/toolchain/sem_ir/impl.h index 3dbd2973d5dd0..a7769477e7e96 100644 --- a/toolchain/sem_ir/impl.h +++ b/toolchain/sem_ir/impl.h @@ -189,7 +189,8 @@ class ImplStore { private: File& sem_ir_; ValueStore values_; - Map, ImplOrLookupBucketId> lookup_; + Map, ImplOrLookupBucketId> + lookup_; // Buckets with at least 2 entries, which will be rare; see LookupBucketRef. llvm::SmallVector> lookup_buckets_; }; From 13c121c56ca87ef4f4713252926b4834278cd1ec Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Tue, 7 Jan 2025 15:33:44 -0800 Subject: [PATCH 3/5] Address clang-tidy findings from #4763. (#4768) --- toolchain/sem_ir/inst_fingerprinter.cpp | 6 +++--- toolchain/sem_ir/inst_namer.cpp | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/toolchain/sem_ir/inst_fingerprinter.cpp b/toolchain/sem_ir/inst_fingerprinter.cpp index 8140f7dbbc1d5..0ca3c5555a1a4 100644 --- a/toolchain/sem_ir/inst_fingerprinter.cpp +++ b/toolchain/sem_ir/inst_fingerprinter.cpp @@ -233,7 +233,7 @@ struct Worklist { } auto Add(ImportIRId ir_id) -> void { - auto* ir = sem_ir->import_irs().Get(ir_id).sem_ir; + const auto* ir = sem_ir->import_irs().Get(ir_id).sem_ir; Add(ir->package_id()); Add(ir->library_id()); } @@ -267,7 +267,7 @@ struct Worklist { using Kind = decltype(kind); // Build a lookup table to add an argument of the given kind. - static constexpr std::array table = [] { + static constexpr std::array Table = [] { std::array table; table[Kind::None.ToIndex()] = [](Worklist& /*worklist*/, uint64_t /*arg*/) {}; @@ -283,7 +283,7 @@ struct Worklist { return table; }(); - table[kind.ToIndex()](*this, arg); + Table[kind.ToIndex()](*this, arg); } // Ensure all the instructions on the todo list have fingerprints. diff --git a/toolchain/sem_ir/inst_namer.cpp b/toolchain/sem_ir/inst_namer.cpp index 73de9b7e07008..86bea57299bb3 100644 --- a/toolchain/sem_ir/inst_namer.cpp +++ b/toolchain/sem_ir/inst_namer.cpp @@ -249,7 +249,7 @@ auto InstNamer::Namespace::AllocateName( // Append location information to try to disambiguate. // TODO: Consider handling inst_id cases. - if (LocId* loc_id = std::get_if(&loc_id_or_fingerprint)) { + if (auto* loc_id = std::get_if(&loc_id_or_fingerprint)) { if (loc_id->is_node_id()) { const auto& tree = inst_namer.sem_ir_->parse_tree(); auto token = tree.node_token(loc_id->node_id()); From 7ed3b986b9a8e13834112b65a5d9ba9bd82bf8cd Mon Sep 17 00:00:00 2001 From: ottmar-zittlau <74417452+ottmar-zittlau@users.noreply.github.com> Date: Wed, 8 Jan 2025 01:04:16 +0100 Subject: [PATCH 4/5] Fix unchecked optional access in compile subcommand (#4756) Hi, I fixed a small issue that I found inside the "compile subcommand" component: The program can be crashed by running ```bazel run -- toolchain:carbon compile --dump-mem-usage "non-existing-file.carbon"``` - i.e. by activating the memory usage dump flag and passing a non-existing file. Best regards, oz --- toolchain/driver/compile_subcommand.cpp | 11 +++++++---- toolchain/driver/driver_test.cpp | 11 +++++++++++ 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/toolchain/driver/compile_subcommand.cpp b/toolchain/driver/compile_subcommand.cpp index 175a0c6313063..f0cb9e47d1e85 100644 --- a/toolchain/driver/compile_subcommand.cpp +++ b/toolchain/driver/compile_subcommand.cpp @@ -369,14 +369,17 @@ class CompilationUnit { source_ = SourceBuffer::MakeFromFileOrStdin(*driver_env_->fs, input_filename_, *consumer_); }); - if (mem_usage_) { - mem_usage_->Add("source_", source_->text().size(), - source_->text().size()); - } + if (!source_) { success_ = false; return; } + + if (mem_usage_) { + mem_usage_->Add("source_", source_->text().size(), + source_->text().size()); + } + CARBON_VLOG("*** SourceBuffer ***\n```\n{0}\n```\n", source_->text()); LogCall("Lex::Lex", "lex", diff --git a/toolchain/driver/driver_test.cpp b/toolchain/driver/driver_test.cpp index 3e6fd0d77944e..3c8112d9b4589 100644 --- a/toolchain/driver/driver_test.cpp +++ b/toolchain/driver/driver_test.cpp @@ -122,6 +122,17 @@ TEST_F(DriverTest, CompileCommandErrors) { StrEq("error: not all required positional arguments were provided; first " "missing and required positional argument: `FILE`\n")); + // Pass non-existing file + EXPECT_FALSE(driver_ + .RunCommand({"compile", "--dump-mem-usage", + "non-existing-file.carbon"}) + .success); + EXPECT_THAT( + test_error_stream_.TakeStr(), + ContainsRegex("No such file or directory[\\n]*non-existing-file.carbon")); + // Flush output stream, because it's ok that it's not empty here. + test_output_stream_.TakeStr(); + // Invalid output filename. No reliably error message here. // TODO: Likely want a different filename on Windows. auto empty_file = MakeTestFile(""); From aaef5166002d6b73d81ea3418770fc1e954ed450 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Tue, 7 Jan 2025 16:08:41 -0800 Subject: [PATCH 5/5] Fix job name for clang tidy (#4760) I think this will affect the name GH shows in some action UIs; this will more clearly disambiguate from tests.yaml actions. --- .github/workflows/clang_tidy.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/clang_tidy.yaml b/.github/workflows/clang_tidy.yaml index 760ce84b28a84..4a5673303d81b 100644 --- a/.github/workflows/clang_tidy.yaml +++ b/.github/workflows/clang_tidy.yaml @@ -20,7 +20,7 @@ concurrency: cancel-in-progress: true jobs: - test: + clang-tidy: runs-on: ubuntu-22.04 steps: