WIP: Adding static class attribute definition functionality #312

Draft
Aadityavardhan wants to merge 2 commits from Aadityavardhan/nac3_sca:master into master
4 changed files with 49 additions and 6 deletions
Showing only changes of commit 92cc3d799f - Show all commits

View File

@ -74,6 +74,7 @@ pub fn get_exn_constructor(
constructor: Some(signature),
resolver: None,
loc: None,
static_fields: Default::default(),
Outdated
Review

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

Is this the best place for this in this block of code?
};
(fun_def, class_def, signature, exn_type)
}
@ -175,6 +176,7 @@ pub fn get_builtins(primitives: &mut (PrimitiveStore, Unifier)) -> BuiltinInfo {
type_vars: Default::default(),
fields: exception_fields,
methods: Default::default(),
static_fields: Default::default(),
ancestors: vec![],
constructor: None,
resolver: None,
@ -200,6 +202,7 @@ pub fn get_builtins(primitives: &mut (PrimitiveStore, Unifier)) -> BuiltinInfo {
object_id: DefinitionId(10),
type_vars: vec![option_ty_var],
fields: vec![],
static_fields: vec![],
methods: vec![
("is_some".into(), is_some_ty.0, DefinitionId(11)),
("is_none".into(), is_some_ty.0, DefinitionId(12)),

View File

@ -1040,6 +1040,7 @@ impl TopLevelComposer {
class_body_ast,
_class_ancestor_def,
class_fields_def,
class_static_fields_def, // Introduce static class attribute list into the function
Outdated
Review

What function?
Comment is incomprehensible.

What function? Comment is incomprehensible.
class_methods_def,
class_type_vars_def,
class_resolver,
@ -1047,6 +1048,7 @@ impl TopLevelComposer {
object_id,
ancestors,
fields,
static_fields,
methods,
resolver,
type_vars,
@ -1054,7 +1056,7 @@ impl TopLevelComposer {
} = &mut *class_def
{
if let ast::StmtKind::ClassDef { name, bases, body, .. } = &class_ast {
(*object_id, *name, bases, body, ancestors, fields, methods, type_vars, resolver)
(*object_id, *name, bases, body, ancestors, fields, static_fields, methods, type_vars, resolver)
} else {
unreachable!("here must be class def ast");
}
@ -1268,7 +1270,8 @@ impl TopLevelComposer {
.unify(method_dummy_ty, method_type)
.map_err(|e| e.to_display(unifier).to_string())?;
}
ast::StmtKind::AnnAssign { target, annotation, value: None, .. } => {
// Reset value from none since fields in the form "ATTR_0: int32 = 10" need to be initialised
Review

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
ast::StmtKind::AnnAssign { target, annotation, value, .. } => {
if let ast::ExprKind::Name { id: attr, .. } = &target.node {
if defined_fields.insert(attr.to_string()) {
let dummy_field_type = unifier.get_dummy_var().0;
@ -1294,6 +1297,10 @@ impl TopLevelComposer {
_ if core_config.kernel_ann.is_none() => (annotation, true),
_ => continue, // ignore fields annotated otherwise
};
// If the value node is provided, then it must be a static class attribute
Review

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

Why must it? What happens if it isn't?
if let Option::Some(..) = &value{
class_static_fields_def.push((*attr, dummy_field_type, mutable));
}
class_fields_def.push((*attr, dummy_field_type, mutable));
let parsed_annotation = parse_ast_to_type_annotation_kinds(
@ -1335,7 +1342,34 @@ impl TopLevelComposer {
));
}
}
ast::StmtKind::Assign { .. } => {}, // we don't class attributes
// Add assign branch since fields in the form "ATTR_0 = 5" in the class body qualify as static class attributes
Review

"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.
// However, type checking and expression folding needs to be performed in order to correctly
// Infer the type of target
ast::StmtKind::Assign { targets, value, .. } => {
for target in targets {
if let ast::ExprKind::Name { id: attr, .. } = &target.node {
if defined_fields.insert(attr.to_string()) {
let dummy_field_type = unifier.get_dummy_var().0;
class_static_fields_def.push((*attr, dummy_field_type, true));
class_fields_def.push((*attr, dummy_field_type, true));
Review

style, remove blank line here

style, remove blank line here
} else {
return Err(format!(
"same class fields `{}` defined twice (at {})",
Review

fields or field?

fields or field?
attr, target.location
));
}
} else {
return Err(format!(
"unsupported statement type in class definition body (at {})",
target.location
));
}
}
},
ast::StmtKind::Pass { .. } => {}
ast::StmtKind::Expr { value: _, .. } => {} // typically a docstring; ignoring all expressions matches CPython behavior
_ => {
@ -1516,6 +1550,7 @@ impl TopLevelComposer {
ancestors,
methods,
fields,
static_fields, // Introduce static fields for (un)initialization check
Outdated
Review

"Introduce"?

"Introduce"?
type_vars,
name: class_name,
object_id,
@ -1618,11 +1653,13 @@ impl TopLevelComposer {
unreachable!("must be init function here")
}
let all_inited = Self::get_all_assigned_field(body.as_slice())?;
for (f, _, _) in fields {
if !all_inited.contains(f) {
// If a field is uninitialized but also a static class attribute, don't
// throw an error due to uninitialization
Outdated
Review

Again the code comments should be about the entire code, and not focused on this one patch.

Again the code comments should be about the entire code, and not focused on this one patch.
for f in fields {
if !all_inited.contains(&f.0) && !static_fields.contains(&f) {
return Err(format!(
"fields `{}` of class `{}` not fully initialized in the initializer (at {})",
f,
&f.0,
class_name,
body[0].location,
));

View File

@ -162,6 +162,7 @@ impl TopLevelComposer {
object_id: DefinitionId(index),
type_vars: Default::default(),
fields: Default::default(),
static_fields: Default::default(), // Initialize for constructor
Outdated
Review

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(),
ancestors: Default::default(),
constructor,

View File

@ -92,6 +92,8 @@ pub enum TopLevelDef {
// name, type, is mutable
fields: Vec<(StrRef, Type, bool)>,
// class methods, pointing to the corresponding function definition.
static_fields: Vec<(StrRef, Type, bool)>,
// list of static data members
Outdated
Review

Misplaced comment.

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