From 7720cdfc8499c044803ba5630076e51556ff9ce1 Mon Sep 17 00:00:00 2001 From: Volkan Sagcan Date: Mon, 3 Feb 2025 16:18:09 +0100 Subject: [PATCH] refactor: Polish code, part 2 --- compiler/plc_driver/src/pipelines.rs | 4 +- .../plc_driver/src/pipelines/validator.rs | 2 +- src/lowering/property.rs | 186 ++++++++++-------- src/lowering/validator.rs | 23 ++- 4 files changed, 114 insertions(+), 101 deletions(-) diff --git a/compiler/plc_driver/src/pipelines.rs b/compiler/plc_driver/src/pipelines.rs index e4f943df51..671f322b66 100644 --- a/compiler/plc_driver/src/pipelines.rs +++ b/compiler/plc_driver/src/pipelines.rs @@ -258,10 +258,12 @@ impl BuildPipeline { 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()); diff --git a/compiler/plc_driver/src/pipelines/validator.rs b/compiler/plc_driver/src/pipelines/validator.rs index befa2a18d7..d5f27c7d32 100644 --- a/compiler/plc_driver/src/pipelines/validator.rs +++ b/compiler/plc_driver/src/pipelines/validator.rs @@ -10,7 +10,7 @@ impl PipelineParticipantMut for ParticipantValidator { self.validate_properties(&unit.properties); } - self.report(); + self.report_diagnostics(); ParsedProject { units } } diff --git a/src/lowering/property.rs b/src/lowering/property.rs index 44660d7a89..30c678d3e5 100644 --- a/src/lowering/property.rs +++ b/src/lowering/property.rs @@ -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; @@ -88,92 +88,14 @@ 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> = 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 => { @@ -181,6 +103,8 @@ impl PropertyLowerer { } } + // 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}", @@ -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(); @@ -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_()` + 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; @@ -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)] diff --git a/src/lowering/validator.rs b/src/lowering/validator.rs index d5efea13bb..56314ca1ec 100644 --- a/src/lowering/validator.rs +++ b/src/lowering/validator.rs @@ -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; @@ -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 } } @@ -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!( @@ -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") @@ -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( @@ -65,6 +62,7 @@ impl ParticipantValidator { ); continue; } + if get_blocks.len() > 1 { self.diagnostics.push( Diagnostic::new("Property has more than one GET block") @@ -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") @@ -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 {