Skip to content

Commit

Permalink
refactor: Polish code, part 2
Browse files Browse the repository at this point in the history
  • Loading branch information
@volsa
volsa committed Feb 3, 2025
1 parent d420b50 commit 7720cdf
Show file tree
Hide file tree
Showing 4 changed files with 114 additions and 101 deletions.
4 changes: 3 additions & 1 deletion compiler/plc_driver/src/pipelines.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -258,10 +258,12 @@ impl<T: SourceContainer> BuildPipeline<T> {
InitParticipant::new(&self.project.get_init_symbol_name(), self.context.provider());
self.register_mut_participant(Box::new(init_participant));

// Note: The order matters here, the PropertyLowerer will drain the `properties` field, meaning the
// PariticipantValidator has to run first since it uses these `properties`
self.register_mut_participant(Box::new(ParticipantValidator::new(
&self.context,
self.compile_parameters.as_ref().map(|it| it.error_format).unwrap_or_default(),
))); // XXX: I think this has to run first, because the PropertyLowerer will drain the `properties` field resulting in no validation
)));
self.register_mut_participant(Box::new(PropertyLowerer::new(self.context.provider())));

let aggregate_return_participant = AggregateTypeLowerer::new(self.context.provider());
Expand Down
2 changes: 1 addition & 1 deletion compiler/plc_driver/src/pipelines/validator.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ impl PipelineParticipantMut for ParticipantValidator {
self.validate_properties(&unit.properties);
}

self.report();
self.report_diagnostics();

ParsedProject { units }
}
Expand Down
186 changes: 99 additions & 87 deletions src/lowering/property.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,15 @@
use std::collections::HashMap;

use helper::create_internal_assignment;
use helper::{create_internal_assignment, patch_prefix_to_name};
use plc_ast::{
ast::{
AccessModifier, ArgumentProperty, AstFactory, AstNode, AstStatement, CompilationUnit, Implementation,
LinkageType, Pou, PouType, Property, PropertyKind, ReferenceAccess, ReferenceExpr, Variable,
VariableBlock, VariableBlockType,
LinkageType, Pou, PouType, Property, PropertyKind, Variable, VariableBlock, VariableBlockType,
},
mut_visitor::AstVisitorMut,
provider::IdProvider,
try_from,
};
use plc_source::source_location::SourceLocation;
use plc_util::convention::qualified_name;
Expand All @@ -88,99 +88,23 @@ impl PropertyLowerer {
pub fn lower_references_to_calls(&mut self, unit: &mut CompilationUnit) {
self.visit_compilation_unit(unit);
}
}

impl AstVisitorMut for PropertyLowerer {
fn visit_compilation_unit(&mut self, unit: &mut CompilationUnit) {
for implementation in &mut unit.implementations {
self.visit_implementation(implementation);
}
}

fn visit_implementation(&mut self, implementation: &mut Implementation) {
if let PouType::Method { property: Some(qualified_name), .. } = &implementation.pou_type {
self.context = Some(qualified_name.clone())
}

for statement in &mut implementation.statements {
self.visit(statement);
}

self.context = None;
}

fn visit_assignment(&mut self, node: &mut AstNode) {
let AstStatement::Assignment(data) = &mut node.stmt else {
unreachable!();
};

self.visit(&mut data.right);

match self.annotations.as_ref().and_then(|map| map.get(&data.left)) {
Some(annotation) if annotation.is_property() => {
if self.context.as_deref() == annotation.qualified_name() {
return;
}

patch_prefix_to_name("__set_", &mut data.left);
let call = AstFactory::create_call_statement(
data.left.as_ref().clone(),
Some(data.right.as_ref().clone()),
self.id_provider.next_id(),
node.location.clone(),
);

let _ = std::mem::replace(node, call);
}

_ => (),
}
}

fn visit_reference_expr(&mut self, node: &mut AstNode) {
if let Some(annotation) = self.annotations.as_ref().unwrap().get(node) {
if !annotation.is_property() {
return;
}

if self.context.as_deref() == annotation.qualified_name() {
return;
}

patch_prefix_to_name("__get_", node);
let call = AstFactory::create_call_statement(
node.clone(),
None,
self.id_provider.next_id(),
node.location.clone(),
);

let _ = std::mem::replace(node, call);
}
}
}

fn patch_prefix_to_name(prefix: &str, node: &mut AstNode) {
let AstStatement::ReferenceExpr(ReferenceExpr { ref mut access, .. }) = &mut node.stmt else { return };
let ReferenceAccess::Member(member) = access else { return };
let AstStatement::Identifier(name) = &mut member.stmt else { return };

name.insert_str(0, prefix);
}

