Skip to content
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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions Numscript.g4
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ source:
| valueExpr # srcAccount
| LBRACE allotmentClauseSrc+ RBRACE # srcAllotment
| LBRACE source* RBRACE # srcInorder
| 'oneof' LBRACE source+ RBRACE # srcOneof
| MAX cap = valueExpr FROM source # srcCapped;
allotmentClauseSrc: allotment FROM source;

Expand All @@ -94,9 +95,10 @@ keptOrDestination:
destinationInOrderClause: MAX valueExpr keptOrDestination;

destination:
valueExpr # destAccount
| LBRACE allotmentClauseDest+ RBRACE # destAllotment
| LBRACE destinationInOrderClause* REMAINING keptOrDestination RBRACE # destInorder;
valueExpr # destAccount
| LBRACE allotmentClauseDest+ RBRACE # destAllotment
| LBRACE destinationInOrderClause* REMAINING keptOrDestination RBRACE # destInorder
| 'oneof' LBRACE destinationInOrderClause* REMAINING keptOrDestination RBRACE # destOneof;
allotmentClauseDest: allotment keptOrDestination;

sentValue: valueExpr # sentLiteral | sentAllLit # sentAll;
Expand Down
12 changes: 12 additions & 0 deletions internal/analysis/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -503,6 +503,11 @@
res.checkSource(source)
}

case *parser.SourceOneof:
for _, source := range source.Sources {
res.checkSource(source)
}
Comment on lines +506 to +509
Copy link

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.

 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.

Suggested change
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)
}


case *parser.SourceCapped:
onExit := res.enterCappedSource()

Expand Down Expand Up @@ -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)

Check warning on line 584 in internal/analysis/check.go

View check run for this annotation

Codecov / codecov/patch

internal/analysis/check.go#L579-L584

Added lines #L579 - L584 were not covered by tests

Comment on lines +579 to +585
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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

‼️ 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.

Suggested change
case *parser.DestinationOneof:
for _, clause := range destination.Clauses {
res.checkExpression(clause.Cap, TypeMonetary)
res.checkKeptOrDestination(clause.To)
}
res.checkKeptOrDestination(destination.Remaining)
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

case *parser.DestinationAllotment:
var remainingAllotment *parser.RemainingAllotment
var variableLiterals []parser.Variable
Expand Down
25 changes: 25 additions & 0 deletions internal/analysis/check_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,31 @@ func TestUnboundVarInSource(t *testing.T) {
)
}

func TestUnboundVarInSourceOneof(t *testing.T) {
t.Parallel()

input := `send [C 1] (
source = oneof { $unbound_var }
destination = @dest
)`

program := parser.Parse(input).Value

diagnostics := analysis.CheckProgram(program).Diagnostics
require.Len(t, diagnostics, 1)

assert.Equal(t,
[]analysis.Diagnostic{
{
Range: parser.RangeOfIndexed(input, "$unbound_var", 0),
Kind: &analysis.UnboundVariable{Name: "unbound_var"},
},
},
diagnostics,
)

}

func TestUnboundVarInDest(t *testing.T) {
t.Parallel()

Expand Down
5 changes: 5 additions & 0 deletions internal/cmd/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
var runOutFormatOpt string

var overdraftFeatureFlag bool
var oneOfFeatureFlag bool

type inputOpts struct {
Script string `json:"script"`
Expand Down Expand Up @@ -121,6 +122,9 @@
if overdraftFeatureFlag {
featureFlags[interpreter.ExperimentalOverdraftFunctionFeatureFlag] = struct{}{}
}
if oneOfFeatureFlag {
featureFlags[interpreter.ExperimentalOneofFeatureFlag] = struct{}{}
}

Check warning on line 127 in internal/cmd/run.go

View check run for this annotation

Codecov / codecov/patch

internal/cmd/run.go#L125-L127

Added lines #L125 - L127 were not covered by tests
Comment on lines +125 to +127
Copy link

Choose a reason for hiding this comment

The 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:

  • 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:

  1. Program behavior when the flag is enabled
  2. Program behavior when the flag is disabled
  3. 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


result, err := interpreter.RunProgram(context.Background(), parseResult.Value, opt.Variables, interpreter.StaticStore{
Balances: opt.Balances,
Expand Down Expand Up @@ -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")

Check warning on line 208 in internal/cmd/run.go

View check run for this annotation

Codecov / codecov/patch

internal/cmd/run.go#L208

Added line #L208 was not covered by tests

// Output options
cmd.Flags().StringVar(&runOutFormatOpt, "output-format", OutputFormatPretty, "Set the output format. Available options: pretty, json.")
Expand Down
9 changes: 9 additions & 0 deletions internal/interpreter/batch_balances_query.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,15 @@
}
return nil

case *parser.SourceOneof:
for _, subSource := range source.Sources {
err := st.findBalancesQueries(subSource)
if err != nil {
return err
}

Check warning on line 136 in internal/interpreter/batch_balances_query.go

View check run for this annotation

Codecov / codecov/patch

internal/interpreter/batch_balances_query.go#L135-L136

Added lines #L135 - L136 were not covered by tests
}
return nil

case *parser.SourceCapped:
// TODO can this be optimized in some cases?
return st.findBalancesQueries(source.From)
Expand Down
89 changes: 80 additions & 9 deletions internal/interpreter/interpreter.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -198,10 +201,7 @@

CurrentBalanceQuery: BalanceQuery{},
ctx: ctx,
}

if _, ok := featureFlags[ExperimentalOverdraftFunctionFeatureFlag]; ok {
st.OverdraftFunctionFeatureFlag = true
FeatureFlags: featureFlags,
}

err := st.parseVars(program.Vars, vars)
Expand Down Expand Up @@ -261,7 +261,7 @@

CurrentBalanceQuery BalanceQuery

OverdraftFunctionFeatureFlag bool
FeatureFlags map[string]struct{}
}

func (st *programState) pushSender(name string, monetary *big.Int) {
Expand Down Expand Up @@ -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
Copy link

@coderabbitai coderabbitai bot Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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:

  1. Either documenting that sendAll always uses the first source
  2. Or implementing the same backtracking logic as in trySendingUpTo

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link

Choose a reason for hiding this comment

The 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
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.

case *parser.SourceCapped:
monetary, err := evaluateExprAs(s, source.Cap, expectMonetaryOfAsset(s.CurrentAsset))
if err != nil {
Expand Down Expand Up @@ -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
}

Check warning on line 597 in internal/interpreter/interpreter.go

View check run for this annotation

Codecov / codecov/patch

internal/interpreter/interpreter.go#L596-L597

Added lines #L596 - L597 were not covered by tests

// 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 {
Expand Down Expand Up @@ -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
}

Check warning on line 727 in internal/interpreter/interpreter.go

View check run for this annotation

Codecov / codecov/patch

internal/interpreter/interpreter.go#L726-L727

Added lines #L726 - L727 were not covered by tests

// 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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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}
}
}
Loading