feat: asset inference #94
Conversation
WalkthroughAdds a lightweight type system and unification for assets and type variables, integrates per-expression and per-declaration type tracking into the checker, exposes declared variables via CheckResult, introduces AssetMismatch diagnostics, updates symbol extraction to use public DeclaredVars, and adds tests for unification and asset-mismatch scenarios. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant A as Analyzer
participant C as Checker
participant U as Unifier
participant D as Diagnostics
Dev->>A: CheckSource(code)
A->>C: newCheckResult(code)
note right of C #e6f7ff: Initialize DeclaredVars,\nExprTypes, VarTypes, stmtType
loop For each statement
C->>C: checkStatement(stmt)
C->>U: unify(expr/var types, stmtType / built-in constraints)
alt Unify succeeds
U-->>C: binds TVar -> TAsset / TVar -> TVar
C->>C: record in ExprTypes / VarTypes
else Unify fails
U-->>C: false
C->>D: append AssetMismatch(Expected, Got)
end
end
C-->>A: CheckResult {DeclaredVars, ExprTypes, VarTypes, Diagnostics}
A-->>Dev: Result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #94 +/- ##
==========================================
+ Coverage 70.56% 70.90% +0.33%
==========================================
Files 41 42 +1
Lines 4631 4733 +102
==========================================
+ Hits 3268 3356 +88
- Misses 1212 1224 +12
- Partials 151 153 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/analysis/union_find.go (1)
28-32: Clarify the comment to reflect path compressionThe comment is incomplete. Suggest making it explicit that this is path compression.
- // This bit doesn't change the behaviour but + // Path compression: cache the resolved root to flatten the chain. t.resolution = resolvedinternal/analysis/union_find_test.go (1)
1-1: Consider adding a conflict test (TVar bound to asset A then unify with asset B)Would assert Unify returns false after a TVar is already bound to a different concrete asset.
Example to add:
func TestUnifyVarAlreadyBoundMismatch(t *testing.T) { v := &analysis.TVar{} usd := analysis.TAsset("USD") eur := analysis.TAsset("EUR") require.True(t, analysis.Unify(v, &usd)) require.False(t, analysis.Unify(v, &eur)) require.Same(t, v.Resolve(), &usd) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
internal/analysis/check.go(15 hunks)internal/analysis/check_test.go(1 hunks)internal/analysis/diagnostic_kind.go(1 hunks)internal/analysis/document_symbols.go(1 hunks)internal/analysis/union_find.go(1 hunks)internal/analysis/union_find_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
internal/analysis/diagnostic_kind.go (1)
internal/jsonrpc2/messages.go (1)
Message(47-49)
internal/analysis/check_test.go (4)
internal/analysis/check.go (2)
Diagnostic(119-123)CheckSource(296-304)internal/parser/range.go (2)
Range(13-16)RangeOfIndexed(155-164)internal/analysis/diagnostic_kind.go (2)
AssetMismatch(130-133)AssetMismatch(139-141)internal/analysis/union_find.go (1)
TAsset(38-38)
internal/analysis/union_find_test.go (1)
internal/analysis/union_find.go (3)
TAsset(38-38)Unify(44-70)TVar(34-36)
internal/analysis/check.go (5)
internal/parser/ast.go (17)
VarDeclaration(308-313)Variable(13-13)Variable(64-67)Variable(81-81)FnCallIdentifier(262-265)ValueExpr(8-11)FnCall(21-21)FnCall(258-258)FnCall(267-271)BinaryInfix(20-20)BinaryInfix(69-74)NumberLiteral(18-18)NumberLiteral(37-40)StringLiteral(19-19)StringLiteral(42-45)SentValueLiteral(278-281)SentValueLiteral(287-287)internal/parser/range.go (1)
Range(13-16)internal/analysis/union_find.go (5)
Type(9-11)TVar(34-36)Unify(44-70)TypeToString(72-83)TAsset(38-38)internal/analysis/diagnostic_kind.go (2)
AssetMismatch(130-133)AssetMismatch(139-141)internal/interpreter/value.go (4)
Asset(18-18)Asset(32-32)Monetary(22-25)Monetary(30-30)
internal/analysis/union_find.go (1)
internal/utils/utils.go (1)
NonExhaustiveMatchPanic(8-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Dirty
- GitHub Check: Tests
🔇 Additional comments (15)
internal/analysis/diagnostic_kind.go (1)
130-141: New AssetMismatch diagnostic: LGTMConsistent with TypeMismatch; message and severity look good.
internal/analysis/union_find_test.go (8)
10-14: LGTM: concrete asset resolves to itselfCovers expected Resolve() behavior for TAsset.
16-21: LGTM: asset vs asset mismatchVerifies Unify returns false for differing assets.
23-28: LGTM: identical assets unifyHappy path coverage.
30-36: LGTM: self-unify no-opGood guard against self-cycles.
38-41: LGTM: unbound TVar resolves to itselfBaseline behavior covered.
43-51: LGTM: TVar binds to concrete assetAsserts pointer identity post-resolution.
53-73: LGTM: transitive unification collapses to concreteCovers chain resolution and identity.
75-95: LGTM: inverse transitivityComplements previous test.
internal/analysis/check_test.go (5)
972-993: LGTM: Asset mismatch surfaced at cap expressionAsserts mismatch location and message for differing send asset vs cap asset.
994-1026: Confirm Expected/Got orientation for variable-induced mismatchThe TODO suggests ambiguity. Please confirm the convention: Expected = context (e.g., statement/send), Got = offending expr/var. If different, adjust either the diagnostic construction or the expected test.
1028-1051: LGTM: Enforces asset constraint from balance(...)Good coverage of derived-asset mismatches.
1053-1075: LGTM: Var-to-var inference coherenceAsserts both vars resolve to the same asset type within a statement.
1077-1093: LGTM: get_asset inferenceValidates asset extraction into a variable.
internal/analysis/document_symbols.go (1)
28-36: AlldeclaredVarsreferences removed. Verified via ripgrep across the codebase; no matches found. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/analysis/check.go (1)
356-364: Prevent index-out-of-range panics in arity handlingWhen too few args are passed, we emit BadArity but still index validArgs[1]/[0], causing a crash. Add guards.
Apply this diff:
switch fnCall.Caller.Name { case FnVarOriginBalance, FnVarOriginOverdraft: + if len(validArgs) <= 1 { + break + } // we run unify(<expr>, <asset>) in: // <expr> := balance(@acc, <asset>) res.unifyNodeWith(fnCall, res.getExprType(validArgs[1])) case FnVarOriginGetAsset: + if len(validArgs) == 0 { + break + } res.unifyNodeWith(fnCall, res.getExprType(validArgs[0])) }
🧹 Nitpick comments (3)
internal/analysis/union_find.go (1)
73-81: Keep'prefix; consider stable TVar IDs for diagnostics (optional)The leading single quote is intentional here. If you later need stable, non-pointer-based messages across runs, consider mapping TVars to deterministic names (e.g., 't0, 't1) scoped to a CheckResult. Optional; current pointer-based form is fine for internal use.
Based on learnings
internal/analysis/check_test.go (1)
1076-1092: Inference of get_asset result to concrete assetSolid end-to-end check of get_asset. Consider adding a negative test for bad arity to prevent regressions in fn-call handling.
internal/analysis/check.go (1)
142-160: Lazy creation of type vars is fine; minor note on map keysUsing parser.ValueExpr and parser.VarDeclaration as map keys is acceptable here. Consider documenting that AST nodes are immutable and comparable to justify the choice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
internal/analysis/check.go(15 hunks)internal/analysis/check_test.go(1 hunks)internal/analysis/union_find.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-25T12:11:46.252Z
Learnt from: ascandone
PR: formancehq/numscript#94
File: internal/analysis/union_find.go:72-78
Timestamp: 2025-09-25T12:11:46.252Z
Learning: In the numscript codebase, type variables in TypeToString use single quote prefix notation (e.g., `'t0`, `'0x1234abcd`) where the single quote is a prefix marker, not a paired quote that needs closing. This is intentional formatting for type variable representation.
Applied to files:
internal/analysis/union_find.go
🧬 Code graph analysis (3)
internal/analysis/check.go (4)
internal/parser/ast.go (14)
VarDeclaration(308-313)Variable(13-13)Variable(64-67)Variable(81-81)FnCallIdentifier(262-265)Program(325-329)ValueExpr(8-11)FnCall(21-21)FnCall(258-258)FnCall(267-271)BinaryInfix(20-20)BinaryInfix(69-74)SentValueLiteral(278-281)SentValueLiteral(287-287)internal/parser/range.go (1)
Range(13-16)internal/analysis/union_find.go (5)
Type(9-11)TVar(35-37)Unify(45-71)TypeToString(73-84)TAsset(39-39)internal/analysis/diagnostic_kind.go (4)
AssetMismatch(130-133)AssetMismatch(139-141)DuplicateVariable(79-81)DuplicateVariable(87-89)
internal/analysis/check_test.go (4)
internal/analysis/check.go (2)
Diagnostic(119-123)CheckSource(296-304)internal/parser/range.go (2)
Range(13-16)RangeOfIndexed(155-164)internal/analysis/diagnostic_kind.go (2)
AssetMismatch(130-133)AssetMismatch(139-141)internal/analysis/union_find.go (1)
TAsset(39-39)
internal/analysis/union_find.go (1)
internal/utils/utils.go (1)
NonExhaustiveMatchPanic(8-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests
🔇 Additional comments (7)
internal/analysis/union_find.go (1)
45-71: Union-find logic looks solid for this scopeResolve + path compression and var/asset cases are handled cleanly. The self-unification guard avoids trivial cycles, and the recursive flip for (asset,var) is correct.
internal/analysis/check_test.go (4)
971-992: Good coverage for direct asset mismatchNicely asserts the range on the monetary literal and Expected/Got assets. This exercises the new unification path well.
994-1025: Catches mismatch through a shared monetary varGreat scenario validating that a single monetary var bound to different statements yields a mismatch at the later site.
1027-1050: Balance asset constraint test reads wellVerifies function-origin asset constraints; range and Expected/Got look correct.
1052-1074: Inference of monetary vars to same assetAsserting Same on resolved types is appropriate here.
internal/analysis/check.go (2)
436-444: Unification at monetary literal is effectiveBinding monetary expressions to their asset types here enables downstream statement-level constraints to work as intended.
611-613: Ordering unify before check ensures precise rangesUnifying cap with stmtType before checking makes the later mismatch land on the monetary literal, matching test intent.
| if varDecl.Origin != nil { | ||
| res.checkExpression(*varDecl.Origin, varDecl.Type.Name) | ||
| res.unifyNodeWith(*varDecl.Origin, res.getVarDeclType(varDecl)) | ||
| } |
There was a problem hiding this comment.
Guard against nil Type when Origin is present
If a variable has an origin expression but no explicit type, dereferencing varDecl.Type panics. Fall back to TypeAny when Type is nil.
Apply this diff:
- if varDecl.Origin != nil {
- res.checkExpression(*varDecl.Origin, varDecl.Type.Name)
- res.unifyNodeWith(*varDecl.Origin, res.getVarDeclType(varDecl))
- }
+ if varDecl.Origin != nil {
+ if varDecl.Type != nil {
+ res.checkExpression(*varDecl.Origin, varDecl.Type.Name)
+ } else {
+ res.checkExpression(*varDecl.Origin, TypeAny)
+ }
+ res.unifyNodeWith(*varDecl.Origin, res.getVarDeclType(varDecl))
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if varDecl.Origin != nil { | |
| res.checkExpression(*varDecl.Origin, varDecl.Type.Name) | |
| res.unifyNodeWith(*varDecl.Origin, res.getVarDeclType(varDecl)) | |
| } | |
| if varDecl.Origin != nil { | |
| if varDecl.Type != nil { | |
| res.checkExpression(*varDecl.Origin, varDecl.Type.Name) | |
| } else { | |
| res.checkExpression(*varDecl.Origin, TypeAny) | |
| } | |
| res.unifyNodeWith(*varDecl.Origin, res.getVarDeclType(varDecl)) | |
| } |
🤖 Prompt for AI Agents
In internal/analysis/check.go around lines 243 to 246, guard against a nil
varDecl.Type when varDecl.Origin is present by using TypeAny as a fallback:
compute a local typ := varDecl.Type and if typ == nil set typ = TypeAny, then
call res.checkExpression(*varDecl.Origin, typ.Name) and
res.unifyNodeWith(*varDecl.Origin, res.getVarDeclType(varDecl)) (or pass typ
into getVarDeclType if appropriate) so no dereference of nil occurs.
No description provided.