From c40f28e9e9024e8a4478f9cded700df79f35ecfd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Zanitti?= Date: Fri, 25 Oct 2024 11:48:12 -0300 Subject: [PATCH] Structs: Fields check (#1907) Note: Like #1910, this PR also includes the same modifications to expression processing, in order to run tests. --- ast/src/analyzed/display.rs | 4 +- ast/src/parsed/mod.rs | 14 ++- pil-analyzer/src/expression_processor.rs | 28 +++++- pil-analyzer/src/lib.rs | 1 + pil-analyzer/src/pil_analyzer.rs | 47 +++++++++- pil-analyzer/src/structural_checks.rs | 110 +++++++++++++++++++++++ pil-analyzer/tests/types.rs | 72 +++++++++++++++ 7 files changed, 264 insertions(+), 12 deletions(-) create mode 100644 pil-analyzer/src/structural_checks.rs diff --git a/ast/src/analyzed/display.rs b/ast/src/analyzed/display.rs index 63e7ee725..8cbbcdd3b 100644 --- a/ast/src/analyzed/display.rs +++ b/ast/src/analyzed/display.rs @@ -64,9 +64,7 @@ impl Display for Analyzed { if matches!( definition, Some(FunctionValueDefinition::TypeConstructor(_, _)) - ) || matches!( - definition, - Some(FunctionValueDefinition::TraitFunction(_, _)) + | Some(FunctionValueDefinition::TraitFunction(_, _)) ) { // These are printed as part of the enum / trait. continue; diff --git a/ast/src/parsed/mod.rs b/ast/src/parsed/mod.rs index 6a76c7fde..e3e0c55f5 100644 --- a/ast/src/parsed/mod.rs +++ b/ast/src/parsed/mod.rs @@ -40,19 +40,25 @@ pub enum SymbolCategory { /// A type constructor, i.e. an enum variant, which can be used as a function or constant inside an expression /// or to deconstruct a value in a pattern. TypeConstructor, - /// A trait declaration, which can be used as a type. + /// A trait declaration TraitDeclaration, + /// A struct, which can be used as a type. + Struct, } impl SymbolCategory { /// Returns if a symbol of a given category can satisfy a request for a certain category. pub fn compatible_with_request(&self, request: SymbolCategory) -> bool { match self { - SymbolCategory::Value => request == SymbolCategory::Value, - SymbolCategory::Type => request == SymbolCategory::Type, + SymbolCategory::Struct => { + // Structs can also satisfy requests for types. + request == SymbolCategory::Struct || request == SymbolCategory::Type + } SymbolCategory::TypeConstructor => { // Type constructors can also satisfy requests for values. request == SymbolCategory::TypeConstructor || request == SymbolCategory::Value } + SymbolCategory::Value => request == SymbolCategory::Value, + SymbolCategory::Type => request == SymbolCategory::Type, SymbolCategory::TraitDeclaration => request == SymbolCategory::TraitDeclaration, } } @@ -153,7 +159,7 @@ impl PilStatement { ), ), PilStatement::StructDeclaration(_, StructDeclaration { name, .. }) => { - Box::new(once((name, None, SymbolCategory::Type))) + Box::new(once((name, None, SymbolCategory::Struct))) } PilStatement::TraitDeclaration( _, diff --git a/pil-analyzer/src/expression_processor.rs b/pil-analyzer/src/expression_processor.rs index b8c0901e1..b97fbe441 100644 --- a/pil-analyzer/src/expression_processor.rs +++ b/pil-analyzer/src/expression_processor.rs @@ -4,8 +4,9 @@ use powdr_ast::{ parsed::{ self, asm::SymbolPath, types::Type, ArrayExpression, ArrayLiteral, BinaryOperation, BlockExpression, IfExpression, LambdaExpression, LetStatementInsideBlock, MatchArm, - MatchExpression, NamespacedPolynomialReference, Number, Pattern, SelectedExpressions, - StatementInsideBlock, SymbolCategory, UnaryOperation, + MatchExpression, NamedExpression, NamespacedPolynomialReference, Number, Pattern, + SelectedExpressions, StatementInsideBlock, StructExpression, SymbolCategory, + UnaryOperation, }, }; @@ -189,7 +190,28 @@ impl<'a, D: AnalysisDriver> ExpressionProcessor<'a, D> { self.process_block_expression(statements, expr, src) } PExpression::FreeInput(_, _) => panic!(), - PExpression::StructExpression(_, _) => unimplemented!("Structs are not supported yet"), + PExpression::StructExpression(src, StructExpression { name, fields }) => { + let type_args = name + .type_args + .map(|args| args.into_iter().map(|t| self.process_type(t)).collect()); + + Expression::StructExpression( + src, + StructExpression { + name: Reference::Poly(PolynomialReference { + name: self.driver.resolve_ref(&name.path, SymbolCategory::Struct), + type_args, + }), + fields: fields + .into_iter() + .map(|named_expr| NamedExpression { + name: named_expr.name, + body: Box::new(self.process_expression(*named_expr.body)), + }) + .collect(), + }, + ) + } } } diff --git a/pil-analyzer/src/lib.rs b/pil-analyzer/src/lib.rs index 325439022..9b56c9ebb 100644 --- a/pil-analyzer/src/lib.rs +++ b/pil-analyzer/src/lib.rs @@ -7,6 +7,7 @@ pub mod expression_processor; mod pil_analyzer; mod side_effect_checker; mod statement_processor; +mod structural_checks; mod traits_resolver; mod type_builtins; mod type_inference; diff --git a/pil-analyzer/src/pil_analyzer.rs b/pil-analyzer/src/pil_analyzer.rs index 442555e61..a55573de8 100644 --- a/pil-analyzer/src/pil_analyzer.rs +++ b/pil-analyzer/src/pil_analyzer.rs @@ -6,6 +6,7 @@ use std::iter::once; use std::path::{Path, PathBuf}; use std::sync::Arc; +use crate::structural_checks::check_structs_fields; use itertools::Itertools; use powdr_ast::parsed::asm::{ parse_absolute_path, AbsoluteSymbolPath, ModuleStatement, SymbolPath, @@ -14,14 +15,13 @@ use powdr_ast::parsed::types::Type; use powdr_ast::parsed::visitor::{AllChildren, Children}; use powdr_ast::parsed::{ self, FunctionKind, LambdaExpression, PILFile, PilStatement, SymbolCategory, - TraitImplementation, + TraitImplementation, TypedExpression, }; use powdr_number::{FieldElement, GoldilocksField}; use powdr_ast::analyzed::{ type_from_definition, Analyzed, DegreeRange, Expression, FunctionValueDefinition, PolynomialType, PublicDeclaration, Reference, StatementIdentifier, Symbol, SymbolKind, - TypedExpression, }; use powdr_parser::{parse, parse_module, parse_type}; use powdr_parser_util::Error; @@ -52,6 +52,7 @@ fn analyze(files: Vec) -> Result, Vec Result<(), Vec> { + let structs_exprs = self + .all_children() + .filter(|expr| matches!(expr, Expression::StructExpression(_, _))); + + check_structs_fields(structs_exprs, &self.definitions) + } + pub fn type_check(&mut self) -> Result<(), Vec> { let query_type: Type = parse_type("int -> std::prelude::Query").unwrap().into(); let mut expressions = vec![]; @@ -538,6 +547,40 @@ impl PILAnalyzer { } } +impl Children for PILAnalyzer { + fn children(&self) -> Box + '_> { + Box::new( + self.definitions + .values() + .filter_map(|(_, def)| def.as_ref()) + .flat_map(|def| def.children()) + .chain( + self.trait_impls + .values() + .flat_map(|impls| impls.iter()) + .flat_map(|impl_| impl_.children()), + ) + .chain(self.proof_items.iter()), + ) + } + + fn children_mut(&mut self) -> Box + '_> { + Box::new( + self.definitions + .values_mut() + .filter_map(|(_, def)| def.as_mut()) + .flat_map(|def| def.children_mut()) + .chain( + self.trait_impls + .values_mut() + .flat_map(|impls| impls.iter_mut()) + .flat_map(|impl_| impl_.children_mut()), + ) + .chain(self.proof_items.iter_mut()), + ) + } +} + #[derive(Clone, Copy)] struct Driver<'a>(&'a PILAnalyzer); diff --git a/pil-analyzer/src/structural_checks.rs b/pil-analyzer/src/structural_checks.rs new file mode 100644 index 000000000..bc327750a --- /dev/null +++ b/pil-analyzer/src/structural_checks.rs @@ -0,0 +1,110 @@ +use itertools::Itertools; +use std::collections::{HashMap, HashSet}; + +use powdr_ast::{ + analyzed::{Expression, FunctionValueDefinition, PolynomialReference, Reference, Symbol}, + parsed::{StructExpression, TypeDeclaration}, +}; +use powdr_parser_util::{Error, SourceRef}; + +/// Verifies that all struct instantiations match their corresponding declarations +/// (existence of field names, completeness) and ensures that both are correct. +pub fn check_structs_fields<'a>( + structs_exprs: impl Iterator, + definitions: &HashMap)>, +) -> Result<(), Vec> { + let mut errors = Vec::new(); + + for expr in structs_exprs { + let Expression::StructExpression( + source, + StructExpression { + name: Reference::Poly(PolynomialReference { name, .. }), + fields, + }, + ) = expr + else { + unreachable!() + }; + + errors.extend(check_struct_expression(definitions, name, fields, source)); + } + + errors.extend(check_struct_declarations(definitions)); + + if errors.is_empty() { + Ok(()) + } else { + Err(errors) + } +} + +fn check_struct_expression( + definitions: &HashMap)>, + name: &String, + fields: &[powdr_ast::parsed::NamedExpression>>], + source: &SourceRef, +) -> Vec { + let mut errors = Vec::new(); + if let ( + _, + Some(FunctionValueDefinition::TypeDeclaration(TypeDeclaration::Struct(struct_decl))), + ) = definitions.get(name).unwrap() + { + let declared_fields: HashSet<_> = struct_decl.fields.iter().map(|f| &f.name).collect(); + let used_fields: HashSet<_> = fields.iter().map(|f| &f.name).collect(); + + errors.extend( + fields + .iter() + .filter(|f| !declared_fields.contains(&f.name)) + .map(|f| { + source.with_error(format!("Struct '{name}' has no field named '{}'", f.name)) + }), + ); + + errors.extend(declared_fields.difference(&used_fields).map(|&f| { + source.with_error(format!("Missing field '{f}' in initializer of '{name}'",)) + })); + + let duplicate_fields: Vec<_> = fields.iter().map(|f| &f.name).duplicates().collect(); + + errors.extend( + duplicate_fields + .into_iter() + .map(|f| source.with_error(format!("Field '{f}' specified more than once"))), + ); + } else { + panic!("Struct '{name}' has not been declared"); + } + + errors +} + +fn check_struct_declarations( + definitions: &HashMap)>, +) -> Vec { + let mut errors = Vec::new(); + for (symbol, def) in definitions.values() { + let Some(FunctionValueDefinition::TypeDeclaration(TypeDeclaration::Struct(struct_decl))) = + def + else { + continue; + }; + + let duplicate_declaration_fields: Vec<_> = struct_decl + .fields + .iter() + .map(|f| &f.name) + .duplicates() + .collect(); + + errors.extend(duplicate_declaration_fields.into_iter().map(|f| { + symbol + .source + .with_error(format!("Field '{f}' is declared more than once")) + })); + } + + errors +} diff --git a/pil-analyzer/tests/types.rs b/pil-analyzer/tests/types.rs index 74c421672..89e947f92 100644 --- a/pil-analyzer/tests/types.rs +++ b/pil-analyzer/tests/types.rs @@ -762,6 +762,78 @@ fn prover_functions() { type_check(input, &[("a", "", "int"), ("b", "", "()[]")]); } +#[test] +#[should_panic = "Struct symbol not found: NotADot"] +fn wrong_struct() { + let input = " + struct Dot { x: int, y: int } + let f: int -> Dot = |i| NotADot{x: 0, y: i}; + "; + type_check(input, &[]); +} + +#[test] +#[should_panic = "Struct 'Dot' has no field named 'a'"] +fn struct_wrong_fields() { + let input = " + struct Dot { x: int, y: int } + let f: int -> Dot = |i| Dot{x: 0, y: i, a: 2}; + let x = f(0); + "; + type_check(input, &[]); +} + +#[test] +#[should_panic = "Missing field 'z' in initializer of 'A'"] +fn test_struct_unused_fields() { + let input = " struct A { + x: int, + y: int, + z: int, + } + let x = A{ y: 0, x: 2 }; +"; + + type_check(input, &[]); +} + +#[test] +#[should_panic = "Field 'y' specified more than once"] +fn test_struct_repeated_fields_expr() { + let input = " struct A { + x: int, + y: int, + } + let x = A{ y: 0, x: 2, y: 1 }; +"; + + type_check(input, &[]); +} + +#[test] +#[should_panic = "Field 'x' is declared more than once"] +fn test_struct_repeated_fields_decl() { + let input = " struct A { + x: int, + y: int, + x: int, + } + let x = A{ y: 0, x: 2 }; +"; + + type_check(input, &[]); +} + +#[test] +#[should_panic(expected = "Expected symbol of kind Struct but got Type: A")] +fn enum_used_as_struct() { + let input = " + enum A { X } + let a = A{x: 8}; + "; + type_check(input, &[]); +} + #[test] fn typed_literals() { let input = "