feat: emit warning when asset is inferred to be a fixed value #104
Conversation
WalkthroughAdds a post-traversal check that emits a new BoundAsset diagnostic for var declarations bound to asset or monetary types, converts several test diagnostics from pointer to value literals, and updates a codeactions test to remove a diagnostic count assertion. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/analysis/check.go (1)
298-304: Consider adding a default case or validation for unexpected type names.The switch statement only handles
TypeAssetandTypeMonetary. IfvarDecl.Type.Namehas an unexpected value,boundAssetTyperemains at its zero value (BoundAssetTypeMonetary), which could produce misleading diagnostics.var boundAssetType BoundAssetType switch varDecl.Type.Name { case TypeAsset: boundAssetType = BoundAssetTypeAsset case TypeMonetary: boundAssetType = BoundAssetTypeMonetary + default: + // Only asset and monetary types should resolve to *TAsset + continue }
📜 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 (4)
internal/analysis/check.go(2 hunks)internal/analysis/check_test.go(5 hunks)internal/analysis/diagnostic_kind.go(1 hunks)internal/lsp/codeactions_test.go(0 hunks)
💤 Files with no reviewable changes (1)
- internal/lsp/codeactions_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-05T11:42:58.472Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 27
File: internal/interpreter/interpreter.go:667-668
Timestamp: 2024-12-05T11:42:58.472Z
Learning: In Go test files within the `internal/interpreter` package (e.g., `reconciler_test.go` and `interpreter_test.go`), it's acceptable to use hardcoded `"<kept>"` strings in test data and comments, and they do not need to be replaced with the `KEPT_ADDR` constant.
Applied to files:
internal/analysis/check_test.go
🧬 Code graph analysis (2)
internal/analysis/diagnostic_kind.go (2)
internal/jsonrpc2/messages.go (1)
Message(47-49)internal/utils/utils.go (1)
NonExhaustiveMatchPanic(8-10)
internal/analysis/check.go (2)
internal/analysis/diagnostic_kind.go (9)
AssetMismatch(145-148)AssetMismatch(154-156)TypeMismatch(132-135)TypeMismatch(141-143)BoundAssetType(329-329)BoundAssetTypeAsset(333-333)BoundAssetTypeMonetary(332-332)BoundAsset(336-339)BoundAsset(356-358)internal/parser/ast.go (1)
Program(325-329)
⏰ 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: Tests
- GitHub Check: Dirty
🔇 Additional comments (7)
internal/analysis/diagnostic_kind.go (1)
328-358: LGTM on the overall structure.The
BoundAssetdiagnostic type follows the established patterns in this file (e.g.,Severitytype alias,Message()/Severity()interface implementation,NonExhaustiveMatchPanicfor default case).internal/analysis/check_test.go (4)
704-709: LGTM on the value literal change.Consistent with the diagnostic pattern used throughout the test file.
987-987: LGTM on the value literal changes for AssetMismatch.These changes (lines 987, 1020, 1045) are consistent with the corresponding change in
check.gowhereAssetMismatchis now emitted as a value type instead of a pointer.
1093-1120: LGTM on TestWarningWhenBoundAsset.The test correctly verifies that an
assetvariable inferred to have a concrete value (USD/2) triggers aBoundAssetdiagnostic with the appropriate type classification and inferred asset value at the declaration range.
1122-1149: LGTM on TestWarningWhenBoundMonetary.The test correctly verifies that a
monetaryvariable inferred to have a concrete asset (COIN) triggers aBoundAssetdiagnostic withBoundAssetTypeMonetaryclassification.internal/analysis/check.go (2)
173-180: LGTM on the value type change for AssetMismatch.Consistent with the interface implementation pattern where
AssetMismatchuses value receivers forMessage()andSeverity().
259-261: LGTM on the placement of checkBoundVars call.Correctly positioned after AST traversal completion, ensuring type inference is complete before checking for bound variables.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/analysis/check.go (1)
302-314: Add default case to handle unexpected type names.The switch statement on
varDecl.Type.Nameonly handlesTypeAssetandTypeMonetary. IfvarDecl.Type.Nameis neither (e.g., due to a logic bug or future code changes),boundAssetTypewill be zero-initialized toBoundAssetTypeMonetary, causing an incorrect diagnostic to be emitted.Apply this diff to add defensive handling:
var boundAssetType BoundAssetType switch varDecl.Type.Name { case TypeAsset: boundAssetType = BoundAssetTypeAsset case TypeMonetary: boundAssetType = BoundAssetTypeMonetary + default: + // varDecl.Type.Name is neither asset nor monetary, skip this variable + continue } res.pushDiagnostic(varDecl.Range, BoundAsset{ BoundAssetType: boundAssetType, InferredAsset: string(*boundType), })Based on the previous review comment suggestion to add a default branch to the switch statement.
🧹 Nitpick comments (1)
internal/analysis/diagnostic_kind.go (1)
341-354: Minor grammar refinement in monetary message.Line 348 reads "Receiving a monetary of different asset" — consider adding the article "a" for improved clarity: "Receiving a monetary of a different asset".
Apply this diff for improved grammar:
case BoundAssetTypeMonetary: - return fmt.Sprintf(`This monetary is inferred to always have asset '%s'. Receiving a monetary of different asset will cause a runtime error. + return fmt.Sprintf(`This monetary is inferred to always have asset '%s'. Receiving a monetary of a different asset will cause a runtime error. You may want to use a variable of type number instead.`, e.InferredAsset)
📜 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 (2)
internal/analysis/check.go(2 hunks)internal/analysis/diagnostic_kind.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/analysis/check.go (2)
internal/analysis/diagnostic_kind.go (9)
AssetMismatch(145-148)AssetMismatch(154-156)TypeMismatch(132-135)TypeMismatch(141-143)BoundAssetType(329-329)BoundAssetTypeAsset(333-333)BoundAssetTypeMonetary(332-332)BoundAsset(336-339)BoundAsset(356-358)internal/analysis/union_find.go (2)
TAsset(39-39)Type(9-11)
internal/analysis/diagnostic_kind.go (2)
internal/jsonrpc2/messages.go (1)
Message(47-49)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). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Dirty
- GitHub Check: Tests
🔇 Additional comments (7)
internal/analysis/check.go (4)
175-179: LGTM: Diagnostic construction updated to value type.The change from pointer to value construction is consistent with the interface-based diagnostic design and aligns with other diagnostic kinds in the codebase.
260-261: LGTM: Post-traversal bound variable check.The placement of
checkBoundVars()after the main AST traversal and unused variable checks is appropriate, ensuring all type inference is complete before checking for bound asset warnings.
263-286: LGTM: Sensible heuristic to reduce diagnostic noise.The early-exit heuristic that skips bound variable checks when type/asset mismatches are already present is well-reasoned. The comments clearly explain the rationale for avoiding diagnostic pollution when unification errors exist.
287-300: LGTM: Nil check addresses previous review concern.The nil check for
varDecl.Typeat lines 298-300 properly addresses the potential nil pointer dereference flagged in the previous review. The logic correctly filters for non-origin variables with concrete asset bindings.internal/analysis/diagnostic_kind.go (3)
329-334: LGTM: Clear type alias and enum pattern.The
BoundAssetTypetype alias with iota-based constants follows Go conventions and provides a clear enumeration for distinguishing asset and monetary binding contexts.
336-339: LGTM: Well-structured diagnostic.The
BoundAssetstruct clearly captures the bound asset type and inferred asset, providing all necessary information for generating meaningful warnings.
356-358: LGTM: Appropriate severity level.Returning
WarningSeverityis correct for this diagnostic, as bound asset inference is a potential runtime issue but not a compile-time error.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #104 +/- ##
==========================================
- Coverage 68.13% 68.12% -0.01%
==========================================
Files 45 45
Lines 4290 4333 +43
==========================================
+ Hits 2923 2952 +29
- Misses 1209 1222 +13
- Partials 158 159 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Leverage the asset inference to emit a warning when a (non-local) var is inferred to have a concrete asset type.
Example for the following script:
Future work
The plan is to enhance those kind of error with auto-fix, so that we can automatically turn the script into: