Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: DSL semantic checking analysis #24

Merged
merged 71 commits into from
Feb 11, 2025
Merged

feat: DSL semantic checking analysis #24

merged 71 commits into from
Feb 11, 2025

Conversation

AlSchlo
Copy link
Collaborator

@AlSchlo AlSchlo commented Feb 11, 2025

Problem

We want to reject DSL programs that are semantically incorrect. This PR analyzes the parsed AST and rejects if an error is detected.

Summary of changes

  • Parser bug fixes
  • Implemented SemanticAnalyzer to perform comprehensive static analysis on DSL programs
  • Added validation for properties:
    • Ensures property types are valid (basic types, maps, arrays, tuples)
    • Rejects function types and operators in properties
  • Added validation for operators:
    • Prevents duplicate operator names
    • Validates field types for both scalar and logical operators
    • Ensures logical operators have all required derived properties
    • Verifies derived properties match declared logical properties
  • Added scope-based identifier validation:
    • Maintains a stack of scopes for variable declarations
    • Prevents duplicate identifiers in the same scope
    • Validates variable references exist in accessible scopes
  • Added recursive expression validation:
    • Validates all sub-expressions in compound expressions
    • Handles various expression types (variables, function calls, conditionals, etc.)
    • Manages scope for closures and pattern matching
  • Added comprehensive test suite:
    • Tests for valid DSL programs
    • Tests for various error conditions (duplicate names, invalid types, etc.)
    • Tests for scope handling and identifier validation

The semantic analyzer provides early error detection for DSL programs, helping users identify issues before runtime.

@AlSchlo AlSchlo changed the title feat: semantic checking on DSL feat: DSL semantic checking analysis Feb 11, 2025
@codecov-commenter
Copy link

codecov-commenter commented Feb 11, 2025

Codecov Report

Attention: Patch coverage is 95.10086% with 17 lines in your changes missing coverage. Please review.

Project coverage is 80.4%. Comparing base (baa09fc) to head (e5ed19b).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
optd-core/src/dsl/analyzer/semantic.rs 95.0% 17 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
optd-core/src/dsl/parser/expr.rs 86.3% <ø> (ø)
optd-core/src/dsl/parser/functions.rs 90.3% <ø> (ø)
optd-core/src/dsl/parser/mod.rs 96.9% <100.0%> (ø)
optd-core/src/dsl/parser/operators.rs 90.4% <ø> (ø)
optd-core/src/dsl/parser/patterns.rs 83.0% <ø> (ø)
optd-core/src/dsl/parser/types.rs 89.3% <ø> (ø)
optd-core/src/dsl/analyzer/semantic.rs 95.0% <95.0%> (ø)

Copy link
Member

@skyzh skyzh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally LGTM

@@ -0,0 +1,148 @@
// Shared properties for Logical operators
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dup w/ parser/programs?

let result = analyzer.validate_file(&file);
assert!(result.is_err());
assert_eq!(
result.unwrap_err(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make it user friendly in the future, we probably need to raise the compiler error at the site where the error occurs, for example, Line X Column Y, (something like a source code span could help, pest should support that)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I was planning to add that in the future. Usually you add that in the AST annotations.

@AlSchlo AlSchlo force-pushed the alexis/dsl-analyzer branch from a884974 to bf92829 Compare February 11, 2025 07:45
@AlSchlo AlSchlo merged commit 44d26e2 into main Feb 11, 2025
12 checks passed
@AlSchlo AlSchlo deleted the alexis/dsl-analyzer branch February 11, 2025 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants