Skip to content

Commit c44e1b4

Browse files
Y-NakY-Nak
authored
Merge pull request #124 from syszery/fix-declare-function-conflict
Make `ModuleBuilder::declare_function` return error on conflicting declarations (#91)
2 parents 084edf2 + eb69903 commit c44e1b4

File tree

11 files changed

+338
-36
lines changed

11 files changed

+338
-36
lines changed

‎crates/filecheck/src/lib.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ impl FileCheckRunner {
7070
let is_success = failed_num == 0;
7171

7272
let mut stdout = StandardStream::stdout(ColorChoice::Always);
73-
writeln!(stdout, "\nrunning {} tests", tests_num).unwrap();
73+
writeln!(stdout, "\nrunning {tests_num} tests").unwrap();
7474
for res in &self.results {
7575
res.print_result(&mut stdout).unwrap();
7676
}
@@ -157,7 +157,7 @@ impl<'a> FileChecker<'a> {
157157
let result = match checker.explain(&func_ir, &()) {
158158
Ok((true, _)) => Ok(()),
159159
Ok((false, err)) => Err(err),
160-
Err(err) => Err(format!("{}", err)),
160+
Err(err) => Err(format!("{err}")),
161161
};
162162

163163
let mut test_path = self.file_path.to_owned();
@@ -185,7 +185,7 @@ impl<'a> FileChecker<'a> {
185185
let mut builder = filecheck::CheckerBuilder::new();
186186
for d in directives {
187187
if !builder.directive(d).unwrap() && d.contains("nextln") {
188-
panic!("not a directive: `{}`", d);
188+
panic!("not a directive: `{d}`");
189189
}
190190
}
191191
builder.finish()
@@ -222,7 +222,7 @@ impl FileCheckResult {
222222
stdout.set_color(ColorSpec::new().set_fg(Color::Red.into()))?;
223223
writeln!(stdout, " FAILED")?;
224224
stdout.reset()?;
225-
writeln!(stdout, "{}", err)?;
225+
writeln!(stdout, "{err}")?;
226226
}
227227
}
228228
Ok(())

‎crates/ir/src/builder/mod.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ pub mod test_util {
3939
ret_ty: Type,
4040
) -> (Evm, FunctionBuilder<InstInserter>) {
4141
let sig = Signature::new("test_func", Linkage::Public, args, ret_ty);
42-
let func_ref = mb.declare_function(sig);
42+
let func_ref = mb.declare_function(sig).unwrap();
4343
(test_isa(), mb.func_builder(func_ref))
4444
}
4545

‎crates/ir/src/builder/module_builder.rs‎

Lines changed: 53 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,11 @@ pub struct ModuleBuilder {
2222
declared_funcs: Arc<DashMap<String, FuncRef>>,
2323
}
2424

25+
#[derive(Debug, Clone)]
26+
pub enum BuilderError {
27+
ConflictingFunctionDeclaration,
28+
}
29+
2530
impl ModuleBuilder {
2631
pub fn new(ctx: ModuleCtx) -> Self {
2732
Self {
@@ -56,17 +61,21 @@ impl ModuleBuilder {
5661
self.ctx.triple
5762
}
5863

59-
// TODO: Return result to check duplicated func declaration.
60-
pub fn declare_function(&self, sig: Signature) -> FuncRef {
64+
pub fn declare_function(&self, sig: Signature) -> Result<FuncRef, BuilderError> {
6165
if let Some(func_ref) = self.declared_funcs.get(sig.name()) {
62-
*func_ref
63-
} else {
64-
let func = Function::new(&self.ctx, &sig);
65-
let func_ref = self.func_store.insert(func);
66-
self.declared_funcs.insert(sig.name().to_string(), func_ref);
67-
self.ctx.declared_funcs.insert(func_ref, sig);
68-
func_ref
66+
return self.ctx.func_sig(*func_ref, |func_sig| {
67+
if func_sig.args() == sig.args() && func_sig.ret_ty() == sig.ret_ty() {
68+
Ok(*func_ref)
69+
} else {
70+
Err(BuilderError::ConflictingFunctionDeclaration)
71+
}
72+
});
6973
}
74+
let func = Function::new(&self.ctx, &sig);
75+
let func_ref = self.func_store.insert(func);
76+
self.declared_funcs.insert(sig.name().to_string(), func_ref);
77+
self.ctx.declared_funcs.insert(func_ref, sig);
78+
Ok(func_ref)
7079
}
7180

7281
pub fn declare_gv(&self, global: GlobalVariableData) -> GlobalVariableRef {
@@ -148,3 +157,38 @@ impl ModuleBuilder {
148157
self.ctx.inst_set
149158
}
150159
}
160+
161+
#[cfg(test)]
162+
mod tests {
163+
use super::*;
164+
use crate::{builder::test_util::test_module_builder, types::Type, Linkage}; //, Signature};
165+
166+
#[test]
167+
fn test_declare_function_success() {
168+
let builder = test_module_builder();
169+
let sig = Signature::new("foo", Linkage::Public, &[], Type::Unit);
170+
171+
let result = builder.declare_function(sig.clone());
172+
assert!(result.is_ok());
173+
174+
// Declaring again with same sig should succeed
175+
let result2 = builder.declare_function(sig);
176+
assert!(result2.is_ok());
177+
}
178+
179+
#[test]
180+
fn test_declare_function_conflict() {
181+
let builder = test_module_builder();
182+
183+
let sig1 = Signature::new("foo", Linkage::Public, &[Type::I32], Type::I32);
184+
let sig2 = Signature::new("foo", Linkage::Public, &[Type::I64], Type::I64);
185+
186+
builder.declare_function(sig1).unwrap();
187+
let result = builder.declare_function(sig2);
188+
189+
match result {
190+
Err(BuilderError::ConflictingFunctionDeclaration) => (),
191+
_ => panic!("Expected conflicting function declaration error"),
192+
}
193+
}
194+
}

‎crates/ir/src/global_variable.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,7 @@ where
191191
W: io::Write,
192192
{
193193
match self {
194-
Self::Immediate(data) => write!(w, "{}", data),
194+
Self::Immediate(data) => write!(w, "{data}"),
195195
Self::Array(data) => {
196196
write!(w, "[")?;
197197
for (i, v) in data.iter().enumerate() {

‎crates/ir/src/graphviz/block.rs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,7 @@ impl BlockNode<'_> {
4343
// Write block header.
4444
write!(
4545
&mut label,
46-
r#"<tr><td bgcolor="gray" align="center" colspan="1">{}</td></tr>"#,
47-
block
46+
r#"<tr><td bgcolor="gray" align="center" colspan="1">{block}</td></tr>"#,
4847
)
4948
.unwrap();
5049

‎crates/ir/src/module_linker.rs‎

Lines changed: 130 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -570,7 +570,8 @@ impl ModuleLinker {
570570
let Some(linked_func_ref) = self.builder.lookup_func(sig.name()) else {
571571
// If the function is not defined in the linked module, declare it and
572572
// make the mapping.
573-
let linked_func_ref = self.builder.declare_function(sig);
573+
// Safe unwrap: no existing function with this name, so declare_function won’t conflict.
574+
let linked_func_ref = self.builder.declare_function(sig).unwrap();
574575
ref_map.func_mapping.insert(func_ref, linked_func_ref);
575576
return Ok(linked_func_ref);
576577
};
@@ -604,3 +605,131 @@ impl ModuleLinker {
604605
Ok(linked_func_ref)
605606
}
606607
}
608+
609+
#[cfg(test)]
610+
mod tests {
611+
use super::*;
612+
use crate::{builder::test_util::test_module_builder, types::Type, Linkage};
613+
614+
#[test]
615+
fn test_linker_conflicting_function_signature_should_fail() {
616+
let mod1 = {
617+
let builder = test_module_builder();
618+
let sig = Signature::new("foo", Linkage::Public, &[Type::I32], Type::I32);
619+
builder.declare_function(sig).unwrap();
620+
builder.build()
621+
};
622+
623+
let mod2 = {
624+
let builder = test_module_builder();
625+
let sig = Signature::new("foo", Linkage::Public, &[Type::I64], Type::I64);
626+
builder.declare_function(sig).unwrap();
627+
builder.build()
628+
};
629+
630+
let result = LinkedModule::link(vec![mod1, mod2]);
631+
632+
match result {
633+
Err(LinkError::InconsistentFuncSignature { name }) => {
634+
assert_eq!(name, "foo");
635+
}
636+
_ => panic!("Expected InconsistentFuncSignature error for function 'foo'"),
637+
}
638+
}
639+
640+
#[test]
641+
fn test_linker_duplicate_public_functions_should_fail() {
642+
let sig = Signature::new("foo", Linkage::Public, &[Type::I32], Type::I32);
643+
644+
let mod1 = {
645+
let builder = test_module_builder();
646+
builder.declare_function(sig.clone()).unwrap();
647+
builder.build()
648+
};
649+
650+
let mod2 = {
651+
let builder = test_module_builder();
652+
builder.declare_function(sig).unwrap();
653+
builder.build()
654+
};
655+
656+
let result = LinkedModule::link(vec![mod1, mod2]);
657+
658+
assert!(
659+
matches!(result, Err(LinkError::InconsistentFuncSignature { name }) if name == "foo"),
660+
"Expected InconsistentFuncSignature error on duplicate Public function 'foo'"
661+
);
662+
}
663+
664+
#[test]
665+
fn test_linker_duplicate_private_should_fail() {
666+
let sig = Signature::new("foo", Linkage::Private, &[Type::I32], Type::I32);
667+
668+
let mod1 = {
669+
let builder = test_module_builder();
670+
builder.declare_function(sig.clone()).unwrap();
671+
builder.build()
672+
};
673+
674+
let mod2 = {
675+
let builder = test_module_builder();
676+
builder.declare_function(sig).unwrap();
677+
builder.build()
678+
};
679+
680+
let result = LinkedModule::link(vec![mod1, mod2]);
681+
682+
assert!(
683+
matches!(result, Err(LinkError::InconsistentFuncSignature { name }) if name == "foo"),
684+
"Expected InconsistentFuncSignature error on duplicate Private function 'foo'"
685+
);
686+
}
687+
688+
#[test]
689+
fn test_linker_public_and_external_should_succeed() {
690+
let sig_public = Signature::new("foo", Linkage::Public, &[Type::I32], Type::I32);
691+
let sig_external = Signature::new("foo", Linkage::External, &[Type::I32], Type::I32);
692+
693+
let mod1 = {
694+
let builder = test_module_builder();
695+
builder.declare_function(sig_public).unwrap();
696+
builder.build()
697+
};
698+
699+
let mod2 = {
700+
let builder = test_module_builder();
701+
builder.declare_function(sig_external).unwrap();
702+
builder.build()
703+
};
704+
705+
let result = LinkedModule::link(vec![mod1, mod2]);
706+
assert!(
707+
result.is_ok(),
708+
"Expected successful link for Public + External"
709+
);
710+
}
711+
712+
#[test]
713+
fn test_linker_different_names_should_succeed() {
714+
let sig1 = Signature::new("foo_mod1", Linkage::Private, &[Type::I32], Type::I32);
715+
let sig2 = Signature::new("foo_mod2", Linkage::Private, &[Type::I32], Type::I32);
716+
717+
let mod1 = {
718+
let builder = test_module_builder();
719+
builder.declare_function(sig1).unwrap();
720+
builder.build()
721+
};
722+
723+
let mod2 = {
724+
let builder = test_module_builder();
725+
builder.declare_function(sig2).unwrap();
726+
builder.build()
727+
};
728+
729+
let result = LinkedModule::link(vec![mod1, mod2]);
730+
assert!(
731+
result.is_ok(),
732+
"Expected successful link for different function names"
733+
);
734+
}
735+
}

‎crates/ir/src/value.rs‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ impl IrWrite<FuncWriteCtx<'_>> for ValueId {
2525
} else {
2626
match ctx.func.dfg.value(*self) {
2727
Value::Immediate { imm, ty } => {
28-
write!(w, "{}.", imm)?;
28+
write!(w, "{imm}.")?;
2929
ty.write(w, ctx)
3030
}
3131

@@ -395,12 +395,12 @@ where
395395
write!(w, "0")
396396
}
397397
}
398-
Self::I8(v) => write!(w, "{}", v),
399-
Self::I16(v) => write!(w, "{}", v),
400-
Self::I32(v) => write!(w, "{}", v),
401-
Self::I64(v) => write!(w, "{}", v),
402-
Self::I128(v) => write!(w, "{}", v),
403-
Self::I256(v) => write!(w, "{}", v),
398+
Self::I8(v) => write!(w, "{v}"),
399+
Self::I16(v) => write!(w, "{v}"),
400+
Self::I32(v) => write!(w, "{v}"),
401+
Self::I64(v) => write!(w, "{v}"),
402+
Self::I128(v) => write!(w, "{v}"),
403+
Self::I256(v) => write!(w, "{v}"),
404404
}
405405
}
406406
}
@@ -415,12 +415,12 @@ impl fmt::Display for Immediate {
415415
write!(f, "0")
416416
}
417417
}
418-
Self::I8(v) => write!(f, "{}", v),
419-
Self::I16(v) => write!(f, "{}", v),
420-
Self::I32(v) => write!(f, "{}", v),
421-
Self::I64(v) => write!(f, "{}", v),
422-
Self::I128(v) => write!(f, "{}", v),
423-
Self::I256(v) => write!(f, "{}", v),
418+
Self::I8(v) => write!(f, "{v}"),
419+
Self::I16(v) => write!(f, "{v}"),
420+
Self::I32(v) => write!(f, "{v}"),
421+
Self::I64(v) => write!(f, "{v}"),
422+
Self::I128(v) => write!(f, "{v}"),
423+
Self::I256(v) => write!(f, "{v}"),
424424
}
425425
}
426426
}

‎crates/parser/src/error.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ impl Error {
141141
Renderer::plain()
142142
};
143143
let disp = rend.render(snippet);
144-
write!(w, "{}", disp)
144+
write!(w, "{disp}")
145145
}
146146

147147
pub fn print_to_string(&self, path: &str, content: &str, colors: bool) -> String {

0 commit comments

Comments
 (0)