Document Laurel passes and their dependencies, and other documentation updates#1222
Document Laurel passes and their dependencies, and other documentation updates#1222keyboardDrummer wants to merge 24 commits into
Conversation
MikaelMayer
left a comment
There was a problem hiding this comment.
🤖🔍 Good refactoring — extracting pass definitions to their respective modules and making the pipeline declarative is a clear improvement. The comesBefore dependency tracking is a nice addition for documentation generation. A few nits below.
MikaelMayer
left a comment
There was a problem hiding this comment.
🤖🔍 All previous comments appear addressed — reviewer sign-off still needed.
MikaelMayer
left a comment
There was a problem hiding this comment.
🤖🔍 All previous comments appear addressed — reviewer sign-off still needed.
MikaelMayer
left a comment
There was a problem hiding this comment.
🤖🔍 All previous comments appear addressed — reviewer sign-off still needed.
|
@keyboardDrummer-bot please resolve merge conflicts |
MikaelMayer
left a comment
There was a problem hiding this comment.
🤖🔍 All previous comments appear addressed — reviewer sign-off still needed.
- Remove import of deleted Strata.Languages.Python.PythonLaurelCorePrelude (moved to StrataPython package) - Replace import of deleted Strata.Languages.Laurel.Laurel with LaurelAST - Remove circular import of LaurelTypes from Resolution; inline type lookup for diamond field access check - Restore missing pass imports in HeapParameterization, EliminateValueInReturns, and InferHoleTypes
MikaelMayer
left a comment
There was a problem hiding this comment.
🤖🔍 All previous comments appear addressed — reviewer sign-off still needed.
| -/ | ||
| private def checkDiamondFieldAccess (model : SemanticModel) (target : StmtExprMd) | ||
| (fieldName : Identifier) (source : Option FileRange) : List DiagnosticModel := | ||
| let targetType := match target.val with |
There was a problem hiding this comment.
Restore the computeExprType-based target typing, or add a regression test that pins this narrowed coverage.
Why
This block replaces computeExprType model target (the prior implementation in TypeHierarchy.lean) with a 4-case approximation. computeExprType (LaurelTypes.lean:39) also resolves .AsType casts, .StaticCall/.InstanceCall results, .PureFieldUpdate, .Block, and .IfThenElse; those now fall to .Unknown and are skipped, so (b as Bottom)#xValue and getBottom()#xValue no longer error where they did before this PR.
The narrowing is forced by the move into Resolution.lean: LaurelTypes.lean publicly imports Resolution, so Resolution cannot call computeExprType without a cycle. Either keep validateDiamondFieldAccesses in a module downstream of LaurelTypes and run it as the first pipeline pass, or — if the narrowing is intended — pin it with a test, e.g. extend T5 with a cast target:
b2 := (b as Bottom)#xValue // errored before this PR; silently accepted now
Either way "refactoring, no additional tests needed" doesn't hold — the set of rejected programs changed.
There was a problem hiding this comment.
Nice find, thanks. I brought back the original computeExprType model target call.
| errors := finalState.errors | ||
| model := semanticModel, | ||
| errors := finalState.errors ++ diamondErrors | ||
| } |
There was a problem hiding this comment.
Confirm diamond-access errors should now be suppressed when emitResolutionErrors := false.
Why
Before this PR validateDiamondFieldAccesses ran in runLaurelPasses and its result was added to allDiags unconditionally. Now the errors are folded into ResolutionResult.errors, which the pipeline keeps only via resolutionErrors := if options.emitResolutionErrors then result.errors.toList else [] (LaurelCompilationPipeline.lean:128). The field defaults to true, so in-tree tests are unaffected, but any caller passing false loses diamond diagnostics it previously received. If diamond errors are intended as resolution-phase errors this is fine — worth a line in the PR description.
There was a problem hiding this comment.
I've removed the emitResolutionErrors option now so it's always true.
| run := fun p m => | ||
| let (p', diags) := constrainedTypeElim m p | ||
| (p', diags, {}) } | ||
| def laurelPipeline : Array LaurelPass := #[ |
There was a problem hiding this comment.
Add a #guard that laurelPipeline satisfies every declared comesBefore, so reordering can't silently violate a constraint.
Why
comesBefore is consumed only by the doc generator; nothing checks it against the actual order. The line-99 resolution ("whoever reorders and runs into an issue will be prompted") only holds if something prompts them — this is that mechanism, and it also catches a comesBefore target that isn't in the pipeline at all.
/-- Every `comesBefore` constraint is respected by the pipeline order. -/
def comesBeforeRespected : Bool :=
let names := laurelPipeline.toList.map (·.name)
laurelPipeline.toList.enum.all fun (i, p) =>
p.comesBefore.all fun cb =>
match names.findIdx? (· == cb.pass.name) with
| some j => i < j
| none => false -- target not in laurelPipeline
#guard comesBeforeRespectedCompare by name since LaurelPass has a function field and thus no DecidableEq. Independent of the String-vs-struct discussion on LaurelPass.lean.
There was a problem hiding this comment.
I've added the snippet you pasted, thanks.
…peline list
TypeAliasElim runs after the initial resolve and before the numbered
Laurel-to-Laurel passes; surface it so the documented order is complete.
Correct step 7: EliminateHoles eliminates only deterministic holes;
non-deterministic ones are lowered to havocs later by LiftExpressionAssignments.
This hand-maintained list is a candidate for replacement by the auto-generated
{laurelPipelineDocs} block from PR #1222 once that lands.
Addresses review comments: copilot LaurelDoc.lean:287, copilot LaurelDoc.lean:281.
Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
Adding or reordering a Laurel-to-Laurel pass must update the Pipeline order section and the manual's pass list. Note PR #1222 supersedes these hand-maintained lists with auto-generated documentation. Addresses review comment: olivier-aws laurel-development.md:81. Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
MikaelMayer
left a comment
There was a problem hiding this comment.
🤖🔍 All previous comments appear addressed — reviewer sign-off still needed.
MikaelMayer
left a comment
There was a problem hiding this comment.
🤖🔍 All previous comments appear addressed — reviewer sign-off still needed.
| | some j => i < j | ||
| | none => false -- target not in laurelPipeline | ||
|
|
||
| #guard comesBeforeRespected |
There was a problem hiding this comment.
Make comesBeforeRespected evaluate at elaboration — this #guard currently fails to build (CI red on both Build and test Lean and Build documentation).
Why + verified fix
#guard runs in the interpreter, which has no IR for the cross-module LaurelPass values laurelPipeline holds:
(interpreter) IR of declaration 'Strata.Laurel.constrainedTypeElimPass' not available;
this may point to a missing `meta` check in a metaprogram
decide gets stuck (kernel can't reduce the Array/String operations) and native_decide hits the same IR gap. Two fixes that build:
meta importthe pass-defining modules in this file (DesugarShortCircuit,MergeAndLiftReturns,EliminateValueInReturns,ModifiesClauses,HeapParameterization,TypeHierarchy,InferHoleTypes,EliminateDeterministicHoles,LiftImperativeExpressions,ConstrainedTypeElim,TypeAliasElim). I verifiedlake buildthen succeeds (194 jobs).- Or move
comesBeforeRespected+ the#guardinto aStrataTestmodule that doesmeta import all Strata.Languages.Laurel.LaurelCompilationPipeline(the patternLiftHolesTest.leanalready uses), keepingmeta imports out of the core library.
Build documentation fails for the same reason — LaurelDoc.lean imports this module.
| let result := resolve program | ||
| let resolutionErrors : List DiagnosticModel := | ||
| if options.emitResolutionErrors then result.errors.toList else [] | ||
| let resolutionErrors : List DiagnosticModel := result.errors.toList |
There was a problem hiding this comment.
Remove the now-dead emitResolutionErrors field at LaurelToCoreTranslator.lean:619.
Why
This line was its last reader. After this commit, grep -rn emitResolutionErrors Strata/ matches only the field definition (emitResolutionErrors : Bool := true), so it is dead config.
Changes
Update Laurel documentation based on inspiration from #1144 and by making the dependencies between passes explicit
Testing
Refactoring, no additional tests needed