From ecee5b7192291a519cd190f7e0ba9b6ac871ecae Mon Sep 17 00:00:00 2001 From: Shunsuke Shibayama Date: Wed, 12 Jul 2023 14:54:33 +0900 Subject: [PATCH] fix: shadowing bug --- Cargo.lock | 8 +-- crates/py2erg/convert.rs | 128 ++++++++++++++++++++++++++++----------- tests/func.py | 5 ++ tests/shadowing.py | 14 +++++ tests/test.rs | 5 ++ 5 files changed, 119 insertions(+), 41 deletions(-) create mode 100644 tests/shadowing.py diff --git a/Cargo.lock b/Cargo.lock index 5031bb6..525f5ba 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -153,7 +153,7 @@ checksum = "7fcaabb2fef8c910e7f4c7ce9f67a1283a1715879a7c230ca9d6d1ae31f16d91" [[package]] name = "els" version = "0.1.28-nightly.7" -source = "git+https://github.com/erg-lang/erg?branch=main#281262b7bf7c41cbb0b738e660cea37926016780" +source = "git+https://github.com/erg-lang/erg?branch=main#953b5806b599ae5061c1d29bb78b88cd758c9d3c" dependencies = [ "erg_common", "erg_compiler", @@ -177,7 +177,7 @@ dependencies = [ [[package]] name = "erg_common" version = "0.6.16-nightly.7" -source = "git+https://github.com/erg-lang/erg?branch=main#281262b7bf7c41cbb0b738e660cea37926016780" +source = "git+https://github.com/erg-lang/erg?branch=main#953b5806b599ae5061c1d29bb78b88cd758c9d3c" dependencies = [ "backtrace-on-stack-overflow", "parking_lot", @@ -187,7 +187,7 @@ dependencies = [ [[package]] name = "erg_compiler" version = "0.6.16-nightly.7" -source = "git+https://github.com/erg-lang/erg?branch=main#281262b7bf7c41cbb0b738e660cea37926016780" +source = "git+https://github.com/erg-lang/erg?branch=main#953b5806b599ae5061c1d29bb78b88cd758c9d3c" dependencies = [ "erg_common", "erg_parser", @@ -196,7 +196,7 @@ dependencies = [ [[package]] name = "erg_parser" version = "0.6.16-nightly.7" -source = "git+https://github.com/erg-lang/erg?branch=main#281262b7bf7c41cbb0b738e660cea37926016780" +source = "git+https://github.com/erg-lang/erg?branch=main#953b5806b599ae5061c1d29bb78b88cd758c9d3c" dependencies = [ "erg_common", "unicode-xid", diff --git a/crates/py2erg/convert.rs b/crates/py2erg/convert.rs index 8bd0af0..e4fdda6 100644 --- a/crates/py2erg/convert.rs +++ b/crates/py2erg/convert.rs @@ -35,6 +35,21 @@ use crate::error::*; pub const ARROW: Token = Token::dummy(TokenKind::FuncArrow, "->"); +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub enum CanShadow { + Yes, + No, +} + +impl CanShadow { + pub const fn is_yes(&self) -> bool { + matches!(self, Self::Yes) + } + pub const fn is_no(&self) -> bool { + matches!(self, Self::No) + } +} + #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum NameKind { Variable, @@ -233,7 +248,7 @@ pub struct ASTConverter { block_id_counter: usize, block_ids: Vec, /// Erg does not allow variables to be defined multiple times, so rename them using this - names: HashMap, + names: Vec>, warns: CompileErrors, errs: CompileErrors, } @@ -246,18 +261,46 @@ impl ASTConverter { namespace: vec![String::from("")], block_id_counter: 0, block_ids: vec![0], - names: HashMap::new(), + names: vec![HashMap::new()], warns: CompileErrors::empty(), errs: CompileErrors::empty(), } } fn get_name(&self, name: &str) -> Option<&NameInfo> { - self.names.get(name) + for ns in self.names.iter().rev() { + if let Some(ni) = ns.get(name) { + return Some(ni); + } + } + None } fn get_mut_name(&mut self, name: &str) -> Option<&mut NameInfo> { - self.names.get_mut(name) + for ns in self.names.iter_mut().rev() { + if let Some(ni) = ns.get_mut(name) { + return Some(ni); + } + } + None + } + + fn define_name(&mut self, name: String, info: NameInfo) { + self.names.last_mut().unwrap().insert(name, info); + } + + fn declare_name(&mut self, name: String, info: NameInfo) { + self.names.first_mut().unwrap().insert(name, info); + } + + fn grow(&mut self, namespace: String) { + self.namespace.push(namespace); + self.names.push(HashMap::new()); + } + + fn pop(&mut self) { + self.namespace.pop(); + self.names.pop(); } fn cur_block_id(&self) -> usize { @@ -268,14 +311,21 @@ impl ASTConverter { self.namespace.join(".") } - fn register_name_info(&mut self, name: &str, kind: NameKind) { + fn register_name_info(&mut self, name: &str, kind: NameKind) -> CanShadow { let cur_namespace = self.cur_namespace(); - if let Some(name_info) = self.names.get_mut(name) { - if name_info.defined_in == cur_namespace { + let cur_block_id = self.cur_block_id(); + if let Some(name_info) = self.get_mut_name(name) { + if name_info.defined_in == cur_namespace && name_info.defined_block_id == cur_block_id { name_info.defined_times += 1; - } else if name_info.defined_in.is_unknown() { + } + if name_info.defined_in.is_unknown() { name_info.defined_in = DefinedPlace::Known(cur_namespace); - name_info.defined_times += 1; + name_info.defined_times += 1; // 0 -> 1 + } + if name_info.defined_block_id == cur_block_id { + CanShadow::Yes + } else { + CanShadow::No } } else { // In Erg, classes can only be defined in uppercase @@ -287,7 +337,8 @@ impl ASTConverter { }; let defined_in = DefinedPlace::Known(self.cur_namespace()); let info = NameInfo::new(rename, defined_in, self.cur_block_id(), 1); - self.names.insert(String::from(name), info); + self.define_name(String::from(name), info); + CanShadow::Yes } } @@ -328,7 +379,7 @@ impl ASTConverter { }; let mut info = NameInfo::new(None, defined_in, cur_block_id, 0); info.add_referrer(cur_namespace); - self.names.insert(name.clone(), info); + self.declare_name(name.clone(), info); name }; let token = Token::new( @@ -365,6 +416,7 @@ impl ASTConverter { Identifier::new(VisModifierSpec::Public(dot), name) } + // Duplicate param names will result in an error at the parser. So we don't need to check it here. fn convert_param_pattern(&mut self, arg: String, loc: PyLocation) -> ParamPattern { self.register_name_info(&arg, NameKind::Variable); let ident = self.convert_ident(arg, loc); @@ -1091,10 +1143,10 @@ impl ASTConverter { Expr::BinOp(BinOp::new(op, lhs, rhs)) } py_ast::Expr::Lambda(lambda) => { - self.namespace.push("".to_string()); + self.grow("".to_string()); let params = self.convert_params(*lambda.args); let body = vec![self.convert_expr(*lambda.body)]; - self.namespace.pop(); + self.pop(); let sig = LambdaSignature::new(params, None, TypeBoundSpecs::empty()); let op = Token::from_str(TokenKind::FuncArrow, "->"); Expr::Lambda(Lambda::new(sig, op, Block::new(body), DefId(0))) @@ -1372,8 +1424,7 @@ impl ASTConverter { if def .sig .ident() - .map(|id| &id.inspect()[..] == "__init__") - .unwrap_or(false) + .is_some_and(|id| &id.inspect()[..] == "__init__") { if let Some(call_def) = self.extract_init(&mut base_type, def) { attrs.insert(0, ClassAttr::Def(call_def)); @@ -1456,15 +1507,11 @@ impl ASTConverter { range: PySourceRange, ) -> Expr { // if reassigning of a function referenced by other functions is occurred, it is an error - if self - .get_name(&name) - .map(|info| { - info.defined_times > 0 - && info.defined_in == DefinedPlace::Known(self.cur_namespace()) - && !info.referenced.difference(&set! {name.clone()}).is_empty() - }) - .unwrap_or(false) - { + if self.get_name(&name).is_some_and(|info| { + info.defined_times > 0 + && info.defined_in == DefinedPlace::Known(self.cur_namespace()) + && !info.referenced.difference(&set! {name.clone()}).is_empty() + }) { let err = reassign_func_error( self.cfg.input.clone(), pyloc_to_ergloc(range), @@ -1485,7 +1532,7 @@ impl ASTConverter { column: loc.column.saturating_add(4), }; let ident = self.convert_ident(name, func_name_loc); - self.namespace.push(ident.inspect().to_string()); + self.grow(ident.inspect().to_string()); let params = self.convert_params(params); let return_t = returns.map(|ret| { let t_spec = self.convert_type_spec(ret.clone()); @@ -1507,7 +1554,7 @@ impl ASTConverter { let block = self.convert_block(body, BlockKind::Function); let body = DefBody::new(EQUAL, block, DefId(0)); let def = Def::new(sig, body); - self.namespace.pop(); + self.pop(); Expr::Def(def) } } @@ -1550,7 +1597,7 @@ impl ASTConverter { }; let ident = self.convert_ident(name, class_name_loc); let sig = Signature::Var(VarSignature::new(VarPattern::Ident(ident.clone()), None)); - self.namespace.push(ident.inspect().to_string()); + self.grow(ident.inspect().to_string()); let (base_type, methods) = self.extract_method_list(ident, body, inherit); let classdef = if inherit { // TODO: multiple inheritance @@ -1585,7 +1632,7 @@ impl ASTConverter { let def = Def::new(sig, body); ClassDef::new(def, methods) }; - self.namespace.pop(); + self.pop(); Expr::ClassDef(classdef) } @@ -1646,14 +1693,22 @@ impl ASTConverter { let lhs = assign.targets.remove(0); match lhs { py_ast::Expr::Name(name) => { - let block = Block::new(vec![self.convert_expr(*assign.value)]); - let body = DefBody::new(EQUAL, block, DefId(0)); - self.register_name_info(&name.id, NameKind::Variable); + let expr = self.convert_expr(*assign.value); + let can_shadow = self.register_name_info(&name.id, NameKind::Variable); let ident = self.convert_ident(name.id.to_string(), name.location()); - let sig = - Signature::Var(VarSignature::new(VarPattern::Ident(ident), None)); - let def = Def::new(sig, body); - Expr::Def(def) + if can_shadow.is_yes() { + let block = Block::new(vec![expr]); + let body = DefBody::new(EQUAL, block, DefId(0)); + let sig = Signature::Var(VarSignature::new( + VarPattern::Ident(ident), + None, + )); + let def = Def::new(sig, body); + Expr::Def(def) + } else { + let redef = ReDef::new(Accessor::Ident(ident), expr); + Expr::ReDef(redef) + } } py_ast::Expr::Attribute(attr) => { let value = self.convert_expr(*attr.value); @@ -1758,8 +1813,7 @@ impl ASTConverter { let prev_ident = self.convert_ident(name.id.to_string(), name.location()); if self .get_name(name.id.as_str()) - .map(|info| info.defined_block_id == self.cur_block_id()) - .unwrap_or(false) + .is_some_and(|info| info.defined_block_id == self.cur_block_id()) { self.register_name_info(&name.id, NameKind::Variable); let ident = self.convert_ident(name.id.to_string(), name.location()); diff --git a/tests/func.py b/tests/func.py index 2afb691..64736d3 100644 --- a/tests/func.py +++ b/tests/func.py @@ -10,3 +10,8 @@ def g(x: int): if True: x = "a" # ERR return x + +def h(x: str): + if True: + x = "a" # OK + return x diff --git a/tests/shadowing.py b/tests/shadowing.py new file mode 100644 index 0000000..0c6de7c --- /dev/null +++ b/tests/shadowing.py @@ -0,0 +1,14 @@ +i: int = 0 +i: str = "a" # OK + +if True: + i = 1 # ERR +else: + i = 2 # ERR + +while False: + i = 3 # ERR + +def f(x: int): + i = 1 # OK + return x + i diff --git a/tests/test.rs b/tests/test.rs index e8498a1..310595f 100644 --- a/tests/test.rs +++ b/tests/test.rs @@ -118,3 +118,8 @@ fn exec_collections() -> Result<(), String> { fn exec_call() -> Result<(), String> { expect("tests/call.py", 0, 3) } + +#[test] +fn exec_shadowing() -> Result<(), String> { + expect("tests/shadowing.py", 0, 3) +}