-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Experimental/oneof combinator #31
base: main
Are you sure you want to change the base?
Changes from all commits
d10f435
efcf450
be95b06
5985102
87aa2a5
dc7f479
4e578ab
895b7c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -503,6 +503,11 @@ | |||||||||||||||||||||||||||||||||||||||
res.checkSource(source) | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
case *parser.SourceOneof: | ||||||||||||||||||||||||||||||||||||||||
for _, source := range source.Sources { | ||||||||||||||||||||||||||||||||||||||||
res.checkSource(source) | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
case *parser.SourceCapped: | ||||||||||||||||||||||||||||||||||||||||
onExit := res.enterCappedSource() | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
|
@@ -571,6 +576,13 @@ | |||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
res.checkKeptOrDestination(destination.Remaining) | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
case *parser.DestinationOneof: | ||||||||||||||||||||||||||||||||||||||||
for _, clause := range destination.Clauses { | ||||||||||||||||||||||||||||||||||||||||
res.checkExpression(clause.Cap, TypeMonetary) | ||||||||||||||||||||||||||||||||||||||||
res.checkKeptOrDestination(clause.To) | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
res.checkKeptOrDestination(destination.Remaining) | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
Comment on lines
+579
to
+585
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)
} 📝 Committable suggestion
Suggested change
🧰 Tools🪛 GitHub Check: codecov/patch[warning] 579-584: internal/analysis/check.go#L579-L584 |
||||||||||||||||||||||||||||||||||||||||
case *parser.DestinationAllotment: | ||||||||||||||||||||||||||||||||||||||||
var remainingAllotment *parser.RemainingAllotment | ||||||||||||||||||||||||||||||||||||||||
var variableLiterals []parser.Variable | ||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ | |
var runOutFormatOpt string | ||
|
||
var overdraftFeatureFlag bool | ||
var oneOfFeatureFlag bool | ||
|
||
type inputOpts struct { | ||
Script string `json:"script"` | ||
|
@@ -121,6 +122,9 @@ | |
if overdraftFeatureFlag { | ||
featureFlags[interpreter.ExperimentalOverdraftFunctionFeatureFlag] = struct{}{} | ||
} | ||
if oneOfFeatureFlag { | ||
featureFlags[interpreter.ExperimentalOneofFeatureFlag] = struct{}{} | ||
} | ||
Comment on lines
+125
to
+127
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 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:
🔗 Analysis chainAdd test coverage for the oneof feature flag. The feature flag implementation looks correct but lacks test coverage. Please add test cases to verify:
Let's verify the feature flag integration with the interpreter package: 🏁 Scripts executedThe 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 |
||
|
||
result, err := interpreter.RunProgram(context.Background(), parseResult.Value, opt.Variables, interpreter.StaticStore{ | ||
Balances: opt.Balances, | ||
|
@@ -201,6 +205,7 @@ | |
|
||
// Feature flag | ||
cmd.Flags().BoolVar(&overdraftFeatureFlag, interpreter.ExperimentalOverdraftFunctionFeatureFlag, false, "feature flag to enable the overdraft() function") | ||
cmd.Flags().BoolVar(&oneOfFeatureFlag, interpreter.ExperimentalOneofFeatureFlag, false, "feature flag to enable the oneof combinator") | ||
|
||
// Output options | ||
cmd.Flags().StringVar(&runOutFormatOpt, "output-format", OutputFormatPretty, "Set the output format. Available options: pretty, json.") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -179,7 +179,10 @@ | |
|
||
type FeatureFlag = string | ||
|
||
const ExperimentalOverdraftFunctionFeatureFlag FeatureFlag = "experimental-overdraft-function" | ||
const ( | ||
ExperimentalOverdraftFunctionFeatureFlag FeatureFlag = "experimental-overdraft-function" | ||
ExperimentalOneofFeatureFlag FeatureFlag = "experimental-oneof" | ||
) | ||
|
||
func RunProgram( | ||
ctx context.Context, | ||
|
@@ -198,10 +201,7 @@ | |
|
||
CurrentBalanceQuery: BalanceQuery{}, | ||
ctx: ctx, | ||
} | ||
|
||
if _, ok := featureFlags[ExperimentalOverdraftFunctionFeatureFlag]; ok { | ||
st.OverdraftFunctionFeatureFlag = true | ||
FeatureFlags: featureFlags, | ||
} | ||
|
||
err := st.parseVars(program.Vars, vars) | ||
|
@@ -261,7 +261,7 @@ | |
|
||
CurrentBalanceQuery BalanceQuery | ||
|
||
OverdraftFunctionFeatureFlag bool | ||
FeatureFlags map[string]struct{} | ||
} | ||
|
||
func (st *programState) pushSender(name string, monetary *big.Int) { | ||
|
@@ -476,6 +476,16 @@ | |
} | ||
return totalSent, nil | ||
|
||
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) | ||
|
||
Comment on lines
+479
to
+488
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent behavior in The
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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
|
||
case *parser.SourceCapped: | ||
monetary, err := evaluateExprAs(s, source.Cap, expectMonetaryOfAsset(s.CurrentAsset)) | ||
if err != nil { | ||
|
@@ -567,6 +577,36 @@ | |
} | ||
return new(big.Int).Sub(amount, totalLeft), nil | ||
|
||
case *parser.SourceOneof: | ||
err := s.checkFeatureFlag(ExperimentalOneofFeatureFlag) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
// empty oneof is parsing err | ||
leadingSources := source.Sources[0 : len(source.Sources)-1] | ||
|
||
for _, source := range leadingSources { | ||
|
||
// do not move this line below (as .trySendingUpTo() will mutate senders' length) | ||
backtrackingIndex := len(s.Senders) | ||
|
||
sentAmt, err := s.trySendingUpTo(source, amount) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
// if this branch managed to sent all the required amount, return now | ||
if sentAmt.Cmp(amount) == 0 { | ||
return amount, nil | ||
} | ||
|
||
// else, backtrack to remove this branch's sendings | ||
s.Senders = s.Senders[0:backtrackingIndex] | ||
} | ||
|
||
return s.trySendingUpTo(source.Sources[len(source.Sources)-1], amount) | ||
|
||
case *parser.SourceAllotment: | ||
var items []parser.AllotmentValue | ||
for _, i := range source.Items { | ||
|
@@ -675,6 +715,27 @@ | |
// passing "remainingAmount" directly breaks the code | ||
return handler(destination.Remaining, remainingAmountCopy) | ||
|
||
case *parser.DestinationOneof: | ||
err := s.checkFeatureFlag(ExperimentalOneofFeatureFlag) | ||
if err != nil { | ||
return err | ||
} | ||
for _, destinationClause := range destination.Clauses { | ||
cap, err := evaluateExprAs(s, destinationClause.Cap, expectMonetaryOfAsset(s.CurrentAsset)) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
// if the clause cap is >= the amount we're trying to receive, only go through this branch | ||
switch cap.Cmp(amount) { | ||
case 0, 1: | ||
return s.receiveFromKeptOrDest(destinationClause.To, amount) | ||
} | ||
|
||
// otherwise try next clause (keep looping) | ||
} | ||
return s.receiveFromKeptOrDest(destination.Remaining, amount) | ||
|
||
default: | ||
utils.NonExhaustiveMatchPanic[any](destination) | ||
return nil | ||
|
@@ -853,15 +914,16 @@ | |
r parser.Range, | ||
args []Value, | ||
) (*Monetary, InterpreterError) { | ||
if !s.OverdraftFunctionFeatureFlag { | ||
return nil, ExperimentalFeature{FlagName: ExperimentalOverdraftFunctionFeatureFlag} | ||
err := s.checkFeatureFlag(ExperimentalOverdraftFunctionFeatureFlag) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
// TODO more precise args range location | ||
p := NewArgsParser(args) | ||
account := parseArg(p, r, expectAccount) | ||
asset := parseArg(p, r, expectAsset) | ||
err := p.parse() | ||
err = p.parse() | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
@@ -982,3 +1044,12 @@ | |
|
||
return res, nil | ||
} | ||
|
||
func (s programState) checkFeatureFlag(flag string) InterpreterError { | ||
_, ok := s.FeatureFlags[flag] | ||
if ok { | ||
return nil | ||
} else { | ||
return ExperimentalFeature{FlagName: flag} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.
📝 Committable suggestion