impl PropertyLowerer {
pub fn lower_to_methods(&mut self, unit: &mut CompilationUnit) {
// We want to keep track of all parent POUs and their Property definitions to later add a
// variable block of type `Property` to each POU since these properties internally are represented
// by a variable.
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.parent_name) {
Some(values) => values.push(property.clone()),
None => {
parents.insert(property.parent_name.clone(), vec![property.clone()]);
}
}

// Transform the property into a method by creating and pushing a `POU` and an `Implementation` to
// the compilation unit
for property_impl in property.implementations {
let name = format!(
"{parent}.__{kind}_{name}",
Expand Down Expand Up @@ -270,8 +194,8 @@ impl PropertyLowerer {
}
}

// Iterate over all POUs, check if they have one or more properties defined and if so, add a variable block
// of type `Property` consisting of all the properties.
// Iterate over all POUs, check if they have one or more properties defined and if so, add a
// variable block of type `Property` consisting of all the properties.
for pou in &mut unit.units {
if let Some(properties) = parents.get(&pou.name) {
let mut variables = Vec::new();
Expand All @@ -291,9 +215,87 @@ impl PropertyLowerer {
}
}

impl AstVisitorMut for PropertyLowerer {
fn visit_compilation_unit(&mut self, unit: &mut CompilationUnit) {
for implementation in &mut unit.implementations {
self.visit_implementation(implementation);
}
}

fn visit_implementation(&mut self, implementation: &mut Implementation) {
//
if let PouType::Method { property: Some(qualified_name), .. } = &implementation.pou_type {
self.context = Some(qualified_name.clone())
}

for statement in &mut implementation.statements {
self.visit(statement);
}

// ...
self.context = None;
}

fn visit_assignment(&mut self, node: &mut AstNode) {
let AstStatement::Assignment(data) = &mut node.stmt else {
unreachable!();
};

self.visit(&mut data.right);

match self.annotations.as_ref().and_then(|map| map.get(&data.left)) {
// When dealing with an assignment where the left-hand side is a property reference, we have to
// replace the reference with a method call to `__set_<property>(<right-hand-side>)`
Some(annotation) if annotation.is_property() => {
if self.context.as_deref() == annotation.qualified_name() {
return;
}

patch_prefix_to_name("__set_", &mut data.left);
let call = AstFactory::create_call_statement(
data.left.as_ref().clone(),
Some(data.right.as_ref().clone()),
self.id_provider.next_id(),
node.location.clone(),
);

// In-place AST replacement of the assignment statements as a whole with the newly created call
let _ = std::mem::replace(node, call);
}

_ => (),
}
}

fn visit_reference_expr(&mut self, node: &mut AstNode) {
if let Some(annotation) = self.annotations.as_ref().unwrap().get(node) {
if !annotation.is_property() {
return;
}

if self.context.as_deref() == annotation.qualified_name() {
return;
}

// Any property reference that is not the left-hand side of an assignment needs to be replaced
// with a function call to the respective getter property method.
patch_prefix_to_name("__get_", node);
let call = AstFactory::create_call_statement(
node.clone(),
None,
self.id_provider.next_id(),
node.location.clone(),
);

// In-place AST replacement of the reference-expr node with the newly created call
let _ = std::mem::replace(node, call);
}
}
}

mod helper {
use plc_ast::{
ast::{AstFactory, AstNode},
ast::{AstFactory, AstNode, AstStatement, ReferenceAccess, ReferenceExpr},
provider::IdProvider,
};
use plc_source::source_location::SourceLocation;
Expand Down Expand Up @@ -327,6 +329,16 @@ mod helper {
id_provider.next_id(),
)
}

pub fn patch_prefix_to_name(prefix: &str, node: &mut AstNode) {
let AstStatement::ReferenceExpr(ReferenceExpr { ref mut access, .. }) = &mut node.stmt else {
return;
};
let ReferenceAccess::Member(member) = access else { return };
let AstStatement::Identifier(name) = &mut member.stmt else { return };

name.insert_str(0, prefix);
}
}

#[cfg(test)]
Expand Down
23 changes: 11 additions & 12 deletions src/lowering/validator.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use plc_ast::ast::{Property, PropertyKind};
use plc_ast::ast::{Property, PropertyKind, VariableBlockType};
use plc_diagnostics::diagnostics::Diagnostic;
use plc_index::GlobalContext;

Expand All @@ -12,8 +12,6 @@ pub struct ParticipantValidator {
}

impl ParticipantValidator {
// TODO: Temporary solution with that diagnostician, ideally the diagnostician lazy reads source files and
// doesn't rely on register_file
pub fn new(context: &GlobalContext, error_fmt: ErrorFormat) -> ParticipantValidator {
ParticipantValidator { diagnostics: Vec::new(), context: context.clone(), error_fmt }
}
Expand All @@ -22,6 +20,7 @@ impl ParticipantValidator {
for property in properties {
let mut get_blocks = vec![];
let mut set_blocks = vec![];

if !property.parent_kind.is_stateful() {
self.diagnostics.push(
Diagnostic::new(format!(
Expand All @@ -32,11 +31,11 @@ impl ParticipantValidator {
.with_error_code("E114"),
);
}

for implementation in &property.implementations {
// implementation.variable_block.variable_block_type
for variable in &implementation.variable_blocks {
match variable.variable_block_type {
plc_ast::ast::VariableBlockType::Local => {}
VariableBlockType::Local => {}
_ => {
self.diagnostics.push(
Diagnostic::new("Properties only allow variable blocks of type VAR")
Expand All @@ -47,15 +46,13 @@ impl ParticipantValidator {
}
}
}

match implementation.kind {
PropertyKind::Get => {
get_blocks.push(implementation.location.clone());
}
PropertyKind::Set => {
set_blocks.push(implementation.location.clone());
}
PropertyKind::Get => get_blocks.push(implementation.location.clone()),
PropertyKind::Set => set_blocks.push(implementation.location.clone()),
}
}

if set_blocks.len() + get_blocks.len() == 0 {
// one block is required
self.diagnostics.push(
Expand All @@ -65,6 +62,7 @@ impl ParticipantValidator {
);
continue;
}

if get_blocks.len() > 1 {
self.diagnostics.push(
Diagnostic::new("Property has more than one GET block")
Expand All @@ -73,6 +71,7 @@ impl ParticipantValidator {
.with_error_code("E116"),
);
}

if set_blocks.len() > 1 {
self.diagnostics.push(
Diagnostic::new("Property has more than one SET block")
Expand All @@ -84,7 +83,7 @@ impl ParticipantValidator {
}
}

pub fn report(&mut self) {
pub fn report_diagnostics(&mut self) {
self.context.with_error_fmt(self.error_fmt.into());

for diagnostic in &self.diagnostics {
Expand Down

0 comments on commit 7720cdf

Please sign in to comment.