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 1b974cf7db05..2212b68172b6 100644 --- a/prqlc/prqlc/src/semantic/resolver/stmt.rs +++ b/prqlc/prqlc/src/semantic/resolver/stmt.rs @@ -135,21 +135,38 @@ 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; + } + } + // `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(), + 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###"