From 8ab875690a8c4535749e181a3c4da7d0b8731384 Mon Sep 17 00:00:00 2001 From: prql-bot <107324867+prql-bot@users.noreply.github.com> Date: Sat, 8 Aug 2026 06:44:52 +0000 Subject: [PATCH 1/2] fix: report duplicate declarations from enum definitions --- prqlc/prqlc/src/semantic/resolver/stmt.rs | 66 ++++++++------- .../prqlc/tests/integration/error_messages.rs | 83 +++++++++++++++++++ 2 files changed, 120 insertions(+), 29 deletions(-) diff --git a/prqlc/prqlc/src/semantic/resolver/stmt.rs b/prqlc/prqlc/src/semantic/resolver/stmt.rs index 8f7ef1fe28aa..16b7aaaadde1 100644 --- a/prqlc/prqlc/src/semantic/resolver/stmt.rs +++ b/prqlc/prqlc/src/semantic/resolver/stmt.rs @@ -6,7 +6,7 @@ use crate::pr::{Ty, TyKind, TyTupleField}; use crate::semantic::ast_expand::expand_expr; use crate::semantic::{NS_SELF, STD_LIB_SOURCE_ID}; use crate::Result; -use crate::WithErrorInfo; +use crate::{Error, WithErrorInfo}; impl super::Resolver<'_> { // entry point to the resolver @@ -63,56 +63,64 @@ impl super::Resolver<'_> { // Enums get their own module definition, with NS_SELF pointing to the original type and a // name entry in the module for each alias in the enum's tuple. - self.current_module_path.push(ident.name); - let mut module = Module { names: HashMap::new(), redirects: Vec::new(), shadowed: None, }; + module.names.insert( + NS_SELF.to_string(), + Decl { + declared_at: stmt.id, + kind: DeclKind::Ty(ty.clone()), + annotations: stmt.annotations, + ..Default::default() + }, + ); for expr in enum_tuple { let name = expr .alias .clone() .expect("enum literal tuples should always have all fields with aliases"); + let member_span = expr.span; + + // NS_SELF is already in `names`, so a member of that name would + // replace the type entry rather than being reported below. + if name == NS_SELF { + return Err(Error::new_simple(format!( + "`{NS_SELF}` is a reserved name and cannot be an enum member" + )) + .with_span(member_span)); + } + let mut expr = expand_expr(expr)?; expr.ty = Some(ty.clone()); // here, the expr needs to have no alias, so that it doesn't clobber an alias // used in a future tuple expression expr.alias = None; - module.names.insert( - name, - Decl { - declared_at: stmt.id, - kind: DeclKind::Expr(Box::new(expr)), - ..Default::default() - }, - ); - } - module.names.insert( - NS_SELF.to_string(), - Decl { + let member = Decl { declared_at: stmt.id, - kind: DeclKind::Ty(ty), - annotations: stmt.annotations, + kind: DeclKind::Expr(Box::new(expr)), ..Default::default() - }, - ); - - let decl = Decl { - declared_at: stmt.id, - kind: DeclKind::Module(module), - ..Default::default() - }; + }; + + // `names` is a map, so a repeated member would otherwise keep only + // the last value. + if module.names.insert(name.clone(), member).is_some() { + return Err(Error::new_simple(format!( + "duplicate declarations of {ident}.{name}" + )) + .with_span(member_span)); + } + } - let ident = Ident::from_path(self.current_module_path.clone()); + // `declare` reports a name that's already taken; inserting into + // `root_mod.module` directly would overwrite it silently. self.root_mod - .module - .insert(ident, decl) + .declare(ident, DeclKind::Module(module), stmt.id, Vec::new()) .with_span(stmt.span)?; - self.current_module_path.pop(); } } else { let decl = DeclKind::Ty(ty); diff --git a/prqlc/prqlc/tests/integration/error_messages.rs b/prqlc/prqlc/tests/integration/error_messages.rs index d77fb0180a16..a2a301e1006e 100644 --- a/prqlc/prqlc/tests/integration/error_messages.rs +++ b/prqlc/prqlc/tests/integration/error_messages.rs @@ -531,6 +531,89 @@ fn enum_type_2() { "); } +/// An `enum` builds a module and so used to take a different insertion path +/// than every other statement kind, which meant it replaced a name that was +/// already declared instead of reporting the collision. +#[test] +fn enum_duplicate_of_existing_declaration() { + assert_snapshot!(compile(r###" + let Status = 5 + enum Status { Paid = 0 } + from invoices + "###).unwrap_err(), @" + Error: + ╭─[ :2:19 ] + │ + 2 │ ╭─▶ let Status = 5 + 3 │ ├─▶ enum Status { Paid = 0 } + │ │ + │ ╰────────────────────────────────── duplicate declarations of Status + ───╯ + "); + + assert_snapshot!(compile(r###" + module m { let a = 5 } + enum m { Paid = 0 } + from invoices + "###).unwrap_err(), @" + Error: + ╭─[ :2:27 ] + │ + 2 │ ╭─▶ module m { let a = 5 } + 3 │ ├─▶ enum m { Paid = 0 } + │ │ + │ ╰───────────────────────────── duplicate declarations of m + ───╯ + "); + + assert_snapshot!(compile(r###" + enum Status { Paid = 0 } + enum Status { Unpaid = 1 } + from invoices + "###).unwrap_err(), @" + Error: + ╭─[ :2:29 ] + │ + 2 │ ╭─▶ enum Status { Paid = 0 } + 3 │ ├─▶ enum Status { Unpaid = 1 } + │ │ + │ ╰──────────────────────────────────── duplicate declarations of Status + ───╯ + "); +} + +/// The enum's members become entries in a map, so a repeated name — or one that +/// collides with the `_self` entry holding the type itself — used to leave only +/// the last value with no diagnostic. +#[test] +fn enum_duplicate_member() { + assert_snapshot!(compile(r###" + enum Status { Paid = 0, Paid = 1 } + from invoices + "###).unwrap_err(), @" + Error: + ╭─[ :2:36 ] + │ + 2 │ enum Status { Paid = 0, Paid = 1 } + │ ┬ + │ ╰── duplicate declarations of Status.Paid + ───╯ + "); + + assert_snapshot!(compile(r###" + enum Status { _self = 0, Paid = 1 } + from invoices + "###).unwrap_err(), @" + Error: + ╭─[ :2:27 ] + │ + 2 │ enum Status { _self = 0, Paid = 1 } + │ ┬ + │ ╰── `_self` is a reserved name and cannot be an enum member + ───╯ + "); +} + #[test] fn append_by_wrong() { assert_snapshot!(compile(r###" From 0daf6cc36361999fefe67ccedaac9c538a5537ec Mon Sep 17 00:00:00 2001 From: prql-bot <107324867+prql-bot@users.noreply.github.com> Date: Sat, 8 Aug 2026 07:03:07 +0000 Subject: [PATCH 2/2] test: note the untested direction of the module-def collision --- prqlc/prqlc/tests/integration/error_messages.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/prqlc/prqlc/tests/integration/error_messages.rs b/prqlc/prqlc/tests/integration/error_messages.rs index a2a301e1006e..c7cfc7b58ab7 100644 --- a/prqlc/prqlc/tests/integration/error_messages.rs +++ b/prqlc/prqlc/tests/integration/error_messages.rs @@ -534,6 +534,11 @@ fn enum_type_2() { /// An `enum` builds a module and so used to take a different insertion path /// than every other statement kind, which meant it replaced a name that was /// already declared instead of reporting the collision. +/// +/// Only the direction where the `enum` comes second is covered — a `module` +/// declared after an `enum` (or after another `module`) still overwrites +/// silently, since `fold_module_def_stmt` keeps its own `Module::insert`; #6166 +/// tracks that. #[test] fn enum_duplicate_of_existing_declaration() { assert_snapshot!(compile(r###"