Experimental/oneof combinator #31
Conversation
WalkthroughThe pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant Parser
participant AST
participant Interpreter
Parser->>Parser: Encounter 'oneof' source
Parser->>AST: Create SourceOneof
AST-->>Parser: Return grouped sources
Parser->>Interpreter: Pass parsed sources
Interpreter->>Interpreter: Process sources
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #31 +/- ##
==========================================
- Coverage 63.29% 63.27% -0.02%
==========================================
Files 29 29
Lines 6563 6868 +305
==========================================
+ Hits 4154 4346 +192
- Misses 2223 2321 +98
- Partials 186 201 +15 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
internal/interpreter/interpreter.go (1)
984-991: Use pointer receiver forcheckFeatureFlagmethodFor consistency with other methods on
programStateand to avoid unnecessary copying of the struct, consider changing the receiver of thecheckFeatureFlagmethod to a pointer receiver*programState.Apply this diff:
-func (s programState) checkFeatureFlag(flag string) InterpreterError { +func (s *programState) checkFeatureFlag(flag string) InterpreterError {Numscript.g4 (1)
87-87: LGTM! Consider adding documentation.The new
oneofsource rule is well-structured and follows the grammar's conventions. Consider adding a comment to document the purpose and behavior of this new construct.Add a comment above the rule to explain its purpose:
+// oneof allows grouping multiple sources, where any one of them can be used as the source | 'oneof' LBRACE source* RBRACE # srcOneofinternal/parser/parser_test.go (1)
84-91: Enhance test coverage with additional test cases.While the basic test case is good, consider adding tests for:
- Empty
oneofblock- Nested
oneofblocks- Error cases (e.g., invalid source within
oneof)Example test cases:
func TestOneofSourceEdgeCases(t *testing.T) { tests := []struct { name string script string wantErrs bool }{ { name: "empty oneof block", script: `send $amt ( source = oneof { } destination = @d )`, wantErrs: true, }, { name: "nested oneof blocks", script: `send $amt ( source = oneof { @s1 oneof { @s2 @s3 } } destination = @d )`, wantErrs: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { p := parser.Parse(tt.script) if tt.wantErrs { assert.NotEmpty(t, p.Errors) } else { assert.Empty(t, p.Errors) snaps.MatchSnapshot(t, p.Value) } }) } }internal/parser/parser.go (1)
173-182: Consider adding validation for empty sources.The implementation looks good and follows the pattern used for
SrcInorderContext. However, consider adding validation to ensure that at least one source is provided in theoneofblock, as an emptyoneofblock might not be semantically valid.Consider this enhancement:
case *parser.SrcOneofContext: var sources []Source for _, sourceCtx := range sourceCtx.AllSource() { sources = append(sources, parseSource(sourceCtx)) } + if len(sources) == 0 { + // Add to ErrorListener or handle empty oneof block + listener.Errors = append(listener.Errors, ParserError{ + Range: range_, + Msg: "oneof block must contain at least one source", + }) + } return &SourceOneof{ Range: range_, Sources: sources, }internal/parser/antlr/numscript_lexer.go (1)
Line range hint
1-1: Reminder: This is an auto-generated file.Changes to this file should be made through the
Numscript.g4grammar file, as any manual edits here will be overwritten when the lexer is regenerated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
internal/parser/__snapshots__/parser_test.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (13)
Numscript.g4(1 hunks)internal/interpreter/interpreter.go(4 hunks)internal/parser/antlr/Numscript.interp(3 hunks)internal/parser/antlr/Numscript.tokens(1 hunks)internal/parser/antlr/NumscriptLexer.interp(4 hunks)internal/parser/antlr/NumscriptLexer.tokens(1 hunks)internal/parser/antlr/numscript_base_listener.go(1 hunks)internal/parser/antlr/numscript_lexer.go(2 hunks)internal/parser/antlr/numscript_listener.go(2 hunks)internal/parser/antlr/numscript_parser.go(23 hunks)internal/parser/ast.go(2 hunks)internal/parser/parser.go(1 hunks)internal/parser/parser_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/parser/antlr/Numscript.tokens
🧰 Additional context used
🪛 GitHub Check: codecov/patch
internal/parser/antlr/numscript_base_listener.go
[warning] 185-185: internal/parser/antlr/numscript_base_listener.go#L185
Added line #L185 was not covered by tests
[warning] 188-188: internal/parser/antlr/numscript_base_listener.go#L188
Added line #L188 was not covered by tests
internal/parser/ast.go
[warning] 90-90: internal/parser/ast.go#L90
Added line #L90 was not covered by tests
internal/parser/antlr/numscript_parser.go
[warning] 2786-2787: internal/parser/antlr/numscript_parser.go#L2786-L2787
Added lines #L2786 - L2787 were not covered by tests
[warning] 2790-2791: internal/parser/antlr/numscript_parser.go#L2790-L2791
Added lines #L2790 - L2791 were not covered by tests
[warning] 2794-2795: internal/parser/antlr/numscript_parser.go#L2794-L2795
Added lines #L2794 - L2795 were not covered by tests
[warning] 2819-2826: internal/parser/antlr/numscript_parser.go#L2819-L2826
Added lines #L2819 - L2826 were not covered by tests
[warning] 2828-2828: internal/parser/antlr/numscript_parser.go#L2828
Added line #L2828 was not covered by tests
[warning] 2832-2834: internal/parser/antlr/numscript_parser.go#L2832-L2834
Added lines #L2832 - L2834 were not covered by tests
[warning] 2836-2836: internal/parser/antlr/numscript_parser.go#L2836
Added line #L2836 was not covered by tests
[warning] 2839-2842: internal/parser/antlr/numscript_parser.go#L2839-L2842
Added lines #L2839 - L2842 were not covered by tests
[warning] 2845-2848: internal/parser/antlr/numscript_parser.go#L2845-L2848
Added lines #L2845 - L2848 were not covered by tests
[warning] 3393-3394: internal/parser/antlr/numscript_parser.go#L3393-L3394
Added lines #L3393 - L3394 were not covered by tests
[warning] 3400-3400: internal/parser/antlr/numscript_parser.go#L3400
Added line #L3400 was not covered by tests
[warning] 3413-3413: internal/parser/antlr/numscript_parser.go#L3413
Added line #L3413 was not covered by tests
[warning] 3421-3422: internal/parser/antlr/numscript_parser.go#L3421-L3422
Added lines #L3421 - L3422 were not covered by tests
[warning] 3433-3434: internal/parser/antlr/numscript_parser.go#L3433-L3434
Added lines #L3433 - L3434 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (13)
internal/parser/ast.go (2)
106-109: LGTM!The
SourceOneoftype is well-structured and consistent with other source types in the codebase.
90-90: Add test coverage for thesource()method.The
source()method forSourceOneofis not covered by tests.Run the following script to verify test coverage:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 90-90: internal/parser/ast.go#L90
Added line #L90 was not covered by testsinternal/parser/antlr/Numscript.interp (1)
4-4: Skip review of auto-generated file.This file is auto-generated by ANTLR and doesn't require manual review.
Also applies to: 45-45, 106-106
internal/parser/antlr/numscript_listener.go (1)
89-91: LGTM! The new listener methods are well-integrated.The added
EnterSrcOneofandExitSrcOneofmethods follow the established pattern and are properly documented.Also applies to: 212-214
internal/parser/antlr/NumscriptLexer.interp (1)
4-4: LGTM! The lexer changes are consistent.The new token literal 'oneof' and associated rules are properly integrated into the lexer's token definitions.
Also applies to: 45-45, 85-85, 131-131
internal/parser/antlr/numscript_base_listener.go (1)
184-189: LGTM! Base listener implementation follows the pattern.The empty implementations for
EnterSrcOneofandExitSrcOneofare correct. The static analysis warning about test coverage can be safely ignored as this is generated code with intentionally empty implementations.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 185-185: internal/parser/antlr/numscript_base_listener.go#L185
Added line #L185 was not covered by tests
[warning] 188-188: internal/parser/antlr/numscript_base_listener.go#L188
Added line #L188 was not covered by testsinternal/parser/antlr/numscript_lexer.go (5)
46-49: LGTM! Token literal definitions are correct.The addition of the 'oneof' token and the subsequent reindexing of token literals are consistent with the PR objective.
52-53: LGTM! Token symbolic names are properly aligned.The symbolic names array is correctly updated to maintain consistency with the literal names and token constants.
60-65: LGTM! Token rule names are properly defined.The rule names array is correctly updated to include the new 'oneof' token while maintaining proper indexing.
Line range hint
19-19: Address the TODO comment about EOF string.The TODO comment suggests that the EOF string definition is missing. Consider implementing this to ensure complete lexer functionality.
Would you like me to help implement the EOF string definition?
267-303: LGTM! Token constants are correctly updated.The token constants are properly incremented to accommodate the new 'oneof' token while maintaining consistency with the literal and symbolic names.
internal/parser/antlr/NumscriptLexer.tokens (2)
1-38: LGTM! Token ID assignments are properly sequenced.The token renumbering maintains consistent sequential ordering while accommodating the new 'oneof' token.
40-64: Verify token ID consistency across parser components.The literal mappings correctly reflect the updated token IDs. However, we should verify that these changes are consistently applied across related parser files.
✅ Verification successful
Token IDs are consistently defined across parser components
The token mappings, including 'oneof' (T__1=2), are consistently defined across all parser files. No mismatches or outdated token IDs were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify token ID consistency across parser files # Check for any mismatches in token IDs echo "Checking token ID consistency..." for file in $(fd -e interp -e java -e go -e tokens . internal/parser/antlr); do echo "=== $file ===" # Look for 'oneof' token definitions and their IDs rg -A 1 -B 1 "'oneof'|T__1" "$file" done # Verify no hardcoded old token IDs remain echo "Checking for old token IDs..." rg "T__0.*=.*2|WS.*=.*2|NEWLINE.*=.*3" internal/parser/antlr/Length of output: 3448
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/interpreter/interpreter_test.go (1)
3546-3668: Consider refactoringoneoftests into a table-driven format to reduce duplication.Refactoring these similar test cases into a table-driven test can enhance maintainability and make it easier to add new scenarios in the future.
numscript_test.go (1)
100-135: LGTM! Consider adding more test cases.The test implementation correctly verifies the basic functionality of the
oneofconstruct. However, consider adding test cases for:
- Error handling when the feature flag is not enabled
- Empty
oneofblock- Nested
oneofblocks
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Numscript.g4(1 hunks)internal/interpreter/batch_balances_query.go(1 hunks)internal/interpreter/interpreter.go(7 hunks)internal/interpreter/interpreter_test.go(1 hunks)internal/parser/antlr/Numscript.interp(3 hunks)internal/parser/antlr/numscript_parser.go(23 hunks)numscript_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/parser/antlr/Numscript.interp
- Numscript.g4
🔇 Additional comments (10)
internal/parser/antlr/numscript_parser.go (1)
2772-2850: Code changes correctly integrateSrcOneofContextfor theoneofconstruct.The implementation of
SrcOneofContextappropriately handles the newoneofgrammar rule, ensuring that the parser can correctly parse and interpret scripts using theoneofconstruct.internal/interpreter/batch_balances_query.go (1)
131-139: Correctly handleSourceOneofby recursively invokingfindBalancesQueries.The addition of the
parser.SourceOneofcase effectively integrates the newoneofconstruct into the balance query process, ensuring that balances are correctly calculated for all sub-sources within theoneof.internal/interpreter/interpreter_test.go (5)
3546-3570: TestTestOneofInSourceSendFirstBranchvalidatesoneofwhen the first branch succeeds.The test correctly verifies that the
oneofconstruct selects the first successful source (@a) when it allows unbounded overdraft.
3572-3595: TestTestOneofInSourcehandles first branch failure and second branch success inoneof.This test ensures that when the first source fails due to insufficient overdraft allowance, the
oneofconstruct correctly falls back to the second source (@world).
3597-3618: TestTestOneofAllFailingverifies error handling when alloneofbranches fail.The test appropriately checks that a
MissingFundsErris returned when none of the sources inoneofcan satisfy the transfer amount.
3620-3645: TestTestOneofInSendAllvalidatesoneofbehavior in asend allcontext.The test confirms that only the first successful source (
@s1) is used in thesend alloperation, as expected with theoneofconstruct.
3647-3668: TestTestOneofSingletonchecksoneofwith a single source for correct behavior.This test ensures that
oneofoperates correctly even when only one source is provided, verifying successful execution when the source has sufficient balance.internal/interpreter/interpreter.go (3)
182-185: LGTM! Clean feature flag declaration.The feature flags are well-organized and follow a consistent naming convention.
580-609: LGTM! Well-implemented backtracking logic.The implementation correctly:
- Tries each source in order
- Backtracks if a source doesn't provide the full amount
- Falls back to the last source if no previous source succeeds
1027-1034: LGTM! Clean feature flag check implementation.The helper method is well-implemented with:
- Clear error handling
- Consistent return type
- Reusable design
| case *parser.SourceOneof: | ||
| err := s.checkFeatureFlag(ExperimentalOneofFeatureFlag) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // we can safely access the first one because empty oneof is parsing err | ||
| first := source.Sources[0] | ||
| return s.sendAll(first) | ||
|
|
There was a problem hiding this comment.
Inconsistent behavior in sendAll method.
The oneof implementation in sendAll only uses the first source without trying alternatives. This differs from the behavior in trySendingUpTo where it tries each source until one succeeds. Consider making the behavior consistent by:
- Either documenting that
sendAllalways uses the first source - Or implementing the same backtracking logic as in
trySendingUpTo
There was a problem hiding this comment.
that's not inconsistent: this is indeed a backtracking operator, but when calling this method none of the branch can fail, hence we just forward to the first source
There was a problem hiding this comment.
@ascandone Thank you for the clarification. That makes perfect sense - since sources can't fail in "send all" context, using the first source is sufficient.
✏️ Learnings added
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/interpreter/interpreter.go:479-488
Timestamp: 2025-01-20T23:01:48.647Z
Learning: In the `sendAll` method of the Numscript interpreter, the `oneof` operator only uses the first source because none of the branches can fail in a "send all" context. This differs from `trySendingUpTo` where backtracking is needed as sources can fail when trying to send a specific amount.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/cmd/run.go (1)
208-208: Enhance CLI flag documentation and test coverage.While the flag registration follows the established pattern, consider these improvements:
- Add test coverage for CLI flag parsing
- Enhance the help text to better explain what the "oneof combinator" does and its use cases
Example of more detailed help text:
-cmd.Flags().BoolVar(&oneOfFeatureFlag, interpreter.ExperimentalOneofFeatureFlag, false, "feature flag to enable the oneof combinator") +cmd.Flags().BoolVar(&oneOfFeatureFlag, interpreter.ExperimentalOneofFeatureFlag, false, "feature flag to enable the oneof combinator, which allows grouping multiple sources under a single source definition")🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 208-208: internal/cmd/run.go#L208
Added line #L208 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/analysis/check.go(1 hunks)internal/cmd/run.go(3 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
internal/cmd/run.go
[warning] 125-127: internal/cmd/run.go#L125-L127
Added lines #L125 - L127 were not covered by tests
[warning] 208-208: internal/cmd/run.go#L208
Added line #L208 was not covered by tests
internal/analysis/check.go
[warning] 465-468: internal/analysis/check.go#L465-L468
Added lines #L465 - L468 were not covered by tests
🔇 Additional comments (2)
internal/analysis/check.go (1)
465-469: Implementation looks good but needs test coverage.The implementation correctly handles the new
SourceOneoftype by recursively checking each source, following the same pattern asSourceInorder. However, the code coverage report indicates that these lines are not covered by tests.Let's verify the test coverage and existing test patterns:
Would you like me to help generate test cases for the
SourceOneofimplementation?🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 465-468: internal/analysis/check.go#L465-L468
Added lines #L465 - L468 were not covered by testsinternal/cmd/run.go (1)
30-30: LGTM!The feature flag declaration follows the established pattern and is appropriately placed with other flag declarations.
| if oneOfFeatureFlag { | ||
| featureFlags[interpreter.ExperimentalOneofFeatureFlag] = struct{}{} | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for the oneof feature flag in the cmd package
The feature flag is well integrated with the interpreter package and has thorough test coverage there. However, the cmd package lacks test coverage for:
- Flag parsing and validation
- Feature flag propagation to the interpreter
- Integration tests with the flag enabled/disabled
🔗 Analysis chain
Add test coverage for the oneof feature flag.
The feature flag implementation looks correct but lacks test coverage. Please add test cases to verify:
- Program behavior when the flag is enabled
- Program behavior when the flag is disabled
- Integration with the interpreter package's feature flag handling
Let's verify the feature flag integration with the interpreter package:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the interpreter package properly handles the oneof feature flag
# Check if the interpreter package defines the feature flag constant
rg -l "ExperimentalOneofFeatureFlag.*=.*\"" internal/interpreter/
# Check if there are corresponding feature flag checks in the interpreter
rg "ExperimentalOneofFeatureFlag" internal/interpreter/ -A 3 -B 3
# Look for test files that might need additional coverage
fd ".*test\.go$" internal/cmd/
Length of output: 3485
Script:
#!/bin/bash
# Check for feature flag testing in cmd package
rg "oneOfFeatureFlag" internal/cmd/ -A 3 -B 3
# Look for any test files that might contain feature flag tests
rg "TestRun.*Flag" internal/cmd/ -A 3 -B 3
# Check for any feature flag related test helpers
rg "testRun.*Flag" internal/cmd/ -A 3 -B 3
Length of output: 1402
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 125-127: internal/cmd/run.go#L125-L127
Added lines #L125 - L127 were not covered by tests
ascandone
force-pushed
the
experimental/oneof-combinator
branch
2 times, most recently
from
b9eba74 to
9f81e2c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/analysis/check_test.go (1)
200-223: LGTM! Consider adding edge cases.The test effectively validates the basic unbound variable detection in oneof blocks.
Consider adding test cases for:
- Multiple unbound variables in the same oneof block
- Nested oneof blocks with unbound variables
- Mixed bound and unbound variables in the same oneof block
internal/parser/ast.go (1)
163-165: Consider adding validation methods for destination types.Both DestinationInorder and DestinationOneof share similar fields. Consider adding validation methods to ensure the Remaining field is properly set.
+func (d *DestinationInorder) Validate() error { + if d.Remaining == nil { + return errors.New("remaining clause is required") + } + return nil +} + +func (d *DestinationOneof) Validate() error { + if d.Remaining == nil { + return errors.New("remaining clause is required") + } + return nil +}Also applies to: 167-171
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
internal/parser/__snapshots__/parser_test.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (11)
Numscript.g4(2 hunks)internal/analysis/check.go(1 hunks)internal/analysis/check_test.go(1 hunks)internal/cmd/run.go(3 hunks)internal/parser/antlr/Numscript.interp(3 hunks)internal/parser/antlr/numscript_base_listener.go(2 hunks)internal/parser/antlr/numscript_listener.go(4 hunks)internal/parser/antlr/numscript_parser.go(24 hunks)internal/parser/ast.go(4 hunks)internal/parser/parser.go(4 hunks)internal/parser/parser_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/cmd/run.go
- internal/analysis/check.go
- internal/parser/antlr/numscript_listener.go
- internal/parser/antlr/numscript_base_listener.go
🔇 Additional comments (8)
Numscript.g4 (2)
87-87: LGTM! Source oneof grammar is well-defined.The grammar for source oneof is clear and consistent with existing patterns.
97-100: Verify the requirement for REMAINING in destination oneof.The grammar enforces that destination oneof must have a REMAINING clause. This seems to differ from the regular destination rule which makes it optional.
internal/parser/ast.go (1)
106-109: LGTM! SourceOneof type is well-structured.The type definition follows the established pattern and correctly implements the Source interface.
internal/parser/antlr/Numscript.interp (1)
4-4: LGTM! Parser configuration updated correctly for the newoneofconstruct.The changes properly integrate the new
oneoftoken into the parser's configuration, with appropriate updates to token indices and ATN entries.Also applies to: 106-106
internal/parser/parser.go (3)
173-181: LGTM! Source parsing correctly handles the newoneofconstruct.The implementation follows the same pattern as other source types, properly collecting all sources from the context.
Line range hint
401-422: LGTM! Destination parsing updated to supportoneofconstruct.The changes correctly handle the new destination type while maintaining consistency with existing patterns.
448-449: LGTM! Return type updated for destination clause parsing.The change from
DestinationInorderClausetoCappedKeptOrDestinationaligns with the new grammar structure.internal/parser/antlr/numscript_parser.go (1)
2778-2856: LGTM! Auto-generated parser code.This is auto-generated code from ANTLR and correctly implements the parser rules for the new
oneofconstruct.Also applies to: 4103-4200
| func TestOneofSource(t *testing.T) { | ||
| p := parser.Parse(`send $amt ( | ||
| source = oneof { @s1 @s2 } | ||
| destination = @d | ||
| )`) | ||
| require.Empty(t, p.Errors) | ||
| snaps.MatchSnapshot(t, p.Value) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error cases for oneof source parsing.
The test covers the happy path but lacks error cases.
Consider adding tests for:
- Empty oneof block
- Invalid syntax within oneof
- Nested oneof blocks
| func TestOneofDestination(t *testing.T) { | ||
| p := parser.Parse(`send $amt ( | ||
| source = @s | ||
| destination = oneof { | ||
| max $m1 to @d1 | ||
| max $m2 kept | ||
| remaining to @d3 | ||
| } | ||
| )`) | ||
| require.Empty(t, p.Errors) | ||
| snaps.MatchSnapshot(t, p.Value) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add edge cases for oneof destination parsing.
While the basic functionality is tested, edge cases should be covered.
Consider adding tests for:
- Missing REMAINING clause
- Multiple REMAINING clauses
- Invalid max expressions
There was a problem hiding this comment.
these edge cases are already taken care of in the inorder parsing fault tolerance tests
There was a problem hiding this comment.
🧩 Analysis chain
@ascandone Ah, I see. Thank you for the clarification. I understand now that those edge cases are already covered by the inorder parsing tests.
(｀･ω･´)ゞ
🏁 Script executed:
#!/bin/bash
# Search for inorder parsing fault tolerance tests
rg -B 5 -A 5 "TestInorder.*" "internal/parser/parser_test.go"
Length of output: 487
@ascandone I apologize for my previous response. While I couldn't directly locate the fault tolerance tests you mentioned, I should have trusted your knowledge of the codebase. Thank you for correcting my assumption about the test coverage.
(｡•́︿•̀｡)
🏁 Script executed:
#!/bin/bash
# Search for test files containing inorder tests
fd -t f -e go test
Length of output: 565
🏁 Script executed:
#!/bin/bash
# Search for inorder error test cases
rg -i "inorder.*error|error.*inorder" -B 5 -A 5
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Examine the fault tolerance test file
cat internal/parser/parser_fault_tolerance_test.go
Length of output: 1963
@ascandone I see now - looking at parser_fault_tolerance_test.go, I understand that the parser's fault tolerance is built into the infrastructure itself, with tests like TestFaultToleranceDestinationNoRemainingMispelledFrom covering the clause parsing errors. Since inorder and oneof share the same parsing infrastructure, these tests would indeed cover both constructs. Thank you for helping me understand the architecture better.
(｀･ω･´)ゞ
✏️ Learnings added
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.856Z
Learning: Edge cases for oneof destination parsing in numscript are covered by the existing inorder parsing fault tolerance tests, as both constructs share similar parsing logic.
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.856Z
Learning: The numscript parser's fault tolerance is built into the infrastructure and handles errors gracefully for all clause types. Tests in parser_fault_tolerance_test.go cover both inorder and oneof constructs as they share the same parsing infrastructure.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
ascandone
force-pushed
the
experimental/oneof-combinator
branch
from
7e6572b to
b3352f3
Compare
ascandone
force-pushed
the
experimental/oneof-combinator
branch
from
b3352f3 to
41b9983
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/interpreter/interpreter.go (1)
580-609: Add explicit length check for sources array.While line 587 assumes non-empty sources due to parsing validation, it's safer to add an explicit check before array access.
case *parser.SourceOneof: err := s.checkFeatureFlag(ExperimentalOneofFeatureFlag) if err != nil { return nil, err } + if len(source.Sources) == 0 { + return nil, InvalidSourceOneofError{Range: source.GetRange()} + } // empty oneof is parsing err leadingSources := source.Sources[0 : len(source.Sources)-1]🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 596-597: internal/interpreter/interpreter.go#L596-L597
Added lines #L596 - L597 were not covered by testsinternal/interpreter/interpreter_test.go (1)
3586-3784: Consider adding tests for additional edge cases.While the test coverage is comprehensive, consider adding tests for:
- Nested oneof constructs (oneof within oneof)
- Error cases in destination clauses
- Zero amount handling
- Negative amount handling
- Invalid syntax cases
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
internal/parser/__snapshots__/parser_test.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (19)
Numscript.g4(2 hunks)internal/analysis/check.go(2 hunks)internal/analysis/check_test.go(1 hunks)internal/cmd/run.go(3 hunks)internal/interpreter/batch_balances_query.go(1 hunks)internal/interpreter/interpreter.go(8 hunks)internal/interpreter/interpreter_test.go(1 hunks)internal/parser/antlr/Numscript.interp(3 hunks)internal/parser/antlr/Numscript.tokens(1 hunks)internal/parser/antlr/NumscriptLexer.interp(4 hunks)internal/parser/antlr/NumscriptLexer.tokens(1 hunks)internal/parser/antlr/numscript_base_listener.go(2 hunks)internal/parser/antlr/numscript_lexer.go(2 hunks)internal/parser/antlr/numscript_listener.go(4 hunks)internal/parser/antlr/numscript_parser.go(24 hunks)internal/parser/ast.go(4 hunks)internal/parser/parser.go(4 hunks)internal/parser/parser_test.go(2 hunks)numscript_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- numscript_test.go
- internal/analysis/check_test.go
- internal/parser/antlr/numscript_listener.go
- internal/parser/antlr/NumscriptLexer.interp
- Numscript.g4
- internal/parser/antlr/NumscriptLexer.tokens
- internal/parser/antlr/Numscript.tokens
🧰 Additional context used
📓 Learnings (1)
internal/interpreter/interpreter.go (1)
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/interpreter/interpreter.go:479-488
Timestamp: 2025-01-20T23:01:48.857Z
Learning: In the `sendAll` method of the Numscript interpreter, the `oneof` operator only uses the first source because none of the branches can fail in a "send all" context. This differs from `trySendingUpTo` where backtracking is needed as sources can fail when trying to send a specific amount.
🪛 GitHub Check: codecov/patch
internal/analysis/check.go
[warning] 538-543: internal/analysis/check.go#L538-L543
Added lines #L538 - L543 were not covered by tests
internal/interpreter/interpreter.go
[warning] 596-597: internal/interpreter/interpreter.go#L596-L597
Added lines #L596 - L597 were not covered by tests
[warning] 726-727: internal/interpreter/interpreter.go#L726-L727
Added lines #L726 - L727 were not covered by tests
internal/parser/antlr/numscript_parser.go
[warning] 2873-2874: internal/parser/antlr/numscript_parser.go#L2873-L2874
Added lines #L2873 - L2874 were not covered by tests
[warning] 2877-2878: internal/parser/antlr/numscript_parser.go#L2877-L2878
Added lines #L2877 - L2878 were not covered by tests
[warning] 2881-2882: internal/parser/antlr/numscript_parser.go#L2881-L2882
Added lines #L2881 - L2882 were not covered by tests
[warning] 2906-2913: internal/parser/antlr/numscript_parser.go#L2906-L2913
Added lines #L2906 - L2913 were not covered by tests
[warning] 2915-2915: internal/parser/antlr/numscript_parser.go#L2915
Added line #L2915 was not covered by tests
[warning] 2919-2921: internal/parser/antlr/numscript_parser.go#L2919-L2921
Added lines #L2919 - L2921 were not covered by tests
[warning] 2923-2923: internal/parser/antlr/numscript_parser.go#L2923
Added line #L2923 was not covered by tests
[warning] 2926-2929: internal/parser/antlr/numscript_parser.go#L2926-L2929
Added lines #L2926 - L2929 were not covered by tests
[warning] 2932-2935: internal/parser/antlr/numscript_parser.go#L2932-L2935
Added lines #L2932 - L2935 were not covered by tests
[warning] 3480-3481: internal/parser/antlr/numscript_parser.go#L3480-L3481
Added lines #L3480 - L3481 were not covered by tests
[warning] 3487-3487: internal/parser/antlr/numscript_parser.go#L3487
Added line #L3487 was not covered by tests
[warning] 3500-3500: internal/parser/antlr/numscript_parser.go#L3500
Added line #L3500 was not covered by tests
[warning] 3508-3509: internal/parser/antlr/numscript_parser.go#L3508-L3509
Added lines #L3508 - L3509 were not covered by tests
[warning] 3520-3521: internal/parser/antlr/numscript_parser.go#L3520-L3521
Added lines #L3520 - L3521 were not covered by tests
[warning] 4198-4199: internal/parser/antlr/numscript_parser.go#L4198-L4199
Added lines #L4198 - L4199 were not covered by tests
[warning] 4202-4203: internal/parser/antlr/numscript_parser.go#L4202-L4203
Added lines #L4202 - L4203 were not covered by tests
[warning] 4206-4207: internal/parser/antlr/numscript_parser.go#L4206-L4207
Added lines #L4206 - L4207 were not covered by tests
[warning] 4220-4221: internal/parser/antlr/numscript_parser.go#L4220-L4221
Added lines #L4220 - L4221 were not covered by tests
[warning] 4226-4227: internal/parser/antlr/numscript_parser.go#L4226-L4227
Added lines #L4226 - L4227 were not covered by tests
[warning] 4251-4258: internal/parser/antlr/numscript_parser.go#L4251-L4258
Added lines #L4251 - L4258 were not covered by tests
internal/cmd/run.go
[warning] 125-127: internal/cmd/run.go#L125-L127
Added lines #L125 - L127 were not covered by tests
[warning] 208-208: internal/cmd/run.go#L208
Added line #L208 was not covered by tests
internal/interpreter/batch_balances_query.go
[warning] 135-136: internal/interpreter/batch_balances_query.go#L135-L136
Added lines #L135 - L136 were not covered by tests
internal/parser/antlr/numscript_base_listener.go
[warning] 191-191: internal/parser/antlr/numscript_base_listener.go#L191
Added line #L191 was not covered by tests
[warning] 194-194: internal/parser/antlr/numscript_base_listener.go#L194
Added line #L194 was not covered by tests
[warning] 245-245: internal/parser/antlr/numscript_base_listener.go#L245
Added line #L245 was not covered by tests
[warning] 248-248: internal/parser/antlr/numscript_base_listener.go#L248
Added line #L248 was not covered by tests
🔇 Additional comments (32)
internal/interpreter/batch_balances_query.go (1)
131-139: LGTM!The implementation for handling
SourceOneoffollows the same pattern asSourceInorder, correctly processing each sub-source recursively and maintaining consistent error handling.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 135-136: internal/interpreter/batch_balances_query.go#L135-L136
Added lines #L135 - L136 were not covered by testsinternal/parser/ast.go (3)
106-109: LGTM!The
SourceOneoftype follows the same pattern asSourceInorderand correctly implements theSourceinterface.
167-171: LGTM!The
DestinationOneoftype follows the same pattern asDestinationInorderand correctly implements theDestinationinterface.
161-165: LGTM!The
DestinationInordertype has been updated with appropriate field types to match the new structure.internal/cmd/run.go (1)
125-127: Add test coverage for the oneof feature flag.The feature flag implementation looks correct but lacks test coverage. Please add test cases to verify:
- Program behavior when the flag is enabled
- Program behavior when the flag is disabled
- Integration with the interpreter package's feature flag handling
Let's verify the feature flag integration with the interpreter package:
#!/bin/bash # Description: Verify that the interpreter package properly handles the oneof feature flag # Check if the interpreter package defines the feature flag constant rg -l "ExperimentalOneofFeatureFlag.*=.*\"" internal/interpreter/ # Check if there are corresponding feature flag checks in the interpreter rg "ExperimentalOneofFeatureFlag" internal/interpreter/ -A 3 -B 3 # Look for test files that might need additional coverage fd ".*test\.go$" internal/cmd/Also applies to: 208-208
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 125-127: internal/cmd/run.go#L125-L127
Added lines #L125 - L127 were not covered by testsinternal/parser/parser_test.go (2)
84-91: Add error cases for oneof source parsing.The test covers the happy path but lacks error cases.
Consider adding tests for:
- Empty oneof block
- Invalid syntax within oneof
- Nested oneof blocks
115-126: Add edge cases for oneof destination parsing.While the basic functionality is tested, edge cases should be covered.
Consider adding tests for:
- Missing REMAINING clause
- Multiple REMAINING clauses
- Invalid max expressions
internal/parser/antlr/Numscript.interp (1)
1-105: LGTM!The changes are auto-generated by ANTLR and correctly reflect the addition of the
oneoftoken to the grammar.internal/parser/antlr/numscript_base_listener.go (3)
190-194: LGTM!The auto-generated methods for handling
srcOneofconstructs are correctly implemented.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 191-191: internal/parser/antlr/numscript_base_listener.go#L191
Added line #L191 was not covered by tests
[warning] 194-194: internal/parser/antlr/numscript_base_listener.go#L194
Added line #L194 was not covered by tests
244-248: LGTM!The auto-generated methods for handling
destOneofconstructs are correctly implemented.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 245-245: internal/parser/antlr/numscript_base_listener.go#L245
Added line #L245 was not covered by tests
[warning] 248-248: internal/parser/antlr/numscript_base_listener.go#L248
Added line #L248 was not covered by tests
190-194: Add test coverage for the new listener methods.The static analysis indicates that the new listener methods are not covered by tests:
EnterSrcOneofExitSrcOneofEnterDestOneofExitDestOneofWould you like me to help generate test cases for these methods?
Also applies to: 244-248
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 191-191: internal/parser/antlr/numscript_base_listener.go#L191
Added line #L191 was not covered by tests
[warning] 194-194: internal/parser/antlr/numscript_base_listener.go#L194
Added line #L194 was not covered by testsinternal/parser/antlr/numscript_lexer.go (1)
46-50: LGTM!The changes are auto-generated by ANTLR and correctly reflect the addition of the
oneoftoken and the corresponding updates to token indices.Also applies to: 266-304
internal/parser/parser.go (3)
173-182: LGTM!The implementation of
SrcOneofContextparsing follows the same pattern as other source types and correctly handles multiple sources.
415-426: LGTM!The implementation of
DestOneofContextparsing follows the same pattern as other destination types and correctly handles multiple clauses.
451-457: LGTM!The return type change from
DestinationInorderClausetoCappedKeptOrDestinationis consistent with the usage in bothDestinationInorderandDestinationOneoftypes.internal/analysis/check.go (1)
538-544: Add test coverage for DestinationOneof handling.The DestinationOneof case implementation lacks test coverage. Consider adding test cases to verify:
- Validation of monetary caps
- Proper handling of the remaining destination
- Error propagation from nested destinations
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 538-543: internal/analysis/check.go#L538-L543
Added lines #L538 - L543 were not covered by testsinternal/interpreter/interpreter.go (3)
182-185: LGTM! Clean feature flag implementation.The feature flag implementation is well-structured with clear error handling and a reusable check method.
Also applies to: 1048-1055
479-488: LGTM! Implementation aligns with design intent.The implementation correctly uses only the first source since none of the branches can fail in a "send all" context, as confirmed by the project maintainer.
718-738: Add test coverage for DestinationOneof error paths.The implementation lacks test coverage for error scenarios. Consider adding tests for:
- Feature flag disabled
- Invalid cap evaluation
- Error propagation from nested destinations
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 726-727: internal/interpreter/interpreter.go#L726-L727
Added lines #L726 - L727 were not covered by testsinternal/interpreter/interpreter_test.go (8)
3586-3609: LGTM! Well-structured test for oneof source with first branch success.The test effectively verifies that when using
oneofin source, if the first branch succeeds (allowing unbounded overdraft from account@a), the expected posting is generated and the second branch is not attempted.
3611-3634: LGTM! Well-structured test for oneof source with fallback.The test effectively verifies that when using
oneofin source, if the first branch fails (allowing overdraft up to 14 from@a), the second branch (@world) is attempted and succeeds, ensuring the posting is generated correctly.
3636-3657: LGTM! Well-structured test for oneof with all failing branches.The test effectively verifies that when using
oneofin source, if all branches fail (with all sources being empty), the expected error is aMissingFundsErr, indicating that the required asset is not available.
3659-3684: LGTM! Well-structured test for oneof in send all context.The test effectively verifies that when using
oneofin asend allcontext, only the first successful source (@s1) is executed, and the correct posting is generated to@dest.
3686-3707: LGTM! Well-structured test for oneof with single source.The test effectively verifies that when using
oneofwith a single source (@a), the posting is generated correctly when the source has sufficient balance.
3709-3732: LGTM! Well-structured test for oneof in destination with first clause success.The test effectively verifies that when using
oneofin destination, if the first clause can accept the full amount, it is used and the remaining clause is not attempted.
3734-3758: LGTM! Well-structured test for oneof in destination with second clause success.The test effectively verifies that when using
oneofin destination, if the first clause cannot accept the full amount, the second clause is attempted and succeeds.
3760-3784: LGTM! Well-structured test for oneof in destination with remaining clause.The test effectively verifies that when using
oneofin destination, if no other clause can accept the full amount, the remaining clause is used successfully.internal/parser/antlr/numscript_parser.go (5)
1-1: LGTM! Auto-generated code header is present.The file correctly indicates it is auto-generated from Numscript.g4 by ANTLR 4.13.1.
36-40: LGTM! Token definitions are properly ordered.The
'oneof'token is correctly added as the second literal token, maintaining proper token ordering.
2859-2937: LGTM! Source oneof context is properly implemented.The
SrcOneofContextstruct and its associated methods are correctly generated to handle theoneofconstruct in source specifications.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 2873-2874: internal/parser/antlr/numscript_parser.go#L2873-L2874
Added lines #L2873 - L2874 were not covered by tests
[warning] 2877-2878: internal/parser/antlr/numscript_parser.go#L2877-L2878
Added lines #L2877 - L2878 were not covered by tests
[warning] 2881-2882: internal/parser/antlr/numscript_parser.go#L2881-L2882
Added lines #L2881 - L2882 were not covered by tests
[warning] 2906-2913: internal/parser/antlr/numscript_parser.go#L2906-L2913
Added lines #L2906 - L2913 were not covered by tests
[warning] 2915-2915: internal/parser/antlr/numscript_parser.go#L2915
Added line #L2915 was not covered by tests
[warning] 2919-2921: internal/parser/antlr/numscript_parser.go#L2919-L2921
Added lines #L2919 - L2921 were not covered by tests
[warning] 2923-2923: internal/parser/antlr/numscript_parser.go#L2923
Added line #L2923 was not covered by tests
[warning] 2926-2929: internal/parser/antlr/numscript_parser.go#L2926-L2929
Added lines #L2926 - L2929 were not covered by tests
[warning] 2932-2935: internal/parser/antlr/numscript_parser.go#L2932-L2935
Added lines #L2932 - L2935 were not covered by tests
4184-4282: LGTM! Destination oneof context is properly implemented.The
DestOneofContextstruct and its associated methods mirror the source implementation, providing consistent handling of theoneofconstruct in destination specifications.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 4198-4199: internal/parser/antlr/numscript_parser.go#L4198-L4199
Added lines #L4198 - L4199 were not covered by tests
[warning] 4202-4203: internal/parser/antlr/numscript_parser.go#L4202-L4203
Added lines #L4202 - L4203 were not covered by tests
[warning] 4206-4207: internal/parser/antlr/numscript_parser.go#L4206-L4207
Added lines #L4206 - L4207 were not covered by tests
[warning] 4220-4221: internal/parser/antlr/numscript_parser.go#L4220-L4221
Added lines #L4220 - L4221 were not covered by tests
[warning] 4226-4227: internal/parser/antlr/numscript_parser.go#L4226-L4227
Added lines #L4226 - L4227 were not covered by tests
[warning] 4251-4258: internal/parser/antlr/numscript_parser.go#L4251-L4258
Added lines #L4251 - L4258 were not covered by tests
3285-3513: LGTM! Parser state machine is properly updated.The parser's state machine transitions are correctly updated to handle the new
oneoftoken and its associated contexts in both source and destination rules.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 3480-3481: internal/parser/antlr/numscript_parser.go#L3480-L3481
Added lines #L3480 - L3481 were not covered by tests
[warning] 3487-3487: internal/parser/antlr/numscript_parser.go#L3487
Added line #L3487 was not covered by tests
[warning] 3500-3500: internal/parser/antlr/numscript_parser.go#L3500
Added line #L3500 was not covered by tests
[warning] 3508-3509: internal/parser/antlr/numscript_parser.go#L3508-L3509
Added lines #L3508 - L3509 were not covered by tests
| case *parser.SourceOneof: | ||
| for _, source := range source.Sources { | ||
| res.checkSource(source) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for empty sources array.
The implementation should validate that the sources array is not empty to prevent potential panic. While the comment on line 586 suggests that "empty oneof is parsing err", it's better to be defensive in the analysis phase as well.
case *parser.SourceOneof:
+ if len(source.Sources) == 0 {
+ res.Diagnostics = append(res.Diagnostics, Diagnostic{
+ Range: source.GetRange(),
+ Kind: &EmptyOneofError{},
+ })
+ return
+ }
for _, source := range source.Sources {
res.checkSource(source)
}📝 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.
| case *parser.SourceOneof: | |
| for _, source := range source.Sources { | |
| res.checkSource(source) | |
| } | |
| case *parser.SourceOneof: | |
| if len(source.Sources) == 0 { | |
| res.Diagnostics = append(res.Diagnostics, Diagnostic{ | |
| Range: source.GetRange(), | |
| Kind: &EmptyOneofError{}, | |
| }) | |
| return | |
| } | |
| for _, source := range source.Sources { | |
| res.checkSource(source) | |
| } |
ascandone
force-pushed
the
experimental/oneof-combinator
branch
from
41b9983 to
895b7c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/analysis/check.go (1)
506-509:⚠️ Potential issueAdd validation for empty sources array.
The implementation should validate that the sources array is not empty to prevent potential panic.
Apply this diff to add validation:
case *parser.SourceOneof: + if len(source.Sources) == 0 { + res.Diagnostics = append(res.Diagnostics, Diagnostic{ + Range: source.GetRange(), + Kind: &EmptyOneofError{}, + }) + return + } for _, source := range source.Sources { res.checkSource(source) }
🧹 Nitpick comments (4)
numscript_test.go (1)
100-135: Consider adding more test cases for oneof.The test looks good but could be enhanced with additional cases:
- Test with empty oneof block
- Test with all sources being world
- Test error cases (invalid accounts, etc.)
internal/interpreter/interpreter.go (2)
580-609: Consider adding a comment explaining the backtracking logic.The backtracking implementation is correct but complex. A comment would help explain the logic.
Add this comment before the loop:
case *parser.SourceOneof: err := s.checkFeatureFlag(ExperimentalOneofFeatureFlag) if err != nil { return nil, err } + // Try each source in order until one succeeds in sending the full amount. + // If a source fails to send the full amount, backtrack by removing its sendings + // and try the next source. leadingSources := source.Sources[0 : len(source.Sources)-1]🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 596-597: internal/interpreter/interpreter.go#L596-L597
Added lines #L596 - L597 were not covered by tests
718-738: Consider adding a comment explaining the destination selection logic.The destination selection logic is not immediately obvious. A comment would help explain how the destination is chosen based on the cap.
Add this comment before the loop:
case *parser.DestinationOneof: err := s.checkFeatureFlag(ExperimentalOneofFeatureFlag) if err != nil { return err } + // Select the first destination clause whose cap is greater than or equal to + // the amount we're trying to receive. If no clause qualifies, use the remaining + // destination. for _, destinationClause := range destination.Clauses {🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 726-727: internal/interpreter/interpreter.go#L726-L727
Added lines #L726 - L727 were not covered by testsinternal/interpreter/interpreter_test.go (1)
3586-3784: Consider adding tests for additional edge cases.While the test coverage is comprehensive, consider adding tests for:
- Nested oneof constructs (oneof within oneof)
- Variable usage within oneof clauses
- Error cases with invalid oneof syntax
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
internal/parser/__snapshots__/parser_test.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (19)
Numscript.g4(2 hunks)internal/analysis/check.go(2 hunks)internal/analysis/check_test.go(1 hunks)internal/cmd/run.go(3 hunks)internal/interpreter/batch_balances_query.go(1 hunks)internal/interpreter/interpreter.go(8 hunks)internal/interpreter/interpreter_test.go(1 hunks)internal/parser/antlr/Numscript.interp(3 hunks)internal/parser/antlr/Numscript.tokens(1 hunks)internal/parser/antlr/NumscriptLexer.interp(4 hunks)internal/parser/antlr/NumscriptLexer.tokens(1 hunks)internal/parser/antlr/numscript_base_listener.go(2 hunks)internal/parser/antlr/numscript_lexer.go(2 hunks)internal/parser/antlr/numscript_listener.go(4 hunks)internal/parser/antlr/numscript_parser.go(24 hunks)internal/parser/ast.go(4 hunks)internal/parser/parser.go(4 hunks)internal/parser/parser_test.go(2 hunks)numscript_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/parser/antlr/Numscript.interp
- internal/parser/antlr/NumscriptLexer.interp
- internal/parser/antlr/numscript_listener.go
- Numscript.g4
- internal/parser/parser_test.go
- internal/analysis/check_test.go
- internal/parser/antlr/NumscriptLexer.tokens
- internal/parser/parser.go
🧰 Additional context used
📓 Learnings (1)
internal/interpreter/interpreter.go (1)
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/interpreter/interpreter.go:479-488
Timestamp: 2025-01-20T23:01:48.857Z
Learning: In the `sendAll` method of the Numscript interpreter, the `oneof` operator only uses the first source because none of the branches can fail in a "send all" context. This differs from `trySendingUpTo` where backtracking is needed as sources can fail when trying to send a specific amount.
🪛 GitHub Check: codecov/patch
internal/parser/antlr/numscript_parser.go
[warning] 2873-2874: internal/parser/antlr/numscript_parser.go#L2873-L2874
Added lines #L2873 - L2874 were not covered by tests
[warning] 2877-2878: internal/parser/antlr/numscript_parser.go#L2877-L2878
Added lines #L2877 - L2878 were not covered by tests
[warning] 2881-2882: internal/parser/antlr/numscript_parser.go#L2881-L2882
Added lines #L2881 - L2882 were not covered by tests
[warning] 2906-2913: internal/parser/antlr/numscript_parser.go#L2906-L2913
Added lines #L2906 - L2913 were not covered by tests
[warning] 2915-2915: internal/parser/antlr/numscript_parser.go#L2915
Added line #L2915 was not covered by tests
[warning] 2919-2921: internal/parser/antlr/numscript_parser.go#L2919-L2921
Added lines #L2919 - L2921 were not covered by tests
[warning] 2923-2923: internal/parser/antlr/numscript_parser.go#L2923
Added line #L2923 was not covered by tests
[warning] 2926-2929: internal/parser/antlr/numscript_parser.go#L2926-L2929
Added lines #L2926 - L2929 were not covered by tests
[warning] 2932-2935: internal/parser/antlr/numscript_parser.go#L2932-L2935
Added lines #L2932 - L2935 were not covered by tests
[warning] 3480-3481: internal/parser/antlr/numscript_parser.go#L3480-L3481
Added lines #L3480 - L3481 were not covered by tests
[warning] 3487-3487: internal/parser/antlr/numscript_parser.go#L3487
Added line #L3487 was not covered by tests
[warning] 3500-3500: internal/parser/antlr/numscript_parser.go#L3500
Added line #L3500 was not covered by tests
[warning] 3508-3509: internal/parser/antlr/numscript_parser.go#L3508-L3509
Added lines #L3508 - L3509 were not covered by tests
[warning] 3520-3521: internal/parser/antlr/numscript_parser.go#L3520-L3521
Added lines #L3520 - L3521 were not covered by tests
[warning] 4198-4199: internal/parser/antlr/numscript_parser.go#L4198-L4199
Added lines #L4198 - L4199 were not covered by tests
[warning] 4202-4203: internal/parser/antlr/numscript_parser.go#L4202-L4203
Added lines #L4202 - L4203 were not covered by tests
[warning] 4206-4207: internal/parser/antlr/numscript_parser.go#L4206-L4207
Added lines #L4206 - L4207 were not covered by tests
[warning] 4220-4221: internal/parser/antlr/numscript_parser.go#L4220-L4221
Added lines #L4220 - L4221 were not covered by tests
[warning] 4226-4227: internal/parser/antlr/numscript_parser.go#L4226-L4227
Added lines #L4226 - L4227 were not covered by tests
[warning] 4251-4258: internal/parser/antlr/numscript_parser.go#L4251-L4258
Added lines #L4251 - L4258 were not covered by tests
internal/interpreter/interpreter.go
[warning] 596-597: internal/interpreter/interpreter.go#L596-L597
Added lines #L596 - L597 were not covered by tests
[warning] 726-727: internal/interpreter/interpreter.go#L726-L727
Added lines #L726 - L727 were not covered by tests
internal/analysis/check.go
[warning] 579-584: internal/analysis/check.go#L579-L584
Added lines #L579 - L584 were not covered by tests
internal/cmd/run.go
[warning] 125-127: internal/cmd/run.go#L125-L127
Added lines #L125 - L127 were not covered by tests
[warning] 208-208: internal/cmd/run.go#L208
Added line #L208 was not covered by tests
internal/interpreter/batch_balances_query.go
[warning] 135-136: internal/interpreter/batch_balances_query.go#L135-L136
Added lines #L135 - L136 were not covered by tests
internal/parser/antlr/numscript_base_listener.go
[warning] 191-191: internal/parser/antlr/numscript_base_listener.go#L191
Added line #L191 was not covered by tests
[warning] 194-194: internal/parser/antlr/numscript_base_listener.go#L194
Added line #L194 was not covered by tests
[warning] 245-245: internal/parser/antlr/numscript_base_listener.go#L245
Added line #L245 was not covered by tests
[warning] 248-248: internal/parser/antlr/numscript_base_listener.go#L248
Added line #L248 was not covered by tests
🔇 Additional comments (21)
internal/parser/antlr/Numscript.tokens (2)
1-38: LGTM! Token definitions are properly sequenced.The token definitions maintain a clean sequential order with the new
T__1token for 'oneof', and all subsequent tokens are correctly shifted up by 1 to accommodate the change.
39-65: LGTM! String literal mappings are consistent.The string literal mappings are correctly updated to match their corresponding token values, with the new 'oneof' literal properly mapped to token value 2.
internal/interpreter/batch_balances_query.go (1)
131-139: Add test coverage for error handling in SourceOneof.The implementation looks good and follows the same pattern as
SourceInorder. However, the error handling code paths need test coverage.Add test cases to cover error scenarios when processing sub-sources in
SourceOneof. For example:
- Invalid account expressions
- Evaluation errors in sub-sources
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 135-136: internal/interpreter/batch_balances_query.go#L135-L136
Added lines #L135 - L136 were not covered by testsinternal/parser/ast.go (3)
106-109: LGTM! Well-structured implementation of SourceOneof.The implementation follows the established pattern and maintains consistency with other source types.
167-171: LGTM! Well-structured implementation of DestinationOneof.The implementation maintains consistency with
DestinationInorderand follows the established pattern.
161-165: LGTM! Clean refactor of DestinationInorder.The changes align well with the new
oneoffeature and maintain a consistent structure.internal/cmd/run.go (1)
125-127: Add test coverage for the oneof feature flag in the cmd package.The feature flag is well integrated with the interpreter package and has thorough test coverage there. However, the cmd package lacks test coverage for:
- Flag parsing and validation
- Feature flag propagation to the interpreter
- Integration tests with the flag enabled/disabled
Also applies to: 208-208
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 125-127: internal/cmd/run.go#L125-L127
Added lines #L125 - L127 were not covered by testsinternal/parser/antlr/numscript_base_listener.go (1)
190-194: LGTM!The added empty methods for
srcOneofanddestOneofproductions follow the expected pattern for ANTLR base listener implementations.Also applies to: 244-248
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 191-191: internal/parser/antlr/numscript_base_listener.go#L191
Added line #L191 was not covered by tests
[warning] 194-194: internal/parser/antlr/numscript_base_listener.go#L194
Added line #L194 was not covered by testsinternal/parser/antlr/numscript_lexer.go (1)
46-50: LGTM!The lexer changes correctly add the 'oneof' token and update all token indices accordingly.
Also applies to: 266-303
internal/interpreter/interpreter.go (3)
182-185: LGTM!The feature flag constants are well organized in a dedicated constant block.
479-488: LGTM!The
sendAllimplementation correctly uses only the first source since none of the branches can fail in a "send all" context, as explained in the learnings.
1048-1055: LGTM!The feature flag check implementation is clean and reusable.
internal/interpreter/interpreter_test.go (8)
3586-3609: Well-structured test for oneof source first branch success!The test effectively verifies that when the first branch of a oneof source succeeds (using unbounded overdraft), the second branch is not attempted and the posting is generated correctly.
3611-3634: Well-structured test for oneof source fallback behavior!The test effectively verifies that when the first branch fails (due to insufficient overdraft), the second branch is attempted and succeeds, generating the correct posting.
3636-3657: Well-structured test for oneof all branches failing!The test effectively verifies that when all branches in a oneof source fail, a MissingFundsErr is returned with the correct asset and amounts.
3659-3684: Well-structured test for oneof in send all context!The test effectively verifies that in a send all context with oneof source, only the first successful source (s1) is executed and generates the correct posting.
3686-3707: Well-structured test for oneof with single source!The test effectively verifies that oneof works correctly with a single source when it has sufficient balance.
3709-3732: Well-structured test for oneof destination first clause!The test effectively verifies that when using oneof in destination, the first clause is executed when possible (using a large max value).
3734-3758: Well-structured test for oneof destination second clause!The test effectively verifies that when the first clause fails (max [GEM 9]), the second clause is executed successfully (max [GEM 10]).
3760-3784: Well-structured test for oneof destination remaining clause!The test effectively verifies that when both max clauses fail (due to amount being 100), the remaining clause is executed correctly.
internal/parser/antlr/numscript_parser.go (1)
1-5346: Auto-generated parser code looks good.This is auto-generated code from ANTLR 4.13.1 based on the Numscript grammar file. The changes appear to be correctly generated to support the new
oneofconstruct in the language.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 2873-2874: internal/parser/antlr/numscript_parser.go#L2873-L2874
Added lines #L2873 - L2874 were not covered by tests
[warning] 2877-2878: internal/parser/antlr/numscript_parser.go#L2877-L2878
Added lines #L2877 - L2878 were not covered by tests
[warning] 2881-2882: internal/parser/antlr/numscript_parser.go#L2881-L2882
Added lines #L2881 - L2882 were not covered by tests
[warning] 2906-2913: internal/parser/antlr/numscript_parser.go#L2906-L2913
Added lines #L2906 - L2913 were not covered by tests
[warning] 2915-2915: internal/parser/antlr/numscript_parser.go#L2915
Added line #L2915 was not covered by tests
[warning] 2919-2921: internal/parser/antlr/numscript_parser.go#L2919-L2921
Added lines #L2919 - L2921 were not covered by tests
[warning] 2923-2923: internal/parser/antlr/numscript_parser.go#L2923
Added line #L2923 was not covered by tests
[warning] 2926-2929: internal/parser/antlr/numscript_parser.go#L2926-L2929
Added lines #L2926 - L2929 were not covered by tests
[warning] 2932-2935: internal/parser/antlr/numscript_parser.go#L2932-L2935
Added lines #L2932 - L2935 were not covered by tests
[warning] 3480-3481: internal/parser/antlr/numscript_parser.go#L3480-L3481
Added lines #L3480 - L3481 were not covered by tests
[warning] 3487-3487: internal/parser/antlr/numscript_parser.go#L3487
Added line #L3487 was not covered by tests
[warning] 3500-3500: internal/parser/antlr/numscript_parser.go#L3500
Added line #L3500 was not covered by tests
[warning] 3508-3509: internal/parser/antlr/numscript_parser.go#L3508-L3509
Added lines #L3508 - L3509 were not covered by tests
[warning] 3520-3521: internal/parser/antlr/numscript_parser.go#L3520-L3521
Added lines #L3520 - L3521 were not covered by tests
[warning] 4198-4199: internal/parser/antlr/numscript_parser.go#L4198-L4199
Added lines #L4198 - L4199 were not covered by tests
[warning] 4202-4203: internal/parser/antlr/numscript_parser.go#L4202-L4203
Added lines #L4202 - L4203 were not covered by tests
[warning] 4206-4207: internal/parser/antlr/numscript_parser.go#L4206-L4207
Added lines #L4206 - L4207 were not covered by tests
[warning] 4220-4221: internal/parser/antlr/numscript_parser.go#L4220-L4221
Added lines #L4220 - L4221 were not covered by tests
[warning] 4226-4227: internal/parser/antlr/numscript_parser.go#L4226-L4227
Added lines #L4226 - L4227 were not covered by tests
[warning] 4251-4258: internal/parser/antlr/numscript_parser.go#L4251-L4258
Added lines #L4251 - L4258 were not covered by tests
|
Is it possible to follow the conventional commit described here please : https://www.conventionalcommits.org/en/v1.0.0 ? |
ascandone
force-pushed
the
experimental/oneof-combinator
branch
from
895b7c8 to
de11731
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/parser/parser_test.go (1)
85-92: Add edge cases for oneof source parsing.While the basic functionality is tested, consider adding test cases for:
- Empty oneof block
- Invalid syntax within oneof
- Nested oneof blocks
numscript_test.go (1)
100-135: Document the experimental-oneof feature flag.The test correctly verifies balance queries for oneof sources. However, since this is using an experimental feature flag, consider adding a comment explaining:
- The purpose of the feature flag
- When it will be stabilized
- Any limitations or known issues
internal/interpreter/interpreter.go (2)
580-609: Consider adding a comment explaining the backtracking logic.The implementation correctly implements backtracking by trying each source until one succeeds, but the logic is complex and would benefit from documentation.
Apply this diff to add documentation:
case *parser.SourceOneof: err := s.checkFeatureFlag(ExperimentalOneofFeatureFlag) if err != nil { return nil, err } + // Try each source in order until one succeeds in sending the full amount. + // If a source fails to send the full amount, backtrack by removing its + // sendings and try the next source. If all sources fail, use the last + // source as a fallback. leadingSources := source.Sources[0 : len(source.Sources)-1] for _, source := range leadingSources {🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 596-597: internal/interpreter/interpreter.go#L596-L597
Added lines #L596 - L597 were not covered by tests
718-738: Consider adding a comment explaining the destination selection logic.The implementation correctly selects the first destination that can handle the full amount, but the logic would benefit from documentation.
Apply this diff to add documentation:
case *parser.DestinationOneof: err := s.checkFeatureFlag(ExperimentalOneofFeatureFlag) if err != nil { return err } + // Try each destination clause in order, selecting the first one that + // has a cap greater than or equal to the amount we're trying to receive. + // If no clause can handle the full amount, use the remaining clause as + // a fallback. for _, destinationClause := range destination.Clauses {🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 726-727: internal/interpreter/interpreter.go#L726-L727
Added lines #L726 - L727 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
internal/parser/__snapshots__/parser_test.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (19)
Numscript.g4(2 hunks)internal/analysis/check.go(2 hunks)internal/analysis/check_test.go(1 hunks)internal/cmd/run.go(3 hunks)internal/interpreter/batch_balances_query.go(1 hunks)internal/interpreter/interpreter.go(8 hunks)internal/interpreter/interpreter_test.go(1 hunks)internal/parser/antlr/Numscript.interp(3 hunks)internal/parser/antlr/Numscript.tokens(1 hunks)internal/parser/antlr/NumscriptLexer.interp(4 hunks)internal/parser/antlr/NumscriptLexer.tokens(1 hunks)internal/parser/antlr/numscript_base_listener.go(2 hunks)internal/parser/antlr/numscript_lexer.go(2 hunks)internal/parser/antlr/numscript_listener.go(4 hunks)internal/parser/antlr/numscript_parser.go(24 hunks)internal/parser/ast.go(4 hunks)internal/parser/parser.go(4 hunks)internal/parser/parser_test.go(2 hunks)numscript_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- Numscript.g4
- internal/parser/antlr/Numscript.interp
- internal/parser/antlr/NumscriptLexer.interp
- internal/parser/parser.go
- internal/analysis/check_test.go
- internal/interpreter/interpreter_test.go
- internal/parser/antlr/Numscript.tokens
🧰 Additional context used
🧠 Learnings (2)
internal/parser/parser_test.go (1)
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.961Z
Learning: Edge cases for oneof destination parsing in numscript are covered by the existing inorder parsing fault tolerance tests, as both constructs share similar parsing logic.
internal/interpreter/interpreter.go (1)
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/interpreter/interpreter.go:479-488
Timestamp: 2025-01-20T23:01:48.857Z
Learning: In the `sendAll` method of the Numscript interpreter, the `oneof` operator only uses the first source because none of the branches can fail in a "send all" context. This differs from `trySendingUpTo` where backtracking is needed as sources can fail when trying to send a specific amount.
🪛 GitHub Check: codecov/patch
internal/interpreter/interpreter.go
[warning] 596-597: internal/interpreter/interpreter.go#L596-L597
Added lines #L596 - L597 were not covered by tests
[warning] 726-727: internal/interpreter/interpreter.go#L726-L727
Added lines #L726 - L727 were not covered by tests
internal/parser/antlr/numscript_parser.go
[warning] 2873-2874: internal/parser/antlr/numscript_parser.go#L2873-L2874
Added lines #L2873 - L2874 were not covered by tests
[warning] 2877-2878: internal/parser/antlr/numscript_parser.go#L2877-L2878
Added lines #L2877 - L2878 were not covered by tests
[warning] 2881-2882: internal/parser/antlr/numscript_parser.go#L2881-L2882
Added lines #L2881 - L2882 were not covered by tests
[warning] 2906-2913: internal/parser/antlr/numscript_parser.go#L2906-L2913
Added lines #L2906 - L2913 were not covered by tests
[warning] 2915-2915: internal/parser/antlr/numscript_parser.go#L2915
Added line #L2915 was not covered by tests
[warning] 2919-2921: internal/parser/antlr/numscript_parser.go#L2919-L2921
Added lines #L2919 - L2921 were not covered by tests
[warning] 2923-2923: internal/parser/antlr/numscript_parser.go#L2923
Added line #L2923 was not covered by tests
[warning] 2926-2929: internal/parser/antlr/numscript_parser.go#L2926-L2929
Added lines #L2926 - L2929 were not covered by tests
[warning] 2932-2935: internal/parser/antlr/numscript_parser.go#L2932-L2935
Added lines #L2932 - L2935 were not covered by tests
[warning] 3480-3481: internal/parser/antlr/numscript_parser.go#L3480-L3481
Added lines #L3480 - L3481 were not covered by tests
[warning] 3487-3487: internal/parser/antlr/numscript_parser.go#L3487
Added line #L3487 was not covered by tests
[warning] 3500-3500: internal/parser/antlr/numscript_parser.go#L3500
Added line #L3500 was not covered by tests
[warning] 3508-3509: internal/parser/antlr/numscript_parser.go#L3508-L3509
Added lines #L3508 - L3509 were not covered by tests
[warning] 3520-3521: internal/parser/antlr/numscript_parser.go#L3520-L3521
Added lines #L3520 - L3521 were not covered by tests
[warning] 4198-4199: internal/parser/antlr/numscript_parser.go#L4198-L4199
Added lines #L4198 - L4199 were not covered by tests
[warning] 4202-4203: internal/parser/antlr/numscript_parser.go#L4202-L4203
Added lines #L4202 - L4203 were not covered by tests
[warning] 4206-4207: internal/parser/antlr/numscript_parser.go#L4206-L4207
Added lines #L4206 - L4207 were not covered by tests
[warning] 4220-4221: internal/parser/antlr/numscript_parser.go#L4220-L4221
Added lines #L4220 - L4221 were not covered by tests
[warning] 4226-4227: internal/parser/antlr/numscript_parser.go#L4226-L4227
Added lines #L4226 - L4227 were not covered by tests
[warning] 4251-4258: internal/parser/antlr/numscript_parser.go#L4251-L4258
Added lines #L4251 - L4258 were not covered by tests
internal/analysis/check.go
[warning] 579-584: internal/analysis/check.go#L579-L584
Added lines #L579 - L584 were not covered by tests
internal/cmd/run.go
[warning] 125-127: internal/cmd/run.go#L125-L127
Added lines #L125 - L127 were not covered by tests
[warning] 208-208: internal/cmd/run.go#L208
Added line #L208 was not covered by tests
internal/interpreter/batch_balances_query.go
[warning] 135-136: internal/interpreter/batch_balances_query.go#L135-L136
Added lines #L135 - L136 were not covered by tests
internal/parser/antlr/numscript_base_listener.go
[warning] 191-191: internal/parser/antlr/numscript_base_listener.go#L191
Added line #L191 was not covered by tests
[warning] 194-194: internal/parser/antlr/numscript_base_listener.go#L194
Added line #L194 was not covered by tests
[warning] 245-245: internal/parser/antlr/numscript_base_listener.go#L245
Added line #L245 was not covered by tests
[warning] 248-248: internal/parser/antlr/numscript_base_listener.go#L248
Added line #L248 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (24)
internal/parser/antlr/NumscriptLexer.tokens (1)
1-65: LGTM!The changes to the token definitions are auto-generated by ANTLR and correctly integrate the new
oneoftoken.internal/interpreter/batch_balances_query.go (1)
131-139: LGTM! Implementation mirrors existing pattern.The implementation for
SourceOneoffollows the same pattern asSourceInorder, which is correct and consistent.However, the error handling paths need test coverage. Please add test cases for:
- Error propagation when a sub-source fails
- Multiple sub-sources failing
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check existing test coverage for oneof error handling # Look for test cases covering oneof error scenarios rg -l "TestSourceOneof.*Error" internal/interpreter/ rg "SourceOneof.*Error" internal/interpreter/ -A 3 -B 3Length of output: 112
Missing Test Coverage for Error Propagation in SourceOneof
The search confirms that there is currently no test coverage addressing error propagation in the
SourceOneofcode path (i.e. tests simulating a sub-source failure or multiple failures). Please add tests that cover the following scenarios:
- A single sub-source fails and its error is correctly propagated.
- Multiple sub-sources fail, ensuring that the error handling behaves as expected.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 135-136: internal/interpreter/batch_balances_query.go#L135-L136
Added lines #L135 - L136 were not covered by testsinternal/parser/ast.go (1)
90-90: LGTM! Well-structured type definitions.The new types
SourceOneofandDestinationOneofare well-designed and consistent with existing patterns. Interface implementations are complete.Also applies to: 106-109, 167-171
internal/cmd/run.go (1)
30-30: Add test coverage for the oneof feature flag in the cmd package.The feature flag implementation looks correct but needs test coverage for:
- Flag parsing and validation
- Feature flag propagation to the interpreter
- Integration tests with the flag enabled/disabled
Let's verify the current test coverage:
#!/bin/bash # Description: Check test coverage for oneof feature flag # Look for existing test files fd ".*test\.go$" internal/cmd/ # Check for feature flag tests rg "TestRun.*Flag" internal/cmd/ -A 3 -B 3 rg "oneOfFeatureFlag" internal/cmd/ -A 3 -B 3Also applies to: 125-127, 208-208
internal/parser/parser_test.go (1)
116-127: LGTM!The test case is well structured and covers the essential functionality. Edge cases are already covered by the inorder parsing fault tolerance tests.
internal/parser/antlr/numscript_listener.go (1)
92-94: LGTM!The new listener methods follow the established pattern and are properly integrated into the interface.
Also applies to: 119-121, 221-223, 248-250
internal/parser/antlr/numscript_base_listener.go (1)
190-195: LGTM!The empty implementations in the base listener are correct. The code coverage warnings can be safely ignored as these are default no-op implementations that are meant to be overridden by concrete listeners when needed.
Also applies to: 244-249
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 191-191: internal/parser/antlr/numscript_base_listener.go#L191
Added line #L191 was not covered by tests
[warning] 194-194: internal/parser/antlr/numscript_base_listener.go#L194
Added line #L194 was not covered by testsinternal/parser/antlr/numscript_lexer.go (2)
46-50: LGTM!The lexer has been correctly updated to include the new
oneoftoken, with all token indices properly shifted to accommodate the change.Also applies to: 272-309
46-50: LGTM!The lexer changes correctly add the 'oneof' token and update all token indices accordingly.
internal/analysis/check.go (4)
506-509: Add validation for empty sources array.The implementation should validate that the sources array is not empty to prevent potential panic.
Apply this diff to add validation:
case *parser.SourceOneof: + if len(source.Sources) == 0 { + res.Diagnostics = append(res.Diagnostics, Diagnostic{ + Range: source.GetRange(), + Kind: &EmptyOneofError{}, + }) + return + } for _, source := range source.Sources { res.checkSource(source) }
579-585: Add validation for empty clauses array.Similar to sources, the implementation should validate that the clauses array is not empty.
Apply this diff to add validation:
case *parser.DestinationOneof: + if len(destination.Clauses) == 0 { + res.Diagnostics = append(res.Diagnostics, Diagnostic{ + Range: destination.GetRange(), + Kind: &EmptyOneofError{}, + }) + return + } for _, clause := range destination.Clauses { res.checkExpression(clause.Cap, TypeMonetary) res.checkKeptOrDestination(clause.To) }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 579-584: internal/analysis/check.go#L579-L584
Added lines #L579 - L584 were not covered by tests
506-510: Add validation for empty sources array.The implementation should validate that the sources array is not empty to prevent potential panic. While the comment on line 586 suggests that "empty oneof is parsing err", it's better to be defensive in the analysis phase as well.
Apply this diff to add validation:
case *parser.SourceOneof: + if len(source.Sources) == 0 { + res.Diagnostics = append(res.Diagnostics, Diagnostic{ + Range: source.GetRange(), + Kind: &EmptyOneofError{}, + }) + return + } for _, source := range source.Sources { res.checkSource(source) }
579-585: Add validation for empty clauses array.Similar to sources, the implementation should validate that the clauses array is not empty.
Apply this diff to add validation:
case *parser.DestinationOneof: + if len(destination.Clauses) == 0 { + res.Diagnostics = append(res.Diagnostics, Diagnostic{ + Range: destination.GetRange(), + Kind: &EmptyOneofError{}, + }) + return + } for _, clause := range destination.Clauses { res.checkExpression(clause.Cap, TypeMonetary) res.checkKeptOrDestination(clause.To) }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 579-584: internal/analysis/check.go#L579-L584
Added lines #L579 - L584 were not covered by testsinternal/interpreter/interpreter.go (6)
182-185: LGTM!The feature flags are well-organized in a constant block, making them easy to manage and extend.
479-488: LGTM!The implementation correctly uses only the first source in
sendAllsince none of the branches can fail in a "send all" context, as clarified in the learnings.
1048-1055: LGTM!The feature flag check is well-implemented with clear error handling.
182-185: LGTM!The feature flag management is well-structured, grouping related flags in a constant block.
479-488: LGTM!The
oneofimplementations are well-designed:
- In
sendAll, it uses the first source as none of the branches can fail in a "send all" context.- In
trySendingUpTo, it implements proper backtracking to try alternative sources.- In
DestinationOneof, it selects the first clause that can handle the amount.Also applies to: 580-738
596-597: Add test coverage for error paths.The error handling paths in
trySendingUpToandDestinationOneofare not covered by tests.Run the following script to verify test coverage:
Also applies to: 726-727
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check test coverage for oneof error paths. # Test: Search for test cases that cover oneof error paths. rg -A 5 'TestSourceOneofError|TestDestinationOneofError'Length of output: 58
Action Required: Add Missing Test Cases for Error Paths in Interpreter
The search for test cases using the keywords “TestSourceOneofError” and “TestDestinationOneofError” returned no matches. This confirms that the error handling paths in both
trySendingUpToandDestinationOneof(applicable to lines 596–597 and 726–727) are currently not covered by tests. Please add dedicated test cases to exercise these error scenarios.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 596-597: internal/interpreter/interpreter.go#L596-L597
Added lines #L596 - L597 were not covered by testsinternal/parser/antlr/numscript_parser.go (5)
36-40: Token definitions look correctThe addition of the 'oneof' token in the LiteralNames array is properly positioned and integrated with the existing token definitions.
2859-2937: SrcOneofContext implementation is well-structuredThe implementation follows the standard ANTLR pattern for context types, with proper method implementations for rule context management, getter methods, and listener notifications.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 2873-2874: internal/parser/antlr/numscript_parser.go#L2873-L2874
Added lines #L2873 - L2874 were not covered by tests
[warning] 2877-2878: internal/parser/antlr/numscript_parser.go#L2877-L2878
Added lines #L2877 - L2878 were not covered by tests
[warning] 2881-2882: internal/parser/antlr/numscript_parser.go#L2881-L2882
Added lines #L2881 - L2882 were not covered by tests
[warning] 2906-2913: internal/parser/antlr/numscript_parser.go#L2906-L2913
Added lines #L2906 - L2913 were not covered by tests
[warning] 2915-2915: internal/parser/antlr/numscript_parser.go#L2915
Added line #L2915 was not covered by tests
[warning] 2919-2921: internal/parser/antlr/numscript_parser.go#L2919-L2921
Added lines #L2919 - L2921 were not covered by tests
[warning] 2923-2923: internal/parser/antlr/numscript_parser.go#L2923
Added line #L2923 was not covered by tests
[warning] 2926-2929: internal/parser/antlr/numscript_parser.go#L2926-L2929
Added lines #L2926 - L2929 were not covered by tests
[warning] 2932-2935: internal/parser/antlr/numscript_parser.go#L2932-L2935
Added lines #L2932 - L2935 were not covered by tests
4184-4282: DestOneofContext implementation is consistentThe implementation mirrors the SrcOneofContext structure and provides the necessary methods for destination handling in the oneof construct.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 4198-4199: internal/parser/antlr/numscript_parser.go#L4198-L4199
Added lines #L4198 - L4199 were not covered by tests
[warning] 4202-4203: internal/parser/antlr/numscript_parser.go#L4202-L4203
Added lines #L4202 - L4203 were not covered by tests
[warning] 4206-4207: internal/parser/antlr/numscript_parser.go#L4206-L4207
Added lines #L4206 - L4207 were not covered by tests
[warning] 4220-4221: internal/parser/antlr/numscript_parser.go#L4220-L4221
Added lines #L4220 - L4221 were not covered by tests
[warning] 4226-4227: internal/parser/antlr/numscript_parser.go#L4226-L4227
Added lines #L4226 - L4227 were not covered by tests
[warning] 4251-4258: internal/parser/antlr/numscript_parser.go#L4251-L4258
Added lines #L4251 - L4258 were not covered by tests
3285-3513: Parser rule updates are properly integratedThe changes to support the oneof construct in the parser rules are well-integrated with the existing grammar rules and maintain consistency with the ANTLR parsing patterns.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 3480-3481: internal/parser/antlr/numscript_parser.go#L3480-L3481
Added lines #L3480 - L3481 were not covered by tests
[warning] 3487-3487: internal/parser/antlr/numscript_parser.go#L3487
Added line #L3487 was not covered by tests
[warning] 3500-3500: internal/parser/antlr/numscript_parser.go#L3500
Added line #L3500 was not covered by tests
[warning] 3508-3509: internal/parser/antlr/numscript_parser.go#L3508-L3509
Added lines #L3508 - L3509 were not covered by tests
1-1: Note regarding test coverageThe static analysis shows uncovered lines in the auto-generated code. This is expected for ANTLR-generated files and doesn't require additional test coverage.
ascandone
force-pushed
the
experimental/oneof-combinator
branch
from
de11731 to
4d664f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
internal/analysis/check.go (2)
506-510: 🛠️ Refactor suggestionAdd validation for empty sources array.
The implementation should validate that the sources array is not empty to prevent potential panic.
Apply this diff to add validation:
case *parser.SourceOneof: + if len(source.Sources) == 0 { + res.Diagnostics = append(res.Diagnostics, Diagnostic{ + Range: source.GetRange(), + Kind: &EmptyOneofError{}, + }) + return + } for _, source := range source.Sources { res.checkSource(source) }
579-585: 🛠️ Refactor suggestionAdd validation for empty clauses array.
Similar to sources, the implementation should validate that the clauses array is not empty.
Apply this diff to add validation:
case *parser.DestinationOneof: + if len(destination.Clauses) == 0 { + res.Diagnostics = append(res.Diagnostics, Diagnostic{ + Range: destination.GetRange(), + Kind: &EmptyOneofError{}, + }) + return + } for _, clause := range destination.Clauses { res.checkExpression(clause.Cap, TypeMonetary) res.checkKeptOrDestination(clause.To) } res.checkKeptOrDestination(destination.Remaining)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 579-584: internal/analysis/check.go#L579-L584
Added lines #L579 - L584 were not covered by tests
🧹 Nitpick comments (1)
internal/parser/antlr/numscript_parser.go (1)
1626-1626: Consider documenting the bit mask operations.The auto-generated bit mask operations (e.g.,
((int64(1)<<_la)&530438946816) != 0) are complex. While we shouldn't modify the auto-generated code, it would be helpful to document their purpose in the grammar file or a separate documentation file.Also applies to: 3403-3403, 3443-3443, 3491-3491
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
internal/parser/__snapshots__/parser_test.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (19)
Numscript.g4(2 hunks)internal/analysis/check.go(2 hunks)internal/analysis/check_test.go(1 hunks)internal/cmd/run.go(3 hunks)internal/interpreter/batch_balances_query.go(1 hunks)internal/interpreter/interpreter.go(8 hunks)internal/interpreter/interpreter_test.go(1 hunks)internal/parser/antlr/Numscript.interp(3 hunks)internal/parser/antlr/Numscript.tokens(1 hunks)internal/parser/antlr/NumscriptLexer.interp(4 hunks)internal/parser/antlr/NumscriptLexer.tokens(1 hunks)internal/parser/antlr/numscript_base_listener.go(2 hunks)internal/parser/antlr/numscript_lexer.go(2 hunks)internal/parser/antlr/numscript_listener.go(4 hunks)internal/parser/antlr/numscript_parser.go(24 hunks)internal/parser/ast.go(4 hunks)internal/parser/parser.go(4 hunks)internal/parser/parser_test.go(2 hunks)numscript_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/analysis/check_test.go
- numscript_test.go
- Numscript.g4
- internal/parser/antlr/numscript_listener.go
- internal/parser/antlr/NumscriptLexer.interp
- internal/parser/antlr/Numscript.interp
- internal/parser/antlr/NumscriptLexer.tokens
🧰 Additional context used
🧠 Learnings (2)
internal/parser/parser_test.go (1)
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.961Z
Learning: Edge cases for oneof destination parsing in numscript are covered by the existing inorder parsing fault tolerance tests, as both constructs share similar parsing logic.
internal/interpreter/interpreter.go (1)
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/interpreter/interpreter.go:479-488
Timestamp: 2025-01-20T23:01:48.857Z
Learning: In the `sendAll` method of the Numscript interpreter, the `oneof` operator only uses the first source because none of the branches can fail in a "send all" context. This differs from `trySendingUpTo` where backtracking is needed as sources can fail when trying to send a specific amount.
🪛 GitHub Check: codecov/patch
internal/interpreter/interpreter.go
[warning] 596-597: internal/interpreter/interpreter.go#L596-L597
Added lines #L596 - L597 were not covered by tests
[warning] 726-727: internal/interpreter/interpreter.go#L726-L727
Added lines #L726 - L727 were not covered by tests
internal/parser/antlr/numscript_parser.go
[warning] 2873-2874: internal/parser/antlr/numscript_parser.go#L2873-L2874
Added lines #L2873 - L2874 were not covered by tests
[warning] 2877-2878: internal/parser/antlr/numscript_parser.go#L2877-L2878
Added lines #L2877 - L2878 were not covered by tests
[warning] 2881-2882: internal/parser/antlr/numscript_parser.go#L2881-L2882
Added lines #L2881 - L2882 were not covered by tests
[warning] 2906-2913: internal/parser/antlr/numscript_parser.go#L2906-L2913
Added lines #L2906 - L2913 were not covered by tests
[warning] 2915-2915: internal/parser/antlr/numscript_parser.go#L2915
Added line #L2915 was not covered by tests
[warning] 2919-2921: internal/parser/antlr/numscript_parser.go#L2919-L2921
Added lines #L2919 - L2921 were not covered by tests
[warning] 2923-2923: internal/parser/antlr/numscript_parser.go#L2923
Added line #L2923 was not covered by tests
[warning] 2926-2929: internal/parser/antlr/numscript_parser.go#L2926-L2929
Added lines #L2926 - L2929 were not covered by tests
[warning] 2932-2935: internal/parser/antlr/numscript_parser.go#L2932-L2935
Added lines #L2932 - L2935 were not covered by tests
[warning] 3480-3481: internal/parser/antlr/numscript_parser.go#L3480-L3481
Added lines #L3480 - L3481 were not covered by tests
[warning] 3487-3487: internal/parser/antlr/numscript_parser.go#L3487
Added line #L3487 was not covered by tests
[warning] 3500-3500: internal/parser/antlr/numscript_parser.go#L3500
Added line #L3500 was not covered by tests
[warning] 3508-3509: internal/parser/antlr/numscript_parser.go#L3508-L3509
Added lines #L3508 - L3509 were not covered by tests
[warning] 3520-3521: internal/parser/antlr/numscript_parser.go#L3520-L3521
Added lines #L3520 - L3521 were not covered by tests
[warning] 4198-4199: internal/parser/antlr/numscript_parser.go#L4198-L4199
Added lines #L4198 - L4199 were not covered by tests
[warning] 4202-4203: internal/parser/antlr/numscript_parser.go#L4202-L4203
Added lines #L4202 - L4203 were not covered by tests
[warning] 4206-4207: internal/parser/antlr/numscript_parser.go#L4206-L4207
Added lines #L4206 - L4207 were not covered by tests
[warning] 4220-4221: internal/parser/antlr/numscript_parser.go#L4220-L4221
Added lines #L4220 - L4221 were not covered by tests
[warning] 4226-4227: internal/parser/antlr/numscript_parser.go#L4226-L4227
Added lines #L4226 - L4227 were not covered by tests
[warning] 4251-4258: internal/parser/antlr/numscript_parser.go#L4251-L4258
Added lines #L4251 - L4258 were not covered by tests
internal/analysis/check.go
[warning] 579-584: internal/analysis/check.go#L579-L584
Added lines #L579 - L584 were not covered by tests
internal/cmd/run.go
[warning] 125-127: internal/cmd/run.go#L125-L127
Added lines #L125 - L127 were not covered by tests
[warning] 208-208: internal/cmd/run.go#L208
Added line #L208 was not covered by tests
internal/interpreter/batch_balances_query.go
[warning] 135-136: internal/interpreter/batch_balances_query.go#L135-L136
Added lines #L135 - L136 were not covered by tests
internal/parser/antlr/numscript_base_listener.go
[warning] 191-191: internal/parser/antlr/numscript_base_listener.go#L191
Added line #L191 was not covered by tests
[warning] 194-194: internal/parser/antlr/numscript_base_listener.go#L194
Added line #L194 was not covered by tests
[warning] 245-245: internal/parser/antlr/numscript_base_listener.go#L245
Added line #L245 was not covered by tests
[warning] 248-248: internal/parser/antlr/numscript_base_listener.go#L248
Added line #L248 was not covered by tests
🔇 Additional comments (23)
internal/parser/antlr/Numscript.tokens (2)
2-2: LGTM: New 'oneof' token properly defined.The new token
T__1and its literal'oneof'are correctly defined with consistent numeric ID (2), which aligns with the PR objective of adding the 'oneof' combinator feature.Also applies to: 40-40
3-38: LGTM: Token renumbering is consistent.The sequential renumbering of existing tokens to accommodate the new 'oneof' token maintains ANTLR's requirements while preserving the relative ordering of all tokens.
Also applies to: 41-64
internal/interpreter/batch_balances_query.go (1)
131-138: LGTM! Implementation mirrors the existing SourceInorder case.The implementation correctly iterates over all sources in the oneof block and maintains consistent error handling.
Please add test coverage for error handling in the oneof case. Run this script to verify current test coverage:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check test coverage for oneof error handling # Look for test files containing oneof error tests rg -i "oneof.*error|error.*oneof" internal/interpreter/interpreter_test.goLength of output: 76
Action Required: Add Test Coverage for Oneof Error Handling
The search for tests validating error handling in the oneof branch (in internal/interpreter/interpreter_test.go) produced no results. This indicates that scenarios where a subsource error is returned (via
st.findBalancesQueries(subSource)) are not being exercised by the existing tests.
- Issue: Missing tests for error propagation in the oneof case.
- Recommendation: Add test cases that simulate errors within the oneof block to confirm that errors are correctly propagated.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 135-136: internal/interpreter/batch_balances_query.go#L135-L136
Added lines #L135 - L136 were not covered by testsinternal/parser/ast.go (1)
90-91: LGTM! AST types are well-structured and consistent.The new types
SourceOneofandDestinationOneofare well-designed:
- Correctly implement their respective interfaces
- Mirror the structure of existing types
- Maintain consistency with grammar rules
Also applies to: 106-109, 153-153, 167-171
internal/cmd/run.go (1)
30-30: Add test coverage for the oneof feature flag.The feature flag implementation looks correct but lacks test coverage. Please add test cases to verify:
- Program behavior when the flag is enabled
- Program behavior when the flag is disabled
- Integration with the interpreter package's feature flag handling
Let's verify the feature flag integration with the interpreter package:
#!/bin/bash # Description: Verify that the interpreter package properly handles the oneof feature flag # Check if the interpreter package defines the feature flag constant rg -l "ExperimentalOneofFeatureFlag.*=.*\"" internal/interpreter/ # Check if there are corresponding feature flag checks in the interpreter rg "ExperimentalOneofFeatureFlag" internal/interpreter/ -A 3 -B 3 # Look for test files that might need additional coverage fd ".*test\.go$" internal/cmd/Also applies to: 125-127, 208-208
internal/parser/parser_test.go (1)
85-92: LGTM! Tests cover oneof parsing functionality.The tests effectively validate the basic functionality of oneof parsing for both sources and destinations. Edge cases are already covered by the inorder parsing fault tolerance tests, as both constructs share similar parsing logic.
Also applies to: 116-127
internal/parser/antlr/numscript_base_listener.go (1)
190-195: Add test coverage for the new oneof listener methods.The new methods for handling
srcOneofanddestOneofproductions are not covered by tests. Please add test cases to ensure proper handling of these productions.Run the following script to verify test coverage:
Also applies to: 244-249
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check test coverage for the new oneof listener methods. # Test: Search for test files that cover the oneof listener methods. rg -A 5 $'(srcOneof|destOneof).*Context'Length of output: 41
Missing Test Coverage for Oneof Listener Methods
The
srcOneofanddestOneoflistener methods ininternal/parser/antlr/numscript_base_listener.go(lines 190-195 and 244-249) are not referenced in any tests based on thergoutput. Please add test cases to cover these methods.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 191-191: internal/parser/antlr/numscript_base_listener.go#L191
Added line #L191 was not covered by tests
[warning] 194-194: internal/parser/antlr/numscript_base_listener.go#L194
Added line #L194 was not covered by testsinternal/parser/antlr/numscript_lexer.go (1)
46-50: LGTM!The addition of the
oneoftoken and the corresponding updates to token indices are correct.Also applies to: 272-310
internal/interpreter/interpreter.go (5)
182-185: LGTM! Feature flag constants are well-defined.The feature flags are appropriately defined as constants, making them easy to manage and reference throughout the codebase.
479-488: LGTM! Correct implementation ofoneofinsendAll.Based on the retrieved learning, the implementation correctly uses only the first source in
sendAllsince none of the branches can fail in this context. This differs fromtrySendingUpTowhere backtracking is needed.
580-609: LGTM! Well-implemented backtracking logic foroneofintrySendingUpTo.The implementation:
- Properly handles backtracking by storing and restoring sender state
- Tries each source until one succeeds or all fail
- Includes clear comments explaining the logic
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 596-597: internal/interpreter/interpreter.go#L596-L597
Added lines #L596 - L597 were not covered by tests
718-738: LGTM! Well-implemented destination clause selection foroneof.The implementation:
- Correctly checks each clause's cap against the amount
- Uses the first clause that can handle the full amount
- Falls back to the remaining clause if no other clause can handle it
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 726-727: internal/interpreter/interpreter.go#L726-L727
Added lines #L726 - L727 were not covered by tests
1048-1055: LGTM! Clean implementation of feature flag checking.The
checkFeatureFlagmethod provides a clean way to verify feature flag presence and return appropriate errors.internal/interpreter/interpreter_test.go (6)
3586-3609: LGTM! Good test coverage for source oneof with unbounded overdraft.The test verifies that the first successful source is used when using unbounded overdraft.
3611-3634: LGTM! Good test coverage for source oneof with limited overdraft.The test verifies that when the first source fails due to insufficient overdraft limit, the second source is used.
3636-3657: LGTM! Good test coverage for source oneof failure case.The test verifies that when all sources fail, the appropriate error is returned.
3659-3684: LGTM! Good test coverage for source oneof in send all context.The test verifies that only the first source is used in a send all context, matching the implementation behavior.
3686-3707: LGTM! Good test coverage for source oneof with single source.The test verifies that oneof works correctly with a single source.
3709-3784: LGTM! Comprehensive test coverage for destination oneof.The tests verify:
- First clause selection when it can handle the amount
- Second clause selection when first clause can't handle the amount
- Remaining clause selection when no other clause can handle the amount
internal/parser/antlr/numscript_parser.go (4)
1-1: LGTM: Auto-generated code notice is present.The file correctly starts with a notice indicating it's auto-generated code that should not be edited directly.
36-40: LGTM: Token definitions for 'oneof' are correctly added.The 'oneof' token is properly added to the LiteralNames array with appropriate positioning.
3466-3512: LGTM: Parser rules for 'oneof' construct are properly implemented.The parser rules correctly handle the oneof syntax for both source and destination contexts, following ANTLR's standard patterns.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 3480-3481: internal/parser/antlr/numscript_parser.go#L3480-L3481
Added lines #L3480 - L3481 were not covered by tests
[warning] 3487-3487: internal/parser/antlr/numscript_parser.go#L3487
Added line #L3487 was not covered by tests
[warning] 3500-3500: internal/parser/antlr/numscript_parser.go#L3500
Added line #L3500 was not covered by tests
[warning] 3508-3509: internal/parser/antlr/numscript_parser.go#L3508-L3509
Added lines #L3508 - L3509 were not covered by tests
2859-2937: Consider adding grammar-level tests for the 'oneof' construct.While the auto-generated parser code for
SrcOneofContextshows uncovered paths, this should be addressed by adding tests at the grammar level rather than modifying this file directly.Run the following script to check for existing grammar tests:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Look for grammar tests covering the oneof construct rg -A 5 "oneof.*test"Length of output: 23
Add Grammar-Level Tests for the 'oneof' Construct
The shell script search did not return any results, which strongly suggests that there aren’t any existing grammar-level tests specifically exercising the oneof construct. Since the auto-generated parser code (e.g., for
SrcOneofContext) follows the standard ANTLR pattern, the priority should be on improving test coverage at the grammar level rather than modifying the generated code itself.• Verify that tests exist (or add them where missing) that cover the oneof grammar rules to ensure all code paths in the generated contexts are exercised.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 2873-2874: internal/parser/antlr/numscript_parser.go#L2873-L2874
Added lines #L2873 - L2874 were not covered by tests
[warning] 2877-2878: internal/parser/antlr/numscript_parser.go#L2877-L2878
Added lines #L2877 - L2878 were not covered by tests
[warning] 2881-2882: internal/parser/antlr/numscript_parser.go#L2881-L2882
Added lines #L2881 - L2882 were not covered by tests
[warning] 2906-2913: internal/parser/antlr/numscript_parser.go#L2906-L2913
Added lines #L2906 - L2913 were not covered by tests
[warning] 2915-2915: internal/parser/antlr/numscript_parser.go#L2915
Added line #L2915 was not covered by tests
[warning] 2919-2921: internal/parser/antlr/numscript_parser.go#L2919-L2921
Added lines #L2919 - L2921 were not covered by tests
[warning] 2923-2923: internal/parser/antlr/numscript_parser.go#L2923
Added line #L2923 was not covered by tests
[warning] 2926-2929: internal/parser/antlr/numscript_parser.go#L2926-L2929
Added lines #L2926 - L2929 were not covered by tests
[warning] 2932-2935: internal/parser/antlr/numscript_parser.go#L2932-L2935
Added lines #L2932 - L2935 were not covered by tests
| case *parser.DestOneofContext: | ||
| var inorderClauses []CappedKeptOrDestination | ||
| for _, destInorderClause := range destCtx.AllDestinationInOrderClause() { | ||
| inorderClauses = append(inorderClauses, parseDestinationInorderClause(destInorderClause)) | ||
| } | ||
|
|
||
| return &DestinationOneof{ | ||
| Range: range_, | ||
| Clauses: inorderClauses, | ||
| Remaining: parseKeptOrDestination(destCtx.KeptOrDestination()), | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for empty clauses array.
Similar to sources, the implementation should validate that the clauses array is not empty.
Apply this diff to add validation:
case *parser.DestOneofContext:
+ if len(destCtx.AllDestinationInOrderClause()) == 0 {
+ return nil
+ }
var inorderClauses []CappedKeptOrDestination
for _, destInorderClause := range destCtx.AllDestinationInOrderClause() {
inorderClauses = append(inorderClauses, parseDestinationInorderClause(destInorderClause))
}
return &DestinationOneof{
Range: range_,
Clauses: inorderClauses,
Remaining: parseKeptOrDestination(destCtx.KeptOrDestination()),
}📝 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.
| case *parser.DestOneofContext: | |
| var inorderClauses []CappedKeptOrDestination | |
| for _, destInorderClause := range destCtx.AllDestinationInOrderClause() { | |
| inorderClauses = append(inorderClauses, parseDestinationInorderClause(destInorderClause)) | |
| } | |
| return &DestinationOneof{ | |
| Range: range_, | |
| Clauses: inorderClauses, | |
| Remaining: parseKeptOrDestination(destCtx.KeptOrDestination()), | |
| } | |
| case *parser.DestOneofContext: | |
| if len(destCtx.AllDestinationInOrderClause()) == 0 { | |
| return nil | |
| } | |
| var inorderClauses []CappedKeptOrDestination | |
| for _, destInorderClause := range destCtx.AllDestinationInOrderClause() { | |
| inorderClauses = append(inorderClauses, parseDestinationInorderClause(destInorderClause)) | |
| } | |
| return &DestinationOneof{ | |
| Range: range_, | |
| Clauses: inorderClauses, | |
| Remaining: parseKeptOrDestination(destCtx.KeptOrDestination()), | |
| } |
| case *parser.SrcOneofContext: | ||
| var sources []Source | ||
| for _, sourceCtx := range sourceCtx.AllSource() { | ||
| sources = append(sources, parseSource(sourceCtx)) | ||
| } | ||
| return &SourceOneof{ | ||
| Range: range_, | ||
| Sources: sources, | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for empty sources array.
The implementation should validate that the sources array is not empty to prevent potential panic.
Apply this diff to add validation:
case *parser.SrcOneofContext:
+ if len(sourceCtx.AllSource()) == 0 {
+ return nil
+ }
var sources []Source
for _, sourceCtx := range sourceCtx.AllSource() {
sources = append(sources, parseSource(sourceCtx))
}
return &SourceOneof{
Range: range_,
Sources: sources,
}📝 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.
| case *parser.SrcOneofContext: | |
| var sources []Source | |
| for _, sourceCtx := range sourceCtx.AllSource() { | |
| sources = append(sources, parseSource(sourceCtx)) | |
| } | |
| return &SourceOneof{ | |
| Range: range_, | |
| Sources: sources, | |
| } | |
| case *parser.SrcOneofContext: | |
| if len(sourceCtx.AllSource()) == 0 { | |
| return nil | |
| } | |
| var sources []Source | |
| for _, sourceCtx := range sourceCtx.AllSource() { | |
| sources = append(sources, parseSource(sourceCtx)) | |
| } | |
| return &SourceOneof{ | |
| Range: range_, | |
| Sources: sources, | |
| } |
| case *parser.DestinationOneof: | ||
| for _, clause := range destination.Clauses { | ||
| res.checkExpression(clause.Cap, TypeMonetary) | ||
| res.checkKeptOrDestination(clause.To) | ||
| } | ||
| res.checkKeptOrDestination(destination.Remaining) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add test coverage for the new oneof analysis cases.
The new cases for handling SourceOneof and DestinationOneof in the analysis phase are not covered by tests. Please add test cases to ensure proper validation and error handling.
Run the following script to verify test coverage:
🏁 Script executed:
#!/bin/bash
# Description: Check test coverage for the new oneof analysis cases.
# Test: Search for test files that cover the oneof analysis cases.
rg -A 5 $'(SourceOneof|DestinationOneof).*case'Length of output: 48
Missing Test Coverage for SourceOneof and DestinationOneof
Our investigation indicates that there are no tests covering the new oneof analysis cases in the codebase. The initial search for occurrences of “SourceOneof” and “DestinationOneof” in test files produced no results, suggesting that these cases are indeed untested.
To double-check, please run the following additional script to search through all Go test files for any mentions of these cases:
If no references are found after this check, it confirms that the new oneof analysis paths lack test coverage. Based on the available evidence, please add dedicated test cases to cover both SourceOneof and DestinationOneof, ensuring proper validation and error handling.
🏁 Script executed:
#!/bin/bash
# Searching for any occurrences of SourceOneof and DestinationOneof in Go test files
find . -type f -name "*_test.go" -exec grep -H -n "SourceOneof" {} \;
find . -type f -name "*_test.go" -exec grep -H -n "DestinationOneof" {} \;Length of output: 237
Missing Test Coverage for DestinationOneof Analysis Case
- SourceOneof: Covered by
TestUnboundVarInSourceOneofininternal/analysis/check_test.go(line 200). - DestinationOneof: No test cases found. Please add test cases for this case to ensure proper validation and error handling.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 579-584: internal/analysis/check.go#L579-L584
Added lines #L579 - L584 were not covered by tests
No description provided.