Skip to content

Commit

Permalink
refactor: polish code
Browse files Browse the repository at this point in the history
  • Loading branch information
@abroooo
abroooo committed Jan 30, 2025
1 parent ddcfaed commit 77c1a37
Show file tree
Hide file tree
Showing 9 changed files with 157 additions and 124 deletions.
10 changes: 5 additions & 5 deletions compiler/plc_ast/src/ast.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,17 +67,17 @@ pub struct InterfaceIdentifier {
pub struct Property {
pub name: String,
pub name_location: SourceLocation,
pub kind_parent: PouType,
pub name_parent: String,
pub name_parent_location: SourceLocation,
pub parent_kind: PouType,
pub parent_name: String,
pub parent_name_location: SourceLocation,
pub datatype: DataTypeDeclaration,
pub implementations: Vec<PropertyImplementation>,
}

#[derive(Debug, PartialEq, Clone)]
pub struct PropertyImplementation {
pub kind: PropertyKind,
pub variables: Vec<VariableBlock>,
pub variable_blocks: Vec<VariableBlock>,
pub statements: Vec<AstNode>,
pub location: SourceLocation,
}
Expand Down Expand Up @@ -436,7 +436,7 @@ pub enum VariableBlockType {
Global,
InOut,
External,
/// Compiler internal
/// Compiler internal: when lowering a property we internally create a variable block with this
Property,
}

Expand Down
5 changes: 2 additions & 3 deletions compiler/plc_diagnostics/src/diagnostics/error_codes/E114.md
View file
Original file line number Diff line number Diff line change
@@ -1,17 +1,16 @@
# Property defined in non-stateful POU type

Properties may only be defined in stateful POUs such as a `PROGRAM`,`CLASS` or `FUNCTION_BLOCK`.
Defining the property in any other POU results in this error.


Errouneus code example:
```
FUNCTION foo
// Invalid definition
PROPERTY bar : DINT
PROPERTY bar : DINT
GET
bar := 42;
END_GET
END_PROPERTY
END_FUNCTION
```
```
1 change: 0 additions & 1 deletion src/codegen/generators/expression_generator.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,6 @@ impl<'ink, 'b> ExpressionCodeGenerator<'ink, 'b> {
// If the expression was replaced by the resolver, generate the replacement
if let Some(StatementAnnotation::ReplacementAst { statement }) = self.annotations.get(expression) {
// we trust that the validator only passed us valid parameters (so left & right should be same type)
// dbg!(&statement);
return self.generate_expression(statement);
}

Expand Down
128 changes: 97 additions & 31 deletions src/lowering/property.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,6 @@ impl AstVisitorMut for PropertyLowerer {

fn visit_implementation(&mut self, implementation: &mut Implementation) {
if let PouType::Method { property: Some(qualified_name), .. } = &implementation.pou_type {
// TODO: Two things, first let's maybe introduce a `enter_method` and `exit_method` method and secondly
// I'm not entirely happy with this solution but it seemed to be the easiest way to solve for now
self.context = Some(qualified_name.clone())
}

Expand All @@ -61,15 +59,11 @@ impl AstVisitorMut for PropertyLowerer {
unreachable!();
};

// dbg!(&data);
self.visit(&mut data.right);
// dbg!(&data);

match self.annotations.as_ref().and_then(|map| map.get(&data.left)) {
Some(annotation) if annotation.is_property() => {
// self.visit(&mut data.right);
if self.context.as_deref() == annotation.get_qualified_name() {
// dbg!(node, self.context.clone(), annotation);
return;
}

Expand All @@ -94,7 +88,6 @@ impl AstVisitorMut for PropertyLowerer {
return;
}

// dbg!(&annotation, &self.context);
if self.context.as_deref() == annotation.get_qualified_name() {
return;
}
Expand All @@ -107,9 +100,7 @@ impl AstVisitorMut for PropertyLowerer {
node.location.clone(),
);

// dbg!(&call);
let _ = std::mem::replace(node, call);
// dbg!(&node);
}
}
}
Expand All @@ -122,37 +113,36 @@ fn patch_prefix_to_name(prefix: &str, node: &mut AstNode) {
name.insert_str(0, prefix);
}

// TODO: There are a lot of clone calls here, see if we can reduce them?
impl PropertyLowerer {
pub fn lower_to_methods(&mut self, unit: &mut CompilationUnit) {
let mut parents: HashMap<String, Vec<Property>> = HashMap::new();

for property in &mut unit.properties.drain(..) {
// Keep track of the parent POUs and all their defined properties
match parents.get_mut(&property.name_parent) {
match parents.get_mut(&property.parent_name) {
Some(values) => values.push(property.clone()),
None => {
parents.insert(property.name_parent.clone(), vec![property.clone()]);
parents.insert(property.parent_name.clone(), vec![property.clone()]);
}
}

for property_impl in property.implementations {
let name = format!(
"{parent}.__{kind}_{name}",
parent = property.name_parent,
parent = property.parent_name,
kind = property_impl.kind,
name = property.name
);

let mut pou = Pou {
name,
kind: PouType::Method {
parent: property.name_parent.clone(),
property: Some(qualified_name(&property.name_parent, &property.name)),
parent: property.parent_name.clone(),
property: Some(qualified_name(&property.parent_name, &property.name)),
},
variable_blocks: Vec::new(),
return_type: Some(property.datatype.clone()),
location: SourceLocation::undefined(), // TODO: Update me
location: property.name_location.clone(),
name_location: property.name_location.clone(),
poly_mode: None,
generics: Vec::new(),
Expand Down Expand Up @@ -194,7 +184,6 @@ impl PropertyLowerer {
PropertyKind::Set => {
let parameter_name = "__in";

// TODO: The return type of a setter should be VOID?
pou.variable_blocks.push(VariableBlock {
access: AccessModifier::Public,
constant: false,
Expand All @@ -210,6 +199,7 @@ impl PropertyLowerer {
linkage: LinkageType::Internal,
location: SourceLocation::internal(),
});
pou.return_type = None;

let name_lhs = &property.name;
let name_rhs = parameter_name;
Expand Down Expand Up @@ -432,7 +422,17 @@ mod tests {
},
],
location: SourceLocation {
span: None,
span: Range(
TextLocation {
line: 5,
column: 19,
offset: 102,
}..TextLocation {
line: 5,
column: 25,
offset: 108,
},
),
},
name_location: SourceLocation {
span: Range(
Expand Down Expand Up @@ -523,7 +523,17 @@ mod tests {
},
],
location: SourceLocation {
span: None,
span: Range(
TextLocation {
line: 5,
column: 19,
offset: 102,
}..TextLocation {
line: 5,
column: 25,
offset: 108,
},
),
},
name_location: SourceLocation {
span: Range(
Expand Down Expand Up @@ -567,7 +577,7 @@ mod tests {
right: ReferenceExpr {
kind: Member(
Identifier {
name: "__get_myProp",
name: "myProp",
},
),
base: None,
Expand All @@ -593,7 +603,17 @@ mod tests {
},
],
location: SourceLocation {
span: None,
span: Range(
TextLocation {
line: 14,
column: 19,
offset: 325,
}..TextLocation {
line: 14,
column: 31,
offset: 337,
},
),
},
name_location: SourceLocation {
span: Range(
Expand Down Expand Up @@ -645,7 +665,17 @@ mod tests {
},
],
location: SourceLocation {
span: None,
span: Range(
TextLocation {
line: 14,
column: 19,
offset: 325,
}..TextLocation {
line: 14,
column: 31,
offset: 337,
},
),
},
name_location: SourceLocation {
span: Range(
Expand Down Expand Up @@ -833,7 +863,17 @@ mod tests {
},
],
location: SourceLocation {
span: None,
span: Range(
TextLocation {
line: 5,
column: 19,
offset: 102,
}..TextLocation {
line: 5,
column: 25,
offset: 108,
},
),
},
name_location: SourceLocation {
span: Range(
Expand Down Expand Up @@ -903,7 +943,17 @@ mod tests {
},
],
location: SourceLocation {
span: None,
span: Range(
TextLocation {
line: 5,
column: 19,
offset: 102,
}..TextLocation {
line: 5,
column: 25,
offset: 108,
},
),
},
name_location: SourceLocation {
span: Range(
Expand Down Expand Up @@ -1019,11 +1069,7 @@ mod tests {
"fb.foo",
),
},
return_type: Some(
DataTypeReference {
referenced_type: "DINT",
},
),
return_type: None,
interfaces: [],
}
"#);
Expand Down Expand Up @@ -1074,7 +1120,17 @@ mod tests {
},
],
location: SourceLocation {
span: None,
span: Range(
TextLocation {
line: 6,
column: 21,
offset: 130,
}..TextLocation {
line: 6,
column: 24,
offset: 133,
},
),
},
name_location: SourceLocation {
span: Range(
Expand Down Expand Up @@ -1147,7 +1203,17 @@ mod tests {
},
],
location: SourceLocation {
span: None,
span: Range(
TextLocation {
line: 6,
column: 21,
offset: 130,
}..TextLocation {
line: 6,
column: 24,
offset: 133,
},
),
},
name_location: SourceLocation {
span: Range(
Expand Down
9 changes: 3 additions & 6 deletions src/lowering/validator.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,19 @@ impl ParticipantValidator {
for property in properties {
let mut get_blocks = vec![];
let mut set_blocks = vec![];
if !property.kind_parent.is_stateful_pou() {
if !property.parent_kind.is_stateful_pou() {
self.diagnostics.push(
Diagnostic::new(format!(
"Property `{name}` must be defined in a stateful POU type (PROGRAM, CLASS or FUNCTION_BLOCK)",
name = property.name
))
.with_location(property.name_parent_location.clone())
.with_location(property.parent_name_location.clone())
.with_error_code("E114"),
);
}
for implementation in &property.implementations {
// implementation.variable_block.variable_block_type
for variable in &implementation.variables {
for variable in &implementation.variable_blocks {
match variable.variable_block_type {
plc_ast::ast::VariableBlockType::Local => {}
_ => {
Expand Down Expand Up @@ -62,7 +62,6 @@ impl ParticipantValidator {
);
continue;
}
// TODO: check why END_GET is part of error message in property_with_more_than_one_get_block()
if get_blocks.len() > 1 {
self.diagnostics.push(
Diagnostic::new("Property has more than one GET block")
Expand All @@ -80,10 +79,8 @@ impl ParticipantValidator {
);
}
}
// todo!()
}
}
// -------

#[derive(Default)]
pub struct ParticipantDiagnostician {
Expand Down
Loading

0 comments on commit 77c1a37

Please sign in to comment.