From dd96cea523ebb9297fbb2634439901d703562032 Mon Sep 17 00:00:00 2001 From: dank-openai Date: Thu, 6 Aug 2026 14:37:39 -0400 Subject: [PATCH 1/2] dslx: reject duplicate match alternatives (#30) * dslx: reject duplicate match alternatives with both source locations ## Summary - Reject duplicate match patterns even when an occurrence is embedded in a `|`-separated alternative. - Highlight both the original and duplicate source locations in the compiler error. - Preserve valid grouped alternatives and existing duplicate-arm checks. - Preserve a disabled regression for the separate pre-existing case where an enum type alias gives the same variant two spellings. ## Problem Solved DSLX compared complete match arms when checking for duplicates. Consequently, an enum variant could appear in separate arms and the match would still compile: ```dslx match value { E::A => u32:0, E::B | E::A => u32:1, E::C => u32:2, } ``` The second `E::A` can never match, hiding unreachable code and copy-and-paste mistakes. This change rejects the program with a `TypeInferenceError` and reports both occurrences. ## Implementation Track individual top-level match alternatives alongside the existing whole-arm duplicate check. Retain the first occurrence so the error can name its location and attach both source spans to the diagnostic. Programs that previously compiled with duplicate exact match alternatives now intentionally fail compilation. Valid grouped alternatives and existing range or tuple overlap behavior remain unchanged. Enum type aliases such as `type Alias = E;` can still spell the same member as `E::A` and `Alias::A`. That pre-existing semantic-equivalence case is intentionally not fixed here; a repository-conventional `DISABLED_` regression records the failing behavior for future work. ## Testing - `bazel test //xls/dslx/type_system_v2:typecheck_module_v2_control_flow_test //xls/dslx/exhaustiveness:exhaustiveness_match_test //xls/dslx/type_system:typecheck_module_test` - Regression coverage verifies both source spans, rejects the reported enum pattern, and accepts unique grouped alternatives. - Direct compiler checks cover the checked-in diagnostic fixture, existing whole-arm duplicates, and non-exhaustive enum matches. - Forcing `DISABLED_MatchEnumVariantDuplicatedThroughTypeAlias` to run with `--gtest_also_run_disabled_tests` fails as expected; normal execution passes 100 control-flow tests and reports one disabled test. * dslx: explain unresolved duplicate enum-alias match cases E::A and Alias::A denote the same enum member after type Alias = E, but match validation compares source spellings and incorrectly accepts an unreachable duplicate arm. Record beside the disabled regression that fixing this requires resolving enum-member identity before comparing match patterns. --- .../tests/errors/duplicate_enum_match_arm.x | 23 +++++++ xls/dslx/tests/errors/error_modules_test.py | 12 ++++ xls/dslx/type_system_v2/BUILD | 1 + .../type_system_v2/populate_table_visitor.cc | 33 +++++++--- .../typecheck_module_v2_control_flow_test.cc | 60 +++++++++++++++++++ 5 files changed, 121 insertions(+), 8 deletions(-) create mode 100644 xls/dslx/tests/errors/duplicate_enum_match_arm.x diff --git a/xls/dslx/tests/errors/duplicate_enum_match_arm.x b/xls/dslx/tests/errors/duplicate_enum_match_arm.x new file mode 100644 index 0000000000..ccc4cf7e40 --- /dev/null +++ b/xls/dslx/tests/errors/duplicate_enum_match_arm.x @@ -0,0 +1,23 @@ +// Copyright 2026 The XLS Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +enum Example: u2 { A = 0, B = 1, C = 2 } + +fn main(value: Example) -> u32 { + match value { + Example::A => u32:0, + Example::B | Example::A => u32:1, + Example::C => u32:2, + } +} diff --git a/xls/dslx/tests/errors/error_modules_test.py b/xls/dslx/tests/errors/error_modules_test.py index 730f82b915..98e643e730 100644 --- a/xls/dslx/tests/errors/error_modules_test.py +++ b/xls/dslx/tests/errors/error_modules_test.py @@ -538,6 +538,18 @@ def test_duplicate_match_arm(self): self.assertIn('duplicate_match_arm.x:22:5-22:8', stderr) self.assertIn('Exact-duplicate pattern match detected `FOO`', stderr) + def test_duplicate_enum_match_arm(self): + stderr = self._run( + 'xls/dslx/tests/errors/duplicate_enum_match_arm.x', + ) + self.assertIn('duplicate_enum_match_arm.x:19:5-19:15', stderr) + self.assertIn('duplicate_enum_match_arm.x:20:18-20:28', stderr) + self.assertIn('Exact-duplicate pattern match detected `Example::A`', stderr) + self.assertIn( + 'previously @ xls/dslx/tests/errors/duplicate_enum_match_arm.x:19:5-19:15', + stderr, + ) + def test_trace_fmt_empty_err(self): stderr = self._run( 'xls/dslx/tests/errors/trace_fmt_empty.x', diff --git a/xls/dslx/type_system_v2/BUILD b/xls/dslx/type_system_v2/BUILD index 99e469768d..5d5cf1ceae 100644 --- a/xls/dslx/type_system_v2/BUILD +++ b/xls/dslx/type_system_v2/BUILD @@ -535,6 +535,7 @@ cc_library( "//xls/dslx:errors", "//xls/dslx:import_data", "//xls/dslx:import_routines", + "//xls/dslx:status_payload_utils", "//xls/dslx/frontend:ast", "//xls/dslx/frontend:ast_cloner", "//xls/dslx/frontend:ast_node", diff --git a/xls/dslx/type_system_v2/populate_table_visitor.cc b/xls/dslx/type_system_v2/populate_table_visitor.cc index 6ca429cf30..72e16adfbe 100644 --- a/xls/dslx/type_system_v2/populate_table_visitor.cc +++ b/xls/dslx/type_system_v2/populate_table_visitor.cc @@ -55,6 +55,7 @@ #include "xls/dslx/frontend/pos.h" #include "xls/dslx/import_data.h" #include "xls/dslx/import_routines.h" +#include "xls/dslx/status_payload_utils.h" #include "xls/dslx/type_system/deduce_utils.h" #include "xls/dslx/type_system_v2/import_utils.h" #include "xls/dslx/type_system_v2/inference_table.h" @@ -594,19 +595,28 @@ class PopulateInferenceTableVisitor : public PopulateTableVisitor, file_table_); } + auto duplicate_pattern_error = [this](const Span& original_span, + const Span& duplicate_span, + std::string_view pattern) { + absl::Status status = absl::InvalidArgumentError(absl::StrFormat( + "TypeInferenceError: Exact-duplicate pattern match detected `%s`; " + "only the first could possibly match; previously @ %s", + pattern, original_span.ToString(file_table_))); + AddSpanToStatusPayload(status, duplicate_span, import_data_.file_table()); + AddSpanToStatusPayload(status, original_span, import_data_.file_table()); + return status; + }; + std::vector type_annotation_members; - absl::flat_hash_set seen_patterns; + absl::flat_hash_map seen_arms; + absl::flat_hash_map seen_patterns; for (MatchArm* arm : node->arms()) { // Identify syntactically identical match arms. std::string patterns_string = PatternsToString(arm); - if (auto [it, inserted] = seen_patterns.insert(patterns_string); + if (auto [it, inserted] = seen_arms.try_emplace(patterns_string, arm); !inserted) { - return TypeInferenceErrorStatus( - arm->GetPatternSpan(), nullptr, - absl::StrFormat("Exact-duplicate pattern match detected `%s`; only " - "the first could possibly match", - patterns_string), - file_table_); + return duplicate_pattern_error(it->second->GetPatternSpan(), + arm->GetPatternSpan(), patterns_string); } if (node->IsConst()) { @@ -624,6 +634,13 @@ class PopulateInferenceTableVisitor : public PopulateTableVisitor, } for (const PatternTree& pattern : arm->patterns()) { + if (auto [it, inserted] = seen_patterns.try_emplace( + PatternToString(pattern), GetPatternSpan(pattern)); + !inserted) { + return duplicate_pattern_error(it->second, GetPatternSpan(pattern), + it->first); + } + XLS_RETURN_IF_ERROR( table_.SetTypeVariable(ToAstNode(pattern), matched_var)); } diff --git a/xls/dslx/type_system_v2/typecheck_module_v2_control_flow_test.cc b/xls/dslx/type_system_v2/typecheck_module_v2_control_flow_test.cc index 3f61ebae3a..15dbbf5347 100644 --- a/xls/dslx/type_system_v2/typecheck_module_v2_control_flow_test.cc +++ b/xls/dslx/type_system_v2/typecheck_module_v2_control_flow_test.cc @@ -880,6 +880,66 @@ const Z = match X { HasSubstr("Exact-duplicate pattern match detected `u32:1`"))); } +TEST(TypecheckV2Test, MatchEnumVariantDuplicatedAcrossAlternativeArms) { + constexpr std::string_view kProgram = R"( +enum E: u2 { + A = 0, + B = 1, + C = 2, +} + +fn f(value: E) -> u32 { + match value { + E::A => u32:0, + E::B | E::A => u32:1, + E::C => u32:2, + } +} +)"; + + EXPECT_THAT( + kProgram, + TypecheckFailsWithPayload( + AllOf(HasSubstr("Exact-duplicate pattern match detected `E::A`"), + HasSubstr("previously @ fake.x:12:5-12:9")), + AllOf(HasSpan(11, 4, 11, 8), HasSpan(12, 11, 12, 15)))); +} + +// `E::A` and `Alias::A` denote the same enum member when `type Alias = E`. +// Accepting both match arms is a bug: the later arm can never run, but the +// duplicate check compares their different source spellings and misses it. +// TODO(dank): Resolve each pattern to its enum member before comparing +// identities, then enable this regression. +TEST(TypecheckV2Test, DISABLED_MatchEnumVariantDuplicatedThroughTypeAlias) { + EXPECT_THAT( + R"( +enum E: u2 { A = 0, B = 1 } +type Alias = E; + +fn f(value: E) -> u32 { + match value { + E::A => u32:0, + Alias::A => u32:1, + E::B => u32:2, + } +} +)", + TypecheckFails(HasSubstr("Exact-duplicate pattern match detected"))); +} + +TEST(TypecheckV2Test, MatchEnumVariantAlternativesRemainValid) { + XLS_EXPECT_OK(TypecheckV2(R"( +enum E: u2 { A = 0, B = 1, C = 2 } + +fn f(value: E) -> u32 { + match value { + E::A => u32:0, + E::B | E::C => u32:1, + } +} +)")); +} + TEST(TypecheckV2Test, MatchNonExhaustive) { EXPECT_THAT(R"( fn f(x: u1) -> u32 { From d7457955df26ea3e57db5f88a53fead85a49a01d Mon Sep 17 00:00:00 2001 From: dank-openai Date: Thu, 6 Aug 2026 15:52:02 -0400 Subject: [PATCH 2/2] dslx: reject duplicate enum match alternatives through type aliases Enum type aliases can give the same match alternative different source spellings, allowing unreachable duplicate arms. Resolve enum references through the existing type and import resolver, compare their declared member identities, and preserve the diagnostic spans for both occurrences. Enable the direct-alias regression and cover chained aliases, grouped alternatives, imported aliases, and valid matches with distinct members. --- xls/dslx/type_system_v2/import_utils.cc | 18 ++++ xls/dslx/type_system_v2/import_utils.h | 5 + .../type_system_v2/populate_table_visitor.cc | 16 ++++ .../typecheck_module_v2_control_flow_test.cc | 92 +++++++++++++++++-- 4 files changed, 124 insertions(+), 7 deletions(-) diff --git a/xls/dslx/type_system_v2/import_utils.cc b/xls/dslx/type_system_v2/import_utils.cc index 067e716cfe..7e8f2d75e8 100644 --- a/xls/dslx/type_system_v2/import_utils.cc +++ b/xls/dslx/type_system_v2/import_utils.cc @@ -346,6 +346,24 @@ absl::StatusOr> GetEnumDef( return unwrapper.GetEnumDef(); } +absl::StatusOr> ResolveEnumMember( + const ColonRef* colon_ref, const ImportData& import_data) { + TypeRefUnwrapper unwrapper(import_data); + XLS_RETURN_IF_ERROR(ToAstNode(colon_ref->subject())->Accept(&unwrapper)); + + std::optional enum_member; + if (std::optional enum_def = unwrapper.GetEnumDef(); + enum_def.has_value()) { + for (const EnumMember& member : (*enum_def)->values()) { + if (member.name_def->identifier() == colon_ref->attr()) { + enum_member = member.name_def; + break; + } + } + } + return enum_member; +} + bool IsImport(const ColonRef* colon_ref) { if (colon_ref->ResolveImportSubject().has_value()) { return true; diff --git a/xls/dslx/type_system_v2/import_utils.h b/xls/dslx/type_system_v2/import_utils.h index 61575cb93e..55fe9083c9 100644 --- a/xls/dslx/type_system_v2/import_utils.h +++ b/xls/dslx/type_system_v2/import_utils.h @@ -105,6 +105,11 @@ absl::StatusOr> GetImportedModuleInfo( absl::StatusOr> GetEnumDef( const TypeAnnotation* annotation, const ImportData& import_data); +// Resolves an enum member through type aliases. Returns `nullopt` if the +// reference does not identify an enum or the enum has no matching member. +absl::StatusOr> ResolveEnumMember( + const ColonRef* colon_ref, const ImportData& import_data); + // Returns whether `f` is a `next` function in an impl-style proc. absl::StatusOr IsProcDefNextFunction(const Function* f, const ImportData& import_data); diff --git a/xls/dslx/type_system_v2/populate_table_visitor.cc b/xls/dslx/type_system_v2/populate_table_visitor.cc index 72e16adfbe..f7a4ee7856 100644 --- a/xls/dslx/type_system_v2/populate_table_visitor.cc +++ b/xls/dslx/type_system_v2/populate_table_visitor.cc @@ -610,6 +610,7 @@ class PopulateInferenceTableVisitor : public PopulateTableVisitor, std::vector type_annotation_members; absl::flat_hash_map seen_arms; absl::flat_hash_map seen_patterns; + absl::flat_hash_map seen_enum_members; for (MatchArm* arm : node->arms()) { // Identify syntactically identical match arms. std::string patterns_string = PatternsToString(arm); @@ -641,6 +642,21 @@ class PopulateInferenceTableVisitor : public PopulateTableVisitor, it->first); } + if (std::holds_alternative(pattern)) { + const ColonRef* colon_ref = std::get(pattern); + XLS_ASSIGN_OR_RETURN(std::optional enum_member, + ResolveEnumMember(colon_ref, import_data_)); + if (enum_member.has_value()) { + if (auto [it, inserted] = seen_enum_members.try_emplace( + *enum_member, GetPatternSpan(pattern)); + !inserted) { + return duplicate_pattern_error(it->second, + GetPatternSpan(pattern), + PatternToString(pattern)); + } + } + } + XLS_RETURN_IF_ERROR( table_.SetTypeVariable(ToAstNode(pattern), matched_var)); } diff --git a/xls/dslx/type_system_v2/typecheck_module_v2_control_flow_test.cc b/xls/dslx/type_system_v2/typecheck_module_v2_control_flow_test.cc index 15dbbf5347..a43c3ab6fc 100644 --- a/xls/dslx/type_system_v2/typecheck_module_v2_control_flow_test.cc +++ b/xls/dslx/type_system_v2/typecheck_module_v2_control_flow_test.cc @@ -905,12 +905,9 @@ fn f(value: E) -> u32 { AllOf(HasSpan(11, 4, 11, 8), HasSpan(12, 11, 12, 15)))); } -// `E::A` and `Alias::A` denote the same enum member when `type Alias = E`. -// Accepting both match arms is a bug: the later arm can never run, but the -// duplicate check compares their different source spellings and misses it. -// TODO(dank): Resolve each pattern to its enum member before comparing -// identities, then enable this regression. -TEST(TypecheckV2Test, DISABLED_MatchEnumVariantDuplicatedThroughTypeAlias) { +// Type aliases do not create distinct enum members, even when their match +// patterns have different source spellings. +TEST(TypecheckV2Test, MatchEnumVariantDuplicatedThroughTypeAlias) { EXPECT_THAT( R"( enum E: u2 { A = 0, B = 1 } @@ -924,7 +921,88 @@ fn f(value: E) -> u32 { } } )", - TypecheckFails(HasSubstr("Exact-duplicate pattern match detected"))); + TypecheckFailsWithPayload( + AllOf(HasSubstr("Exact-duplicate pattern match detected `Alias::A`"), + HasSubstr("previously @ fake.x:9:5-9:9")), + AllOf(HasSpan(8, 4, 8, 8), HasSpan(9, 4, 9, 12)))); +} + +TEST(TypecheckV2Test, MatchEnumVariantDuplicatedThroughChainedTypeAliases) { + EXPECT_THAT( + R"( +enum E: u2 { A = 0, B = 1 } +type First = E; +type Second = First; + +fn f(value: E) -> u32 { + match value { + First::A => u32:0, + Second::A => u32:1, + E::B => u32:2, + } +} +)", + TypecheckFails( + HasSubstr("Exact-duplicate pattern match detected `Second::A`"))); +} + +TEST(TypecheckV2Test, + MatchEnumVariantDuplicatedThroughTypeAliasInGroupedAlternatives) { + EXPECT_THAT( + R"( +enum E: u2 { A = 0, B = 1, C = 2 } +type Alias = E; + +fn f(value: E) -> u32 { + match value { + E::A => u32:0, + E::B | Alias::A => u32:1, + E::C => u32:2, + } +} +)", + TypecheckFails( + HasSubstr("Exact-duplicate pattern match detected `Alias::A`"))); +} + +TEST(TypecheckV2Test, MatchImportedEnumVariantDuplicatedThroughTypeAlias) { + constexpr std::string_view kImported = R"( +pub enum E: u2 { A = 0, B = 1 } +pub type ImportedAlias = E; +)"; + constexpr std::string_view kProgram = R"( +import imported; +type Alias = imported::ImportedAlias; + +fn f(value: Alias) -> u32 { + match value { + imported::E::A => u32:0, + Alias::A => u32:1, + imported::E::B => u32:2, + } +} +)"; + + ImportData import_data = CreateImportDataForTest(); + XLS_EXPECT_OK(TypecheckV2(kImported, "imported", &import_data)); + EXPECT_THAT( + TypecheckV2(kProgram, "main", &import_data), + StatusIs(absl::StatusCode::kInvalidArgument, + HasSubstr("Exact-duplicate pattern match detected `Alias::A`"))); +} + +TEST(TypecheckV2Test, MatchDistinctEnumVariantsThroughTypeAliasRemainValid) { + XLS_EXPECT_OK(TypecheckV2(R"( +enum E: u2 { A = 0, B = 1, C = 2 } +type Alias = E; + +fn f(value: E) -> u32 { + match value { + E::A => u32:0, + Alias::B | Alias::C => u32:1, + } +} +)")); } TEST(TypecheckV2Test, MatchEnumVariantAlternativesRemainValid) {