Skip to content

Commit b338ad7

Browse files
Haofeiclaude
andcommitted
rsscript: fix cross-file associated consts and module-prefix collisions
Two namespace-isolation bugs found in review: 1. Cross-file associated constants (`Device.DEFAULT` declared in one module, read from another via `use device.Device`) produced RS0026 — the per-file associated-const desugar only fires when the dotted const is in the same parsed file, and isolation then left `Device.DEFAULT` as a field access. Isolation now recognizes `Type.CONST` access: when the base is a module-scoped type whose module declares the flattened constant, it resolves the access to the module-qualified const symbol. Constants are mangled through `mangle_value` (upper-cased) so the declaration and references agree with the backend's SCREAMING_SNAKE const lowering. 2. Module-prefix collisions: `module a.b` and `module a_b` both joined to the prefix `a_b`. Prefixes are now built injectively (each segment's underscores are doubled before joining with a single underscore), so `a.b` -> `a_b` and `a_b` -> `a__b` stay distinct. Also allow non_camel_case_types / non_upper_case_globals in generated crates (module-isolated symbols carry a lowercase module prefix). Tests: cross-file associated-const lowering, dotted-vs-underscored module paths, and module_prefix injectivity unit tests. Full cargo test -p rsscript green; clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 1136921 commit b338ad7

4 files changed

Lines changed: 195 additions & 21 deletions

File tree

