WIP: Adding static class attribute definition functionality #312

Draft
Aadityavardhan wants to merge 2 commits from Aadityavardhan/nac3_sca:master into master
First-time contributor

Enabled class attribute definition syntax in the class body (Imp: Type checking/expression folding needs to be done).
Modified init checker to ensure that static fields do not need to be initialised in init.

Enabled class attribute definition syntax in the class body (Imp: Type checking/expression folding needs to be done). Modified init checker to ensure that static fields do not need to be initialised in init.
Aadityavardhan added 1 commit 2022-08-21 20:38:28 +08:00
sb10q reviewed 2022-08-21 22:54:58 +08:00
@ -74,6 +74,7 @@ pub fn get_exn_constructor(
constructor: Some(signature),
resolver: None,
loc: None,
static_fields: Default::default(),
Owner

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

Is this the best place for this in this block of code?
sb10q reviewed 2022-08-21 22:55:27 +08:00
@ -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
Owner

What function?
Comment is incomprehensible.

What function? Comment is incomprehensible.
sb10q reviewed 2022-08-21 22:56:43 +08:00
@ -162,6 +162,7 @@ impl TopLevelComposer {
object_id: DefinitionId(index),
type_vars: Default::default(),
fields: Default::default(),
static_fields: Default::default(), // Initialize for constructor
Owner

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?
sb10q reviewed 2022-08-21 22:56:54 +08:00
@ -93,2 +93,4 @@
fields: Vec<(StrRef, Type, bool)>,
// class methods, pointing to the corresponding function definition.
static_fields: Vec<(StrRef, Type, bool)>,
// list of static data members
Owner

Misplaced comment.

Misplaced comment.
sb10q reviewed 2022-08-21 22:58:25 +08:00
@ -1337,2 +1344,3 @@
}
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
Owner

"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.
sb10q reviewed 2022-08-21 22:59:12 +08:00
@ -1516,6 +1550,7 @@ impl TopLevelComposer {
ancestors,
methods,
fields,
static_fields, // Introduce static fields for (un)initialization check
Owner

"Introduce"?

"Introduce"?
sb10q reviewed 2022-08-21 23:01:06 +08:00
@ -1621,2 +1656,2 @@
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
Owner

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.
sb10q reviewed 2022-08-21 23:14:33 +08:00
sb10q reviewed 2022-08-21 23:15:28 +08:00
@ -1269,3 +1271,3 @@
.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
Owner

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
sb10q reviewed 2022-08-21 23:17:22 +08:00
@ -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
Owner

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

Why must it? What happens if it isn't?
sb10q reviewed 2022-08-21 23:18:08 +08:00
@ -1339,0 +1355,4 @@
class_static_fields_def.push((*attr, dummy_field_type, true));
class_fields_def.push((*attr, dummy_field_type, true));
Owner

style, remove blank line here

style, remove blank line here
sb10q reviewed 2022-08-21 23:18:41 +08:00
@ -1339,0 +1358,4 @@
} else {
return Err(format!(
"same class fields `{}` defined twice (at {})",
Owner

fields or field?

fields or field?
Aadityavardhan added 1 commit 2022-08-22 00:40:58 +08:00
sb10q requested review from ychenfo 2022-08-24 16:59:17 +08:00
This pull request has changes conflicting with the target branch.
  • nac3core/src/toplevel/builtins.rs
  • nac3core/src/toplevel/composer.rs
  • nac3core/src/toplevel/helper.rs
  • nac3core/src/toplevel/mod.rs

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u master:Aadityavardhan-master
git checkout Aadityavardhan-master
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/nac3#312
No description provided.