Implemented account interpolation syntax #44
Conversation
WalkthroughThe changes introduce a new lexer grammar (Lexer.g4) with an extensive set of token definitions and modes for account parsing. The Numscript grammar (Numscript.g4) has been refactored to remove inline token definitions in favor of an external lexer by referencing Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant L as Lexer
participant P as Parser
participant A as AST Builder
participant I as Interpreter
U->>L: Submit script with account interpolation literal
L->>P: Provide token stream (includes ACCOUNT_START, COLON, etc.)
P->>A: Build AST with AccountInterpLiteral (composed of Parts)
A->>I: Pass AST for evaluation
I->>I: Loop through each part in AccountInterpLiteral
alt Text part
I->>I: Append literal text to account string
else Variable part
I->>I: Evaluate variable and invoke castToString
end
I-->>U: Return constructed account address
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 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 #44 +/- ##
==========================================
- Coverage 63.64% 63.48% -0.16%
==========================================
Files 31 31
Lines 6824 7101 +277
==========================================
+ Hits 4343 4508 +165
- Misses 2281 2384 +103
- Partials 200 209 +9 ☔ 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: 2
🔭 Outside diff range comments (1)
internal/parser/antlrParser/numscript_listener.go (1)
1-261: 💡 Verification agent🧩 Analysis chain
Verify broader impact of package name change.
The package name change from
parsertoantlrParsermay require updates in import statements throughout the codebase.
🏁 Script executed:
#!/bin/bash # Find any remaining references to the old package name in import statements echo "Checking for import statements referencing the old package..." rg -A 1 -B 1 "import.*parser" --glob "*.go" | grep -v "antlrParser" echo "Checking for usages of the old package name..." rg "\bparser\." --glob "*.go" | grep -v "antlrParser"Length of output: 50654
Action required: Update all outdated package references from
parsertoantlrParserThe renaming of the package in
internal/parser/antlrParser/numscript_listener.gohas a broader impact. Our search indicates that many files still reference the old package name (parser), both in import statements and as a qualifier in type and function calls. Please update all import paths and usages to useantlrParser(or the new alias if applicable) to ensure consistency across the codebase.Files (among others) that require updates:
internal/analysis/goto_definition.go(e.g.,import "github.com/formancehq/numscript/internal/parser")internal/interpreter/args_parser.goand its test fileinternal/lsp/language_server/message_buffer_test.go- Numerous references in files under
internal/analysis,internal/interpreter,internal/lsp, and other directoriesPlease review and update the affected lines accordingly so that all stale
parserreferences are either replaced withantlrParseror updated as per the new package structure.
🧹 Nitpick comments (15)
generate-parser.sh (1)
1-2: Add shebang line and enhance script organization.The script is missing a shebang line to specify which shell to use for execution, which was flagged by the static analysis tool.
+#!/bin/bash + antlr4 -Dlanguage=Go Lexer.g4 Numscript.g4 -o internal/parser/antlrParser -package antlrParser mv internal/parser/antlrParser/_lexer.go internal/parser/antlrParser/lexer.go🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
internal/parser/antlrParser/numscript_base_listener.go (1)
1-278: Consider documenting the new account interpolation syntax.The implementation adds support for the new account interpolation syntax, but it might be helpful to include documentation comments explaining the syntax constraints (e.g., that it supports "something:something" but not more complex interpolations like
@acc{$id}as mentioned in the PR objectives).Consider adding documentation to the package-level comments or to the relevant listener methods to explain the syntax constraints and provide usage examples.
internal/parser/ast.go (1)
105-116:String()method with inline concatenation for account parts.
Building the final string viastrings.Joinis concise. However, consider the TODO comment about parsing for more robust handling of special cases or invalid parts. If expanded interpolation logic is required later, a proper parser step might be safer.internal/parser/antlrParser/lexer.go (1)
21-21: Remove TODO comment before finalizing the PR.The TODO comment indicates an incomplete implementation of the EOF (End of File) token handling. Consider addressing this before merging the PR to ensure proper EOF handling.
internal/interpreter/interpreter.go (1)
185-185: Feature flag follows consistent naming pattern.The new experimental feature flag follows the same naming pattern as existing flags and uses a descriptive name. Consider adding documentation that explains when and how to use this flag.
- ExperimentalAccountInterpolationFlag FeatureFlag = "experimental-account-interpolation" + // ExperimentalAccountInterpolationFlag enables the account interpolation syntax + // Example: @account:$variable:suffix + ExperimentalAccountInterpolationFlag FeatureFlag = "experimental-account-interpolation"internal/interpreter/interpreter_error.go (1)
211-213: Consider adding more context to the error message.The error message could be improved to provide more context about where or why the casting failed, which would help with debugging.
func (e CannotCastToString) Error() string { - return fmt.Sprintf("Cannot cast this value to string: %s", e.Value) + return fmt.Sprintf("Cannot cast value to string for account interpolation: %s (type: %T)", e.Value, e.Value) }internal/interpreter/evaluate_expr.go (1)
15-41: Consider validating the constructed account name.
The logic for assembling parts of the account name is correct, but there's aTODOnote about validating valid names. It might be worth implementing basic checks (e.g., allowable characters, non-empty parts) to ensure invalid interpolated accounts are caught early.internal/parser/parser_test.go (2)
423-430: Enhance test with descriptive name and actual string templateThis test verifies basic parameter parsing but doesn't actually test string templates as the name suggests. Consider either renaming the test to reflect its actual purpose or updating it to test real string template functionality.
-func TestStringTemplate(t *testing.T) { +func TestNumericArgumentsInFunctionCall(t *testing.T) { p := parser.Parse( "set_tx_meta(0, 42)", ) require.Len(t, p.Errors, 0) snaps.MatchSnapshot(t, p.Value) }If the intent was to test actual string templates, consider using an example with interpolation:
-func TestStringTemplate(t *testing.T) { - p := parser.Parse( - "set_tx_meta(0, 42)", - ) +func TestStringTemplate(t *testing.T) { + p := parser.Parse( + `set_tx_meta("template:$id", 42)`, + ) require.Len(t, p.Errors, 0) snaps.MatchSnapshot(t, p.Value) }
432-439: Expand test coverage of account interpolation syntaxThis test only covers one account interpolation pattern. Consider adding tests for additional patterns to ensure comprehensive coverage of the new syntax.
Based on the PR objectives, the implementation supports interpolated values between colons in account names. Consider adding tests for:
- Simple account with one interpolation:
@user:$id- Multiple interpolations:
@user:$id:$status- Edge cases like empty segments:
@user::$id- Invalid interpolations that should fail parsing
For example:
func TestInterpAccount(t *testing.T) { p := parser.Parse( "set_tx_meta(@abc:cde:$id, 42)", ) require.Len(t, p.Errors, 0) snaps.MatchSnapshot(t, p.Value) + + // Test simple interpolation + p = parser.Parse( + "set_tx_meta(@user:$id, 42)", + ) + require.Len(t, p.Errors, 0) + + // Test multiple interpolations + p = parser.Parse( + "set_tx_meta(@user:$id:$status, 42)", + ) + require.Len(t, p.Errors, 0) }internal/interpreter/interpreter_test.go (2)
3839-3868: Confirmed coverage of account interpolation logic.
These tests effectively validate the new interpolation feature by verifying that variables are inserted correctly within account addresses. It would be beneficial to add a test case covering edge scenarios like an empty string or special characters in the$statusvariable.
3869-3898: Good negative testing scenario.
Testing that a monetary value cannot be cast to a string is crucial for validation. Consider expanding the coverage to test interpolation failures with partially invalid segments in addition to fully invalid numeric literals.internal/analysis/check.go (3)
389-395: Consider stricter type checking for each interpolated part.
The current logic treats every part of the interpolated account as[TypeAny], which might fail only at runtime if an invalid type is passed in. Strengthening compile-time checks (e.g., ensuring string-only parts) could help catch incorrect usage earlier.
467-485: Unify logic with non-interpolated account checks.
While this block correctly handles “world” vs. “regular” accounts, there is some duplication of logic also present for plainAccountLiterals elsewhere. Consider extracting common checks to reduce code duplication and ensure consistency across literal types.
488-488: Refactor overdraft checks to avoid repetition.
The check for aworldaccount in an overdraft scenario overlaps with the logic for regular accounts. It might be helpful to unify these code paths for maintainability and clarity.internal/parser/parser.go (1)
308-311: Consider adding documentation for the new account interpolation featureThe
AccountInterpLiteralstructure and implementation are correct, but adding documentation comments would help future maintainers understand the intended behavior and limitations of the account interpolation feature.Consider adding a comment like:
return &AccountInterpLiteral{ + // AccountInterpLiteral supports the syntax @text:$var:text where variables + // can be interpolated between colons in account names Range: litRng, Parts: parts, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
internal/parser/__snapshots__/parser_fault_tolerance_test.snapis excluded by!**/*.snap,!**/*.snapinternal/parser/__snapshots__/parser_test.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (27)
Lexer.g4(1 hunks)Numscript.g4(3 hunks)generate-parser.sh(1 hunks)internal/analysis/check.go(3 hunks)internal/analysis/check_test.go(1 hunks)internal/analysis/hover.go(1 hunks)internal/analysis/hover_test.go(1 hunks)internal/interpreter/evaluate_expr.go(3 hunks)internal/interpreter/interpreter.go(1 hunks)internal/interpreter/interpreter_error.go(1 hunks)internal/interpreter/interpreter_test.go(1 hunks)internal/parser/antlr/Numscript.interp(0 hunks)internal/parser/antlr/Numscript.tokens(0 hunks)internal/parser/antlr/NumscriptLexer.interp(0 hunks)internal/parser/antlr/NumscriptLexer.tokens(0 hunks)internal/parser/antlr/numscript_lexer.go(0 hunks)internal/parser/antlrParser/Lexer.interp(1 hunks)internal/parser/antlrParser/Lexer.tokens(1 hunks)internal/parser/antlrParser/Numscript.interp(1 hunks)internal/parser/antlrParser/Numscript.tokens(1 hunks)internal/parser/antlrParser/lexer.go(1 hunks)internal/parser/antlrParser/numscript_base_listener.go(2 hunks)internal/parser/antlrParser/numscript_listener.go(3 hunks)internal/parser/antlrParser/numscript_parser.go(60 hunks)internal/parser/ast.go(4 hunks)internal/parser/parser.go(22 hunks)internal/parser/parser_test.go(2 hunks)
💤 Files with no reviewable changes (5)
- internal/parser/antlr/NumscriptLexer.interp
- internal/parser/antlr/Numscript.interp
- internal/parser/antlr/NumscriptLexer.tokens
- internal/parser/antlr/Numscript.tokens
- internal/parser/antlr/numscript_lexer.go
🧰 Additional context used
🧠 Learnings (2)
internal/parser/parser.go (1)
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/parser/antlr/numscript_parser.go:2786-2850
Timestamp: 2025-01-20T22:55:14.279Z
Learning: Do not suggest adding tests or improving test coverage for auto-generated code, especially files with headers indicating they are generated (e.g., "Code generated from X by Y. DO NOT EDIT.").
internal/parser/antlrParser/numscript_parser.go (1)
undefined
<retrieved_learning>
Learnt from: ascandone
PR: #31
File: internal/parser/antlr/numscript_parser.go:2786-2850
Timestamp: 2025-01-20T22:55:14.279Z
Learning: Do not suggest adding tests or improving test coverage for auto-generated code, especially files with headers indicating they are generated (e.g., "Code generated from X by Y. DO NOT EDIT.").
</retrieved_learning>
🪛 Shellcheck (0.10.0)
generate-parser.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🔇 Additional comments (50)
internal/parser/antlrParser/Lexer.interp (1)
1-142: Lexer definition supports the new account interpolation syntax successfully.The lexer definitions appropriately define modes and tokens for handling the new interpolation syntax. The
ACCOUNT_MODElexer mode, combined with tokens likeACCOUNT_TEXT,VARIABLE_NAME_ACC, andCOLON, provides the foundation for parsing account names with interpolated variable values as described in the PR objectives.internal/parser/antlrParser/numscript_listener.go (3)
3-3: Package name updated correctly.The package name has been changed from
parsertoantlrParserto reflect the new directory structure.
13-18: Interface extended with account interpolation support.Added methods to support entering the text and variable parts of account literals, which is essential for the new account interpolation syntax.
139-144: Corresponding exit methods added correctly.The corresponding exit methods for the account text and variable parts have been added, maintaining the symmetry of the listener interface.
internal/parser/antlrParser/numscript_base_listener.go (2)
3-3: Package name updated correctly.The package name has been changed from
parsertoantlrParserto reflect the new directory structure.
29-40: Base implementation provided for account interpolation methods.The base implementations for the new account text and variable part methods have been added. These empty implementations provide a good starting point for custom listeners that extend this base class.
internal/parser/ast.go (5)
5-5: Importing "strings" looks appropriate.
This import is needed for thestrings.Joinusage below. No issues here.
13-20: ImplementingvalueExpr()for literal and expression types.
Each type now properly satisfies theValueExprinterface with a no-op method. This design pattern ensures these types are recognized as valid expressions in the parsing and interpretation flow.
52-55: NewAccountInterpLiteralstruct.
Storing parts in a[]AccountNamePartallows for more flexible handling of text vs. variable-based segments. This design lays a solid foundation for the interpolation feature.
76-80:AccountNamePartinterface andAccountTextPartstruct introduction.
These types create a clear separation between text-based and variable-based account segments. The pointer vs. value usage is consistent (*Variablevs.AccountTextPart).
92-103:IsWorld()method forAccountInterpLiteral.
Checking for a single-part literal named"world"is straightforward. The switch statement is correct, and returningfalsefor non-text parts is appropriate.internal/parser/antlrParser/Numscript.tokens (2)
1-41: Token definitions for whitespace, comments, and keywords.
All tokens appear consistently assigned to their integer IDs. Ensure these IDs remain stable if other tokens are added in the future to avoid mismatches.
42-70: String literal references for token names.
These mappings ('vars'=5,'max'=6, etc.) align each keyword with its corresponding token ID. Good approach for maintainability.internal/parser/antlrParser/Lexer.tokens (2)
1-41: Lexer token definitions for whitespace, comments, and language constructs.
This file parallels the Numscript tokens, ensuring a synchronized lexer. Keeping these definitions autonomous helps with modular maintenance.
42-70: Literal and numeric ID mappings for Lexer tokens.
Assigning explicit numeric IDs and providing string forms ensures consistency with your grammar references. Looks good.Numscript.g4 (7)
3-5: Switching grammar to rely on external lexer (tokenVocab = 'Lexer').
Removing inline token definitions maintains a clean separation of concerns and aligns with best practices for ANTLR grammar structuring.
10-13: RefinedaccountLiteralPartrule usingACCOUNT_TEXTorVARIABLE_NAME_ACC.
This rule neatly captures text segments or variable segments, enabling more flexible account interpolation.
15-24: ExtendedvalueExprto handle interpolated account literals.
IntegratingACCOUNT_START accountLiteralPart (COLON accountLiteralPart)*allows multi-part account strings. This approach matches the newly introducedAccountInterpLiterallogic in the AST.
26-26:functionCallArgsrule accounts for multiplevalueExprparameters.
Supports a flexible function call syntax with optional comma-separated arguments.
30-30:varOriginproduction referencingfunctionCall.
Associating a variable origin with a function call is clear. This introduction fosters dynamic assignment logic.
50-50: Use ofONEOFtoken for source alternatives.
Shifting from a literal'oneof'to theONEOFtoken is consistent with the external token vocabulary approach and keeps grammar definitions clean.
60-63: Refineddestinationrule to handleONEOFdestination blocks.
Similar to the source block, this maintains consistency for multi-option destination parsing.internal/parser/antlrParser/lexer.go (2)
43-44: Account interpolation mode handling looks good.The lexer implementation properly handles the account interpolation mode transitions. The ACCOUNT_MODE is entered when encountering either the @ symbol or a colon, and is properly exited after processing account text or a variable name.
Also applies to: 47-50
314-315: Well-structured mode definition.The mode constant is clearly defined and aligns with the mode pattern in the Lexer.g4 grammar file.
Lexer.g4 (3)
43-45: Account interpolation grammar looks well-designed.The grammar rules for account interpolation are clean and intuitive. The
pushModeoperations ensure proper context tracking during lexical analysis, which is essential for this syntax feature.
47-50: Good separation of account mode rules.The dedicated mode for account processing makes the grammar more maintainable and clearly separates the contextual rules for account names from the rest of the language.
48-48:✅ Verification successful
Verify account name character restrictions.
The
ACCOUNT_TEXTrule allows alphanumeric characters, underscores, and hyphens. Ensure this matches your intended restrictions for account names and that it's consistent with any validation that happens elsewhere in the codebase.
🏁 Script executed:
#!/bin/bash # Look for existing account name validation logic rg -i "account.*valid" --glob "*.go"Length of output: 125
Action: Confirm Account Name Restrictions Consistency
It appears that the
ACCOUNT_TEXTrule inLexer.g4—which allows[a-zA-Z0-9_-]+—is intended to cover valid account names. The presence of a test case ininternal/interpreter/interpreter_test.go(i.e.TestAccountInvalidString) suggests that account name validation is indeed being verified elsewhere in the codebase.Please ensure that:
- The allowed characters in the lexer rule truly reflect the account naming requirements.
- This validation remains consistent with the logic exercised in the test suite.
If you update the allowed characters or wish to alter these restrictions, remember to update both the lexer and its corresponding tests accordingly.
internal/interpreter/interpreter.go (1)
182-186:❓ Verification inconclusive
Consider updating feature flag checking logic.
Make sure that the
checkFeatureFlagmethod at line 1045 is used consistently to verify this new flag is enabled before using the account interpolation features.
🏁 Script executed:
#!/bin/bash # Check for feature flag usage in the codebase rg "ExperimentalAccountInterpolationFlag" --glob "*.go"Length of output: 521
Action Required: Ensure Consistent Feature Flag Enforcement
I verified that the new
ExperimentalAccountInterpolationFlagis indeed being used to gate the account interpolation feature:
- In internal/interpreter/evaluate_expr.go, the flag is checked via
st.checkFeatureFlag(ExperimentalAccountInterpolationFlag).- The tests in internal/interpreter/interpreter_test.go also reference the flag through calls to
testWithFeatureFlag.Please confirm that the account interpolation logic in internal/interpreter/interpreter.go (around line 1045) is likewise protected using the
checkFeatureFlagmethod so that feature gating is applied consistently across all usage sites.internal/interpreter/interpreter_error.go (2)
206-213: New error type for string casting issues.The
CannotCastToStringerror type is a good addition for handling type conversion issues during account interpolation. It follows the established error pattern and provides a clear error message that includes the problematic value.
206-209:✅ Verification successful
Verify error usage in evaluation code.
This error type should be thrown when interpolating values that cannot be cast to strings. Ensure it's properly used in the expression evaluation code.
🏁 Script executed:
#!/bin/bash # Look for code that uses CannotCastToString rg "CannotCastToString" --glob "*.go"Length of output: 369
Action: Confirm Correct Usage of
CannotCastToStringin Expression EvaluationI've verified that the
CannotCastToStringerror is properly thrown in the expression evaluation code. Specifically:
- In
internal/interpreter/evaluate_expr.go, the error is returned as expected when a value cannot be converted to a string.- Tests in
internal/interpreter/interpreter_test.goconfirm its use and proper behavior.No changes are needed in this area.
internal/analysis/hover.go (1)
153-162: Looks good: handles variable parts in account interpolation.
This allows hover information to be retrieved for variables within an interpolated account literal. The implementation is consistent with existing patterns of returning on the first non-nil hover result.internal/interpreter/evaluate_expr.go (2)
5-5: Import statement is valid.
The addition of"strings"is required for thestrings.Joinusage in building the interpolated account.
178-192: Covers string-cast logic cleanly.
This function properly handles only the supported types. Returning an error for others is logical and aligns with the stated domain constraints.internal/analysis/hover_test.go (1)
499-525: Good test coverage for string interpolation hover.
This test effectively verifies that hover information is retrieved correctly for an interpolated account using$id. The approach follows the established testing pattern.internal/analysis/check_test.go (2)
1794-1808: LGTM: Variable usage within account interpolation properly detectedThe test correctly verifies that variables used inside account interpolation are detected as "used," avoiding unnecessary "unused variable" diagnostics. This is important for the new account interpolation syntax feature.
1811-1811: Document why type checking is deferredThe Skip statement indicates incomplete type checking for account interpolation values. This is important to fix since incorrect types could lead to runtime errors.
Based on the PR objectives, I see this is related to the requirement that interpolated values must be of type account, string, or number. Consider prioritizing the implementation of this type check to ensure type safety for users of the account interpolation feature.
internal/parser/parser_test.go (1)
28-28: LGTM: Added error check for parsingGood addition - explicitly verifying that parsing completes without errors improves test robustness.
internal/parser/antlrParser/Numscript.interp (1)
1-112: File addition looks good; consider adding extra documentation.
This file comprehensively defines token names and grammar rules for Numscript. Including a brief summary of each rule and how it is intended to be used would improve maintainability.internal/parser/antlrParser/numscript_parser.go (1)
3-5450: Skip review of auto-generated code.As indicated by the header comment (“DO NOT EDIT”), this file is auto-generated by ANTLR from the grammar source. Please apply any modifications at the grammar or generation level.
internal/parser/parser.go (11)
8-8: Package import updated correctlyThe import path has been updated to reference the new
antlrParserpackage instead of the previousparserpackage. This change is consistent with the overall refactoring mentioned in the PR summary.
51-51: LGTM: Lexer initialization updatedThe lexer initialization has been properly updated to use the new
antlrParser.NewLexerfunction.
57-61: Parser initialization and usage updated correctlyThe parser initialization has been properly updated to use
antlrParser.NewNumscriptParserand the related error listener management is correctly maintained.
295-296: Well-structured approach to parsing account partsThe approach to separate account literal parts into an array of
AccountNamePartitems is a clean design that supports the interpolation feature while maintaining the structure needed for runtime evaluation.
297-306: Account interpolation parsing looks correctThe implementation correctly handles both text parts and variable parts of account literals:
- Text parts are directly stored as
AccountTextPart- Variable parts are parsed and stored as
VariableobjectsThis approach will allow for proper evaluation of interpolated account names at runtime.
367-414: LGTM: Case type assertions correctly updatedAll case type assertions in the destination parsing switch statement have been correctly updated to use the
antlrParserpackage prefix. This maintains consistency with the package restructuring.
434-445: LGTM: KeptOrDestination type assertions correctly updatedThe case statements for KeptOrDestination types have been properly updated to use the
antlrParserpackage prefix.
452-469: LGTM: Destination allotment parsing correctly updatedThe parseDestinationAllotment function has been properly updated to use the
antlrParserpackage prefix in all type assertions.
513-530: LGTM: Statement parsing correctly updatedThe parseStatement function has been properly updated to use the
antlrParserpackage prefix in all type assertions.
532-553: LGTM: SentValue parsing correctly updatedThe parseSentValue function has been properly updated to use the
antlrParserpackage prefix in all type assertions.
293-311:❓ Verification inconclusive
Verify the new account interpolation implementation
The implementation of account interpolation appears to correctly parse both text parts and variable parts of account names. This supports the new syntax like
@user:$id:pendingas described in the PR objectives. However, there's no validation at parse time to prevent potentially invalid account names.Could you verify that there's runtime validation elsewhere in the codebase that would prevent invalid account names when variables are interpolated? For example, if
$statusevaluates to!<invalid>, the PR objectives mention this would result in an invalid account name.
🏁 Script executed:
#!/bin/bash # Search for validation logic for account names in the codebase rg -A 10 -B 5 "account.*(validate|invalid)" --type go # Additionally, look for any implementations that might handle runtime validation of interpolated account names rg -A 10 "AccountInterpLiteral" --type goLength of output: 5942
Account Interpolation Validation – Further Verification Needed
The current implementation correctly constructs the
AccountInterpLiteralby parsing both text and variable parts. I verified that the codebase performs runtime checks for account interpolation in several places (e.g. ininternal/analysis/check.goandinternal/interpreter/evaluate_expr.go), where variable parts are evaluated (viacheckExpression) and specific cases—such as the special handling of “world” accounts—are diagnosed. However, I did not find an explicit runtime check that validates the final, composite account name when an interpolated variable produces an invalid string (for example, if$statusevaluates to!<invalid>).
Parser Construction (internal/parser/parser.go):
• The parser builds theAccountInterpLiteralwithout performing syntax validation on the constructed account name.Runtime Validation (internal/analysis/check.go & internal/interpreter/evaluate_expr.go):
• These modules inspect parts of the account interpolation and, for variable parts, defer to type checking (checkExpression).
• Specific diagnostics (e.g. for unbounded world accounts) are appended, but there’s no comprehensive check against arbitrary invalid account name contents.Please verify whether the absence of a dedicated validation for the resulting account name—beyond handling specific cases—is intentional under the PR objectives. If not, consider adding further validation to catch cases where a variable’s value leads to an improperly formatted account name.
| l.LiteralNames = staticData.LiteralNames | ||
| l.SymbolicNames = staticData.SymbolicNames | ||
| l.GrammarFileName = "Lexer.g4" | ||
| // TODO: l.EOF = antlr.TokenEOF |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement EOF token assignment.
This TODO indicates that the End-of-File token handling is incomplete. Missing EOF token assignment could potentially lead to issues with parsing input streams that don't end correctly.
- // TODO: l.EOF = antlr.TokenEOF
+ l.EOF = antlr.TokenEOF📝 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.
| // TODO: l.EOF = antlr.TokenEOF | |
| l.EOF = antlr.TokenEOF |
There was a problem hiding this comment.
didn't you read that this was an autogenerated file? you moron
There was a problem hiding this comment.
@ascandone You're absolutely right, and I sincerely apologize for my error. I completely missed the "DO NOT EDIT" comment at the top of the file that clearly indicates this is auto-generated code from Lexer.g4.
Suggesting edits to auto-generated files was completely inappropriate - these changes would be overwritten the next time the parser is regenerated. If this TODO needs to be addressed, it should be done in the grammar source files or in the generation process itself.
Thank you for the correction. I'll be more careful to observe file headers and respect code generation patterns in the future.
ascandone
force-pushed
the
feat/experimental-interpolation-syntax
branch
from
090b32c to
bcef409
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/analysis/check_test.go (1)
1810-1840: 🛠️ Refactor suggestionFix variable reference mismatch in assertion.
The test is currently skipped with a TODO, but there's a variable name mismatch that will cause it to fail once enabled. The test uses
$min the input (line 1817), but the assertion looks for$n(line 1837).- assert.Equal(t, - parser.RangeOfIndexed(input, "$n", 1), - d1.Range, - ) + assert.Equal(t, + parser.RangeOfIndexed(input, "$m", 1), + d1.Range, + )Also, the TODO comment indicates that you're planning to implement a more formal type system to handle interpolated values properly. Consider creating a tracking issue for this work so it's not forgotten.
🧹 Nitpick comments (2)
internal/interpreter/evaluate_expr.go (2)
31-34: Code coverage issue on error handling path.The static analysis indicates that this error handling path isn't covered by tests. Consider adding a test case where the variable evaluation fails.
Would you like me to help create a test case that covers this error path for variable evaluation?
179-192: Implementation of castToString function looks good.This function handles the conversion of various value types to strings, with appropriate error handling for types that cannot be cast. The function correctly supports
AccountAddress,String, andMonetaryIntas specified in the PR objectives, while rejecting unsupported types.There might be additional value types added in the future. Consider documenting this limitation or making the error message more descriptive about which types are supported.
- // No asset nor ratio can be implicitly cast to string - return "", CannotCastToString{Value: v, Range: rng} + // Only AccountAddress, String, and MonetaryInt can be implicitly cast to string + return "", CannotCastToString{Value: v, Range: rng}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/analysis/check.go(3 hunks)internal/analysis/check_test.go(1 hunks)internal/analysis/hover.go(1 hunks)internal/analysis/hover_test.go(1 hunks)internal/interpreter/evaluate_expr.go(3 hunks)internal/interpreter/interpreter.go(1 hunks)internal/interpreter/interpreter_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/interpreter/interpreter.go
- internal/analysis/hover.go
- internal/analysis/check.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
internal/interpreter/evaluate_expr.go
[warning] 29-30: internal/interpreter/evaluate_expr.go#L29-L30
Added lines #L29 - L30 were not covered by tests
🔇 Additional comments (8)
internal/interpreter/evaluate_expr.go (2)
5-5: Added necessary import for strings package.The
stringspackage is required forstrings.Joinfunction used to concatenate account parts with a colon delimiter.
15-40:❓ Verification inconclusive
Implementation of account interpolation looks good.
The implementation correctly handles both text parts and variables in account interpolation, with appropriate error handling and feature flag checking. The approach of building parts and joining them with a colon aligns well with the PR objectives.
However, there's a TODO comment about validating account names that should be addressed. Consider implementing validation to reject account names that might be invalid after interpolation.
🏁 Script executed:
#!/bin/bash # Check if there are any account name validation implementations elsewhere in the codebase rg -A 3 -B 3 "validate.*account.*name" --type go rg -A 3 -B 3 "isValidAccount" --type goLength of output: 90
Account interpolation implementation is solid, but account name validation remains unaddressed.
- The implementation correctly handles both account text parts and variable interpolation with proper error handling and feature flag checks.
- A search for any existing account name validation (using patterns like "validate.*account.*name" or "isValidAccount") returned no results, reinforcing that the TODO comment ("// TODO validate valid names") is still unresolved.
- Please implement robust account name validation to ensure that interpolated names conform to expected standards and reject any invalid patterns.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 29-30: internal/interpreter/evaluate_expr.go#L29-L30
Added lines #L29 - L30 were not covered by testsinternal/analysis/hover_test.go (1)
500-525: Good test for hovering on variables within account interpolation.This test correctly verifies that the hover functionality works with the new account interpolation syntax. It confirms that variables used in account interpolation are properly tracked and can be hovered over in the editor.
The test uses a realistic example that aligns with the PR objectives of supporting interpolation with the format
@user:$idand ensures that the variable resolution works correctly.internal/analysis/check_test.go (1)
1794-1808: Excellent test for ensuring variables in interpolation aren't marked as unused.This test verifies an important aspect of the feature: variables used within account interpolation syntax should not be flagged as unused. The test is well-structured and comprehensive.
internal/interpreter/interpreter_test.go (4)
3839-3867: New account interpolation test is well-implemented.This test properly validates that the interpolation works correctly with various types (number, string, and account). The test verifies that account interpolation can combine multiple variables into a complex account address, which aligns with the PR's stated objectives.
The use of
testWithFeatureFlagwithExperimentalAccountInterpolationFlagensures that this feature is properly gated behind a feature flag, allowing for controlled rollout.
3869-3897: Error handling test for account interpolation is comprehensive.This test properly verifies that the implementation rejects invalid interpolations with monetary values, exactly as described in the PR objectives. The test ensures that the system generates the appropriate error type (
CannotCastToString) with the correct range information, which will provide useful debugging information to developers.The test confirms that the constraint limiting interpolation to only account, string, and number types is enforced at runtime, which is an important requirement from the PR description.
3846-3846: Comprehensive interpolation pattern is well-tested.The test case includes a complex interpolation pattern with multiple variables:
@acc:$id:$status:$acc. This effectively tests nested interpolation and ensures that variables of different types can be combined in a single account identifier.This test provides good coverage for the flexible interpolation syntax described in the PR objectives, demonstrating how variables can be interpolated between colons to form dynamic account names.
3874-3874: Appropriate test for type constraint enforcement.The use of
@acc:$mwith a monetary variable correctly tests that the implementation enforces type constraints. This aligns with the PR objectives which state that interpolated values are limited to specific types (account, string, and number) and that other types will result in runtime errors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/interpreter/evaluate_expr.go (1)
178-191: Enhance the string casting with additional value typesThe
castToStringfunction handles AccountAddress, String, and MonetaryInt types but could be extended to handle more types in the future. The comment correctly notes that assets and ratios aren't implicitly castable to strings.Consider adding support for more value types as the interpolation feature evolves, potentially with explicit conversion methods for assets and ratios:
func castToString(v Value, rng parser.Range) (string, InterpreterError) { switch v := v.(type) { case AccountAddress: return v.String(), nil case String: return v.String(), nil case MonetaryInt: return v.String(), nil + // Future enhancement opportunities: + // case Asset: + // return v.String(), nil // With appropriate formatting + // case Portion: + // return v.String(), nil // With appropriate formatting default: // No asset nor ratio can be implicitly cast to string return "", CannotCastToString{Value: v, Range: rng} } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/interpreter/evaluate_expr.go(3 hunks)internal/interpreter/interpreter.go(2 hunks)internal/interpreter/interpreter_error.go(1 hunks)internal/interpreter/interpreter_test.go(1 hunks)internal/interpreter/value.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/interpreter/interpreter.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
internal/interpreter/evaluate_expr.go
[warning] 29-30: internal/interpreter/evaluate_expr.go#L29-L30
Added lines #L29 - L30 were not covered by tests
internal/interpreter/interpreter_error.go
[warning] 211-212: internal/interpreter/interpreter_error.go#L211-L212
Added lines #L211 - L212 were not covered by tests
[warning] 220-221: internal/interpreter/interpreter_error.go#L220-L221
Added lines #L220 - L221 were not covered by tests
🔇 Additional comments (10)
internal/interpreter/interpreter_error.go (2)
205-214: LGTM: Well-defined error type for string casting failuresThe
CannotCastToStringerror type is well-structured with appropriate fields for range and value, and the error message is clear.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 211-212: internal/interpreter/interpreter_error.go#L211-L212
Added lines #L211 - L212 were not covered by tests
215-222: LGTM: Clear error type for invalid account namesThe
InvalidAccountNameerror type is properly structured with appropriate fields, and the error message clearly indicates when an account name is invalid.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 220-221: internal/interpreter/interpreter_error.go#L220-L221
Added lines #L220 - L221 were not covered by testsinternal/interpreter/evaluate_expr.go (2)
5-5: LGTM: Appropriate import for string manipulationAdding the
stringsimport is necessary for the string joining operation used in account interpolation.
15-40: LGTM: Well-implemented account interpolation logicThe account interpolation implementation correctly handles different part types, checks feature flags, and performs proper error handling for variable evaluation and string casting.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 29-30: internal/interpreter/evaluate_expr.go#L29-L30
Added lines #L29 - L30 were not covered by testsinternal/interpreter/value.go (2)
34-39: LGTM: Robust account address validationThe
NewAccountAddressfunction properly validates account names before creating them, returning appropriate errors for invalid names.
239-246: LGTM: Well-defined account validation patternThe account validation pattern is clear and well-structured, allowing alphanumeric characters, underscores, and hyphens in segments separated by colons.
internal/interpreter/interpreter_test.go (4)
3839-3863: LGTM: Comprehensive test for invalid account namesThis test verifies that invalid account names are properly rejected with the appropriate error message.
3865-3889: LGTM: Test for invalid interpolated account namesThe test correctly verifies that accounts with invalid interpolated values are rejected with appropriate error messages. The feature flag check ensures that the test is skipped when the feature is not enabled.
3891-3919: LGTM: Test for valid account interpolationThis test comprehensively verifies the account interpolation feature with multiple variables of different types, ensuring they are correctly combined into a valid account name.
3921-3949: LGTM: Test for inappropriate value types in account interpolationThis test correctly verifies that monetary values cannot be cast to strings for account interpolation, ensuring type safety.
refactor: inject io.Stdin and io.Stdout as interfaces WIP refactor: rewritten LS and improved testing capabilities chore: added test fix: fixed thread safety remove example code refactor: move bindings in their separate module chore: add documentation refactor: simplify code refactor: renamed struct feat: added request/response structs implementation refactor: removing jsonrpc2 dependency for request/response refactor: removed sourcegraph/jsonrpc2 dependency refactor: change module name refactor: extract testing utility refactor: change func signature chore: added jsonrpc server test test: prevent data races in tests refactor: extracted test utility test: test jsonrpc notifications refactor: changed api fix: fix request/response (un)marshaling refactor: exposed functions and changed sig test: finally testing the ls handlers properly test: added new test fix: fix test by enforcing deterministic ordering of symbols refactor: rename test test: add more hover tests empty commit to trigger codecov test: add textDocument/definition test fix: improve error handling handling server exit fix close channels refactor
Backwards compatibility
The feature is backward compatible: the described syntax was previously invalid, therefore no previous programs with such syntax exist
Specs
That's a minimal, very constrained version of a general string/account interpolation syntax:
this can lead to invalid account names:
@user:$statuscould be evaluated as@user:!<invalid>, which is currently not a valid usernameInterpolated values are implicitly cast to string. This only applies to the following types: account, string, number. Other types will raise a runtime error
Limitations
This interpolation implementation is very constrained: one can only interpolate using the "something:something" naming convention. As a consequence, the following values will not be possible:
the reason of this limitation is that
Technical notes
The huge diff in grammar definition is due to the fact that the tokens were split in a different file in order to use antlr's lexical modes