feat(compiler): improve constraints, guards, and codegen#116
Merged
Conversation
Valid() typechecked but emitted `false` at runtime, so every ensure
using it failed silently after passing the compiler. Reject with an
explicit diagnostic and drop Valid from the builtin constraint allowlist.
Example:
\`\`\`forst
func save(email String) {
ensure email is Valid() // error: reserved placeholder
}
\`\`\`
Co-authored-by: Cursor <cursoragent@cursor.com>
Drop the always-false Valid handler now that typecheck rejects Valid(). Lazy-init builtin constraint handlers via sync.Once to avoid init cycles after removing the Valid entry. Add constraintArgAsExpr helper for lowering constraint arguments (used by if-is codegen next). Co-authored-by: Cursor <cursoragent@cursor.com>
Successor narrowing works for simple identifiers; compound field paths remain deferred. Shapes/ensure marked experimental where docs overclaimed boundary validation. Fix ensure-if-ok Go tab to match Result lowering. Co-authored-by: Cursor <cursoragent@cursor.com>
Register example tasks for union_error_narrowing.ft and map_catalog.ft. Skip anonymous_objects.ft in the pipeline until array literal parsing lands. Co-authored-by: Cursor <cursoragent@cursor.com>
Delegate single-constraint if-is conditions to TransformBuiltinConstraint,
matching ensure lowering. Unblocks type guards using dynamic bounds.
Example:
\`\`\`forst
is (password Password) Strong(min Int) {
if password is Min(min) {
println("ok")
}
}
\`\`\`
Co-authored-by: Cursor <cursoragent@cursor.com>
Implement MapLiteral printing for forst fmt round-trip on map values.
Example:
\`\`\`forst
m := map[String]Int{ "a": 1, "b": 2 }
\`\`\`
Co-authored-by: Cursor <cursoragent@cursor.com>
Parse `label: for` loops, validate labeled break/continue against an
enclosing loop label stack, and emit goast.LabeledStmt in Go output.
Example:
\`\`\`forst
outer:
for i := 0; i < 10; i++ {
break outer
}
\`\`\`
Co-authored-by: Cursor <cursoragent@cursor.com>
ValidateShapeGuard accepts an optional resolver so typedefs like
type MutationArg = Shape are treated as shape subjects.
Example:
\`\`\`forst
type MutationArg = Shape
is (m MutationArg) Input(input Shape) {
ensure m is { input }
}
\`\`\`
Co-authored-by: Cursor <cursoragent@cursor.com>
Sort Defs keys before emission so repeated builds produce stable Go output and golden tests stop flapping. Co-authored-by: Cursor <cursoragent@cursor.com>
Accept Array(T) with inline structural element types in shape field
annotations, enabling nested anonymous object params.
Example:
\`\`\`forst
func process(order { items: Array({ id: String, quantity: Int }) }) {}
\`\`\`
Co-authored-by: Cursor <cursoragent@cursor.com>
Register destructured field names from shape param types, typecheck
uses, and emit separate Go parameters for { input, ctx }: Type.
Example:
\`\`\`forst
func handler({ input, ctx }: { input: String, ctx: Int }) {
println(input)
}
\`\`\`
Co-authored-by: Cursor <cursoragent@cursor.com>
Stop skipping guards whose body is only ensure m is { field }. Emit
stub G_* functions returning true and lower is constraints to true
until full runtime field checks land.
Example:
\`\`\`forst
is (m MutationArg) Input(input Shape) {
ensure m is { input }
}
\`\`\`
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Warning Review limit reached
More reviews will be available in 47 minutes and 19 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
WalkthroughThis PR expands the Forst compiler with labeled for-loops (parse, typecheck, emit, print), full destructured-parameter support across typechecker and Go transformer, ChangesForst compiler multi-feature expansion
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
haveyaseen
changed the title
Add targeted unit tests for compound assign helpers, shape formatting, type guards, providers metadata, and streaming discovery edge cases. Make Ok/Err/Index isExpression markers coverable with receiver bodies. Example: ```go ch := ast.NewChannelType(ast.NewBuiltinType(ast.TypeInt)) // ch.String() == "chan Int" ```
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/language/shapes-and-constraints.mdx`:
- Line 38: The current wording in the `placeOrder` example still implies invalid
dynamic input is blocked before the function body runs. Update the sentence near
`placeOrder` to make it clear that runtime safety comes from explicit `ensure`
checks or equivalent validation inside the function body, and that invalid SKUs
or quantities can reach the function before those checks execute.
In `@docs/snippets/ensure-if-ok.mdx`:
- Around line 11-14: The Forst and Go snippets are out of sync because the Go
tab introduces one() and xErr without showing where x comes from, while the
Forst tab still starts from an existing x. Update the example so both tabs
reflect the same setup: either add the missing x := one() source to the Forst
side or clearly annotate in the Go snippet that x is assumed to come from a
Result-returning call. Use the existing snippet structure in the ensure-if-ok
example to keep the two tabs aligned.
In `@docs/snippets/type-guards-mutation.mdx`:
- Line 17: Update the note in the type-guards mutation snippet to match the new
behavior: type-level guards now emit stub Go declarations, so remove the stale
“do not emit Go helpers yet” wording and rewrite the comment to describe the
stub output. Keep the change localized to the snippet in the docs and ensure any
accompanying text around type-level guards reflects the new emitted Go
declaration behavior.
In `@forst/internal/ast/typeguard_test.go`:
- Line 143: Add a unit test case in ValidateShapeGuard coverage that uses a
non-Shape/alias-like subject type and passes a non-nil isShape callback
returning true, so the injected predicate branch is exercised instead of only
the nil fallback. Update the relevant table or subtest in typeguard_test.go
(around the ValidateShapeGuard calls) with a precise test name that describes
the injected-callback behavior, and ensure the new case still verifies the
expected guard result when the predicate is supplied.
In `@forst/internal/ast/typeguard.go`:
- Around line 62-70: Add a direct unit test for the non-nil isShape branch in
ValidateShapeGuard: the current coverage only exercises ValidateShapeGuard(...,
nil), so add a new case that passes a custom resolver and verifies an
alias-backed Shape subject is accepted. Place it alongside the existing
ValidateShapeGuard tests and use a precise test name that clearly describes the
typedef-aware path being validated.
In `@forst/internal/parser/type.go`:
- Around line 18-24: The Array(...) branch in parseType is not propagating the
lowercase-type allowance into the nested element parse, so Array(io.Reader) can
fail where other composite types succeed. Update the Array token handling in
parseType to pass the widened parse options when calling parseType for the
elementType, matching the behavior already used by the []T, map[K]V, and chan T
branches. Keep the fix localized to the Array(...) path and preserve the
existing ast.NewArrayType construction.
In `@forst/internal/transformer/go/emit_validation_coverage_test.go`:
- Around line 69-87: Add a focused test in emit_validation_coverage_test.go to
cover a labeled range loop in transformForNode, since the existing
TestEmitValidation_labeledBreak only exercises the ForStmt path. Create a new
precise test name for the RangeStmt case, compile a source snippet with a
labeled range and break to that label, and assert the emitted output preserves
both the label and labeled break before calling assertGoParses. This should
specifically validate the loop = rs path alongside the existing classic for
coverage.
In `@forst/internal/transformer/go/ensure_constraint.go`:
- Around line 45-48: The ensure_constraint.go handling for ensureConstraint
should not turn constraint.Name == "is" into a constant true, because that makes
shape assertions always succeed. Update the ensure lowering path in
ensure_constraint.go to emit an explicit codegen error or unsupported-constraint
failure for "is" until real runtime checking is implemented, and keep the
existing behavior only for supported constraint kinds.
In `@forst/internal/transformer/go/examples_pipeline_test.go`:
- Around line 15-32: The new golden for union_error_narrowing is only
regeneratable and never verified in CI, so the compiler output can drift
silently. Add an assertion that reads examples/out/union_error_narrowing.go and
compares it against compileForstPipeline output from union_error_narrowing.ft,
either by extending TestPipeline_examplesInKnownGood or adding a dedicated
golden test alongside TestWriteUnionErrorNarrowingGolden. Keep the test name
specific to the example being checked so it clearly documents what is under
test.
In `@forst/internal/transformer/go/expression_if_is_test.go`:
- Around line 74-80: The test in transformIfIsCondition only checks for a
non-nil result, which does not verify that the builtin Min lowering path was
used. Update the test to assert the exact lowered AST shape produced by
transformIfIsCondition, specifically that it returns a unary not expression
wrapping the builtin constraint expression. Use the existing
transformIfIsCondition call in expression_if_is_test.go to inspect expr’s
concrete structure rather than just nil-ness.
In `@forst/internal/transformer/go/pipeline_integration_test.go`:
- Around line 579-584: The integration test in compileForstPipeline is asserting
on a bare “!” token, which is too broad and can pass for unrelated generated
code. Update the check to look for a more specific fragment from the lowered `if
name is Min(1)` condition in the generated Go output, alongside the existing
symbols like func checkLen, len(, and package main, so the test uniquely
verifies the intended codegen path.
In `@forst/internal/transformer/go/shape_context_typeguard_test.go`:
- Around line 441-446: The test around transformTypeGuard currently only checks
that a non-nil FuncDecl is returned, which can miss regressions in the generated
stub body. Update the assertion in shape_context_typeguard_test.go to verify the
emitted declaration shape from transformTypeGuard and the generated body/result,
specifically that the type-level guard still produces the expected return-true
stub rather than only checking decl != nil.
In `@forst/internal/transformer/go/typeguard.go`:
- Around line 142-172: The type-level guard stub in the guard-emission path is
currently lowering unsupported guards to an always-true function, which makes
runtime checks unsound. In the branch that builds the goast.FuncDecl for the
type-level shape guard, stop returning a successful boolean for every input and
instead preserve the “not lowerable” behavior by emitting a diagnostic or a
conservative failure path. Use the existing type guard transformation flow
around transformTypeGuardParams and the guardIdent stub generation to locate and
adjust this behavior.
In `@forst/internal/typechecker/infer_ensure_test.go`:
- Around line 98-99: The test for inferEnsureType is using a broad
strings.Contains check, so it can pass on the wrong error message; tighten the
assertion in infer_ensure_test by checking the exact diagnostic emitted from
inferEnsureType instead of a substring match. Use the existing err path in the
test and compare the full message string (or an equivalent precise assertion) so
the reserved-placeholder rejection is pinned exactly.
In `@forst/internal/typechecker/infer_for.go`:
- Around line 34-38: The current loop label handling in infer_for.go only uses
loopLabelStack for active nesting, so sibling loops can reuse the same label and
slip through typechecking. Update the label checks in the loop/typechecker path
around the InferFor logic to also track labels declared within the current
function/scope and reject any redefinition immediately. Use the existing
loopLabelStack for nesting, but add a separate per-function label registry and
report an error when n.Label.ID is already present before allowing Go emission.
In `@forst/internal/typechecker/infer_node_dispatch_test.go`:
- Around line 39-60: Split TestInferNodeType_breakAndContinueLabeled into named
subtests so the valid labeled-flow case and the undefined-label case report
independently. Keep the existing inferNodeType assertions, but wrap the
nested-loop break/continue success path and the missing-label error path in
separate t.Run cases (or a small table) under
TestInferNodeType_breakAndContinueLabeled. Use the existing symbols
inferNodeType, BreakNode, ContinueNode, and the label "outer"/"missing" to keep
the test cases easy to locate.
In `@forst/internal/typechecker/param_shape_test.go`:
- Around line 56-59: The test in ShapeFieldsFromParamType only checks that one
field is returned, so it can pass even if the extracted field name or type is
wrong. Tighten the assertion in param_shape_test.go by verifying the returned
map contains the expected key from ShapeFieldsFromParamType (the x field) and
that its type is Int, using a structured check or cmp.Diff-style assertion
rather than only len(fields). This should pin the contract of
ShapeFieldsFromParamType more precisely.
In `@forst/internal/typechecker/param_shape.go`:
- Around line 74-87: The destructured-parameter path in
registerDestructuredParamSymbols currently skips non-shape params and missing or
untyped fields, which lets invalid destructuring pass type checking. Change this
helper to return an error instead of silently returning/continuing, and
propagate that error through inferFunctionNode and inferTypeGuardNode so
malformed destructured params are rejected during type checking rather than
later in Go emission. Make sure the handling aligns with
ShapeFieldsFromParamType and ShapeFieldTypeNode when a field is absent or has no
type node.
In `@forst/internal/typechecker/receiver_methods.go`:
- Around line 45-48: The receiver method signature builder is coercing
destructured parameters into a fake identifier via ParameterSignature.Ident in
the receiver_methods.go logic. Update the branch that handles
DestructuredParamNode/GetIdent so it does not store the display string as
ast.Ident; instead represent destructured params explicitly in
ParameterSignature and keep Ident only for real binders. Use the existing
parameter conversion path around params[i] and ParameterSignature to locate and
adjust the model.
In `@forst/internal/typechecker/register.go`:
- Around line 179-182: The ParameterSignature construction in register.go is
incorrectly using p.GetIdent() for destructured parameters, which produces an
invalid synthetic identifier and breaks the FunctionSignature.Parameters
contract. Update the parameter mapping logic in the relevant
register/typechecker path so destructured params do not write a fake name into
ParameterSignature.Ident; instead, preserve the fields separately or leave Ident
unset for that variant, while keeping the existing type assignment intact.
In `@forst/internal/typechecker/shape_type_test.go`:
- Around line 9-44: The IsShapeType test currently only covers direct shape and
simple alias cases, so it misses the GetTypeAliasChain fallback path and gives
poor failure context. Refactor TestIsShapeType into a table-driven test with
t.Run subtests, using tc.IsShapeType and the existing typedef setup points
(TypeDefShapeExpr and TypeDefAssertionExpr), and add at least one chained-alias
case that resolves through multiple aliases to Shape. Name each subtest by
resolution path so a regression clearly identifies which branch in IsShapeType
broke.
In `@ROADMAP.md`:
- Line 78: The binary types entry in ROADMAP.md contains raw pipe characters
inside a table cell, which breaks the row structure. Update the table cell that
describes binary type expressions so it no longer uses unescaped `|` characters;
use an escaped or otherwise safe representation while keeping the references to
the parser, AST, typechecker, and emit symbols intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: de1e6791-da49-476f-9037-c481592d0d47
📒 Files selected for processing (50)
ROADMAP.mdTaskfile.ymldocs/language/shapes-and-constraints.mdxdocs/resources/roadmap.mdxdocs/snippets/ensure-if-ok.mdxdocs/snippets/type-guards-mutation.mdxexamples/in/union_error_narrowing.ftforst/internal/ast/for_stmt.goforst/internal/ast/param.goforst/internal/ast/param_test.goforst/internal/ast/typeguard.goforst/internal/ast/typeguard_test.goforst/internal/parser/array_shape_field_test.goforst/internal/parser/shape.goforst/internal/parser/statement.goforst/internal/parser/type.goforst/internal/printer/printer.goforst/internal/printer/printer_test.goforst/internal/transformer/go/emit_validation_coverage_test.goforst/internal/transformer/go/ensure_builtins.goforst/internal/transformer/go/ensure_constraint.goforst/internal/transformer/go/ensure_types.goforst/internal/transformer/go/examples_pipeline_test.goforst/internal/transformer/go/expression_if_is.goforst/internal/transformer/go/expression_if_is_test.goforst/internal/transformer/go/for_loop.goforst/internal/transformer/go/function.goforst/internal/transformer/go/function_destructured_test.goforst/internal/transformer/go/pipeline_integration_test.goforst/internal/transformer/go/shape_context_typeguard_test.goforst/internal/transformer/go/transformer.goforst/internal/transformer/go/transformer_test.goforst/internal/transformer/go/typeguard.goforst/internal/transformer/go/typeguard_test.goforst/internal/typechecker/collect.goforst/internal/typechecker/infer.goforst/internal/typechecker/infer_assertion.goforst/internal/typechecker/infer_ensure_test.goforst/internal/typechecker/infer_for.goforst/internal/typechecker/infer_function_node.goforst/internal/typechecker/infer_node_dispatch_test.goforst/internal/typechecker/infer_typeguard_node.goforst/internal/typechecker/param_shape.goforst/internal/typechecker/param_shape_test.goforst/internal/typechecker/receiver_methods.goforst/internal/typechecker/register.goforst/internal/typechecker/shape_type.goforst/internal/typechecker/shape_type_test.goforst/internal/typechecker/typechecker.goforst/internal/typechecker/unify_typeguard.go
💤 Files with no reviewable changes (1)
- forst/internal/transformer/go/transformer_test.go
| <CatalogOrder /> | ||
|
|
||
| When `placeOrder` runs, the compiler has already proven the shape at compile time where values are known, and emits checks for dynamic input. Invalid SKUs and quantities never reach your catalog logic. | ||
| When `placeOrder` runs, the compiler has already proven the shape at compile time where values are known; use `ensure` (or explicit checks) for dynamic input. Invalid SKUs and quantities never reach your catalog logic. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This still overstates the runtime guarantee.
The surrounding edits correctly say dynamic validation happens via explicit ensure checks in the function body, but “Invalid SKUs and quantities never reach your catalog logic” still reads like automatic pre-body validation exists. Until prologue emission lands, invalid dynamic input reaches the function and is only blocked once those explicit checks run.
🧰 Tools
🪛 LanguageTool
[style] ~38-~38: It might be better to use ‘time’ with the time-relative pronoun ‘when’. (Alternatively, use ‘in/on which’.)
Context: ...has already proven the shape at compile time where values are known; use ensure (or expl...
(WHEN_WHERE)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/language/shapes-and-constraints.mdx` at line 38, The current wording in
the `placeOrder` example still implies invalid dynamic input is blocked before
the function body runs. Update the sentence near `placeOrder` to make it clear
that runtime safety comes from explicit `ensure` checks or equivalent validation
inside the function body, and that invalid SKUs or quantities can reach the
function before those checks execute.
Comment on lines
+11
to
+14
| // forst build — Result lowers to (T, error) | ||
| x, xErr := one() | ||
| if xErr == nil { | ||
| println(x) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the Generated Go tab aligned with the Forst tab.
The Forst example starts from an existing x, but the Go example now introduces one() and xErr without showing the corresponding setup. Either include the x := one() source in the Forst tab or annotate that this Go snippet assumes x came from a Result-returning call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/snippets/ensure-if-ok.mdx` around lines 11 - 14, The Forst and Go
snippets are out of sync because the Go tab introduces one() and xErr without
showing where x comes from, while the Forst tab still starts from an existing x.
Update the example so both tabs reflect the same setup: either add the missing x
:= one() source to the Forst side or clearly annotate in the Go snippet that x
is assumed to come from a Result-returning call. Use the existing snippet
structure in the ensure-if-ok example to keep the two tabs aligned.
| <Tab title="Generated Go"> | ||
| ```go | ||
| // forst build | ||
| // forst build — illustrative; type-level guards do not emit Go helpers yet |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This comment contradicts the feature added in this PR.
The stack context for this change says type-level guards now emit stub Go declarations, so saying they “do not emit Go helpers yet” is immediately stale. Please update the note and snippet to describe the stub output instead of framing it as purely illustrative.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/snippets/type-guards-mutation.mdx` at line 17, Update the note in the
type-guards mutation snippet to match the new behavior: type-level guards now
emit stub Go declarations, so remove the stale “do not emit Go helpers yet”
wording and rewrite the comment to describe the stub output. Keep the change
localized to the snippet in the docs and ensure any accompanying text around
type-level guards reflects the new emitted Go declaration behavior.
Comment on lines
+62
to
+70
| // ValidateShapeGuard validates that a type guard is a valid shape guard. | ||
| // When isShape is non-nil it resolves typedef aliases (e.g. via typechecker.IsShapeType). | ||
| func ValidateShapeGuard(node TypeGuardNode, isShape func(TypeNode) bool) error { | ||
| check := isShapeType | ||
| if isShape != nil { | ||
| check = isShape | ||
| } | ||
| // Check if the receiver type is a Shape type | ||
| if !isShapeType(node.Subject.GetType()) { | ||
| if !check(node.Subject.GetType()) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a direct test for the non-nil isShape branch.
The new typedef-aware path is not exercised by the adjacent coverage shown here—those tests only call ValidateShapeGuard(..., nil). Please add a case that passes a resolver and proves an alias-backed Shape subject is accepted. As per coding guidelines, "Ensure presence of precise, reproducing unit or integration tests (preferably unit tests) with precise names describing exactly what's under test."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/internal/ast/typeguard.go` around lines 62 - 70, Add a direct unit test
for the non-nil isShape branch in ValidateShapeGuard: the current coverage only
exercises ValidateShapeGuard(..., nil), so add a new case that passes a custom
resolver and verifies an alias-backed Shape subject is accepted. Place it
alongside the existing ValidateShapeGuard tests and use a precise test name that
clearly describes the typedef-aware path being validated.
Source: Coding guidelines
Comment on lines
+74
to
+87
| func (tc *TypeChecker) registerDestructuredParamSymbols(fields []string, paramType ast.TypeNode, symbolKind SymbolKind) { | ||
| shapeFields, ok := tc.ShapeFieldsFromParamType(paramType) | ||
| if !ok { | ||
| return | ||
| } | ||
| for _, name := range fields { | ||
| sf, ok := shapeFields[name] | ||
| if !ok { | ||
| continue | ||
| } | ||
| if tn, ok := ShapeFieldTypeNode(sf); ok { | ||
| tc.storeSymbol(ast.Identifier(name), []ast.TypeNode{tn}, symbolKind) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject bad destructuring during type checking.
This helper silently returns/continues when the parameter type is not shape-backed or when a requested field is missing/untyped. forst/internal/transformer/go/function.go:69-80 and forst/internal/transformer/go/typeguard.go:92-103 now fail hard for those same cases, so malformed destructured params can survive type checking and only break during Go emission. Please make this path return an error and propagate it from both inferFunctionNode and inferTypeGuardNode.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/internal/typechecker/param_shape.go` around lines 74 - 87, The
destructured-parameter path in registerDestructuredParamSymbols currently skips
non-shape params and missing or untyped fields, which lets invalid destructuring
pass type checking. Change this helper to return an error instead of silently
returning/continuing, and propagate that error through inferFunctionNode and
inferTypeGuardNode so malformed destructured params are rejected during type
checking rather than later in Go emission. Make sure the handling aligns with
ShapeFieldsFromParamType and ShapeFieldTypeNode when a field is absent or has no
type node.
Comment on lines
+45
to
+48
| params[i] = ParameterSignature{ | ||
| Ident: ast.Ident{ID: ast.Identifier(p.GetIdent())}, | ||
| Type: p.Type, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Don't persist a display string as ParameterSignature.Ident.
DestructuredParamNode.GetIdent() now returns strings like "a, b", so this branch stores an impossible identifier into the method signature. Any downstream consumer that treats Ident as a real binder now sees a name that can never appear in source. Model destructured params explicitly instead of coercing them into ast.Ident.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/internal/typechecker/receiver_methods.go` around lines 45 - 48, The
receiver method signature builder is coercing destructured parameters into a
fake identifier via ParameterSignature.Ident in the receiver_methods.go logic.
Update the branch that handles DestructuredParamNode/GetIdent so it does not
store the display string as ast.Ident; instead represent destructured params
explicitly in ParameterSignature and keep Ident only for real binders. Use the
existing parameter conversion path around params[i] and ParameterSignature to
locate and adjust the model.
Comment on lines
+179
to
+182
| params[i] = ParameterSignature{ | ||
| Ident: ast.Ident{ID: ast.Identifier(p.GetIdent())}, | ||
| Type: p.Type, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
This repeats the invalid synthetic identifier for destructured params.
p.GetIdent() yields a comma-joined field list, not a legal parameter identifier. Writing that value into FunctionSignature.Parameters[i].Ident breaks the signature contract and makes function metadata disagree with source-level names. Store destructured fields separately, or leave Ident unset for this variant.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/internal/typechecker/register.go` around lines 179 - 182, The
ParameterSignature construction in register.go is incorrectly using p.GetIdent()
for destructured parameters, which produces an invalid synthetic identifier and
breaks the FunctionSignature.Parameters contract. Update the parameter mapping
logic in the relevant register/typechecker path so destructured params do not
write a fake name into ParameterSignature.Ident; instead, preserve the fields
separately or leave Ident unset for that variant, while keeping the existing
type assignment intact.
Comment on lines
+9
to
+44
| func TestIsShapeType(t *testing.T) { | ||
| tc := New(nil, false) | ||
|
|
||
| if !tc.IsShapeType(ast.TypeNode{Ident: ast.TypeShape}) { | ||
| t.Fatal("Shape builtin should be a shape type") | ||
| } | ||
| if tc.IsShapeType(ast.TypeNode{Ident: ast.TypeString}) { | ||
| t.Fatal("String should not be a shape type") | ||
| } | ||
|
|
||
| tc.Defs["User"] = ast.TypeDefNode{ | ||
| Ident: "User", | ||
| Expr: ast.TypeDefShapeExpr{ | ||
| Shape: ast.ShapeNode{ | ||
| Fields: map[string]ast.ShapeFieldNode{ | ||
| "id": {Type: &ast.TypeNode{Ident: ast.TypeString}}, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
| if !tc.IsShapeType(ast.TypeNode{Ident: "User"}) { | ||
| t.Fatal("typedef to shape literal should be a shape type") | ||
| } | ||
|
|
||
| tc.Defs["AppContext"] = ast.TypeDefNode{ | ||
| Ident: "AppContext", | ||
| Expr: ast.TypeDefAssertionExpr{ | ||
| Assertion: &ast.AssertionNode{ | ||
| BaseType: typeIdentPtr(string(ast.TypeShape)), | ||
| }, | ||
| }, | ||
| } | ||
| if !tc.IsShapeType(ast.TypeNode{Ident: "AppContext"}) { | ||
| t.Fatal("typedef alias to Shape should be a shape type") | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Cover the alias-chain branch and split these cases into named subtests.
This test only hits the direct Shape, direct typedef-shape, and direct assertion-alias paths. It never reaches the new GetTypeAliasChain fallback in IsShapeType, and a failure here would not tell you which resolution path regressed. Please convert this into a table-driven test with t.Run(...) cases and add at least one chained-alias case.
As per coding guidelines, "forst/**/*_test.go: Implement table-driven tests for multiple inputs and name subtests with t.Run(\"case\", ...)." and "**/*_test.{go,ts}: Ensure presence of precise, reproducing unit or integration tests..."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/internal/typechecker/shape_type_test.go` around lines 9 - 44, The
IsShapeType test currently only covers direct shape and simple alias cases, so
it misses the GetTypeAliasChain fallback path and gives poor failure context.
Refactor TestIsShapeType into a table-driven test with t.Run subtests, using
tc.IsShapeType and the existing typedef setup points (TypeDefShapeExpr and
TypeDefAssertionExpr), and add at least one chained-alias case that resolves
through multiple aliases to Shape. Name each subtest by resolution path so a
regression clearly identifies which branch in IsShapeType broke.
Source: Coding guidelines
| | `is` operator for `ensure` conditions | ✅ done | Parser requires `ensure … is …` (see `forst/internal/parser/ensure.go`; `ensure !ident` uses implicit `Nil()`). The typechecker enforces presence (`Present`) and type-guard subject compatibility (`forst/internal/typechecker/unify_typeguard.go`, `infer_ensure.go`); it does **not** fully validate arbitrary built-in constraint semantics beyond that. Emission: `forst/internal/transformer/go/ensure.go`, `ensure_constraint.go`. Examples: `examples/in/ensure.ft` and `task example:ensure`. | | ||
| | Type guards (beyond shape guards) | 🔬 experimental | Top-level `is (subject T) Name { … }` declarations are **implemented** end-to-end: parse (`forst/internal/parser/typeguard.go`), typecheck (`unify_typeguard.go`, registration in `Defs`), Go emit (`forst/internal/transformer/go/typeguard.go`, `ensure_typeguard.go`). Examples: `examples/in/rfc/guard/` (`task example:shape-guard`, `task example:basic-guard`). **Not** at full RFC “done” yet: narrowing across all control-flow positions and other polish overlap **Control-flow type narrowing**; language design: [type guards RFC](./examples/in/rfc/guard/guard.md); Go / `.d.ts` interop: [interop](./examples/in/rfc/guard/interop.md). | | ||
| | Immutability guarantees (`ensure`-scoped; unsafe mode for Go interop) | 📋 planned | Not implemented. | | ||
| | Binary type expressions (`A \| B`, `A & B`) | 🔬 experimental | Parser, AST, and typechecker support **`&`** / **`|`** on typedef bodies: unions flatten to **`TypeUnion`**, intersections use meet (`forst/internal/typechecker/typedef_binary.go`); Go emit lowers via canonical **`TypeNode`** (`forst/internal/transformer/go/typedef_expr.go`). **Closed unions of nominal errors** emit sealed Go interfaces (`error_union_sealed.go`); examples `union_error_types.ft`, `union_error_narrowing.ft`. **Gaps vs “done”:** general non-error unions, full intersection algebra, union narrowing in all control-flow positions, and TS/Go emit for arbitrary **`A \| B`** combinations—do not rely on binary typedef semantics beyond tested nominal-error and alias paths. **Narrowing** and future optionals should share one internal type algebra (assertions / `TypeNode`), not a parallel representation—see control-flow narrowing. | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Escape the pipe in this table cell.
Line 78 is currently parsed as a 4-column row, so the binary-types entry will not render reliably. Use a representation that avoids a raw | inside the table cell.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 78-78: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 78-78: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 78-78: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 78-78: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ROADMAP.md` at line 78, The binary types entry in ROADMAP.md contains raw
pipe characters inside a table cell, which breaks the row structure. Update the
table cell that describes binary type expressions so it no longer uses unescaped
`|` characters; use an escaped or otherwise safe representation while keeping
the references to the parser, AST, typechecker, and emit symbols intact.
Source: Linters/SAST tools
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
1 participant
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.
Valid()typechecked but emittedfalseat runtime, so every ensureusing it failed silently after passing the compiler. Reject with an
explicit diagnostic and drop
Validfrom the builtin constraint allowlist.Example:
Summary by CodeRabbit
New Features
forloops and labeledbreak/continuehandling.ensure/type-guard narrowing now works better for simple identifiers andischecks.Documentation
Bug Fixes