refactor: Replace direct expression building with analysis framework and AST-based compilation#16
Merged
Conversation
…eware - Added VariableScope class to manage named variables and support lexical scoping with shadowing. - Introduced Interpreter and InterpreterBuilder classes for orchestrating middleware and AST transformations. - Created ReferenceEqualityComparer for reference-based equality checks. - Developed SemanticAnalysisMiddleware to enrich AST nodes with semantic information. - Added SemanticExtensions for managing semantic analysis data in InterpretationContext. - Implemented custom transformer registry for handling specific node patterns. - Defined transformation pipeline interfaces and delegates for middleware processing. - Created LinqExpressionTransformer to convert AST nodes into LINQ expression trees. - Added TerminalTransformMiddleware to delegate transformations to the final transformer. - Established a transformation result cache to optimize repeated computations.
There was a problem hiding this comment.
Pull request overview
This PR implements a major architectural refactoring that introduces a middleware-based interpreter with semantic analysis capabilities, replacing the direct Value.BuildExpression() pattern with a composable transformation pipeline.
Changes:
- Introduces abstract syntax tree (AST) nodes as immutable records replacing the
Valueclass hierarchy - Implements middleware transformation pipeline with
ITransformationMiddleware<TResult>,ITransformer<TResult>, andLinqExpressionTransformer - Adds semantic analysis middleware that enriches nodes with type and member information stored in
InterpretationContext - Creates variable scope management with
VariableScopesupporting lexical scoping and shadowing - Establishes transformation result caching and custom transformer registry for domain-specific patterns
Reviewed changes
Copilot reviewed 144 out of 144 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| Poly/Interpretation/AbstractSyntaxTree/* | New immutable AST node records (Node, Constant, Parameter, Variable, operators) |
| Poly/Interpretation/TransformationPipeline/* | Middleware infrastructure (ITransformer, ITransformationMiddleware, LinqExpressionTransformer, TerminalTransformMiddleware) |
| Poly/Interpretation/SemanticAnalysis/* | Semantic analysis middleware and extensions for type/member resolution |
| Poly/Interpretation/ReferenceEqualityComparer.cs | Reference-based equality comparer for caching |
| Poly/Interpretation/TransformationResultCache.cs | Pipeline-level cache for transformation results |
| Poly/Interpretation/Interpreter*.cs | Interpreter and InterpreterBuilder for middleware orchestration |
| Poly/Validation/* | Updated to use Node instead of Value in rules and constraints |
| Poly/Introspection/CommonLanguageRuntime/* | Updated member accessors to use Node and Transform pattern |
| Poly/GlobalUsings.cs | Alias System.Linq.Expressions to avoid naming conflicts |
| Poly.Tests/* | Test updates with NodeTestHelpers and proper middleware usage |
Comment on lines
+272
to
+279
| foreach (var param in _parameters) | ||
| { | ||
| var expr = param.BuildNode(this); | ||
| if (expr is Exprs.ParameterExpression paramExpr) | ||
| { | ||
| yield return paramExpr; | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
Comment on lines
363
to
364
| var valueLiteral = Wrap(value); | ||
| var accessor = addMethod!.GetMemberAccessor(listLiteral, [valueLiteral]); |
There was a problem hiding this comment.
This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
Suggested change
| var valueLiteral = Wrap(value); | |
| var accessor = addMethod!.GetMemberAccessor(listLiteral, [valueLiteral]); | |
| var accessor = addMethod!.GetMemberAccessor(listLiteral, [Wrap(value)]); |
| var paramExpr = param.BuildExpression(context); | ||
| var lambda = Expression.Lambda<Func<T, TResult>>(expression, paramExpr); | ||
| var expression = expr.BuildNode(context); | ||
| var paramExpr = param.ToParameterExpression(); |
There was a problem hiding this comment.
Call to obsolete method ToParameterExpression.
| var paramExpr2 = param2.BuildExpression(context); | ||
| var lambda = Expression.Lambda<Func<T1, T2, TResult>>(expression, paramExpr1, paramExpr2); | ||
| var expression = expr.BuildNode(context); | ||
| var paramExpr1 = param1.ToParameterExpression(); |
There was a problem hiding this comment.
Call to obsolete method ToParameterExpression.
| var lambda = Expression.Lambda<Func<T1, T2, TResult>>(expression, paramExpr1, paramExpr2); | ||
| var expression = expr.BuildNode(context); | ||
| var paramExpr1 = param1.ToParameterExpression(); | ||
| var paramExpr2 = param2.ToParameterExpression(); |
There was a problem hiding this comment.
Call to obsolete method ToParameterExpression.
| [Obsolete("Use the middleware interpreter instead.")] | ||
| public IEnumerable<Exprs.ParameterExpression> GetParameterNodes() | ||
| { | ||
| var transformer = new LinqExpressionTransformer(); |
There was a problem hiding this comment.
This assignment to transformer is useless, since its value is never read.
| var expression = expr.BuildExpression(context); | ||
| var paramExpr = param.BuildExpression(context); | ||
| var lambda = Expression.Lambda<Func<T, TResult>>(expression, paramExpr); | ||
| var expression = expr.BuildNode(context); |
There was a problem hiding this comment.
This assignment to expression is useless, since its value is never read.
| var paramExpr1 = param1.BuildExpression(context); | ||
| var paramExpr2 = param2.BuildExpression(context); | ||
| var lambda = Expression.Lambda<Func<T1, T2, TResult>>(expression, paramExpr1, paramExpr2); | ||
| var expression = expr.BuildNode(context); |
There was a problem hiding this comment.
This assignment to expression is useless, since its value is never read.
| // Push scope and declare 'y' within a block's scope | ||
| context.PushScope(); | ||
| var y = context.DeclareVariable("y", Value.Wrap(10)); | ||
| var y = context.DeclareVariable("y", Wrap(10)); |
Comment on lines
+272
to
+279
| if (leftType != null && rightType != null) | ||
| { | ||
| if (leftType.ReflectedType != rightType.ReflectedType) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"Type mismatch in {node.GetType().Name}: " + | ||
| $"{leftType.Name} and {rightType.Name} are incompatible"); | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
Suggested change
| if (leftType != null && rightType != null) | |
| { | |
| if (leftType.ReflectedType != rightType.ReflectedType) | |
| { | |
| throw new InvalidOperationException( | |
| $"Type mismatch in {node.GetType().Name}: " + | |
| $"{leftType.Name} and {rightType.Name} are incompatible"); | |
| } | |
| if (leftType != null && | |
| rightType != null && | |
| leftType.ReflectedType != rightType.ReflectedType) | |
| { | |
| throw new InvalidOperationException( | |
| $"Type mismatch in {node.GetType().Name}: " + | |
| $"{leftType.Name} and {rightType.Name} are incompatible"); |
…ation rules - Deleted ClrTypeFieldInterpretationAccessor, ClrTypeIndexInterpretationAccessor, and ClrTypePropertyInterpretationAccessor as they are no longer needed. - Updated ITypeMember interface by removing the GetMemberAccessor method. - Refactored TypeDefinitionProviderCollection to implement ICollection<ITypeDefinitionProvider> for better collection management. - Cleaned up validation builders by removing unnecessary references to Poly.Interpretation.AbstractSyntaxTree. - Introduced new interfaces and classes for transformation middleware and semantic analysis, enhancing the interpretation pipeline. - Added LinqExpressionMiddleware for compiling AST nodes into LINQ expressions. - Implemented semantic analysis extensions for managing type and member resolution during interpretation.
…eference for type handling - Introduced unit tests for arithmetic operations (Add, Subtract, Multiply, Divide, Modulo) in ArithmeticNodeTests. - Added ConditionalNodeTests for testing conditional expressions and coalescing behavior. - Created ConstantNodeTests to validate constant expressions of various types. - Implemented ParameterNodeTests to ensure parameter expressions compile and execute correctly. - Developed InterpreterIntegrationTests for complex multi-node expressions and integration scenarios. - Added TypeReference class to facilitate type handling in expressions. - Commented out unused code in RuleBuildingContext for clarity.
…line type definition resolution
… parameter management
…interpretation context management
- Updated various test files to replace manual expression building and compilation with the new CompileLambda method for cleaner and more maintainable code. - Modified ConditionalTests, InterpreterIntegrationTests, ModuloTests, NumericTypePromotionTests, ParameterNodeTests, TypeCastTests, and UnaryMinusTests to streamline the compilation process. - Enhanced Block and Coalesce node implementations to support new constructor patterns. - Improved semantic analysis middleware to handle variable types and block expressions more effectively. - Adjusted RuleBuildingContext and RuleSet to simplify the interpretation tree building process.
… enhanced metadata management
…onPass for improved analysis and type resolution
…n metadata handling
…th new resolver implementations
…c analysis processes across AST nodes
…, DoWhile, For, Goto, If, Label, Return, Switch, Throw, TryCatchFinally, Using, and While constructs
…es and introduce LinqExpressionGenerator for AST to LINQ expression compilation
…le, DoWhile, For, Break, Continue, Return, TryCatchFinally, and Using statements in LinqExpressionGenerator
… diagnostic reporting methods
…amline node analysis process
…ion system - Deleted `InterpreterBuilder` class, which was responsible for building interpreters with middleware. - Removed `LinqExpressionMiddleware`, which compiled AST nodes to LINQ expressions. - Eliminated `LinqExpressionMiddlewareExtensions` for adding LINQ expression compilation middleware. - Removed `ISemanticInfoProvider` interface and its implementation, which provided semantic information about AST nodes. - Deleted `SemanticAnalysisExtensions` for accessing and storing semantic analysis information. - Removed `SemanticAnalysisMiddleware`, which enriched AST nodes with semantic information. - Deleted `TransformationDelegate` delegate type used in the middleware pipeline. - Removed `DelegateTransformationMiddleware`, which wrapped transformation functions for middleware.
…ate member resolution to use NodeId
… management and node analysis
…lysisMetadata constraint
…peResolutionMetadata with record struct
…d AST visualization - Implemented `ArithmeticParserEvaluatorTests` to cover various arithmetic operations, operator precedence, and error handling. - Created `MermaidAstVisualizationTests` to validate the generation of Mermaid diagrams from AST nodes, including simple and complex expressions. - Developed `MermaidAstGenerator` to produce Mermaid markdown for visualizing AST structures, supporting various node types and shapes. - Added examples and usage notes in `README.md` for the Mermaid AST generator. - Updated `MiddlewareInterpreterIntegrationTests` to improve type mismatch detection during code generation.
…ved metadata handling
…ation with enhanced node handling
…or improved metadata handling
- Implemented Control Flow Analysis with a new ControlFlowAnalysisPass that builds a control flow graph (CFG) from the AST and performs reachability analysis. - Introduced BasicBlock and ControlFlowGraph classes to represent and manage the structure of the CFG. - Added support for various control flow constructs including if statements, loops, and switch statements. - Implemented dead code detection and reporting for unreachable statements in the CFG. - Created ConstantFoldingPass to evaluate constant expressions at analysis time, optimizing the AST by folding constant operations. - Added unit tests for control flow analysis to ensure correctness of CFG generation and dead code detection.
…rences - Added `OptionalTypeReference` to represent nullable types in the AST. - Introduced `PrimitiveTypeReference` for referencing primitive types by ID, including nullable support. - Created `PropertyDefinitionNode` for defining properties in types, with support for default values and constraints. - Implemented `TypeDefinitionNode` to represent type definitions, including properties, methods, fields, and base types. - Developed `TypeDefinitionNodeAnalyzer` to extract and store type definitions from the AST. - Enhanced type resolution with support for new type references in `TypeResolver`. - Added `PrimitiveTypeId` and `TypeCategory` enums for better type classification. - Updated constraints to include applicability checks based on type categories. - Introduced `ConstraintApplicabilityException` for handling constraint application errors. - Enhanced LINQ expression generation with support for custom node compilers.
…ditional methods and improved documentation
…esolution into a single method
- Added DataPropertyPath for managing property paths. - Created DataType to define data types with properties, rules, and mutations. - Implemented DataTypeBuilder for constructing DataType instances fluently. - Developed DataTypeValidator to validate instances against defined rules. - Introduced DataModelMemberAccessAnalyzer for transforming member accesses in data models. - Added DataModelPropertyAccessor for accessing properties in dynamic objects. - Implemented DataTypeDefinition to represent data types in the introspection system. - Created mutation-related classes including Mutation, MutationParameter, and various effects. - Added validation classes for rule evaluation and error handling. - Introduced type expressions for modeling various data types including collections, enums, and maps. - Updated README to reflect changes in the data modeling structure.
Owner
Author
|
@copilot Re-review this PR and suggest a better Title. |
scoizzle
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…eware