crates/rsscript/src/rust_lower/lowerer.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,11 @@ impl<'a> RustLowerer<'a> {
137137
out.push_str(
138138
"// Runtime hooks are intentionally explicit while Rust lowering is stabilizing.\n",
139139
);
140-
out.push_str("#![allow(dead_code, non_snake_case)]\n");
140+
// Module-isolated symbols carry a lowercase module prefix (`device__Device`,
141+
// `helpers__count`), so the camel/snake/upper-case lints don't apply.
142+
out.push_str(
143+
"#![allow(dead_code, non_snake_case, non_camel_case_types, non_upper_case_globals)]\n",
144+
);
141145
let feature_names = lowered_feature_names(&self.program.features);
142146
if feature_names.is_empty() {
143147
out.push_str("// RSScript features: <none>\n");

crates/rsscript/src/syntax/module_isolation.rs

Lines changed: 127 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,27 @@ use super::ast::{
2323
/// the boundary readable in diagnostics and generated Rust (`a_b__name`).
2424
const MODULE_SEP: &str = "__";
2525

26+
/// Encode a dotted module path as a single Rust-identifier prefix, injectively:
27+
/// each segment's own underscores are doubled before segments are joined with a
28+
/// single underscore, so distinct paths (`a.b` vs `a_b`) never collide.
29+
fn module_prefix(segments: &[String]) -> String {
30+
segments
31+
.iter()
32+
.map(|segment| segment.replace('_', "__"))
33+
.collect::<Vec<_>>()
34+
.join("_")
35+
}
36+
37+
/// The module prefix for a dotted namespace string (`a.b` -> `a_b`).
38+
fn module_prefix_from_dotted(namespace: &str) -> String {
39+
module_prefix(
40+
&namespace
41+
.split('.')
42+
.map(str::to_string)
43+
.collect::<Vec<_>>(),
44+
)
45+
}
46+
2647
/// Rewrite a program so each `module`-scoped symbol becomes globally unique.
2748
pub fn isolate_module_namespaces(program: &mut Program) {
2849
let file_module = collect_file_modules(program);
@@ -42,7 +63,7 @@ fn collect_file_modules(program: &Program) -> HashMap<String, String> {
4263
if let Item::Module(module) = item {
4364
file_module
4465
.entry(module.span.file.clone())
45-
.or_insert_with(|| module.path.join("_"));
66+
.or_insert_with(|| module_prefix(&module.path));
4667
}
4768
}
4869
file_module
@@ -57,6 +78,10 @@ struct Resolver {
5778
/// module prefix -> the type names it declares (for `Type.method` / `Type.X`
5879
/// namespace resolution).
5980
module_types: HashMap<String, HashSet<String>>,
81+
/// module prefix -> the constant names it declares. Constants lower to
82+
/// `SCREAMING_SNAKE_CASE`, so their mangled symbol is upper-cased to stay
83+
/// consistent between declaration and reference.
84+
module_consts: HashMap<String, HashSet<String>>,
6085
/// file -> (imported bare name -> module prefix it was imported from).
6186
file_imports: HashMap<String, HashMap<String, String>>,
6287
}
@@ -65,6 +90,7 @@ impl Resolver {
6590
fn build(program: &Program, file_module: &HashMap<String, String>) -> Self {
6691
let mut module_defs: HashMap<String, HashSet<String>> = HashMap::new();
6792
let mut module_types: HashMap<String, HashSet<String>> = HashMap::new();
93+
let mut module_consts: HashMap<String, HashSet<String>> = HashMap::new();
6894
let mut file_imports: HashMap<String, HashMap<String, String>> = HashMap::new();
6995

7096
for item in &program.items {
@@ -92,6 +118,10 @@ impl Resolver {
92118
.entry(prefix.clone())
93119
.or_default()
94120
.insert(decl.name.clone());
121+
module_consts
122+
.entry(prefix.clone())
123+
.or_default()
124+
.insert(decl.name.clone());
95125
}
96126
}
97127
Item::Type(decl) => {
@@ -127,7 +157,7 @@ impl Resolver {
127157
Item::Use(decl) => {
128158
if decl.path.len() >= 2 {
129159
let name = decl.path[decl.path.len() - 1].clone();
130-
let import_prefix = decl.path[..decl.path.len() - 1].join("_");
160+
let import_prefix = module_prefix(&decl.path[..decl.path.len() - 1]);
131161
file_imports
132162
.entry(decl.span.file.clone())
133163
.or_default()
@@ -142,10 +172,27 @@ impl Resolver {
142172
file_module: file_module.clone(),
143173
module_defs,
144174
module_types,
175+
module_consts,
145176
file_imports,
146177
}
147178
}
148179

180+
/// Build the mangled value symbol for `name` declared in module `prefix`,
181+
/// upper-casing constants so the symbol matches the Rust backend's
182+
/// `SCREAMING_SNAKE_CASE` const lowering at both declaration and reference.
183+
fn mangle_value(&self, prefix: &str, name: &str) -> String {
184+
let mangled = format!("{prefix}{MODULE_SEP}{name}");
185+
if self
186+
.module_consts
187+
.get(prefix)
188+
.is_some_and(|consts| consts.contains(name))
189+
{
190+
mangled.to_uppercase()
191+
} else {
192+
mangled
193+
}
194+
}
195+
149196
/// The mangled name for a bare value/type symbol `name` referenced from
150197
/// `file`, or `None` to leave it unchanged (root symbol, builtin, external,
151198
/// or unresolved).
@@ -157,7 +204,7 @@ impl Resolver {
157204
.get(prefix)
158205
.is_some_and(|names| names.contains(name))
159206
{
160-
return Some(format!("{prefix}{MODULE_SEP}{name}"));
207+
return Some(self.mangle_value(prefix, name));
161208
}
162209
// Otherwise a `use module.name` import that names a known module symbol.
163210
if let Some(import_prefix) = self
@@ -169,36 +216,43 @@ impl Resolver {
169216
.get(import_prefix)
170217
.is_some_and(|names| names.contains(name))
171218
{
172-
return Some(format!("{import_prefix}{MODULE_SEP}{name}"));
219+
return Some(self.mangle_value(import_prefix, name));
173220
}
174221
None
175222
}
176223

177-
/// Resolve a `Type.` namespace (for `Type.method` / `Type.CONST`): if the
178-
/// namespace names a module-scoped type, return its mangled type name.
179-
fn resolve_type_namespace(&self, file: &str, namespace: &str) -> Option<String> {
224+
/// The module prefix that owns type `name` as seen from `file` (own module,
225+
/// then `use` import), or `None` if it is not a module-scoped type.
226+
fn resolve_type_module(&self, file: &str, name: &str) -> Option<String> {
180227
if let Some(prefix) = self.file_module.get(file)
181228
&& self
182229
.module_types
183230
.get(prefix)
184-
.is_some_and(|types| types.contains(namespace))
231+
.is_some_and(|types| types.contains(name))
185232
{
186-
return Some(format!("{prefix}{MODULE_SEP}{namespace}"));
233+
return Some(prefix.clone());
187234
}
188235
if let Some(import_prefix) = self
189236
.file_imports
190237
.get(file)
191-
.and_then(|imports| imports.get(namespace))
238+
.and_then(|imports| imports.get(name))
192239
&& self
193240
.module_types
194241
.get(import_prefix)
195-
.is_some_and(|types| types.contains(namespace))
242+
.is_some_and(|types| types.contains(name))
196243
{
197-
return Some(format!("{import_prefix}{MODULE_SEP}{namespace}"));
244+
return Some(import_prefix.clone());
198245
}
199246
None
200247
}
201248

249+
/// Resolve a `Type.` namespace (for `Type.method` / `Type.CONST`): if the
250+
/// namespace names a module-scoped type, return its mangled type name.
251+
fn resolve_type_namespace(&self, file: &str, namespace: &str) -> Option<String> {
252+
self.resolve_type_module(file, namespace)
253+
.map(|prefix| format!("{prefix}{MODULE_SEP}{namespace}"))
254+
}
255+
202256
fn rewrite(&self, program: &mut Program) {
203257
for item in &mut program.items {
204258
let Some(file) = item_file(item).map(str::to_string) else {
@@ -243,8 +297,13 @@ impl Resolver {
243297
}
244298

245299
fn rewrite_const(&self, decl: &mut ConstDecl, file: &str, in_module: bool) {
246-
if in_module {
247-
decl.name = self.mangle_decl_name(file, &decl.name);
300+
// Constants are mangled through `mangle_value` (upper-cased) so the
301+
// declaration matches references and the backend's const lowering.
302+
if in_module
303+
&& !decl.name.contains('.')
304+
&& let Some(prefix) = self.file_module.get(file)
305+
{
306+
decl.name = self.mangle_value(prefix, &decl.name);
248307
}
249308
if let Some(ty) = &mut decl.type_annotation {
250309
self.rewrite_type(ty, file);
@@ -472,7 +531,28 @@ impl Resolver {
472531
self.rewrite_expr(&mut arg.value, file, scope);
473532
}
474533
}
475-
Expr::Field { base, .. } => self.rewrite_expr(base, file, scope),
534+
Expr::Field { base, name, span } => {
535+
// `Type.CONST` associated-const access: when the base is a
536+
// module-scoped type and that module declares the (per-file
537+
// flattened) associated constant, resolve the whole access to the
538+
// module-qualified const symbol. This handles the cross-file case,
539+
// where the access lives in a different file than the declaration.
540+
if let Expr::Ident(type_name, _) = base.as_ref()
541+
&& !scope.contains(type_name)
542+
&& let Some(type_module) = self.resolve_type_module(file, type_name)
543+
{
544+
let flat = flatten_associated_const(type_name, name);
545+
if self
546+
.module_defs
547+
.get(&type_module)
548+
.is_some_and(|names| names.contains(&flat))
549+
{
550+
*expr = Expr::Ident(self.mangle_value(&type_module, &flat), span.clone());
551+
return;
552+
}
553+
}
554+
self.rewrite_expr(base, file, scope);
555+
}
476556
Expr::Index { base, index, .. } => {
477557
self.rewrite_expr(base, file, scope);
478558
self.rewrite_expr(index, file, scope);
@@ -545,14 +625,13 @@ impl Resolver {
545625
}
546626
// `module.function`: a module-qualified free call collapses to the
547627
// module's mangled function symbol.
548-
let module_prefix = namespace.replace('.', "_");
628+
let prefix = module_prefix_from_dotted(namespace);
549629
if self
550630
.module_defs
551-
.get(&module_prefix)
631+
.get(&prefix)
552632
.is_some_and(|names| names.contains(name))
553633
{
554-
let mangled = format!("{module_prefix}{MODULE_SEP}{name}");
555-
*callee = Callee::Name(mangled);
634+
*callee = Callee::Name(format!("{prefix}{MODULE_SEP}{name}"));
556635
}
557636
}
558637
Callee::ReceiverCall { receiver, .. } => {
@@ -704,6 +783,28 @@ mod tests {
704783
);
705784
}
706785

786+
#[test]
787+
fn module_prefix_is_injective_across_dotted_and_underscored_paths() {
788+
// `a.b` and `a_b` are distinct module paths and must not share a prefix.
789+
let dotted = module_prefix(&["a".to_string(), "b".to_string()]);
790+
let underscored = module_prefix(&["a_b".to_string()]);
791+
assert_ne!(dotted, underscored);
792+
assert_eq!(dotted, "a_b");
793+
assert_eq!(underscored, "a__b");
794+
assert_eq!(module_prefix_from_dotted("a.b"), "a_b");
795+
}
796+
797+
#[test]
798+
fn two_modules_with_colliding_join_keep_distinct_symbols() {
799+
let ab = parse_source("ab.rss", "module a.b\n\nfn count() -> Int {\n return 1\n}\n");
800+
let a_b = parse_source("a_b.rss", "module a_b\n\nfn count() -> Int {\n return 2\n}\n");
801+
let mut program = merge_programs([ab, a_b]);
802+
isolate_module_namespaces(&mut program);
803+
let names = function_names(&program);
804+
assert!(names.contains(&"a_b__count".to_string()), "{names:?}");
805+
assert!(names.contains(&"a__b__count".to_string()), "{names:?}");
806+
}
807+
707808
#[test]
708809
fn entry_point_main_is_never_qualified() {
709810
let mut program = parse_source(
@@ -715,6 +816,13 @@ mod tests {
715816
}
716817
}
717818

819+
/// The flattened symbol for an associated constant `Type.MEMBER`, matching the
820+
/// `desugar::associated_consts` flattening (`.` -> `_`, upper-cased) so isolation
821+
/// resolves to the same constant the per-file desugar produced.
822+
fn flatten_associated_const(type_name: &str, member: &str) -> String {
823+
format!("{type_name}_{member}").to_uppercase()
824+
}
825+
718826
fn item_file(item: &Item) -> Option<&str> {
719827
Some(match item {
720828
Item::Function(function) => function.span.file.as_str(),

crates/rsscript/tests/checker_lowering/misc.rs

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,68 @@ fn module_isolation_lets_two_modules_share_a_symbol_name() {
8484
);
8585
}
8686

87+
#[test]
88+
fn module_isolation_resolves_cross_file_associated_constant() {
89+
// `Device.DEFAULT` is declared in one module and read from another (via
90+
// `use device.Device`). The associated-const access must resolve to the
91+
// module-qualified, upper-cased constant rather than a dangling field access.
92+
let sources = vec![
93+
(
94+
"device.rss".to_string(),
95+
"module device\n\nstruct Device {\n name: String\n}\n\nconst Device.DEFAULT: String = \"cpu\"\n".to_string(),
96+
),
97+
(
98+
"main.rss".to_string(),
99+
concat!(
100+
"module app\n\n",
101+
"use device.Device\n\n",
102+
"fn main() -> Unit {\n",
103+
" Log.write(message: read Device.DEFAULT)\n",
104+
" return Unit\n",
105+
"}\n",
106+
)
107+
.to_string(),
108+
),
109+
];
110+
let package =
111+
lower_sources_to_rust_package_with_options(&sources, "assoc-demo", "/rt", &[], &[])
112+
.expect("cross-file associated constant should lower");
113+
// The constant lowers to a module-qualified SCREAMING_SNAKE symbol, used at
114+
// both declaration and the cross-file reference.
115+
assert!(
116+
package.lib_rs.contains("DEVICE__DEVICE_DEFAULT"),
117+
"associated const should lower to a module-qualified symbol:\n{}",
118+
package.lib_rs
119+
);
120+
}
121+
122+
#[test]
123+
fn module_isolation_distinguishes_dotted_and_underscored_module_paths() {
124+
// `module a.b` and `module a_b` are distinct and must not collide (RS0005).
125+
let sources = vec![
126+
(
127+
"ab.rss".to_string(),
128+
"module a.b\n\nfn count() -> Int {\n return 1\n}\n".to_string(),
129+
),
130+
(
131+
"a_b.rss".to_string(),
132+
"module a_b\n\nfn count() -> Int {\n return 2\n}\n".to_string(),
133+
),
134+
(
135+
"main.rss".to_string(),
136+
"module app\n\nfn main() -> Unit {\n return Unit\n}\n".to_string(),
137+
),
138+
];
139+
let package =
140+
lower_sources_to_rust_package_with_options(&sources, "collide-demo", "/rt", &[], &[])
141+
.expect("distinct module paths should lower without collision");
142+
assert!(
143+
package.lib_rs.contains("fn a_b__count(") && package.lib_rs.contains("fn a__b__count("),
144+
"distinct module paths must yield distinct symbols:\n{}",
145+
package.lib_rs
146+
);
147+
}
148+
87149
#[test]
88150
fn lower_name_pin_renames_definition_and_call_sites() {
89151
let source = r#"

crates/rsscript/tests/golden/lowering/simple.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
// Generated by RSScript. Edit the .rss source instead.
22
// Runtime hooks are intentionally explicit while Rust lowering is stabilizing.
3-
#![allow(dead_code, non_snake_case)]
3+
#![allow(dead_code, non_snake_case, non_camel_case_types, non_upper_case_globals)]
44
// RSScript features: <none>
55

66
// rss:span kind=type file=simple.rss line=1 column=1 length=6

0 commit comments

Comments
 (0)