M-Labs/nac3
M-Labs
/
nac3
8
0
Fork 10
Code Issues 98 Pull Requests 20 Releases Wiki Activity

WIP: Adding static class attribute definition functionality #312

Draft
Aadityavardhan wants to merge 2 commits from Aadityavardhan/nac3_sca:master into master
pull from: Aadityavardhan/nac3_sca:master
merge into: M-Labs:master
Conversation 0 Commits 2 Files Changed 4 +8 -19
4 changed files with 8 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 5a643bffca - Show all commits

2
nac3core/src/toplevel/builtins.rs
View File

@ -66,6 +66,7 @@ pub fn get_exn_constructor(
object_id: DefinitionId(class_id), object_id: DefinitionId(class_id),
type_vars: Default::default(), type_vars: Default::default(),
fields: exception_fields, fields: exception_fields,
static_fields: Default::default(),
methods: vec![("__init__".into(), signature, DefinitionId(cons_id))], methods: vec![("__init__".into(), signature, DefinitionId(cons_id))],
ancestors: vec![ ancestors: vec![
TypeAnnotation::CustomClass { id: DefinitionId(class_id), params: Default::default() }, TypeAnnotation::CustomClass { id: DefinitionId(class_id), params: Default::default() },
@ -74,7 +75,6 @@ pub fn get_exn_constructor(
constructor: Some(signature), constructor: Some(signature),
resolver: None, resolver: None,
loc: None, loc: None,
sb10q commented 2022-08-21 22:54:58 +08:00
Outdated
Review
Copy Link

Is this the best place for this in this block of code?

Is this the best place for this in this block of code?
static_fields: Default::default(),
}; };
(fun_def, class_def, signature, exn_type) (fun_def, class_def, signature, exn_type)
} }

19
nac3core/src/toplevel/composer.rs
View File

