fix: report duplicate declarations from enum definitions - #6164
Conversation
prql-bot
left a comment
There was a problem hiding this comment.
One gap, and one note for merge ordering.
fold_module_def_stmt still takes the old path, so the enum-vs-module order-dependence only closes in one direction. The third case in enum_duplicate_of_existing_declaration asserts that module m { let a = 5 } followed by enum m { … } now errors. The reverse still compiles clean and silently discards the enum:
$ printf 'enum m { Paid = 0 }\nmodule m { let a = 5 }\nfrom t | filter x == m.Paid' | prqlc compile -
Error:
╭─[ :3:22 ]
│
3 │ from t | filter x == m.Paid
│ ───┬──
│ ╰──── Unknown name `m.Paid`
That's the same shape the description calls out — a whole declaration gone with no diagnostic — just with the surviving Module::insert on the other side. In fold_module_def_stmt:
self.root_mod
.module
.insert(ident, decl)
.with_span(stmt.span)?;Two module m { … } blocks collide the same way: module m { let a = 5 } then module m { let b = 6 } compiles, and m.a is gone. So "moving to declare covers all of it" holds for the let/type/enum axis but not for module defs, and the suite now asserts one direction of a still-symmetric bug. Routing module defs through declare is a separate breaking change and reads like its own PR next to #6150 rather than something to fold in here — but it might be worth a line in the description so the remaining half doesn't look covered.
#6150 edits fold_import_def_stmt in this same file. Different function, complementary change (it also adds a CHANGELOG.md entry, which this one skips on the grounds that enum is unreleased — that reasoning checks out, #6104 is still under [unreleased]). Just worth knowing they're queued together.
What I checked
cargo test -p prqlc --lib --tests— green (78 lib / 27 / 486 integration).- Enum inside a module still resolves:
module m { enum Status { Paid = 0 } }+m.Status.Paid→status = 0. - The
_selfguard is the right scope — of theNS_*constants, onlyNS_SELFlands in the enum's own module, so_infer,_param,std,this,that,mainare all still usable as member names and resolve correctly (Status._infer→ its value). - No new panic path: the pre-existing
.expect("enum literal tuples should always have all fields with aliases")stays unreachable from source —literal_tuple()in the parser rejects an unaliased member withmust specify an alias for this value, andenum_defalways wraps aTuple. declare(ident, …, stmt.id, Vec::new())builds the sameDeclthe oldDecl { declared_at, kind, ..Default::default() }did (order: 0, empty annotations), withstmt.annotationsstill on theNS_SELFentry — so no annotation or ordering drift.- Dropping the
current_module_pathpush/pop also removes a leak: the old code returned early fromexpand_expr(expr)?and from the insert without popping. - Enum name vs std lib name is unaffected (
enum JoinSide { … }compiles) since std sits under its own module;import std.math+enum math { … }is a new error, consistent with #6150's direction.
|
Confirmed both against Agreed it's its own change, and there's a concrete reason beyond "breaking": the straight swap to Scoped the description accordingly, and added the same note to One thing the issue rules out: multi-file projects never reach this path, since |
fold_type_def_stmtbuilds a module for anenumand pushed it in withModule::insert, which overwrites whatever is already under that name. Every other statement kind goes throughRootModule::declare, which reports a duplicate instead. The result was order-dependent: the same pair of statements errored one way round and silently discarded a declaration the other.The same shape swallowed a whole module —
module m { let a = 5 }followed byenum m { … }compiled clean, andm.awas simply gone. Moving todeclarecovers all of that, includingenumvsenum.Scope: this changes
fold_type_def_stmtonly, so the fix is one-directional where amoduleis the second declaration.enum m { … }followed bymodule m { … }, and twomodule m { … }blocks, both still overwrite silently —fold_module_def_stmtkeeps its ownModule::insert. Filed as #6166 rather than fixed here: the straight swap todeclarefails 31 tests withduplicate declarations of std, becauseModule::new_rootpre-seeds astdentry thatload_std_lib's module def then collides with, so it needs its own change.fold_import_def_stmtis the third instance and is covered by #6150.Two smaller silent drops inside the enum's own module, both from the same cause —
namesis aHashMap, so an insert that collides keeps only the last value:enum Status { Paid = 0, Paid = 1 }compiled, andStatus.Paidwas1. Nowduplicate declarations of Status.Paid.enum Status { _self = 0 }compiled, and the member vanished —NS_SELFis the key the enum's own type is stored under, and it's written after the members._selfis a legal identifier, so this was reachable from source. The type entry now goes in first and a member of that name is rejected:._selfis a reserved name and cannot be an enum memberenumlanded in #6104 and hasn't been in a release, so nothing here changes behavior against a published version — that's also why there's no CHANGELOG entry, since the feature's own entry already covers the release. Happy to add one if you'd rather have it recorded.The
current_module_pathpush/pop went away because the only thing it fed wasIdent::from_path(current_module_path), which reconstructed theidentalready passed in;expand_expris a free function and doesn't consult it.Verification
cargo test -p prqlc -p prqlc-parser— green (lib 78 passed / 1 ignored,tests/integration486 passed / 5 ignored, parser 97 passed).cargo clippy -p prqlc --all-targets,cargo fmt --check -p prqlc— clean.task prqlc:pull-requestcouldn't run here — it shells out tocargo insta, which isn't on the sandbox's PATH (ci: expose cargo-insta and cargo-nextest to the tend sandbox #6144). Rancargo testover the same packages instead.stmt.rswith the tests in place: bothenum_duplicate_of_existing_declarationandenum_duplicate_memberfail, so they do gate the change.join y (==id) side:leftandfilter status == InvoiceStatus.Paidcompile unchanged.One rough edge worth flagging: the caret for a member-level error lands on the member's value, not its name (
Paid = 0, Paid = 1underlines the1).aliasis a bareOption<String>onExprwith no span of its own, so the value span is the closest anchor available without changing the AST.