Feat/version hint #51
Conversation
WalkthroughThis pull request changes how comments and versioning are handled within the application. The lexer now directs line comments to the HIDDEN channel instead of skipping them. The analysis package has been enhanced with version data in check results and new diagnostics for mismatches and experimental features. Additionally, version-checking logic has been introduced along with corresponding tests. Feature flag references have been centralized in a new flags package, and the parser has been updated to capture and store comments. Changes
Sequence Diagram(s)sequenceDiagram
participant Parser
participant TokenStream
participant CommentHandler
Parser->>TokenStream: Retrieve tokens
TokenStream-->>Parser: Returns visible & hidden tokens
Parser->>CommentHandler: Check for Hidden tokens
CommentHandler-->>Parser: Extract Comment (Range, Content)
Parser->>Program: Append Comment to Comments array
sequenceDiagram
participant Expression
participant CheckResult
participant Diagnostic
Expression->>CheckResult: Invoke checkInfixVersion()
CheckResult->>CheckResult: Call requireVersion() with VersionClause
alt Version requirement met
CheckResult-->>Expression: Continue evaluation
else Version mismatch
CheckResult->>Diagnostic: Emit VersionMismatch diagnostic
end
Possibly related PRs
Poem
✨ Finishing Touches
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
internal/parser/parser.go (1)
63-70: Consider handling different comment formats.The implementation correctly processes tokens from the hidden channel to capture comments. However, the code assumes all comments start with exactly two characters (like
//), which might not handle block comments (/* */) correctly.Consider enhancing the comment extraction to handle different comment formats:
- parsed.Comments = append(parsed.Comments, Comment{ - Content: tk.GetText()[2:], - Range: tokenToRange(tk), - }) + text := tk.GetText() + content := "" + if strings.HasPrefix(text, "//") { + content = text[2:] + } else if strings.HasPrefix(text, "/*") && strings.HasSuffix(text, "*/") { + content = text[2 : len(text)-2] + } else { + content = text + } + parsed.Comments = append(parsed.Comments, Comment{ + Content: content, + Range: tokenToRange(tk), + })internal/analysis/diagnostic_kind.go (1)
275-299: Consider revisiting severity for the ExperimentalFeature diagnostic.
Marking the experimental feature usage as an error might be too severe, since the user could interpret this as blocking. A lower severity level (e.g., Warning) might reduce friction for early adopters of experimental features.internal/analysis/version_check.go (2)
9-21: Clarify function name to better indicate the division check requirement.
Currently,checkInfixVersiononly checks whether both sides are numeric. However, it doesn't confirm that the operator is indeed division. Consider making the naming or logic more explicit if the intended scope is only division operators.
39-85: Consider generating all applicable diagnostics in a single pass.
Right now, the loop exits after pushing the first failed clause. If multiple version constraints are applied, you may want to gather each failing clause before returning so that users see a comprehensive view of all mismatches.internal/parser/version.go (3)
55-75: Add validation for non-negative integers in version parsing.The function correctly parses semantic versions but doesn't validate that version components are non-negative, which is standard for semantic versioning.
Consider checking for negative values:
func parseSemanticVersion(src string) (bool, int, int, int) { parts := strings.Split(src, ".") if len(parts) < 3 { return false, 0, 0, 0 } major, err := strconv.Atoi(parts[0]) if err != nil || major < 0 { return false, 0, 0, 0 } minor, err := strconv.Atoi(parts[1]) if err != nil || minor < 0 { return false, 0, 0, 0 } patch, err := strconv.Atoi(parts[2]) if err != nil || patch < 0 { return false, 0, 0, 0 } return true, major, minor, patch }
77-112: Reduce code duplication in string trimming operations.Both
parseVersionandparseFlagfunctions use the same string trimming operations. This pattern could be extracted to a helper function.Consider adding a helper function:
func trimComment(comment string) string { comment = strings.TrimLeft(comment, " ") return strings.TrimRight(comment, " \n") }Then use it in both functions:
func parseVersion(comment string) Version { - comment = strings.TrimLeft(comment, " ") - comment = strings.TrimRight(comment, " \n") + comment = trimComment(comment) // ... } func parseFlag(comment string) string { - comment = strings.TrimLeft(comment, " ") - comment = strings.TrimRight(comment, " \n") + comment = trimComment(comment) // ... }Also applies to: 124-138
140-150: Consider adding error handling for duplicate feature flags.The current implementation silently overrides duplicate feature flags. Consider adding logging or validation to detect this scenario.
func (p Program) GetFlags() map[string]struct{} { flags := make(map[string]struct{}) for _, comment := range p.Comments { flag := parseFlag(comment.Content) if flag != "" { + // Optional: Add logging for duplicate flags + // _, exists := flags[flag] + // if exists { + // log.Printf("Warning: Duplicate feature flag found: %s", flag) + // } flags[flag] = struct{}{} } } return flags }
📜 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 (18)
Lexer.g4(1 hunks)internal/analysis/check.go(5 hunks)internal/analysis/check_test.go(2 hunks)internal/analysis/diagnostic_kind.go(2 hunks)internal/analysis/version_check.go(1 hunks)internal/cmd/run.go(3 hunks)internal/flags/flags.go(1 hunks)internal/interpreter/batch_balances_query.go(1 hunks)internal/interpreter/evaluate_expr.go(2 hunks)internal/interpreter/interpreter.go(5 hunks)internal/interpreter/interpreter_test.go(16 hunks)internal/parser/antlrParser/Lexer.interp(1 hunks)internal/parser/antlrParser/lexer.go(2 hunks)internal/parser/ast.go(1 hunks)internal/parser/parser.go(1 hunks)internal/parser/parser_test.go(1 hunks)internal/parser/version.go(1 hunks)internal/parser/version_test.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (8)
internal/parser/parser.go (3)
internal/parser/ast.go (1) (1)
Comment(317-320)internal/lsp/bindings.go (2) (2)
Comment(5170-5170)Range(3072-3081)internal/parser/range.go (1) (1)
Range(13-16)
internal/parser/ast.go (2)
internal/parser/range.go (1) (1)
Range(13-16)internal/lsp/bindings.go (2) (2)
Range(3072-3081)Comment(5170-5170)
internal/interpreter/evaluate_expr.go (2)
internal/interpreter/interpreter.go (7) (7)
st(263-268)st(270-275)st(277-314)st(316-330)st(332-365)st(367-415)st(975-997)internal/flags/flags.go (1) (1)
ExperimentalAccountInterpolationFlag(8-8)
internal/interpreter/interpreter.go (1)
internal/flags/flags.go (2) (2)
ExperimentalOneofFeatureFlag(7-7)ExperimentalOverdraftFunctionFeatureFlag(6-6)
internal/cmd/run.go (1)
internal/flags/flags.go (3) (3)
ExperimentalOverdraftFunctionFeatureFlag(6-6)ExperimentalOneofFeatureFlag(7-7)ExperimentalAccountInterpolationFlag(8-8)
internal/analysis/diagnostic_kind.go (4)
internal/parser/version.go (1) (1)
Version(9-12)internal/interpreter/interpreter_error.go (17) (17)
e(17-19)e(26-28)e(35-37)e(45-47)e(55-57)e(64-66)e(74-76)e(83-85)e(92-94)e(102-104)e(111-113)e(121-123)e(130-132)e(138-140)e(147-149)e(156-158)ExperimentalFeature(197-200)internal/interpreter/value.go (2) (2)
String(17-17)String(27-27)internal/lsp/bindings.go (1) (1)
String(5310-5310)
internal/analysis/version_check.go (2)
internal/analysis/check.go (1) (1)
CheckResult(94-105)internal/analysis/diagnostic_kind.go (5) (5)
_(17-17)VersionMismatch(276-279)VersionMismatch(285-287)ExperimentalFeature(289-291)ExperimentalFeature(297-299)
internal/interpreter/interpreter_test.go (1)
internal/flags/flags.go (3) (3)
ExperimentalOverdraftFunctionFeatureFlag(6-6)ExperimentalOneofFeatureFlag(7-7)ExperimentalAccountInterpolationFlag(8-8)
🔇 Additional comments (47)
internal/interpreter/batch_balances_query.go (1)
60-60: Clean and simplified syntax!The code now uses a more concise version of
slices.Containswithout explicit type parameters, allowing the Go compiler to infer the types automatically. This improves readability while maintaining the same functionality.internal/flags/flags.go (1)
1-9: Good centralization of feature flags!Creating a dedicated package for feature flags is an excellent architectural decision. This centralization makes feature flag management more maintainable and creates a single source of truth for these flags across the codebase.
The type alias
FeatureFlag = stringprovides good semantic meaning while allowing string constants to be used as flags. The naming of the constants is clear and descriptive.internal/cmd/run.go (5)
11-11: Appropriate import for the new flags package.Adding the import for the flags package is necessary for the refactoring of feature flag references.
125-125: Good refactoring to use centralized feature flags.Using the flag from the dedicated flags package instead of directly from the interpreter package improves code organization.
128-128: Good refactoring to use centralized feature flags.Using the flag from the dedicated flags package instead of directly from the interpreter package improves code organization.
131-131: Good refactoring to use centralized feature flags.Using the flag from the dedicated flags package instead of directly from the interpreter package improves code organization.
212-214: Good refactoring of command flag definitions.Using the flag constants from the dedicated flags package in the command flag definitions ensures consistency between the flag names used in the command-line interface and the internal code.
internal/interpreter/evaluate_expr.go (2)
7-7: Appropriate import for the new flags package.Adding the import for the flags package is necessary for the refactoring of feature flag references.
23-23: Good refactoring to use centralized feature flags.Using the flag from the dedicated flags package instead of directly from the interpreter package improves code organization and maintainability.
internal/interpreter/interpreter.go (5)
10-10: Added new import for feature flags package.The feature flags have been centralized in the
flagspackage, which is a good practice for maintainability and consistency.
476-476: Updated feature flag reference to use the centralized package.The code now references
flags.ExperimentalOneofFeatureFlaginstead of a local constant, maintaining the same functionality while improving code organization.
577-577: Updated feature flag reference to use the centralized package.This instance of checking the "oneof" feature flag has been updated to use the centralized flag definition.
715-715: Updated feature flag reference to use the centralized package.Another reference to the "oneof" feature flag was correctly updated to use the flags package.
909-909: Updated overdraft feature flag reference to use the centralized package.The experimental overdraft function feature flag reference has been updated to use the centralized flags package.
internal/interpreter/interpreter_test.go (4)
8-8: Added import for the flags package.Added import for the centralized feature flags package, which is necessary for the refactored feature flag references throughout the test file.
3422-3422: Updated overdraft feature flag references in tests.Feature flag references in the overdraft function tests have been updated to use the centralized flags package. This ensures consistency with the implementation code.
Also applies to: 3445-3445, 3469-3469, 3504-3504
3662-3662: Updated oneof feature flag references in tests.All references to the experimental "oneof" feature flag have been updated to use the centralized flags package across multiple test cases, maintaining consistency with the implementation code.
Also applies to: 3687-3687, 3710-3710, 3737-3737, 3760-3760, 3785-3785, 3811-3811, 3837-3837
3889-3889: Updated account interpolation feature flag references in tests.References to the experimental account interpolation feature flag have been updated to use the centralized flags package, ensuring consistency with the implementation.
Also applies to: 3919-3919, 3949-3949
internal/parser/antlrParser/lexer.go (2)
151-152: Line comment handling now routes to HIDDEN channelThe changes in the serialized ATN (Abstract Syntax Tree) reflect the corresponding grammar change in Lexer.g4. Instead of skipping line comments entirely, they're now routed to the HIDDEN channel, making them accessible for later processing by the parser.
218-219: Mode transitions in lexer updated for consistencyThese ATN state changes to the lexer's push mode behavior for ACCOUNT_START and COLON tokens align with the modified comment handling approach. The transition data now properly maintains token channel information.
Also applies to: 226-227
internal/parser/antlrParser/Lexer.interp (1)
142-142: ATN serialization updated to match new comment handlingThe changes to the serialized ATN (Abstract Syntax Tree) in this file are compiler-generated reflections of the grammar changes in Lexer.g4, ensuring that line comments are properly routed to the HIDDEN channel.
Lexer.g4 (1)
5-5: Line comments now preserved instead of skippedThis is the core change that enables comment preservation in the lexer. By routing line comments to the HIDDEN channel instead of skipping them, they become accessible to the parser for further processing, likely to support version hints or other metadata specified in comments.
This is a well-established pattern in language parsers when comments need to be preserved for documentation or semantic purposes.
internal/parser/parser_test.go (1)
441-454: New test validating comment handlingThis test ensures that line comments are properly preserved during parsing. The test parses a script with line comments before function calls and verifies that the parser correctly processes it without errors. The snapshot validation confirms that the comment content is properly captured in the AST.
This test appropriately validates the new comment handling behavior introduced by the lexer changes.
internal/parser/ast.go (2)
317-320: Well-structured Comment type with clear purpose.The new
Commentstruct definition is clean and follows the established pattern of embeddingRangefor position tracking.
325-325: Good addition to Program struct.Adding the
Commentsfield to theProgramstruct allows for preserving comments in the AST, which is essential for the new versioning functionality.internal/analysis/check.go (5)
95-95: Good addition of version field to CheckResult.Adding the
versionfield toCheckResultis a clean way to make version information available during analysis.
145-145: Proper initialization of version field.The initialization of the version field in
newCheckResultproperly retrieves the version from the program being analyzed.
356-357: Well-placed version check for division operator.The
checkInfixVersioncall for division operators is appropriately placed to validate version compatibility before processing the operation.
479-480: Version verification for oneof source construct.The
checkOneofVersioncall forSourceOneofensures that this feature is only used in compatible versions.
635-636: Version verification for oneof destination construct.The
checkOneofVersioncall forDestinationOneofensures that this feature is only used in compatible versions.internal/parser/version_test.go (6)
10-16: Good test for machine version parsing.This test correctly verifies that machine version declarations are parsed properly from comments.
18-28: Good test for interpreter version parsing.This test verifies that interpreter versions with semantic versioning (major.minor.patch) are correctly parsed from comments.
30-39: Good edge case test for version parsing.Testing that version information can be extracted even when it's not in the first comment is a good edge case to cover.
41-47: Good test for invalid version handling.This test verifies that invalid version strings are handled gracefully by returning nil.
49-61: Comprehensive test for feature flag parsing.This test effectively verifies that multiple feature flags can be parsed from comments and stored correctly.
63-175: Thorough version comparison tests.The test suite for
GtEqis comprehensive, covering all relevant comparison scenarios including same version, different major/minor/patch versions, and edge cases.internal/analysis/diagnostic_kind.go (1)
8-8: No concerns for the new parser import.
Everything looks standard here, and the reference aligns with the usage of parser types below.internal/analysis/version_check.go (2)
23-30: Good introduction of checkOneofVersion logic.
The method properly callsrequireVersionwith both a version and a feature flag. No issues spotted with logic or structure.
34-37: Struct encapsulation looks clean and extensible.
VersionClauseeffectively binds the version with an optional feature flag. Implementation is straightforward and will be easy to maintain.internal/analysis/check_test.go (6)
7-7: Import usage looks correct.
Theflagsimport is properly leveraged in the new tests.
675-697: TestDoNoCheckVersionWhenNotSpecified is well-rounded.
It confirms that no version is treated as valid, returning zero diagnostics. This ensures backward compatibility for users not specifying versions.
699-728: TestRequireVersionForInfixDiv effectively checks machine vs. interpreter mismatch.
This test adequately proves that a machine version triggers a mismatch when the code requires an interpreter version ≥0.0.15.
730-760: TestRequireVersionForInfixDivWhenVersionLt elegantly covers lower interpreter versions.
Great demonstration of how a version below the threshold rejects the use of division in the expression.
762-784: TestRequireFlagForOneofWhenMissing confirms the need for the experimental flag.
This is an excellent validation that ensures the user is shown the correct diagnostic if the flag is not set.
786-801: TestRequireFlagForOneofWhenGiven illustrates successful usage of the feature flag.
Proper coverage of the enabled scenario with no diagnostics, ensuring the code respects experimental feature toggles.internal/parser/version.go (2)
9-12: Simple and well-defined interface design.The
Versioninterface provides a clean abstraction with a marker method for type identification and a string representation method.
114-122:✅ Verification successful
Verify version handling consistency across the codebase.
The
GetVersionmethod returns the first valid version it finds. Ensure that this aligns with the expected behavior if multiple version tags are present in the code.
🏁 Script executed:
#!/bin/bash # Check if there are any test cases covering multiple version tags # and verify the expected behavior # Look for test cases that might test multiple version scenarios rg -l "TestVersion|test.*version|Version.*test" --type goLength of output: 123
Version handling behavior verified against test cases.
The tests in internal/parser/version_test.go and internal/analysis/check_test.go confirm that when multiple version tags are present, the
GetVersionmethod correctly returns the first valid version it encounters. This aligns with the intended design, and no further changes are required at this time.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #51 +/- ##
==========================================
+ Coverage 62.25% 62.52% +0.26%
==========================================
Files 33 35 +2
Lines 7235 7439 +204
==========================================
+ Hits 4504 4651 +147
- Misses 2505 2553 +48
- Partials 226 235 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ascandone
force-pushed
the
feat/version-hint
branch
from
134c83c to
c601a68
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/parser/version.go (1)
42-53:⚠️ Potential issueFix comparison logic in
GtEqmethod.The current implementation has a logical error in the version comparison. When major versions are equal but minor versions differ (e.g., comparing 1.2.0 with 1.1.0), it should return true if the first version's minor is greater, but the function is missing this check.
Apply this fix:
func (v VersionInterpreter) GtEq(other VersionInterpreter) bool { if v.Major > other.Major { return true } if v.Major < other.Major { return false } + if v.Minor > other.Minor { + return true + } if v.Minor < other.Minor { return false } return v.Patch >= other.Patch }
🧹 Nitpick comments (4)
internal/flags/flags.go (1)
1-11: Created centralized feature flags packageThis new file creates a dedicated package for feature flag definitions, which is an excellent architectural improvement. Centralizing feature flags makes them easier to manage, document, and evolve over time.
The package defines a type alias for feature flags and establishes constants for all experimental features, ensuring consistent naming and string values across the codebase.
Consider adding documentation comments to each feature flag explaining:
- What functionality the flag enables
- When/if it's expected to become a standard feature
- Any risks or limitations associated with enabling it
This would make it easier for developers to understand the purpose and status of each flag.
internal/parser/version.go (3)
55-75: Add tests for edge cases inparseSemanticVersion.This function handles critical version parsing logic but lacks test coverage for failure scenarios like invalid formats or non-numeric values.
Consider adding tests that verify:
- Handling of incomplete version strings (fewer than 3 components)
- Handling of non-numeric version components
- Edge cases like "1.2.3.4" (extra components)
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 58-59: internal/parser/version.go#L58-L59
Added lines #L58 - L59 were not covered by tests
[warning] 62-63: internal/parser/version.go#L62-L63
Added lines #L62 - L63 were not covered by tests
[warning] 67-68: internal/parser/version.go#L67-L68
Added lines #L67 - L68 were not covered by tests
[warning] 72-73: internal/parser/version.go#L72-L73
Added lines #L72 - L73 were not covered by tests
114-122: Consider returning a default version instead of nil.The method
GetVersion()returns nil when no version is found, which could lead to nil pointer dereferences downstream if not handled carefully.Consider returning a default version (like VersionMachine{}) or clearly documenting that nil is a possible return value that must be checked.
77-112: Add test coverage forparseVersionfunction.This function handles version detection from comments but lacks test coverage for several branches.
Add tests covering:
- Comments with different version formats
- Invalid semantic version values
- Comments without the @Version tag
- Edge cases like empty comments or malformed inputs
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 83-84: internal/parser/version.go#L83-L84
Added lines #L83 - L84 were not covered by tests
[warning] 95-96: internal/parser/version.go#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 100-101: internal/parser/version.go#L100-L101
Added lines #L100 - L101 were not covered by tests
📜 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 (18)
Lexer.g4(1 hunks)internal/analysis/check.go(5 hunks)internal/analysis/check_test.go(2 hunks)internal/analysis/diagnostic_kind.go(2 hunks)internal/analysis/version_check.go(1 hunks)internal/cmd/run.go(3 hunks)internal/flags/flags.go(1 hunks)internal/interpreter/evaluate_expr.go(3 hunks)internal/interpreter/interpreter.go(6 hunks)internal/interpreter/interpreter_test.go(20 hunks)internal/parser/antlrParser/Lexer.interp(1 hunks)internal/parser/antlrParser/lexer.go(2 hunks)internal/parser/ast.go(1 hunks)internal/parser/parser.go(1 hunks)internal/parser/parser_test.go(1 hunks)internal/parser/version.go(1 hunks)internal/parser/version_test.go(1 hunks)numscript_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- Lexer.g4
- internal/parser/parser.go
- internal/parser/parser_test.go
- internal/parser/version_test.go
- internal/parser/ast.go
- internal/parser/antlrParser/lexer.go
- internal/interpreter/evaluate_expr.go
- internal/parser/antlrParser/Lexer.interp
🧰 Additional context used
🧬 Code Definitions (7)
internal/interpreter/interpreter.go (1)
internal/flags/flags.go (3) (3)
ExperimentalMidScriptFunctionCall(9-9)ExperimentalOneofFeatureFlag(7-7)ExperimentalOverdraftFunctionFeatureFlag(6-6)
internal/analysis/diagnostic_kind.go (4)
internal/parser/version.go (1) (1)
Version(9-12)internal/interpreter/interpreter_error.go (17) (17)
e(17-19)e(26-28)e(35-37)e(45-47)e(55-57)e(64-66)e(74-76)e(83-85)e(92-94)e(102-104)e(111-113)e(121-123)e(130-132)e(138-140)e(147-149)e(156-158)ExperimentalFeature(197-200)internal/interpreter/value.go (2) (2)
String(17-17)String(27-27)internal/lsp/bindings.go (1) (1)
String(5310-5310)
numscript_test.go (1)
internal/flags/flags.go (2) (2)
ExperimentalOneofFeatureFlag(7-7)ExperimentalMidScriptFunctionCall(9-9)
internal/analysis/version_check.go (2)
internal/analysis/check.go (17) (17)
res(155-181)res(183-209)res(227-285)res(291-295)res(297-305)res(307-321)res(323-326)res(328-394)res(396-409)res(411-420)res(422-429)res(431-538)res(549-586)res(589-621)res(623-682)res(684-691)CheckResult(94-105)internal/analysis/diagnostic_kind.go (5) (5)
_(17-17)VersionMismatch(276-279)VersionMismatch(285-287)ExperimentalFeature(289-291)ExperimentalFeature(297-299)
internal/analysis/check_test.go (3)
internal/analysis/check.go (1) (1)
Diagnostic(88-92)internal/analysis/diagnostic_kind.go (2) (2)
VersionMismatch(276-279)VersionMismatch(285-287)internal/parser/version.go (2) (2)
VersionMachine(14-14)NewVersionInterpreter(28-34)
internal/interpreter/interpreter_test.go (1)
internal/flags/flags.go (4) (4)
ExperimentalOverdraftFunctionFeatureFlag(6-6)ExperimentalOneofFeatureFlag(7-7)ExperimentalAccountInterpolationFlag(8-8)ExperimentalMidScriptFunctionCall(9-9)
internal/cmd/run.go (1)
internal/flags/flags.go (4) (4)
ExperimentalOverdraftFunctionFeatureFlag(6-6)ExperimentalOneofFeatureFlag(7-7)ExperimentalAccountInterpolationFlag(8-8)ExperimentalMidScriptFunctionCall(9-9)
🪛 GitHub Check: codecov/patch
internal/analysis/check.go
[warning] 640-641: internal/analysis/check.go#L640-L641
Added lines #L640 - L641 were not covered by tests
internal/analysis/diagnostic_kind.go
[warning] 281-282: internal/analysis/diagnostic_kind.go#L281-L282
Added lines #L281 - L282 were not covered by tests
[warning] 285-286: internal/analysis/diagnostic_kind.go#L285-L286
Added lines #L285 - L286 were not covered by tests
[warning] 293-294: internal/analysis/diagnostic_kind.go#L293-L294
Added lines #L293 - L294 were not covered by tests
[warning] 297-298: internal/analysis/diagnostic_kind.go#L297-L298
Added lines #L297 - L298 were not covered by tests
internal/analysis/version_check.go
[warning] 51-60: internal/analysis/version_check.go#L51-L60
Added lines #L51 - L60 were not covered by tests
[warning] 75-76: internal/analysis/version_check.go#L75-L76
Added lines #L75 - L76 were not covered by tests
internal/parser/version.go
[warning] 16-16: internal/parser/version.go#L16
Added line #L16 was not covered by tests
[warning] 18-19: internal/parser/version.go#L18-L19
Added lines #L18 - L19 were not covered by tests
[warning] 28-33: internal/parser/version.go#L28-L33
Added lines #L28 - L33 were not covered by tests
[warning] 36-36: internal/parser/version.go#L36
Added line #L36 was not covered by tests
[warning] 38-39: internal/parser/version.go#L38-L39
Added lines #L38 - L39 were not covered by tests
[warning] 58-59: internal/parser/version.go#L58-L59
Added lines #L58 - L59 were not covered by tests
[warning] 62-63: internal/parser/version.go#L62-L63
Added lines #L62 - L63 were not covered by tests
[warning] 67-68: internal/parser/version.go#L67-L68
Added lines #L67 - L68 were not covered by tests
[warning] 72-73: internal/parser/version.go#L72-L73
Added lines #L72 - L73 were not covered by tests
[warning] 83-84: internal/parser/version.go#L83-L84
Added lines #L83 - L84 were not covered by tests
[warning] 95-96: internal/parser/version.go#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 100-101: internal/parser/version.go#L100-L101
Added lines #L100 - L101 were not covered by tests
[warning] 130-131: internal/parser/version.go#L130-L131
Added lines #L130 - L131 were not covered by tests
[warning] 134-135: internal/parser/version.go#L134-L135
Added lines #L134 - L135 were not covered by tests
internal/cmd/run.go
[warning] 126-126: internal/cmd/run.go#L126
Added line #L126 was not covered by tests
[warning] 129-129: internal/cmd/run.go#L129
Added line #L129 was not covered by tests
[warning] 132-132: internal/cmd/run.go#L132
Added line #L132 was not covered by tests
[warning] 135-135: internal/cmd/run.go#L135
Added line #L135 was not covered by tests
[warning] 217-220: internal/cmd/run.go#L217-L220
Added lines #L217 - L220 were not covered by tests
🔇 Additional comments (28)
internal/cmd/run.go (3)
11-11: Import of flags package added for feature flag referencesThe import of the new flags package centralizes feature flag references. This is a good practice for better organization and maintainability of feature flags.
217-220:✅ Verification successful
CLI flag definitions now use the flags package constants
The CLI flag definitions now directly reference the constants from the flags package. This ensures consistency between CLI flags and internal feature flag references.
🏁 Script executed:
#!/bin/bash # Check the CLI flag execution to verify the code changes work as expected # Look for any test files that verify the CLI flag behavior fd ".*test.*\.go" | xargs rg -l "experimental.*flag" # Check if any tests actually validate this code path rg -A 2 -B 2 "RunWithFeatureFlags|cmd\.Flags\(\)" --type goLength of output: 2990
CLI flags now correctly reference the flags package constants
The tests confirm that the CLI flag definitions (including experimental overdraft, oneof, account interpolation, and mid-script function call flags) are properly wired up as intended. No issues were found in the updated flag usage.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 217-220: internal/cmd/run.go#L217-L220
Added lines #L217 - L220 were not covered by tests
126-136:✅ Verification successful
Updated feature flag references to use centralized flags package
Feature flag references have been migrated from the interpreter package to the new flags package, maintaining consistent behavior while improving code organization.
🏁 Script executed:
#!/bin/bash # Verify that all feature flag references in the codebase now use the flags package consistently # Check for any remaining references to interpreter package feature flags rg -l "interpreter\.Experimental.*Flag|interpreter\.Experimental.*Function" --type go # Count the number of references to the flags package to verify adoption rg -c "flags\.Experimental" --type goLength of output: 362
Feature flags now consistently reference the centralized flags package
The updated code in
internal/cmd/run.gocorrectly replaces the interpreter package feature flag references with those from the newflagspackage. Our grep results confirm that no residual references tointerpreter.Experimental…exist, and the use offlags.Experimentalis consistent across the codebase (e.g., ininternal/interpreter/evaluate_expr.go,internal/interpreter/interpreter_test.go, etc.).No further changes are required.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 126-126: internal/cmd/run.go#L126
Added line #L126 was not covered by tests
[warning] 129-129: internal/cmd/run.go#L129
Added line #L129 was not covered by tests
[warning] 132-132: internal/cmd/run.go#L132
Added line #L132 was not covered by tests
[warning] 135-135: internal/cmd/run.go#L135
Added line #L135 was not covered by testsnumscript_test.go (3)
10-10: Import of flags package added for test referencesThe flags package is now imported in the test file, replacing the interpreter package as the source of feature flag definitions.
170-170: Updated feature flag reference in TestGetBalancesOneofThe test now uses the feature flag from the flags package instead of the interpreter package, maintaining the same behavior while aligning with the architectural changes.
446-446: Updated feature flag references for mid-script function callsBoth test cases now use the feature flag from the flags package, ensuring consistency with the new architectural approach.
Also applies to: 497-497
internal/analysis/check.go (5)
95-95: Added version field to CheckResult structA version field has been added to the CheckResult struct to track and validate the version of the program being analyzed. This is essential for providing version-specific diagnostics.
145-145: Initialize the version field in newCheckResultThe version is now properly initialized when creating a new CheckResult by calling program.GetVersion(), ensuring that version information is always available for subsequent checks.
358-359: Added version check for division operatorThe code now validates if the division operator is supported in the current version, helping prevent usage of features that might not be available in earlier versions.
484-485: Added version check for source oneof constructThe code now validates if the oneof construct is supported in the current version for source specifications, preventing usage where it's not supported.
640-641:❓ Verification inconclusive
Added version check for destination oneof construct
The code now validates if the oneof construct is supported in the current version for destination specifications, preventing usage where it's not supported.
🏁 Script executed:
#!/bin/bash # Check test coverage for the destination oneof version check # Look for tests that cover this code path rg -B 5 -A 5 "checkOneofVersion.*destination" --type go rg -B 5 -A 5 "DestinationOneof" "test.*\.go"Length of output: 765
Action Required: Manual Verification of Test Coverage for Oneof Version Check
The oneof version check in
internal/analysis/check.go(lines ~640–641) is confirmed to be present. However, our search did not reveal any tests specifically covering this functionality. Please verify whether tests exist under a different naming convention (e.g.,*_test.go) or ensure that unit tests are added to validate that the oneof construct is supported only in appropriate versions, preventing unintended usage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 640-641: internal/analysis/check.go#L640-L641
Added lines #L640 - L641 were not covered by testsinternal/interpreter/interpreter.go (3)
10-10: Good refactoring to centralize feature flagsAdding the import for the new
flagspackage is a clean approach to centralize feature flag definitions.
142-142: Updated feature flag referenceCorrectly updated to use the centralized flag from the flags package.
502-502: Consistent use of centralized feature flagsAll instances of feature flag references have been consistently updated to use the new flags package.
Also applies to: 603-603, 935-935
internal/analysis/diagnostic_kind.go (2)
276-287:✅ Verification successful
Well-structured version mismatch diagnostic
The
VersionMismatchstruct and its methods are well-designed and follow the same pattern as other diagnostic types in this file. The error message is clear and provides actionable information.The static analysis indicates these methods aren't covered by tests:
🏁 Script executed:
#!/bin/bash # Check if there are any tests that use VersionMismatch grep -r "VersionMismatch" --include="*_test.go" .Length of output: 189
Approval: VersionMismatch Diagnostic Implementation Verified
The
VersionMismatchdiagnostic is well-designed and follows the established pattern. The previously noted potential test coverage gap has been resolved, as evidence ininternal/analysis/check_test.goconfirms that tests indeed referenceVersionMismatch.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 281-282: internal/analysis/diagnostic_kind.go#L281-L282
Added lines #L281 - L282 were not covered by tests
[warning] 285-286: internal/analysis/diagnostic_kind.go#L285-L286
Added lines #L285 - L286 were not covered by tests
289-299:✅ Verification successful
Good implementation of experimental feature diagnostic
The
ExperimentalFeaturediagnostic provides clear guidance to users on how to enable experimental features with a comment. This is a good user experience improvement.The static analysis indicates these methods aren't covered by tests:
🏁 Script executed:
#!/bin/bash # Check if there are any tests that use ExperimentalFeature grep -r "ExperimentalFeature" --include="*_test.go" .Length of output: 330
Approved: Experimental Feature Diagnostic Implementation Meets Test Coverage Expectations
The implementation in
internal/analysis/diagnostic_kind.go(lines 289-299) clearly instructs users on how to enable experimental features through an in-script comment, demonstrating an excellent user experience improvement. The additional test references found in bothinternal/analysis/check_test.goandinternal/interpreter/interpreter_test.goconfirm that theExperimentalFeaturetype—and by extension itsMessageandSeveritymethods—are now covered by tests. No further changes are required.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 293-294: internal/analysis/diagnostic_kind.go#L293-L294
Added lines #L293 - L294 were not covered by tests
[warning] 297-298: internal/analysis/diagnostic_kind.go#L297-L298
Added lines #L297 - L298 were not covered by testsinternal/analysis/version_check.go (4)
8-21: Good implementation of version checking for infix expressionsThe
checkInfixVersionfunction correctly identifies when an infix expression requires a newer version, with a special exemption for number literals.
23-30: Well-structured feature flag check for oneofThe
checkOneofVersionfunction properly combines version checking with feature flag validation, using theVersionClausestruct.
34-37: Good design for version requirementsThe
VersionClausestruct elegantly combines a version requirement with an optional feature flag.
39-86:✅ Verification successful
Comprehensive version checking logic
The
requireVersionmethod handles different version types appropriately and provides clear diagnostics when version requirements aren't met. It also includes feature flag validation.Some code paths seem to be uncovered by tests according to static analysis:
🏁 Script executed:
#!/bin/bash # Check test coverage for the VersionMachine case grep -r "VersionMachine" --include="*_test.go" . | grep -v "NewVersionMachine"Length of output: 258
Verified VersionMachine Test Coverage
The version checking logic handles both machine and interpreter version requirements correctly, with proper feature flag validation and diagnostics. The static analysis concern regarding the VersionMachine branch is resolved—existing tests in both
./internal/analysis/check_test.goand./internal/parser/version_test.goconfirm that this code path is covered.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 51-60: internal/analysis/version_check.go#L51-L60
Added lines #L51 - L60 were not covered by tests
[warning] 75-76: internal/analysis/version_check.go#L75-L76
Added lines #L75 - L76 were not covered by testsinternal/analysis/check_test.go (5)
676-697: Good test for unspecified versionThis test verifies that no diagnostics are generated when no version is specified, which is the correct behavior.
699-728: Comprehensive test for machine versionThis test correctly verifies that using the
machineversion with an infix division operation generates the appropriate version mismatch diagnostic.
730-760: Good test for version comparisonThis test ensures that using a version that's less than the required version generates a version mismatch diagnostic, which is important for backward compatibility.
762-784: Proper test for missing feature flagThis test validates that using the
oneoffeature without the corresponding feature flag generates the appropriate diagnostic.
786-801: Complete test suite with positive caseThis test verifies that the
oneoffeature works correctly when the appropriate feature flag is provided, completing the test coverage for this feature.internal/parser/version.go (1)
28-39:❓ Verification inconclusive
Add tests for
VersionInterpreterconstructor and methods.The
NewVersionInterpreterfunction and associated methods lack test coverage, which is important for ensuring version comparisons work correctly.
🏁 Script executed:
#!/bin/bash # Check if there are any tests for VersionInterpreter implementations rg -A 2 "Test.*VersionInterpreter" --type goLength of output: 46
🏁 Script executed:
#!/bin/bash # Check if there are any test files that reference VersionInterpreter in test files. rg -l 'VersionInterpreter' -g '*_test.go'Length of output: 105
🏁 Script executed:
#!/bin/bash # Check if NewVersionInterpreter or its methods (e.g., String) are referenced/tested in internal/parser/version_test.go rg -n "(NewVersionInterpreter|VersionInterpreter).*\(" internal/parser/version_test.goLength of output: 88
Action: Add Explicit Tests for VersionInterpreter Functionality
The current test coverage for the VersionInterpreter appears incomplete. Although the repository contains two test files referencing "VersionInterpreter" (i.e.,
internal/parser/version_test.goandinternal/analysis/check_test.go), our searches indicate that none of these tests explicitly call or validate the behavior of theNewVersionInterpreterconstructor or its methods (version()andString()). This raises concerns about whether version comparisons and string formatting are adequately verified.
- Files requiring attention:
internal/parser/version.go(constructor and methods implementation)- Test files (especially
internal/parser/version_test.go) should include tests that:
- Confirm that
NewVersionInterpretercorrectly assigns the major, minor, and patch fields.- Verify that the
String()method produces the expected output, e.g.,"interpreter <major>.<minor>.<patch>".- Clarify the role or intended behavior of the
version()method if it should exhibit functional behavior.Please add unit tests to explicitly cover these functionalities.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 28-33: internal/parser/version.go#L28-L33
Added lines #L28 - L33 were not covered by tests
[warning] 36-36: internal/parser/version.go#L36
Added line #L36 was not covered by tests
[warning] 38-39: internal/parser/version.go#L38-L39
Added lines #L38 - L39 were not covered by testsinternal/interpreter/interpreter_test.go (2)
8-8: Well-structured import of the centralized flags package.Good refactoring to import the new
flagspackage which centralizes feature flag constants.
3517-3517: Correctly updated feature flag references.The changes from
machine.ExperimentalOverdraftFunctionFeatureFlagtoflags.ExperimentalOverdraftFunctionFeatureFlaghelp centralize feature flag definitions.
| func (v VersionMachine) version() {} | ||
|
|
||
| func (v VersionMachine) String() string { | ||
| return "machine" | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add unit tests for VersionMachine methods.
The static analysis shows these methods aren't covered by tests, which could lead to undetected regressions in the future.
🏁 Script executed:
#!/bin/bash
# Check if there are any tests for VersionMachine implementations
rg -A 2 "Test.*VersionMachine" --type goLength of output: 42
Please run this script to verify if any tests cover the VersionMachine methods. If the output remains empty, it confirms that these methods currently lack test coverage and unit tests should be added.
🏁 Script executed:
#!/bin/bash
# Check for any test files explicitly mentioning VersionMachine,
# which might indicate that tests for VersionMachine methods exist.
rg -l "VersionMachine" --glob '*_test.go'Length of output: 227
ACTION: Add Unit Tests for VersionMachine Methods
The static analysis indicates that while there are test files referencing VersionMachine (specifically internal/parser/version_test.go and internal/analysis/check_test.go), they do not appear to exercise the actual implementations of VersionMachine.version() and VersionMachine.String(). This omission might lead to undetected regressions if these methods change in the future.
Recommendations:
- Add explicit unit tests targeting:
- The
version()method. - The
String()method.
- The
- Place your tests in an appropriate file (for example, in
internal/parser/version_test.go) to ensure any updates to these methods are properly covered. - Verify that any new test not only compiles but also properly asserts the expected behavior of these methods.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 16-16: internal/parser/version.go#L16
Added line #L16 was not covered by tests
[warning] 18-19: internal/parser/version.go#L18-L19
Added lines #L18 - L19 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/analysis/version_check.go (1)
72-119: Consider adding a return statement after pushing diagnostic for experimental features.The function correctly handles version mismatch cases by returning after pushing diagnostics, but doesn't return after pushing diagnostics for missing feature flags. This creates inconsistent behavior where a program could fail both version checks and feature flag checks, but only see the version error.
if !flagEnabled { res.pushDiagnostic(rng, ExperimentalFeature{ Name: clause.FeatureFlag, }) + return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/analysis/check.go(7 hunks)internal/analysis/check_test.go(2 hunks)internal/analysis/version_check.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/analysis/check.go
🧰 Additional context used
🧬 Code Definitions (1)
internal/analysis/check_test.go (6)
internal/parser/antlrParser/numscript_parser.go (16) (16)
t(375-375)t(782-782)t(844-844)t(962-962)t(1037-1037)t(1095-1095)t(1570-1570)t(1747-1747)t(1902-1902)t(2046-2046)t(2221-2221)t(2378-2378)t(2415-2415)t(2589-2589)t(2782-2782)t(2977-2977)internal/analysis/check.go (1) (1)
Diagnostic(88-92)internal/parser/range.go (2) (2)
Range(13-16)RangeOfIndexed(155-164)internal/analysis/diagnostic_kind.go (4) (4)
VersionMismatch(276-279)VersionMismatch(285-287)ExperimentalFeature(289-291)ExperimentalFeature(297-299)internal/parser/version.go (2) (2)
VersionMachine(14-14)NewVersionInterpreter(28-34)internal/interpreter/interpreter_error.go (1) (1)
ExperimentalFeature(197-200)
🔇 Additional comments (12)
internal/analysis/version_check.go (5)
11-23: Looks good - implementation for version checking on infix operations.The function correctly identifies when a binary infix operation uses non-literal values and enforces version 0.0.15 as a requirement. This ensures backward compatibility while supporting newer syntax.
25-32: Clear implementation of oneof feature version check.This function appropriately requires both version 0.0.15 and the experimental oneof feature flag, ensuring that experimental features are properly gated behind version requirements and feature flags.
34-45: Appropriate validation for overdraft function versioning.The function correctly checks if the function call is specifically for "origin_overdraft" before enforcing version and feature flag requirements. This ensures that only relevant function calls are subject to version checks.
47-63: Good implementation of account interpolation version check.The function efficiently uses
slices.ContainsFuncto detect variable interpolation in account names, and only enforces version requirements when actual interpolation is present. The early return for non-interpolated accounts is a nice optimization.
67-70: Well-structured version requirement clause.The VersionClause struct cleanly encapsulates both version requirements and optional feature flags in a reusable component.
internal/analysis/check_test.go (7)
676-698: Good test for ensuring version checks are skipped when no version is specified.This test verifies the system's default behavior of not enforcing version constraints when no version directive is present in the source code.
700-731: Comprehensive test for version mismatch with machine version.This test properly verifies that using non-literal division with the "machine" version produces the correct version mismatch diagnostic.
733-765: Thorough test for version compatibility checking.This test confirms that the system correctly identifies when a program is using a version (0.0.1) that's too old for a feature requiring version 0.0.15.
767-791: Good test for experimental feature flag requirements.The test correctly verifies that using the "oneof" feature without enabling the corresponding feature flag produces the appropriate diagnostic, even when the version requirement is met.
793-810: Well-structured test for feature flag enablement.This test confirms that the system allows the "oneof" feature when both the version requirement is met and the feature flag is enabled.
812-838: Effective test for account interpolation feature flag requirements.The test properly verifies that using account interpolation with variables requires the experimental feature flag to be enabled.
840-863: Good test coverage for overdraft function versioning.This test ensures that using the overdraft function requires the appropriate feature flag, maintaining consistency with the implementation in version_check.go.
This PR allows the user to specify the required version for the script, so that both in the user CI and the editor, errors will be shown when a feature doesn't satisfy such version
Specs
The version can be specified via a comment like:
This will allow the static analysis to emit errors for features which aren't compatible with that version
If the user is targeting the virtual machine implementation, the annotation is
This applies to feature flags as well, for example:
This will allow the user to specify flags without passing them to the numscript cli- which would lead to a bad UX
To use many flags, comments can be stacked:
From the runtime perspective, those are backwards-compatible comments without any special meaning, and will be ignored when executing the program
Edge cases
Specifying many version has undefined behaviour
The order/position of the comments is irrelevant
Comments which don't follow the correct syntax are ignored