fix: refactored grammar of prefix negation #103
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughDecoupled negative-number lexing from NUMBER, added unary prefix-minus grammar and AST node, and implemented evaluation/type-checking/hover support and runtime negation for numeric and monetary values; regenerated parser artifacts and added tests for prefix and infix minus scenarios. Changes
Sequence DiagramsequenceDiagram
participant Lexer
participant Parser
participant AST as "AST Builder"
participant Checker as "Type Checker"
participant Interpreter
participant Evaluator
Lexer->>Lexer: Tokenize "-5" -> MINUS, NUMBER
note over Lexer: NUMBER no longer includes leading '-'
Lexer->>Parser: emit tokens
Parser->>Parser: match prefixExpr (MINUS valueExpr)
Parser->>AST: create Prefix node (op='-', expr=NumberLiteral(5))
AST->>Checker: checkType(Prefix)
Checker->>Checker: ensure inner is Number|Monetary
AST->>Interpreter: evaluate Prefix
Interpreter->>Evaluator: unaryNegOp(innerExpr)
Evaluator->>Evaluator: evaluate inner -> Number or Monetary
Evaluator->>Evaluator: evalNeg() -> negated value
Evaluator->>Interpreter: return negated Value
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/interpreter/testdata/script-tests/prefix-minus-monetary.num (1)
1-4: Test file name may be misleading.This test demonstrates infix subtraction (
[EUR/2 100] - [EUR/2 1]), not prefix negation. The PR enables both features, but the filenameprefix-minus-monetary.numsuggests prefix usage (e.g.,-[EUR/2 1]).Consider either:
- Renaming to
infix-minus-monetary.num, or- Adding a true prefix test case such as
send -[EUR/2 -100]if semantically validinternal/interpreter/testdata/script-tests/prefix-minus-number.num (1)
1-4: Test file name may be misleading.Similar to the monetary test, this demonstrates infix subtraction (
100-1) rather than prefix negation. For a prefix test, consider expressions like[EUR/2 -$offset]where$offsetis a variable.The current tests are valuable for validating infix parsing without parentheses, but the naming doesn't reflect the content.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
internal/interpreter/testdata/script-tests/prefix-minus-monetary.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/prefix-minus-number.num.specs.jsonis excluded by!**/*.jsoninternal/parser/__snapshots__/parser_test.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (15)
Lexer.g4(1 hunks)Numscript.g4(1 hunks)internal/analysis/hover.go(1 hunks)internal/interpreter/evaluate_expr.go(2 hunks)internal/interpreter/infix_overloads.go(1 hunks)internal/interpreter/testdata/script-tests/prefix-minus-monetary.num(1 hunks)internal/interpreter/testdata/script-tests/prefix-minus-number.num(1 hunks)internal/parser/antlrParser/Lexer.interp(1 hunks)internal/parser/antlrParser/Numscript.interp(1 hunks)internal/parser/antlrParser/lexer.go(2 hunks)internal/parser/antlrParser/numscript_base_listener.go(1 hunks)internal/parser/antlrParser/numscript_listener.go(2 hunks)internal/parser/antlrParser/numscript_parser.go(57 hunks)internal/parser/ast.go(2 hunks)internal/parser/parser.go(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ascandone
Repo: formancehq/numscript PR: 55
File: internal/parser/antlrParser/numscript_parser.go:2953-2967
Timestamp: 2025-04-22T16:45:07.842Z
Learning: In numscript, type constraints for color expressions are not enforced at the grammar level but are instead handled during static analysis or at runtime. The grammar is intentionally kept permissive (accepting any valueExpr after the RESTRICT token) to maintain separation of concerns.
📚 Learning: 2025-02-06T17:48:48.962Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.962Z
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.
Applied to files:
internal/parser/antlrParser/numscript_parser.go
📚 Learning: 2025-04-22T16:45:07.842Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 55
File: internal/parser/antlrParser/numscript_parser.go:2953-2967
Timestamp: 2025-04-22T16:45:07.842Z
Learning: In numscript, type constraints for color expressions are not enforced at the grammar level but are instead handled during static analysis or at runtime. The grammar is intentionally kept permissive (accepting any valueExpr after the RESTRICT token) to maintain separation of concerns.
Applied to files:
internal/parser/antlrParser/numscript_parser.go
📚 Learning: 2025-02-06T17:48:48.961Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 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.
Applied to files:
internal/parser/antlrParser/numscript_parser.go
🧬 Code graph analysis (8)
internal/interpreter/infix_overloads.go (1)
internal/interpreter/interpreter.go (1)
InterpreterError(73-76)
internal/parser/antlrParser/numscript_base_listener.go (1)
internal/parser/antlrParser/numscript_parser.go (1)
PrefixExprContext(737-740)
internal/parser/antlrParser/numscript_listener.go (1)
internal/parser/antlrParser/numscript_parser.go (1)
PrefixExprContext(737-740)
internal/parser/ast.go (1)
internal/parser/range.go (1)
Range(13-16)
internal/parser/parser.go (3)
internal/parser/antlrParser/numscript_parser.go (1)
PrefixExprContext(737-740)internal/parser/ast.go (4)
Prefix(21-21)Prefix(76-80)PrefixOperator(24-24)ValueExpr(8-11)internal/parser/range.go (1)
Range(13-16)
internal/analysis/hover.go (1)
internal/parser/ast.go (2)
Prefix(21-21)Prefix(76-80)
internal/interpreter/evaluate_expr.go (4)
internal/parser/ast.go (5)
Prefix(21-21)Prefix(76-80)PrefixOperator(24-24)PrefixOperatorMinus(27-27)ValueExpr(8-11)internal/utils/utils.go (1)
NonExhaustiveMatchPanic(8-10)internal/interpreter/value.go (5)
Value(12-15)Monetary(22-25)Monetary(30-30)MonetaryInt(21-21)MonetaryInt(29-29)internal/interpreter/interpreter.go (1)
InterpreterError(73-76)
internal/parser/antlrParser/numscript_parser.go (1)
internal/parser/ast.go (6)
ValueExpr(8-11)Statement(266-269)Source(135-138)Destination(202-205)KeptOrDestination(247-249)SentValue(286-289)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Dirty
- GitHub Check: Tests
🔇 Additional comments (13)
Lexer.g4 (1)
41-41: LGTM!Removing the optional leading minus from
NUMBERenables the parser to correctly distinguish between prefix negation (e.g.,-$x) and infix subtraction (e.g.,a - b). This aligns with the PR objectives.internal/parser/antlrParser/Lexer.interp (1)
145-145: Generated file looks consistent with grammar changes.The ATN data reflects the regenerated lexer from the updated
Lexer.g4. No issues detected.internal/analysis/hover.go (1)
186-188: LGTM!The Prefix handling correctly delegates hover resolution to the inner expression. This follows the established pattern for composite expressions in this function.
internal/parser/antlrParser/numscript_listener.go (1)
22-24: Generated listener interface updated correctly.The
EnterPrefixExprandExitPrefixExprmethods are correctly added to the listener interface, matching the newprefixExprgrammar production.Also applies to: 157-159
internal/parser/antlrParser/Numscript.interp (1)
115-115: Generated file looks consistent with grammar changes.The ATN data reflects the regenerated parser from the updated
Numscript.g4with the newprefixExprproduction. No issues detected.Numscript.g4 (1)
22-22: Prefix minus operator is correctly implemented and functional.The
prefixExprrule successfully adds support for unary minus expressions. The parser correctly generates PrefixExprContext and handles cases like[USD/2 -50]and[COIN -10]as shown in the test files. The operator properly binds to its operand with expected behavior.internal/parser/antlrParser/numscript_base_listener.go (1)
47-52: Generated code additions look correct.The new
EnterPrefixExprandExitPrefixExprlistener hooks follow the established pattern for other expression types and are correctly placed in the listener interface.internal/parser/antlrParser/lexer.go (1)
71-231: Generated ATN data updated correctly.The serialized ATN data has been regenerated to reflect the grammar changes. The
MINUStoken (line 300:LexerMINUS = 30) remains correctly defined, and the ATN now properly handles the minus sign as a separate token rather than part of theNUMBERtoken.internal/parser/parser.go (1)
365-370: LGTM - Prefix expression parsing is correctly implemented.The new case for
PrefixExprContextfollows the established pattern used forInfixExprContext. The implementation correctly:
- Extracts the range from the context
- Converts the operator token text to
PrefixOperator- Recursively parses the inner
ValueExprinternal/interpreter/evaluate_expr.go (1)
226-243: LGTM - Unary negation operator implementation is correct.The
unaryNegOpmethod follows the established pattern used byplusOpandsubOp, correctly handling bothMonetaryandMonetaryInttypes through theopNeginterface.internal/parser/ast.go (1)
21-21: LGTM - Prefix AST node is well-structured.The
Prefixstruct correctly:
- Implements
ValueExprvia the marker method (line 21)- Embeds
Rangefor source location tracking- Follows the same pattern as
BinaryInfixfor consistencyAlso applies to: 76-80
internal/interpreter/infix_overloads.go (1)
83-105: LGTM! Clean implementation of unary negation support.The
opNeginterface and implementations follow the established patterns fromopAddandopSub. The unusedst *programStateparameter maintains interface consistency with other operators, which is a reasonable design choice. The negation logic correctly preserves the asset type forMonetaryvalues while negating only the amount.internal/parser/antlrParser/numscript_parser.go (1)
1-1: Auto-generated parser code - no manual review required.This file is generated from
Numscript.g4by ANTLR 4.13.2. The generated code correctly introducesPrefixExprContextfor handling unary minus expressions with appropriate precedence (level 5, higher than infix operators at levels 3-4). Any issues should be addressed in the grammar file rather than this generated code.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 18 files
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="internal/interpreter/testdata/script-tests/prefix-minus-monetary.num">
<violation number="1" location="internal/interpreter/testdata/script-tests/prefix-minus-monetary.num:1">
P2: Test file name suggests prefix minus (negation) test but content tests infix minus (subtraction). For a file named `prefix-minus-monetary.num`, consider testing actual prefix negation like `-[EUR/2 100]` rather than infix subtraction `[EUR/2 100] - [EUR/2 1]`. Note: `sub-monetaries.num` already exists for testing infix subtraction between monetaries.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
ascandone
force-pushed
the
fix/fix-minus-prefix
branch
from
cccfc2c to
77caca1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/parser/ast.go (1)
24-28: Type mismatch:PrefixOperatorMinusdeclared asInfixOperatorinstead ofPrefixOperator.The constant
PrefixOperatorMinusis typed asInfixOperator, but thePrefix.Operatorfield expectsPrefixOperator. This will require type conversion or cause type-check issues.Apply this fix:
const ( - PrefixOperatorMinus InfixOperator = "-" + PrefixOperatorMinus PrefixOperator = "-" )
🧹 Nitpick comments (1)
internal/interpreter/testdata/script-tests/minus-infix-number.num (1)
1-4: Test case correctly validates infix subtraction.This test appropriately demonstrates that the refactored grammar now parses
100-1as infix subtraction without requiring parentheses, which was the core objective of the PR.Consider adding test cases for edge cases to strengthen coverage:
- Chained subtraction:
100-1-2- Prefix negation followed by infix:
-100-1- Double minus (prefix on negative):
100--1- Prefix negation of variables:
-$x(mentioned in PR objectives)These could be separate test files or additional test cases if the test harness supports multiple scenarios per file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
internal/interpreter/testdata/script-tests/minus-infix-monetary.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/minus-infix-number.num.specs.jsonis excluded by!**/*.jsoninternal/parser/__snapshots__/parser_test.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (15)
Lexer.g4(1 hunks)Numscript.g4(1 hunks)internal/analysis/hover.go(1 hunks)internal/interpreter/evaluate_expr.go(2 hunks)internal/interpreter/infix_overloads.go(1 hunks)internal/interpreter/testdata/script-tests/minus-infix-monetary.num(1 hunks)internal/interpreter/testdata/script-tests/minus-infix-number.num(1 hunks)internal/parser/antlrParser/Lexer.interp(1 hunks)internal/parser/antlrParser/Numscript.interp(1 hunks)internal/parser/antlrParser/lexer.go(2 hunks)internal/parser/antlrParser/numscript_base_listener.go(1 hunks)internal/parser/antlrParser/numscript_listener.go(2 hunks)internal/parser/antlrParser/numscript_parser.go(57 hunks)internal/parser/ast.go(2 hunks)internal/parser/parser.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/analysis/hover.go
- internal/parser/antlrParser/numscript_listener.go
- internal/parser/antlrParser/Lexer.interp
- internal/interpreter/infix_overloads.go
- internal/interpreter/evaluate_expr.go
- internal/parser/parser.go
- Lexer.g4
- Numscript.g4
- internal/parser/antlrParser/Numscript.interp
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-02-06T17:48:48.962Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.962Z
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.
Applied to files:
internal/parser/antlrParser/numscript_parser.go
📚 Learning: 2025-04-22T16:45:07.842Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 55
File: internal/parser/antlrParser/numscript_parser.go:2953-2967
Timestamp: 2025-04-22T16:45:07.842Z
Learning: In numscript, type constraints for color expressions are not enforced at the grammar level but are instead handled during static analysis or at runtime. The grammar is intentionally kept permissive (accepting any valueExpr after the RESTRICT token) to maintain separation of concerns.
Applied to files:
internal/parser/antlrParser/numscript_parser.go
📚 Learning: 2025-02-06T17:48:48.961Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 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.
Applied to files:
internal/parser/antlrParser/numscript_parser.go
🧬 Code graph analysis (3)
internal/parser/ast.go (1)
internal/parser/range.go (1)
Range(13-16)
internal/parser/antlrParser/numscript_parser.go (1)
internal/parser/ast.go (7)
ValueExpr(8-11)VarDeclaration(321-326)Statement(266-269)Source(135-138)Destination(202-205)KeptOrDestination(247-249)SentValue(286-289)
internal/parser/antlrParser/numscript_base_listener.go (1)
internal/parser/antlrParser/numscript_parser.go (1)
PrefixExprContext(737-740)
🔇 Additional comments (6)
internal/interpreter/testdata/script-tests/minus-infix-monetary.num (1)
1-4: Infix monetary subtraction test matches naming and PR intentThe script correctly exercises infix
-between two monetary literals with standardsource/destinationbindings, and the filename now clearly reflects that this is an infix-minus monetary test rather than prefix negation. Looks good as a focused regression/coverage case for the new grammar behavior.internal/parser/ast.go (1)
76-80: LGTM!The
Prefixstruct is well-designed with appropriate fields:Rangefor source location,Operatorfor the prefix operator type, andExprfor the operand expression.internal/parser/antlrParser/numscript_base_listener.go (1)
47-51: LGTM!Auto-generated listener methods for
PrefixExprfollow the established pattern and correctly implement theNumscriptListenerinterface.internal/parser/antlrParser/numscript_parser.go (2)
737-790: LGTM!The auto-generated
PrefixExprContextcorrectly implements the context for prefix expressions with proper token handling and listener integration.
1407-1425: LGTM!The prefix expression parsing logic correctly handles the unary minus operator with appropriate precedence (5), ensuring proper binding relative to infix operators.
internal/parser/antlrParser/lexer.go (1)
1-1: Generated lexer code correctly reflects grammar changes — LGTM.This file is auto-generated from
Lexer.g4by ANTLR 4.13.2. TheNUMBERtoken rule excludes a leading minus sign ([0-9]+ ('_' [0-9]+)*), whileMINUSis defined as a separate token ('-'). This allows proper parsing of prefix negation (e.g.,-5parses as unary MINUS followed by NUMBER) versus infix usage. The updatedserializedATNdata correctly encodes these grammar rules.
ascandone
force-pushed
the
fix/fix-minus-prefix
branch
from
77caca1 to
eff50a4
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #103 +/- ##
==========================================
- Coverage 68.13% 67.83% -0.31%
==========================================
Files 45 45
Lines 4290 4352 +62
==========================================
+ Hits 2923 2952 +29
- Misses 1209 1241 +32
- Partials 158 159 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
|
@ascandone: I'll review the changes in this PR. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Numscript.g4 (1)
14-26: Check unary-minus precedence:op=MINUS valueExprcurrently binds to entire remaining expressionWith the current grammar where all
valueExpralternatives are at the same precedence level, expressions like-1 + 2are parsed as-(1 + 2)rather than(-1) + 2. This occurs because theprefixExpralternative on line 22 callsvalueExprrecursively for its operand, which can match the entire additive expression including the+ 2.To preserve standard arithmetic precedence where unary minus binds only to its immediate operand and
+/-remain left-associative, restructure into explicit precedence layers:valueExpr : additiveExpr ; additiveExpr : multiplicativeExpr ( (PLUS | MINUS) multiplicativeExpr )* ; multiplicativeExpr : prefixExpr ( DIV prefixExpr )* ; prefixExpr : MINUS prefixExpr | primary ; primary : VARIABLE_NAME | ASSET | STRING | ACCOUNT_START accountLiteralPart (COLON accountLiteralPart)* | NUMBER | PERCENTAGE_PORTION_LITERAL | monetaryLit | LPARENS valueExpr RPARENS | functionCall ;
♻️ Duplicate comments (1)
internal/parser/ast.go (1)
13-28: Prefix AST node and operator typing are consistent and fix the earlier mismatch
PrefiximplementsValueExprviafunc (*Prefix) valueExpr(), so it’s fully part of the expression AST.- Introducing
PrefixOperatorand declaringPrefixOperatorMinusasPrefixOperator = "-"fixes the earlier type mismatch where it was anInfixOperator.- The
Prefixstruct (Range,Operator,Expr) matches how other expression nodes are modeled.Looks good from the AST side.
Also applies to: 76-80
🧹 Nitpick comments (1)
internal/analysis/check.go (1)
471-479: Type checking for prefix minus is consistent with binary minus and constrained to numeric/monetaryThe new
*parser.Prefixhandling incheckTypeOf:
- Unifies the prefix node’s type with its inner expression (
res.unifyNodeWith(lit, res.GetExprType(lit.Expr))), matching how other composite expressions are treated.- Uses
checkHasOneOfTypes(lit.Expr, []string{TypeNumber, TypeMonetary})to restrict the operand of unary minus to number or monetary, and emits aTypeMismatchif it isn’t.The helper
checkHasOneOfTypesmirrorscheckInfixOverload’s semantics for a single operand and centralises the “one of these types or any” pattern, which keeps the code clear and avoids duplicating the diagnostic logic.One minor nuance:
checkHasOneOfTypespassesallowed[0]as the type hint intocheckTypeOf, so for-<monetary>the hint used in some diagnostics will benumber, notmonetary, butassertHasTypestill enforces the actual required type at the call sites. This is mostly cosmetic and probably acceptable.Also applies to: 524-536
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (4)
internal/interpreter/testdata/script-tests/minus-infix-monetary.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/minus-infix-number.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/minus-prefix-number.num.specs.jsonis excluded by!**/*.jsoninternal/parser/__snapshots__/parser_test.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (17)
Lexer.g4(1 hunks)Numscript.g4(1 hunks)internal/analysis/check.go(3 hunks)internal/analysis/hover.go(1 hunks)internal/interpreter/evaluate_expr.go(2 hunks)internal/interpreter/infix_overloads.go(1 hunks)internal/interpreter/testdata/script-tests/minus-infix-monetary.num(1 hunks)internal/interpreter/testdata/script-tests/minus-infix-number.num(1 hunks)internal/interpreter/testdata/script-tests/minus-prefix-number.num(1 hunks)internal/parser/antlrParser/Lexer.interp(1 hunks)internal/parser/antlrParser/Numscript.interp(1 hunks)internal/parser/antlrParser/lexer.go(2 hunks)internal/parser/antlrParser/numscript_base_listener.go(1 hunks)internal/parser/antlrParser/numscript_listener.go(2 hunks)internal/parser/antlrParser/numscript_parser.go(57 hunks)internal/parser/ast.go(2 hunks)internal/parser/parser.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/interpreter/testdata/script-tests/minus-prefix-number.num
🚧 Files skipped from review as they are similar to previous changes (8)
- Lexer.g4
- internal/parser/antlrParser/Lexer.interp
- internal/interpreter/testdata/script-tests/minus-infix-monetary.num
- internal/parser/antlrParser/numscript_base_listener.go
- internal/parser/antlrParser/Numscript.interp
- internal/interpreter/testdata/script-tests/minus-infix-number.num
- internal/analysis/hover.go
- internal/parser/parser.go
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-03-18T11:14:21.411Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 45
File: internal/parser/ast.go:21-21
Timestamp: 2025-03-18T11:14:21.411Z
Learning: In Go, empty methods like `valueExpr()` are commonly used as interface markers and don't require dedicated unit tests as they have no functionality to test. The actual behavior is tested through integration tests that use the types in context.
Applied to files:
internal/parser/ast.go
📚 Learning: 2025-05-30T13:51:20.130Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 62
File: internal/interpreter/evaluate_expr.go:143-144
Timestamp: 2025-05-30T13:51:20.130Z
Learning: In the numscript interpreter color system: `nil` color means "any color" (no color filter applied), while an empty string `""` color is treated as a specific color value, distinct from nil. The `evaluateColor` function in `internal/interpreter/evaluate_expr.go` should return a pointer to an empty string when no color expression is provided, not nil, to maintain this semantic distinction.
Applied to files:
internal/interpreter/evaluate_expr.go
📚 Learning: 2025-02-06T17:48:48.962Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.962Z
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.
Applied to files:
internal/parser/antlrParser/numscript_parser.go
📚 Learning: 2025-04-22T16:45:07.842Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 55
File: internal/parser/antlrParser/numscript_parser.go:2953-2967
Timestamp: 2025-04-22T16:45:07.842Z
Learning: In numscript, type constraints for color expressions are not enforced at the grammar level but are instead handled during static analysis or at runtime. The grammar is intentionally kept permissive (accepting any valueExpr after the RESTRICT token) to maintain separation of concerns.
Applied to files:
internal/parser/antlrParser/numscript_parser.go
📚 Learning: 2025-02-06T17:48:48.961Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 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.
Applied to files:
internal/parser/antlrParser/numscript_parser.go
🧬 Code graph analysis (5)
internal/interpreter/infix_overloads.go (1)
internal/interpreter/interpreter.go (1)
InterpreterError(73-76)
internal/parser/ast.go (1)
internal/parser/range.go (1)
Range(13-16)
internal/parser/antlrParser/numscript_listener.go (1)
internal/parser/antlrParser/numscript_parser.go (1)
PrefixExprContext(737-740)
internal/parser/antlrParser/numscript_parser.go (1)
internal/parser/ast.go (7)
ValueExpr(8-11)VarDeclaration(321-326)Statement(266-269)Source(135-138)Destination(202-205)KeptOrDestination(247-249)SentValue(286-289)
internal/analysis/check.go (2)
internal/parser/ast.go (4)
Prefix(21-21)Prefix(76-80)PrefixOperatorMinus(27-27)ValueExpr(8-11)internal/analysis/diagnostic_kind.go (2)
TypeMismatch(132-135)TypeMismatch(141-143)
🔇 Additional comments (8)
internal/parser/antlrParser/lexer.go (1)
71-231: Regenerated lexer ATN looks consistent with grammar changesThe only modification here is the serialized ATN; assuming this was regenerated from the updated
Lexer.g4(NUMBER without leading-and separateMINUStoken), it’s fine and shouldn’t be edited by hand.internal/parser/antlrParser/numscript_parser.go (2)
737-790:PrefixExprContextwiring matches the new grammarThe
PrefixExprContextstruct, itsValueExpr()accessor, MINUS token accessor, and Enter/Exit hooks are consistent with the newprefixExpralternative in the grammar and the existing pattern used for other labeled alternatives. Nothing to change here.
1259-1579: GeneratedvalueExprparser logic aligns with the new prefix-minus alternativeThe regenerated
valueExprmethod correctly:
- Treats MINUS as a base alternative that creates a
PrefixExprContext, capturing the operator token.- Recurses into
valueExpr(5)for the operand, and- Leaves infix DIV / PLUS / MINUS handling unchanged in the left-recursive section.
This matches the updated grammar; any precedence/associativity tuning should be done in
Numscript.g4rather than editing this generated code directly.internal/parser/antlrParser/numscript_listener.go (1)
22-24: Listener interface correctly extended forprefixExprThe additions of
EnterPrefixExprandExitPrefixExprare consistent with the newPrefixExprContextand follow the existing listener pattern. No further changes needed here.Also applies to: 157-159
internal/interpreter/evaluate_expr.go (2)
87-96: Prefix evaluation wiring matches new AST operatorThe new
case *parser.Prefixbranch correctly:
- Switches on
expr.Operatorwith a dedicatedPrefixOperatorMinusarm.- Delegates to
unaryNegOpand treats any future operators as a panic viaNonExhaustiveMatchPanic.This is consistent with how binary infix operators are handled.
226-243:unaryNegOpcorrectly restricts prefix minus to numeric/monetary operands
unaryNegOpmirrors the existingplusOp/subOphelpers:
- Uses
evaluateExprAswithexpectOneOf(expectMonetary, expectNumber)and maps both toopNegimplementations (Monetary/MonetaryInt).- Propagates any type/interpretation errors from
evaluateExprAs.- Calls
evalNegon the resolved operand value.This cleanly enforces “only numbers and monetaries are negatable” at runtime and keeps the operator extensible via
opNeg.internal/interpreter/infix_overloads.go (1)
84-105: Unary negation interface and implementations look correct
opNegcleanly abstracts unary negation, parallel toopAdd/opSub.MonetaryInt.evalNegandMonetary.evalNegboth:
- Convert to
big.Int, callNeg, and wrap the result in the appropriate value type.- Do not mutate the original values, returning fresh instances.
- The interface satisfaction checks with
(*MonetaryInt)(nil)/(*Monetary)(nil)are appropriate given the receiver signatures.This provides a solid foundation for the interpreter’s unary minus semantics.
internal/analysis/check.go (1)
698-709: Static evaluation support for numeric prefix-minus expressions is sensibleExtending
tryEvaluatingNumberExprwith a*parser.Prefix/PrefixOperatorMinuscase allows compile-time evaluation of simple numeric negations like-1and-(1 - 2)where the inner expression is itself statically evaluable. It correctly returnsnilfor other operators (future-proofing) or non-evaluable subexpressions, matching the existing behaviour for binary infix expressions.
This PR fixes the grammar of the prefix
-operator.Before, it was part of the
NUMBERtoken.Now, it's a prefix operator.
This allows the parser to correctly recognise infix
-. (Which would previously require the usage of()in order not to emit a parsing error)Unlike the old machine impl, this will also allow variable expressions:
-$xThe PR addresses all the involved parts: interpreter, static check (including asset inference), hovering, goto, etc.
Semantics
The prefix
-only works with numbers and monetaries