Allow for evaluation of modules in places where expressions (but not …#506
Allow for evaluation of modules in places where expressions (but not …#506julialongtin wants to merge 4 commits intorefactor_module_callingfrom
Conversation
…sub-expressions) are allowed.
There was a problem hiding this comment.
Pull request overview
This PR extends the ExtOpenScad evaluator so that eligible module calls can be evaluated in contexts where a full expression is expected (notably the RHS of assignment), by teaching evalExpr to detect and execute module-valued applications. It also factors module-call utilities into a dedicated evaluator module.
Changes:
- Add
Graphics.Implicit.ExtOpenScad.Eval.Moduleand move module-call helpers there. - Update
evalExprto execute eligible module calls when the top-level expression is an application whose callee evaluates to a module. - Add
OTypeMirrorinstances forSymbolicObj2/SymbolicObj3and update the changelog/cabal module list.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| implicit.cabal | Registers the new Eval.Module module in the library build. |
| Graphics/Implicit/ExtOpenScad/Util/OVal.hs | Adds OTypeMirror instances for SymbolicObj2 and SymbolicObj3. |
| Graphics/Implicit/ExtOpenScad/Eval/Statement.hs | Refactors suite/module helper usage and (re)introduces local evalSuite. |
| Graphics/Implicit/ExtOpenScad/Eval/Module.hs | New module encapsulating module-call option/instance validation and execution helpers. |
| Graphics/Implicit/ExtOpenScad/Eval/Expr.hs | Implements top-level module-call evaluation in expression evaluation. |
| Graphics/Implicit/ExtOpenScad/Default.hs | Updates copyright header year range. |
| CHANGELOG.md | Documents the new module-calling-in-expression capability. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| evalSuite varlookup sourcePos suite = do | ||
| vals <- runSuiteCapture varlookup suite | ||
| when (null vals) (errorC sourcePos "Suite required, but none provided.") | ||
| runSuiteCapture varlookup suite |
There was a problem hiding this comment.
evalSuite executes runSuiteCapture twice, which will run the suite twice (duplicating side effects and messages) and can return inconsistent results. Capture the result once, error if it's empty, and return the captured value instead of re-running the suite.
| runSuiteCapture varlookup suite | |
| pure vals |
| import Data.Text.Lazy as DTL (concat, intercalate) | ||
|
|
There was a problem hiding this comment.
Data.Text.Lazy is imported qualified as DTL but only referenced inside a commented-out block, so it is unused in compiled code and will trigger an unused-imports warning. Remove the DTL import (or re-enable/use the code that needs it).
| import Data.Text.Lazy as DTL (concat, intercalate) |
| -- | The entry point from StateC. Evaluates either an expression or an eligible module call. | ||
| evalExpr :: SourcePosition -> Expr -> StateC OVal | ||
| evalExpr pos expr = do | ||
| evalExpr sourcePos expr = case expr of | ||
| (maybeMod :$ argExprs) -> do | ||
| -- Yes, we're recursing, after dropping argument expressions, for the OVal | ||
| rVal <- evalExpr sourcePos maybeMod | ||
| if isModule rVal | ||
| then do | ||
| -- Perform a module call. | ||
| res <- runExprModule sourcePos rVal argExprs | ||
| pure $ canonicalizeRes $ OList res | ||
| else | ||
| -- Evaluate expression. | ||
| evalExprStateC sourcePos expr | ||
| _ -> evalExprStateC sourcePos expr |
There was a problem hiding this comment.
This change adds support for evaluating eligible module calls at the top-level expression boundary. There are existing ExecSpec tests for expression evaluation (tests/ExecSpec/Expr.hs), but none appear to cover module-in-expression behavior (e.g., cube(1) on the RHS of runExpr, and error cases for modules requiring suites). Adding an execution test here would prevent regressions.
| * Added `projection(cut=true)` support [#448](https://github.com/Haskell-Things/ImplicitCAD/pull/448) | ||
| * Added `polyhedron()` support [#497](https://github.com/Haskell-Things/ImplicitCAD/pull/497) | ||
| * Added `import()` support [#505](https://github.com/Haskell-Things/ImplicitCAD/pull/505) | ||
| * Improved syntax for module calling: now allows module calls in place of expressions []() |
There was a problem hiding this comment.
Changelog entry has an empty markdown link [](). Please link to the PR/issue for this change (or remove the link markup) to keep the changelog consistent with surrounding entries.
| * Improved syntax for module calling: now allows module calls in place of expressions []() | |
| * Improved syntax for module calling: now allows module calls in place of expressions |
| -- We can't handle any suites, either. | ||
| _ <- case mod of | ||
| (OUModule _ _ _) -> pure mempty :: StateC () | ||
| (ONModule _ _ _) -> pure mempty | ||
| (ONModuleWithSuite _ _ _) -> noSuiteError | ||
| (OVargsModule _ _) -> noSuiteError | ||
| _ -> notModError | ||
|
|
There was a problem hiding this comment.
In runExprModule, modules that require suites (ONModuleWithSuite / OVargsModule) will hit noSuiteError in the pre-check and then again in the later case mod of dispatch, producing duplicate error messages. Consider returning early after the first noSuiteError (or removing the pre-check and handling it only in the dispatch).
| -- We can't handle any suites, either. | |
| _ <- case mod of | |
| (OUModule _ _ _) -> pure mempty :: StateC () | |
| (ONModule _ _ _) -> pure mempty | |
| (ONModuleWithSuite _ _ _) -> noSuiteError | |
| (OVargsModule _ _) -> noSuiteError | |
| _ -> notModError |
…sub-expressions) are allowed.