@ -1040,7 +1040,7 @@ impl TopLevelComposer {
class_body_ast, class_body_ast,
_class_ancestor_def, _class_ancestor_def,
class_fields_def, class_fields_def,
class_static_fields_def, // Introduce static class attribute list into the function class_static_fields_def,
sb10q commented 2022-08-21 22:55:27 +08:00
Outdated
Review
Copy Link

What function?
Comment is incomprehensible.

What function? Comment is incomprehensible.
class_methods_def, class_methods_def,
class_type_vars_def, class_type_vars_def,
class_resolver, class_resolver,
@ -1270,7 +1270,6 @@ impl TopLevelComposer {
.unify(method_dummy_ty, method_type) .unify(method_dummy_ty, method_type)
.map_err(|e| e.to_display(unifier).to_string())?; .map_err(|e| e.to_display(unifier).to_string())?;
} }
// Reset value from none since fields in the form "ATTR_0: int32 = 10" need to be initialised
ast::StmtKind::AnnAssign { target, annotation, value, .. } => { ast::StmtKind::AnnAssign { target, annotation, value, .. } => {
sb10q commented 2022-08-21 23:15:28 +08:00
Review
Copy Link

What does "reset" mean?
I don't see value being changed in that block of code

What does "reset" mean? I don't see ``value`` being changed in that block of code
if let ast::ExprKind::Name { id: attr, .. } = &target.node { if let ast::ExprKind::Name { id: attr, .. } = &target.node {
if defined_fields.insert(attr.to_string()) { if defined_fields.insert(attr.to_string()) {
@ -1297,7 +1296,6 @@ impl TopLevelComposer {
_ if core_config.kernel_ann.is_none() => (annotation, true), _ if core_config.kernel_ann.is_none() => (annotation, true),
_ => continue, // ignore fields annotated otherwise _ => continue, // ignore fields annotated otherwise
}; };
// If the value node is provided, then it must be a static class attribute
if let Option::Some(..) = &value{ if let Option::Some(..) = &value{
class_static_fields_def.push((*attr, dummy_field_type, mutable)); class_static_fields_def.push((*attr, dummy_field_type, mutable));
sb10q commented 2022-08-21 23:17:22 +08:00
Review
Copy Link

Why must it?
What happens if it isn't?

Why must it? What happens if it isn't?
} }
@ -1331,7 +1329,7 @@ impl TopLevelComposer {
type_var_to_concrete_def.insert(dummy_field_type, parsed_annotation); type_var_to_concrete_def.insert(dummy_field_type, parsed_annotation);
} else { } else {
return Err(format!( return Err(format!(
"same class fields `{}` defined twice (at {})", "same class field `{}` defined twice (at {})",
attr, target.location attr, target.location
)); ));
} }
@ -1342,23 +1340,16 @@ impl TopLevelComposer {
)); ));
} }
} }
// Add assign branch since fields in the form "ATTR_0 = 5" in the class body qualify as static class attributes
// However, type checking and expression folding needs to be performed in order to correctly
// Infer the type of target
ast::StmtKind::Assign { targets, value, .. } => { ast::StmtKind::Assign { targets, value, .. } => {
for target in targets { for target in targets {
if let ast::ExprKind::Name { id: attr, .. } = &target.node { if let ast::ExprKind::Name { id: attr, .. } = &target.node {
if defined_fields.insert(attr.to_string()) { if defined_fields.insert(attr.to_string()) {
sb10q commented 2022-08-21 22:58:25 +08:00
Review
Copy Link

"Add assign branch"?
This kind of narration should be in the PR description, not the code comments.

"Add assign branch"? This kind of narration should be in the PR description, not the code comments.
let dummy_field_type = unifier.get_dummy_var().0; let dummy_field_type = unifier.get_dummy_var().0;
class_static_fields_def.push((*attr, dummy_field_type, true)); class_static_fields_def.push((*attr, dummy_field_type, true));
class_fields_def.push((*attr, dummy_field_type, true)); class_fields_def.push((*attr, dummy_field_type, true));
} else { } else {
return Err(format!( return Err(format!(
"same class fields `{}` defined twice (at {})", "same class field `{}` defined twice (at {})",
attr, target.location attr, target.location
)); ));
} }
@ -1550,7 +1541,7 @@ impl TopLevelComposer {
ancestors, ancestors,
methods, methods,
fields, fields,
static_fields, // Introduce static fields for (un)initialization check static_fields,
type_vars, type_vars,
name: class_name, name: class_name,
object_id, object_id,
@ -1653,8 +1644,6 @@ impl TopLevelComposer {
unreachable!("must be init function here") unreachable!("must be init function here")
} }
let all_inited = Self::get_all_assigned_field(body.as_slice())?; let all_inited = Self::get_all_assigned_field(body.as_slice())?;
// If a field is uninitialized but also a static class attribute, don't
// throw an error due to uninitialization
for f in fields { for f in fields {
if !all_inited.contains(&f.0) && !static_fields.contains(&f) { if !all_inited.contains(&f.0) && !static_fields.contains(&f) {
return Err(format!( return Err(format!(

2
nac3core/src/toplevel/helper.rs
View File

@ -162,7 +162,7 @@ impl TopLevelComposer {
object_id: DefinitionId(index), object_id: DefinitionId(index),
type_vars: Default::default(), type_vars: Default::default(),
fields: Default::default(), fields: Default::default(),
static_fields: Default::default(), // Initialize for constructor static_fields: Default::default(),
sb10q commented 2022-08-21 22:56:43 +08:00
Outdated
Review
Copy Link

Is this any different than the other lines in this block of code, which don't have a comment?

Is this any different than the other lines in this block of code, which don't have a comment?
methods: Default::default(), methods: Default::default(),
ancestors: Default::default(), ancestors: Default::default(),
constructor, constructor,

4
nac3core/src/toplevel/mod.rs
View File

@ -91,9 +91,9 @@ pub enum TopLevelDef {
// class fields // class fields
// name, type, is mutable // name, type, is mutable
fields: Vec<(StrRef, Type, bool)>, fields: Vec<(StrRef, Type, bool)>,
// class methods, pointing to the corresponding function definition.
static_fields: Vec<(StrRef, Type, bool)>,
// list of static data members // list of static data members
static_fields: Vec<(StrRef, Type, bool)>,
// class methods, pointing to the corresponding function definition.
sb10q commented 2022-08-21 22:56:54 +08:00
Outdated
Review
Copy Link

Misplaced comment.

Misplaced comment.
methods: Vec<(StrRef, Type, DefinitionId)>, methods: Vec<(StrRef, Type, DefinitionId)>,
// ancestor classes, including itself. // ancestor classes, including itself.
ancestors: Vec<TypeAnnotation>, ancestors: Vec<TypeAnnotation>,