core: allow calling parent methods #497

Merged
sb10q merged 4 commits from issue-133 into master 2024-08-26 18:37:55 +08:00
5 changed files with 287 additions and 28 deletions

View File

@ -3025,6 +3025,9 @@ pub fn gen_expr<'ctx, G: CodeGenerator>(
let Some(val) = generator.gen_expr(ctx, value)? else { return Ok(None) }; let Some(val) = generator.gen_expr(ctx, value)? else { return Ok(None) };
// Handle Class Method calls // Handle Class Method calls
// The attribute will be `DefinitionId` of the method if the call is to one of the parent methods
let func_id = attr.to_string().parse::<usize>();
let id = if let TypeEnum::TObj { obj_id, .. } = let id = if let TypeEnum::TObj { obj_id, .. } =
&*ctx.unifier.get_ty(value.custom.unwrap()) &*ctx.unifier.get_ty(value.custom.unwrap())
{ {
@ -3032,7 +3035,11 @@ pub fn gen_expr<'ctx, G: CodeGenerator>(
} else { } else {
unreachable!() unreachable!()
}; };
let fun_id = {
// Use the `DefinitionID` from attribute if it is available
let fun_id = if let Ok(func_id) = func_id {
DefinitionId(func_id)
} else {
let defs = ctx.top_level.definitions.read(); let defs = ctx.top_level.definitions.read();
let obj_def = defs.get(id.0).unwrap().read(); let obj_def = defs.get(id.0).unwrap().read();
let TopLevelDef::Class { methods, .. } = &*obj_def else { unreachable!() }; let TopLevelDef::Class { methods, .. } = &*obj_def else { unreachable!() };

View File

@ -23,7 +23,7 @@ impl Default for ComposerConfig {
} }
} }
type DefAst = (Arc<RwLock<TopLevelDef>>, Option<Stmt<()>>); pub type DefAst = (Arc<RwLock<TopLevelDef>>, Option<Stmt<()>>);
pub struct TopLevelComposer { pub struct TopLevelComposer {
// list of top level definitions, same as top level context // list of top level definitions, same as top level context
pub definition_ast_list: Vec<DefAst>, pub definition_ast_list: Vec<DefAst>,
@ -1822,7 +1822,12 @@ impl TopLevelComposer {
if *name != init_str_id { if *name != init_str_id {
unreachable!("must be init function here") unreachable!("must be init function here")
} }
let all_inited = Self::get_all_assigned_field(body.as_slice())?;
Outdated
Review

I don't understand this comment, can you elaborate?
Is a string hack the best way of solving this?

I don't understand this comment, can you elaborate? Is a string hack the best way of solving this?

I don't understand this comment, can you elaborate?

Class names in TopLevelDef are stored by first prepending module name to the class name. So,

class A:
   pass

would be named __main__.A. However, in the raw AST that holds the statements, the name is just A, so here I am removing the module name from definition.

Is a string hack the best way of solving this?

Hmm this was the most straightforward method. An alternative could be to have to do the search in the TopLevelDef and get the corresponding DefinitionID to fetch the function statements from the AST.

> I don't understand this comment, can you elaborate? Class names in TopLevelDef are stored by first prepending module name to the class name. So, ``` class A: pass ``` would be named \_\_main\_\_.A. However, in the raw AST that holds the statements, the name is just A, so here I am removing the module name from definition. > Is a string hack the best way of solving this? Hmm this was the most straightforward method. An alternative could be to have to do the search in the TopLevelDef and get the corresponding DefinitionID to fetch the function statements from the AST.
Outdated
Review

Seems to me that doing the alternative method would make it clear what is happening from the code alone, and would not need any of those obscure/complicated comments.

Seems to me that doing the alternative method would make it clear what is happening from the code alone, and would not need any of those obscure/complicated comments.
let all_inited = Self::get_all_assigned_field(
object_id.0,
definition_ast_list,
body.as_slice(),
)?;
for (f, _, _) in fields { for (f, _, _) in fields {

Would this way of getting the fully qualified name of the to-be-initialized class work for inner (nested) classes?

Would this way of getting the fully qualified name of the to-be-initialized class work for inner (nested) classes?

Would this way of getting the fully qualified name of the to-be-initialized class work for inner (nested) classes?

Hmm I tried testing on inner classes, but they are not currently supported in nac3. Also, by matching name against the class names in the TopLevelDef as suggested in #497 (comment) we can avoid the problem altogether.

> Would this way of getting the fully qualified name of the to-be-initialized class work for inner (nested) classes? Hmm I tried testing on inner classes, but they are not currently supported in nac3. Also, by matching name against the class names in the TopLevelDef as suggested in https://git.m-labs.hk/M-Labs/nac3/pulls/497#issuecomment-11572 we can avoid the problem altogether.
if !all_inited.contains(f) { if !all_inited.contains(f) {
return Err(HashSet::from([ return Err(HashSet::from([

View File

@ -3,6 +3,7 @@ use std::convert::TryInto;
use crate::symbol_resolver::SymbolValue; use crate::symbol_resolver::SymbolValue;
use crate::toplevel::numpy::unpack_ndarray_var_tys; use crate::toplevel::numpy::unpack_ndarray_var_tys;
use crate::typecheck::typedef::{into_var_map, iter_type_vars, Mapping, TypeVarId, VarMap}; use crate::typecheck::typedef::{into_var_map, iter_type_vars, Mapping, TypeVarId, VarMap};
use ast::ExprKind;
use nac3parser::ast::{Constant, Location}; use nac3parser::ast::{Constant, Location};
use strum::IntoEnumIterator; use strum::IntoEnumIterator;
use strum_macros::EnumIter; use strum_macros::EnumIter;
@ -733,7 +734,16 @@ impl TopLevelComposer {
) )
} }
pub fn get_all_assigned_field(stmts: &[Stmt<()>]) -> Result<HashSet<StrRef>, HashSet<String>> { /// This function returns the fields that have been initialized in the `__init__` function of a class

Could you add documentation to this function?

Could you add documentation to this function?

Could you add documentation to this function?

Will do.

> Could you add documentation to this function? Will do.
/// The function takes as input:
/// * `class_id`: The `object_id` of the class whose function is being evaluated (check `TopLevelDef::Class`)
/// * `definition_ast_list`: A list of ast definitions and statements defined in `TopLevelComposer`
/// * `stmts`: The body of function being parsed. Each statment is analyzed to check varaible initialization statements
pub fn get_all_assigned_field(
class_id: usize,
definition_ast_list: &Vec<DefAst>,
stmts: &[Stmt<()>],
) -> Result<HashSet<StrRef>, HashSet<String>> {
let mut result = HashSet::new(); let mut result = HashSet::new();
for s in stmts { for s in stmts {
match &s.node { match &s.node {
@ -769,30 +779,138 @@ impl TopLevelComposer {
// TODO: do not check for For and While? // TODO: do not check for For and While?
ast::StmtKind::For { body, orelse, .. } ast::StmtKind::For { body, orelse, .. }
| ast::StmtKind::While { body, orelse, .. } => { | ast::StmtKind::While { body, orelse, .. } => {
result.extend(Self::get_all_assigned_field(body.as_slice())?); result.extend(Self::get_all_assigned_field(
result.extend(Self::get_all_assigned_field(orelse.as_slice())?); class_id,
definition_ast_list,
body.as_slice(),
)?);
result.extend(Self::get_all_assigned_field(
class_id,
definition_ast_list,
orelse.as_slice(),
)?);
} }
ast::StmtKind::If { body, orelse, .. } => { ast::StmtKind::If { body, orelse, .. } => {
let inited_for_sure = Self::get_all_assigned_field(body.as_slice())? let inited_for_sure = Self::get_all_assigned_field(
.intersection(&Self::get_all_assigned_field(orelse.as_slice())?) class_id,
.copied() definition_ast_list,
.collect::<HashSet<_>>(); body.as_slice(),
)?
.intersection(&Self::get_all_assigned_field(
class_id,
definition_ast_list,
orelse.as_slice(),
)?)
.copied()
.collect::<HashSet<_>>();
result.extend(inited_for_sure); result.extend(inited_for_sure);
} }
ast::StmtKind::Try { body, orelse, finalbody, .. } => { ast::StmtKind::Try { body, orelse, finalbody, .. } => {
let inited_for_sure = Self::get_all_assigned_field(body.as_slice())? let inited_for_sure = Self::get_all_assigned_field(
.intersection(&Self::get_all_assigned_field(orelse.as_slice())?) class_id,
.copied() definition_ast_list,
.collect::<HashSet<_>>(); body.as_slice(),
)?
.intersection(&Self::get_all_assigned_field(
class_id,
definition_ast_list,
Outdated
Review

Initiated ?

Initiated ?

Initiated ?

Should have been initialized. This part checks if there are any function calls in the __init__ block where user might have initialized some variables (as all variables must be initialized before creating an object).

> Initiated ? Should have been initialized. This part checks if there are any function calls in the `__init__` block where user might have initialized some variables (as all variables must be initialized before creating an object).
orelse.as_slice(),
)?)
.copied()
.collect::<HashSet<_>>();
result.extend(inited_for_sure); result.extend(inited_for_sure);
result.extend(Self::get_all_assigned_field(finalbody.as_slice())?); result.extend(Self::get_all_assigned_field(
class_id,
definition_ast_list,
finalbody.as_slice(),
)?);
} }

NIt: consider

NIt: consider
ast::StmtKind::With { body, .. } => { ast::StmtKind::With { body, .. } => {
result.extend(Self::get_all_assigned_field(body.as_slice())?); result.extend(Self::get_all_assigned_field(
class_id,
definition_ast_list,
body.as_slice(),
)?);
}
// Variables Initialized in function calls
ast::StmtKind::Expr { value, .. } => {
let ExprKind::Call { func, .. } = &value.node else {
continue;
};
let ExprKind::Attribute { value, attr, .. } = &func.node else {
continue;
};
let ExprKind::Name { id, .. } = &value.node else {
continue;
};
// Need to consider the two cases:
// Case 1) Call to class function i.e. id = `self`
// Case 2) Call to class ancestor function i.e. id = ancestor_name
// We leave checking whether function in case 2 belonged to class ancestor or not to type checker
//
// According to current handling of `self`, function definition are fixed and do not change regardless
// of which object is passed as `self` i.e. virtual polymorphism is not supported
// Therefore, we change class id for case 2 to reflect behavior of our compiler
let class_name = if *id == "self".into() {
let ast::StmtKind::ClassDef { name, .. } =
&definition_ast_list[class_id].1.as_ref().unwrap().node
else {
unreachable!()
};
name
} else {
id
};
let parent_method = definition_ast_list.iter().find_map(|def| {
let (
class_def,
Some(ast::Located {
node: ast::StmtKind::ClassDef { name, body, .. },
..
}),
) = &def
else {
return None;
};
let TopLevelDef::Class { object_id: class_id, .. } = &*class_def.read()
else {
unreachable!()
};
if name == class_name {
body.iter().find_map(|m| {
let ast::StmtKind::FunctionDef { name, body, .. } = &m.node else {
return None;
};
if *name == *attr {
return Some((body.clone(), class_id.0));
}
None
})
} else {
None
}
});
// If method body is none then method does not exist
if let Some((method_body, class_id)) = parent_method {
result.extend(Self::get_all_assigned_field(
class_id,
definition_ast_list,
method_body.as_slice(),
)?);
} else {
return Err(HashSet::from([format!(
"{}.{} not found in class {class_name} at {}",
*id, *attr, value.location
)]));
}
} }
ast::StmtKind::Pass { .. } ast::StmtKind::Pass { .. }
| ast::StmtKind::Assert { .. } | ast::StmtKind::Assert { .. }
| ast::StmtKind::Expr { .. } => {} | ast::StmtKind::AnnAssign { .. } => {}
_ => { _ => {
unimplemented!() unimplemented!()

View File

@ -12,6 +12,7 @@ use super::{
RecordField, RecordKey, Type, TypeEnum, TypeVar, Unifier, VarMap, RecordField, RecordKey, Type, TypeEnum, TypeVar, Unifier, VarMap,
}, },
}; };
use crate::toplevel::type_annotation::TypeAnnotation;
use crate::{ use crate::{
symbol_resolver::{SymbolResolver, SymbolValue}, symbol_resolver::{SymbolResolver, SymbolValue},
toplevel::{ toplevel::{
@ -102,6 +103,7 @@ pub struct Inferencer<'a> {
} }
type InferenceError = HashSet<String>; type InferenceError = HashSet<String>;
type OverrideResult = Result<Option<ast::Expr<Option<Type>>>, InferenceError>;
struct NaiveFolder(); struct NaiveFolder();
impl Fold<()> for NaiveFolder { impl Fold<()> for NaiveFolder {
@ -1672,6 +1674,86 @@ impl<'a> Inferencer<'a> {
Ok(None) Ok(None)
} }
/// Checks whether a class method is calling parent function
/// Returns [`None`] if its not a call to parent method, otherwise
/// returns a new `func` with class name replaced by `self` and method resolved to its `DefinitionID`
///
/// e.g. A.f1(self, ...) returns Some(self.{DefintionID(f1)})

Do you mean it returns Some(DefinitionID(self.f1))? Aren't DefinitionIDs global?

Do you mean it returns `Some(DefinitionID(self.f1))`? Aren't `DefinitionID`s global?

Do you mean it returns Some(DefinitionID(self.f1))? Aren't DefinitionIDs global?

Since each class function is assigned a unique DefintionID, this returns replaces the attribute with the definitionID of the actual function to be called. To illustrate:

class Foo:
  def f1(self):
    pass
class Bar(Foo):
  def f1(self):
    Foo.f1(self)

Here Foo.f1() and Bar.f1() will be assigned different Ids say 1 and 2. The call to Foo.f1(self) will be changed by self.1(). Under normal execution when generating function call in gen_expr, a lookup is done on the class definition to find the definitionId of the corresponding method. Here, I am doing the lookup beforehand to point to the correct method.

> Do you mean it returns `Some(DefinitionID(self.f1))`? Aren't `DefinitionID`s global? Since each class function is assigned a unique DefintionID, this returns replaces the attribute with the definitionID of the actual function to be called. To illustrate: ``` class Foo: def f1(self): pass class Bar(Foo): def f1(self): Foo.f1(self) ``` Here `Foo.f1()` and `Bar.f1() `will be assigned different Ids say 1 and 2. The call to `Foo.f1(self)` will be changed by `self.1()`. Under normal execution when generating function call in `gen_expr`, a lookup is done on the class definition to find the definitionId of the corresponding method. Here, I am doing the lookup beforehand to point to the correct method.

Could you wrap DefinitionID with some symbol (e.g. self.{DefinitionID(f1)}) to indicate that the returned DefinitionID is supposed to be substituted by the function returns?

Could you wrap `DefinitionID` with some symbol (e.g. `self.{DefinitionID(f1)}`) to indicate that the returned `DefinitionID` is supposed to be substituted by the function returns?
fn check_overriding(&mut self, func: &ast::Expr<()>, args: &[ast::Expr<()>]) -> OverrideResult {
// `self` must be first argument for call to parent method
if let Some(Located { node: ExprKind::Name { id, .. }, .. }) = &args.first() {
if *id != "self".into() {
return Ok(None);
}
} else {
return Ok(None);
}
let Located {
node: ExprKind::Attribute { value, attr: method_name, ctx }, location, ..
} = func
else {
return Ok(None);
};
let ExprKind::Name { id: class_name, ctx: class_ctx } = &value.node else {
return Ok(None);
};
let zelf = &self.fold_expr(args[0].clone())?;
// Check whether the method belongs to class ancestors
let def_id = self.unifier.get_ty(zelf.custom.unwrap());
let TypeEnum::TObj { obj_id, .. } = def_id.as_ref() else { unreachable!() };
let defs = self.top_level.definitions.read();
let res = {
if let TopLevelDef::Class { ancestors, .. } = &*defs[obj_id.0].read() {
let res = ancestors.iter().find_map(|f| {
let TypeAnnotation::CustomClass { id, .. } = f else { unreachable!() };
let TopLevelDef::Class { name, methods, .. } = &*defs[id.0].read() else {
unreachable!()
};
// Class names are stored as `__module__.class`
let name = name.to_string();
let (_, name) = name.rsplit_once('.').unwrap();
if name == class_name.to_string() {
return methods.iter().find_map(|f| {
if f.0 == *method_name {
return Some(*f);
}
None
});
}
None
});
res
} else {
None
}
};
match res {
Some(r) => {
let mut new_func = func.clone();
let mut new_value = value.clone();
new_value.node = ExprKind::Name { id: "self".into(), ctx: *class_ctx };
new_func.node =
ExprKind::Attribute { value: new_value.clone(), attr: *method_name, ctx: *ctx };
let mut new_func = self.fold_expr(new_func)?;
let ExprKind::Attribute { value, .. } = new_func.node else { unreachable!() };
new_func.node =
ExprKind::Attribute { value, attr: r.2 .0.to_string().into(), ctx: *ctx };
new_func.custom = Some(r.1);
Ok(Some(new_func))
}
None => report_error(
format!("Ancestor method [{class_name}.{method_name}] should be defined with same decorator as its overridden version").as_str(),
*location,
),
}
}
fn fold_call( fn fold_call(
&mut self, &mut self,
location: Location, location: Location,
@ -1685,8 +1767,20 @@ impl<'a> Inferencer<'a> {
return Ok(spec_call_func); return Ok(spec_call_func);
} }
let func = Box::new(self.fold_expr(func)?); // Check for call to parent method
let args = args.into_iter().map(|v| self.fold_expr(v)).collect::<Result<Vec<_>, _>>()?; let override_res = self.check_overriding(&func, &args)?;
let is_override = override_res.is_some();
let func = if is_override { override_res.unwrap() } else { self.fold_expr(func)? };
let func = Box::new(func);
let mut args =
args.into_iter().map(|v| self.fold_expr(v)).collect::<Result<Vec<_>, _>>()?;
// TODO: Handle passing of self to functions to allow runtime lookup of functions to be called
// Currently removing `self` and using compile time function definitions
if is_override {
args.remove(0);
}
let keywords = keywords let keywords = keywords
.into_iter() .into_iter()
.map(|v| fold::fold_keyword(self, v)) .map(|v| fold::fold_keyword(self, v))

View File

@ -10,23 +10,58 @@ class A:
def __init__(self, a: int32): def __init__(self, a: int32):
self.a = a self.a = a
def f1(self): def output_all_fields(self):
self.f2()
def f2(self):
output_int32(self.a) output_int32(self.a)
def set_a(self, a: int32):
self.a = a
class B(A): class B(A):
b: int32 b: int32
def __init__(self, b: int32): def __init__(self, b: int32):
self.a = b + 1 A.__init__(self, b + 1)
self.set_b(b)
def output_parent_fields(self):
A.output_all_fields(self)
def output_all_fields(self):
A.output_all_fields(self)
output_int32(self.b)
def set_b(self, b: int32):
self.b = b self.b = b
class C(B):
c: int32
def __init__(self, c: int32):
B.__init__(self, c + 1)
self.c = c
def output_parent_fields(self):
B.output_all_fields(self)
def output_all_fields(self):
B.output_all_fields(self)
output_int32(self.c)
def set_c(self, c: int32):
self.c = c
def run() -> int32: def run() -> int32:
aaa = A(5) ccc = C(10)
bbb = B(2) ccc.output_all_fields()
aaa.f1() ccc.set_a(1)
bbb.f1() ccc.set_b(2)
ccc.set_c(3)
ccc.output_all_fields()
bbb = B(10)
bbb.set_a(9)
bbb.set_b(8)
bbb.output_all_fields()
ccc.output_all_fields()
return 0 return 0