From 25d8e4aede67488feab1ea0e297feab3760398f0 Mon Sep 17 00:00:00 2001 From: prql-bot <107324867+prql-bot@users.noreply.github.com> Date: Tue, 18 Aug 2026 07:21:53 +0000 Subject: [PATCH 1/2] fix: extend a re-opened `module` instead of discarding the earlier block --- prqlc/prqlc/src/semantic/resolver/stmt.rs | 49 +++++++++---- .../prqlc/tests/integration/error_messages.rs | 73 ++++++++++++++++++- 2 files changed, 104 insertions(+), 18 deletions(-) diff --git a/prqlc/prqlc/src/semantic/resolver/stmt.rs b/prqlc/prqlc/src/semantic/resolver/stmt.rs index 1b974cf7db05..8a2e47087dad 100644 --- a/prqlc/prqlc/src/semantic/resolver/stmt.rs +++ b/prqlc/prqlc/src/semantic/resolver/stmt.rs @@ -135,21 +135,42 @@ impl super::Resolver<'_> { let module_def = stmt.kind.into_module_def().unwrap(); self.current_module_path.push(ident.name); - let decl = Decl { - declared_at: stmt.id, - kind: DeclKind::Module(Module { - names: HashMap::new(), - redirects: Vec::new(), - shadowed: None, - }), - annotations: stmt.annotations, - ..Default::default() - }; let ident = Ident::from_path(self.current_module_path.clone()); - self.root_mod - .module - .insert(ident, decl) - .with_span(stmt.span)?; + + // A module that already exists is extended rather than replaced. That + // is how a module spread over several files already behaves, and it is + // what lets the standard library fill in the empty `std` placeholder + // seeded by `Module::new_root`. Inserting unconditionally would discard + // whatever the name already held, with no diagnostic; collisions + // between the two blocks' own declarations are still reported, by the + // `fold_statements` below. + match self.root_mod.module.get(&ident).map(|d| d.kind.is_module()) { + Some(true) => { + // The seeded placeholder carries no id or annotations of its + // own, so the first real block to arrive supplies them. + let existing = self.root_mod.module.get_mut(&ident).unwrap(); + if existing.declared_at.is_none() { + existing.declared_at = stmt.id; + existing.annotations = stmt.annotations; + } + } + Some(false) => { + return Err( + Error::new_simple(format!("duplicate declarations of {ident}")) + .with_span(stmt.span), + ) + } + None => { + let decl = DeclKind::Module(Module { + names: HashMap::new(), + redirects: Vec::new(), + shadowed: None, + }); + self.root_mod + .declare(ident, decl, stmt.id, stmt.annotations) + .with_span(stmt.span)?; + } + } self.fold_statements(module_def.stmts)?; self.current_module_path.pop(); diff --git a/prqlc/prqlc/tests/integration/error_messages.rs b/prqlc/prqlc/tests/integration/error_messages.rs index 4f609aa02de2..0d479d001dc3 100644 --- a/prqlc/prqlc/tests/integration/error_messages.rs +++ b/prqlc/prqlc/tests/integration/error_messages.rs @@ -535,10 +535,9 @@ fn enum_type_2() { /// 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. +/// The reverse direction — a `module` declared after an `enum` or after +/// another `module` — is covered by `module_reopened_is_extended` below, which +/// extends the existing module rather than reporting a collision. #[test] fn enum_duplicate_of_existing_declaration() { assert_snapshot!(compile(r###" @@ -619,6 +618,72 @@ fn enum_duplicate_member() { "); } +/// Re-opening a `module` used to replace the earlier block outright, so its +/// declarations disappeared with no diagnostic. The blocks are now merged, and +/// a name declared by both is reported. +#[test] +fn module_reopened_is_extended() { + // Both blocks contribute; neither is discarded. + assert_snapshot!(compile(r###" + module m { let a = 5 } + module m { let b = 6 } + from t + select {x = m.a, y = m.b} + "###).unwrap(), @r" + SELECT + 5 AS x, + 6 AS y + FROM + t + "); + + // An `enum` also builds a module, so it is extended rather than dropped. + assert_snapshot!(compile(r###" + enum m { Paid = 0 } + module m { let a = 5 } + from t + select {x = m.Paid, y = m.a} + "###).unwrap(), @r" + SELECT + 0 AS x, + 5 AS y + FROM + t + "); + + // A name declared by both blocks is a real collision. + assert_snapshot!(compile(r###" + module m { let a = 5 } + module m { let a = 6 } + from t + "###).unwrap_err(), @" + Error: + ╭─[ :3:16 ] + │ + 3 │ module m { let a = 6 } + │ ────┬──── + │ ╰────── duplicate declarations of m.a + ───╯ + "); + + // Re-opening only applies to modules; any other kind of declaration under + // that name is still a collision. + assert_snapshot!(compile(r###" + let m = 5 + module m { let a = 6 } + from t + "###).unwrap_err(), @" + Error: + ╭─[ :2:14 ] + │ + 2 │ ╭─▶ let m = 5 + 3 │ ├─▶ module m { let a = 6 } + │ │ + │ ╰──────────────────────────────── duplicate declarations of m + ───╯ + "); +} + #[test] fn append_by_wrong() { assert_snapshot!(compile(r###" From 2a1923e09944a449fe43bcfab8194fe7a736d383 Mon Sep 17 00:00:00 2001 From: prql-bot <107324867+prql-bot@users.noreply.github.com> Date: Tue, 18 Aug 2026 07:31:11 +0000 Subject: [PATCH 2/2] fix: add changelog entry, fold the non-module arm into `declare` `declare` already returns `duplicate declarations of {ident}` for a name that's taken, so the explicit `Some(false)` arm was a second copy of the same message and span. --- CHANGELOG.md | 9 +++++++++ prqlc/prqlc/src/semantic/resolver/stmt.rs | 10 +++------- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6760222fdcdc..8fd40c768cb1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,15 @@ - _Breaking_: The compile target `sql.glaredb` has been removed, since it was never tested after GlareDB's full rewrite, and GlareDB seems to be no longer maintained. (@eitsupi, #6172) +- Re-opening a `module` extends the earlier block rather than replacing it. + `module m { let a = 5 }` followed by `module m { let b = 6 }` previously kept + only `b` and dropped `a` with no diagnostic; both now resolve, matching how a + module spread over several files already behaves. Two smaller breaking changes + come with it: a name declared by both blocks reports + `duplicate declarations of m.a`, and re-using a name that holds something + other than a module — `let m = 5` followed by `module m { ... }` — reports + `duplicate declarations of m` instead of silently discarding the `let`. + (@prql-bot, #6206) **Features**: diff --git a/prqlc/prqlc/src/semantic/resolver/stmt.rs b/prqlc/prqlc/src/semantic/resolver/stmt.rs index 8a2e47087dad..2212b68172b6 100644 --- a/prqlc/prqlc/src/semantic/resolver/stmt.rs +++ b/prqlc/prqlc/src/semantic/resolver/stmt.rs @@ -154,13 +154,9 @@ impl super::Resolver<'_> { existing.annotations = stmt.annotations; } } - Some(false) => { - return Err( - Error::new_simple(format!("duplicate declarations of {ident}")) - .with_span(stmt.span), - ) - } - None => { + // `declare` inserts a name that's free and reports one that's + // taken, which is what a non-module under this name is. + _ => { let decl = DeclKind::Module(Module { names: HashMap::new(), redirects: Vec::new(),