fix: extend a re-opened module instead of discarding the earlier block

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**:
 

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();

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###"