V2/optim lang #2
Conversation
WalkthroughThis pull request refactors the debugger's variable retrieval system from a locator-based architecture to a provider-based pattern, introduces module-scoped variable support with protocol version 4, enables stream-based text writing, and consolidates language definition word-type handling through a unified trie structure. Changes
Sequence DiagramsequenceDiagram
actor Client
participant DebugSession as OscriptDebugSession
participant StateContainer as ThreadStateContainer
participant Provider as IVariablesProvider
participant DebuggerService as IDebuggerService
participant Machine
Client->>DebugSession: Variables(variablesReference)
DebugSession->>StateContainer: GetVariablesProviderById(ref)
StateContainer-->>DebugSession: Provider
alt Provider type
Note over DebugSession: LocalScopeProvider
DebugSession->>Provider: FetchVariables(service)
Provider->>DebuggerService: GetVariables(threadId, frameIndex, [])
DebuggerService->>Machine: GetFrameLocals(frameIndex)
Machine-->>DebuggerService: locals[]
DebuggerService-->>Provider: Variable[]
Provider-->>DebugSession: Variable[]
else Provider type
Note over DebugSession: ModuleScopeProvider
DebugSession->>Provider: FetchVariables(service)
Provider->>DebuggerService: GetModuleVariables(threadId, frameIndex, [])
DebuggerService->>Machine: GetModuleVariables(frameIndex)
Machine-->>DebuggerService: moduleVars[]
DebuggerService-->>Provider: Variable[]
Provider-->>DebugSession: Variable[]
end
opt Child variable requested
DebugSession->>Provider: CreateChildProvider(index)
Provider-->>DebugSession: ChildVariablesProvider
DebugSession->>StateContainer: RegisterVariablesProvider(childProvider)
StateContainer-->>DebugSession: handle
end
DebugSession-->>Client: Variable[] response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/OneScript.Core/Exceptions/RuntimeException.cs (1)
194-206: New stream exception factories look good.The new
ClosedStream()andNonWritableStream()factory methods follow the established pattern with bilingual messages and are appropriately used byTextWriteImpl.Minor: Inconsistent indentation at line 200-201 (appears to have non-standard spacing before the method declaration).
🔎 Suggested formatting fix
} - - public static RuntimeException NonWritableStream() + + public static RuntimeException NonWritableStream()src/VSCode.DebugAdapter/Variables/ChildVariablesProvider.cs (1)
23-33: Consider adding null validation for constructor parameters.The constructor accepts a
fetchFuncdelegate andpatharray without null checks. While callers within this PR appear to always pass valid values, defensive validation would prevent potentialNullReferenceExceptionat the call site.🔎 Proposed defensive checks
public ChildVariablesProvider( int threadId, int frameIndex, int[] path, Func<IDebuggerService, int, int, int[], Variable[]> fetchFunc) { _threadId = threadId; _frameIndex = frameIndex; - _path = path; - _fetchFunc = fetchFunc; + _path = path ?? throw new ArgumentNullException(nameof(path)); + _fetchFunc = fetchFunc ?? throw new ArgumentNullException(nameof(fetchFunc)); }src/VSCode.DebugAdapter/Variables/EvaluatedExpressionProvider.cs (1)
31-37: Dead code: Private constructor appears unused.The private constructor
EvaluatedExpressionProvider(string expression, int threadId, int frameIndex, int[] path)is never called. TheCreateChildProvidermethod returns aChildVariablesProviderinstead of using this constructor.🔎 Proposed removal
- private EvaluatedExpressionProvider(string expression, int threadId, int frameIndex, int[] path) - { - _expression = expression; - _threadId = threadId; - _frameIndex = frameIndex; - _path = path; - } -
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
src/OneScript.Core/Exceptions/RuntimeException.cs(1 hunks)src/OneScript.DebugProtocol/EvaluatedVariableLocator.cs(0 hunks)src/OneScript.DebugProtocol/IDebuggerService.cs(1 hunks)src/OneScript.DebugProtocol/StackFrame.cs(1 hunks)src/OneScript.DebugProtocol/VariableLocator.cs(0 hunks)src/OneScript.DebugServices/DefaultDebugger.cs(1 hunks)src/OneScript.DebugServices/Internal/DebuggerServiceImpl.cs(1 hunks)src/OneScript.Language/LanguageDef.cs(5 hunks)src/OneScript.Language/LexicalAnalysis/SourceCodeIterator.cs(7 hunks)src/OneScript.Language/LexicalAnalysis/Token.cs(2 hunks)src/OneScript.StandardLibrary/Text/TextWriteImpl.cs(4 hunks)src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs(2 hunks)src/ScriptEngine/Machine/MachineInstance.cs(1 hunks)src/ScriptEngine/Machine/MachineStopManager.cs(2 hunks)src/VSCode.DebugAdapter/DebugeeProcess.cs(1 hunks)src/VSCode.DebugAdapter/OneScriptDebuggerClient.cs(1 hunks)src/VSCode.DebugAdapter/OscriptDebugSession.cs(2 hunks)src/VSCode.DebugAdapter/ThreadStateContainer.cs(1 hunks)src/VSCode.DebugAdapter/Transport/ProtocolVersions.cs(2 hunks)src/VSCode.DebugAdapter/Variables/ChildVariablesProvider.cs(1 hunks)src/VSCode.DebugAdapter/Variables/EvaluatedExpressionProvider.cs(1 hunks)src/VSCode.DebugAdapter/Variables/IVariablesProvider.cs(1 hunks)src/VSCode.DebugAdapter/Variables/LocalScopeProvider.cs(1 hunks)src/VSCode.DebugAdapter/Variables/ModuleScopeProvider.cs(1 hunks)tests/text-write.os(2 hunks)
💤 Files with no reviewable changes (2)
- src/OneScript.DebugProtocol/VariableLocator.cs
- src/OneScript.DebugProtocol/EvaluatedVariableLocator.cs
🧰 Additional context used
🧬 Code graph analysis (9)
src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs (1)
src/OneScript.Language/SyntaxAnalysis/BslSyntaxWalker.cs (1)
VisitBlockEnd(133-135)
src/VSCode.DebugAdapter/Variables/IVariablesProvider.cs (5)
src/VSCode.DebugAdapter/Variables/ChildVariablesProvider.cs (1)
IVariablesProvider(40-46)src/VSCode.DebugAdapter/Variables/EvaluatedExpressionProvider.cs (1)
IVariablesProvider(44-54)src/VSCode.DebugAdapter/Variables/LocalScopeProvider.cs (1)
IVariablesProvider(33-40)src/VSCode.DebugAdapter/Variables/ModuleScopeProvider.cs (1)
IVariablesProvider(33-40)src/VSCode.DebugAdapter/ThreadStateContainer.cs (1)
IVariablesProvider(28-28)
src/VSCode.DebugAdapter/Variables/LocalScopeProvider.cs (3)
src/VSCode.DebugAdapter/Variables/ChildVariablesProvider.cs (4)
IVariablesProvider(40-46)Variable(35-38)ChildVariablesProvider(16-47)ChildVariablesProvider(23-33)src/VSCode.DebugAdapter/Variables/IVariablesProvider.cs (1)
IVariablesProvider(26-26)src/OneScript.DebugServices/Internal/DebuggerServiceImpl.cs (6)
Variable(127-140)Variable(142-155)Variable(157-179)Variable(181-196)Variable(236-240)Variable(242-253)
src/VSCode.DebugAdapter/DebugeeProcess.cs (8)
src/OneScript.DebugProtocol/IDebuggerService.cs (4)
Variable(54-54)Variable(63-63)Variable(72-72)Variable(81-81)src/VSCode.DebugAdapter/Variables/ChildVariablesProvider.cs (2)
Variable(35-38)IVariablesProvider(40-46)src/VSCode.DebugAdapter/Variables/EvaluatedExpressionProvider.cs (2)
Variable(39-42)IVariablesProvider(44-54)src/VSCode.DebugAdapter/Variables/LocalScopeProvider.cs (2)
Variable(27-31)IVariablesProvider(33-40)src/VSCode.DebugAdapter/Variables/ModuleScopeProvider.cs (2)
Variable(27-31)IVariablesProvider(33-40)src/VSCode.DebugAdapter/Variables/IVariablesProvider.cs (2)
Variable(21-21)IVariablesProvider(26-26)src/OneScript.DebugServices/DefaultVariableVisualizer.cs (1)
Variable(20-62)src/VSCode.DebugAdapter/ThreadStateContainer.cs (1)
IVariablesProvider(28-28)
src/VSCode.DebugAdapter/Variables/EvaluatedExpressionProvider.cs (4)
src/VSCode.DebugAdapter/Variables/ChildVariablesProvider.cs (4)
IVariablesProvider(40-46)Variable(35-38)ChildVariablesProvider(16-47)ChildVariablesProvider(23-33)src/VSCode.DebugAdapter/Variables/IVariablesProvider.cs (1)
IVariablesProvider(26-26)src/OneScript.DebugProtocol/IDebuggerService.cs (4)
Variable(54-54)Variable(63-63)Variable(72-72)Variable(81-81)src/OneScript.DebugServices/Internal/DebuggerServiceImpl.cs (6)
Variable(127-140)Variable(142-155)Variable(157-179)Variable(181-196)Variable(236-240)Variable(242-253)
src/VSCode.DebugAdapter/OneScriptDebuggerClient.cs (3)
src/OneScript.DebugProtocol/IDebuggerService.cs (4)
Variable(54-54)Variable(63-63)Variable(72-72)Variable(81-81)src/OneScript.DebugServices/Internal/DebuggerServiceImpl.cs (6)
Variable(127-140)Variable(142-155)Variable(157-179)Variable(181-196)Variable(236-240)Variable(242-253)src/VSCode.DebugAdapter/Variables/ModuleScopeProvider.cs (1)
Variable(27-31)
src/OneScript.StandardLibrary/Text/TextWriteImpl.cs (1)
src/OneScript.Core/Exceptions/RuntimeException.cs (16)
RuntimeException(13-209)RuntimeException(15-17)RuntimeException(19-21)RuntimeException(23-26)RuntimeException(30-35)RuntimeException(37-42)RuntimeException(44-49)RuntimeException(51-56)RuntimeException(58-63)RuntimeException(65-70)RuntimeException(72-77)RuntimeException(79-84)RuntimeException(86-91)RuntimeException(93-98)RuntimeException(100-105)RuntimeException(107-112)
src/VSCode.DebugAdapter/ThreadStateContainer.cs (8)
src/VSCode.DebugAdapter/Variables/ChildVariablesProvider.cs (1)
IVariablesProvider(40-46)src/VSCode.DebugAdapter/Variables/EvaluatedExpressionProvider.cs (1)
IVariablesProvider(44-54)src/VSCode.DebugAdapter/Variables/LocalScopeProvider.cs (1)
IVariablesProvider(33-40)src/VSCode.DebugAdapter/Variables/ModuleScopeProvider.cs (1)
IVariablesProvider(33-40)src/VSCode.DebugAdapter/Variables/IVariablesProvider.cs (1)
IVariablesProvider(26-26)src/VSCode.DebugAdapter/OneScriptDebuggerClient.cs (1)
StackFrame(118-122)src/VSCode.DebugAdapter/DebugeeProcess.cs (1)
StackFrame(336-363)src/VSCode.DebugAdapter/Handles.cs (1)
Create(31-36)
src/VSCode.DebugAdapter/OscriptDebugSession.cs (7)
src/VSCode.DebugAdapter/DebugSession.cs (6)
Scope(48-59)Scope(54-58)Variables(491-491)Variable(61-74)Variable(68-73)Evaluate(500-500)src/VSCode.DebugAdapter/Variables/LocalScopeProvider.cs (3)
LocalScopeProvider(16-41)LocalScopeProvider(21-25)Variable(27-31)src/VSCode.DebugAdapter/Variables/ModuleScopeProvider.cs (3)
ModuleScopeProvider(16-41)ModuleScopeProvider(21-25)Variable(27-31)src/VSCode.DebugAdapter/Protocol.cs (2)
Response(54-78)Response(62-66)src/VSCode.DebugAdapter/OneScriptDebuggerClient.cs (4)
Variable(124-134)Variable(136-146)Variable(148-159)Variable(161-171)src/VSCode.DebugAdapter/Variables/EvaluatedExpressionProvider.cs (4)
Variable(39-42)EvaluatedExpressionProvider(16-55)EvaluatedExpressionProvider(23-29)EvaluatedExpressionProvider(31-37)src/VSCode.DebugAdapter/DebugeeProcess.cs (2)
Variable(365-368)Variable(370-380)
🔇 Additional comments (42)
src/OneScript.Language/LexicalAnalysis/Token.cs (1)
46-66: LGTM! Good organization of token categories.The addition of
UnaryPlus,UnaryMinus, andGototokens is well-structured. The placement of unary operators before the "binary begin" marker correctly distinguishes them from binary operators. The inline comments provide useful documentation for maintaining the enum structure.Consider whether future maintainers might benefit from a more formal mechanism (e.g., a separate enum or attributes) to enforce the binary operator grouping, but the current comment-based approach is acceptable for now.
src/OneScript.Language/LexicalAnalysis/SourceCodeIterator.cs (3)
21-21: LGTM! Safe caching of code length.The
_codeLengthfield is properly initialized alongside_codeinInitOnString, and since neither is mutated after initialization, the cache remains consistent. Whilestring.Lengthis already O(1) in .NET, this caching pattern can provide marginal benefits in hot paths and doesn't introduce any risk.Also applies to: 45-48
89-109: LGTM!The boundary checks in
MoveNextcorrectly use the cached_codeLength. The logic for line tracking and symbol iteration remains sound.
111-121: LGTM!The remaining boundary checks in
PeekNext,SkipSpaces, andGetContentSpanare consistently updated to use_codeLength, maintaining the original logic while benefiting from the cached value.Also applies to: 137-166, 201-226
src/OneScript.Language/LanguageDef.cs (6)
39-40: LGTM! Correct operator precedence.Unary operators (
UnaryPlus,UnaryMinus) are assigned priority 7, higher than binary arithmetic operators (5-6), which is the correct precedence behavior (unary operations should bind tighter than binary).
285-306: LGTM! Clean switch-based implementation.The refactored
IsBinaryOperatormethod is more explicit and maintainable than a range-based check. The exclusion ofToken.Notis correct sinceNotis a unary operator, andUnaryPlus/UnaryMinusare appropriately excluded as well.
321-335: LGTM!The switch-based
IsLiteralimplementation clearly enumerates all literal types, improving readability and making it explicit which lexem types are considered literals.
338-359: LGTM! Reasonable nested switch for property name validation.The nested switch handles the dual logic: first checking lexem types that are always valid property names, then checking specific tokens (logical operators) that can also be valid property names. The structure is clear despite the nesting.
419-436: LGTM!The
IsEndOfBlockTokenswitch explicitly lists all end-of-block tokens, which is clearer and more maintainable than alternative approaches.
261-271: Minor style preference: inlineoutdeclaration.The refactored
GetTokenmethod uses the modern C# inline out variable declaration, which is good. The explicitelsebranch is slightly verbose but harmless.src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs (2)
391-392: LGTM: Consistent block-end marking for while loops.Adding
VisitBlockEndbefore the jump back ensures the debugger can step to the loop's end location, matching the pattern already established inVisitForLoopNodeat line 453.
415-417: LGTM: Block-end marking added for foreach loops.This mirrors the pattern applied to while and for loops, ensuring consistent debugger behavior across all loop constructs.
src/OneScript.StandardLibrary/Text/TextWriteImpl.cs (6)
46-49: LGTM: New stream-aware constructor.The constructor properly delegates to
Openwith the correct parameter mapping for stream-based initialization.
74-95: LGTM: Open method dispatch based on argument type.The polymorphic behavior based on
IStreamWrappertype detection is a reasonable pattern. The use ofConvertValueStrictensures proper type validation for the context-dependent parameters.
216-230: Stream validation logic is reasonable.The closed stream detection (
CanWrite: false, CanRead: false) correctly identifies disposed streams. The non-writable check viaIsReadOnlyproperly prevents writes to read-only stream wrappers.
188-214: LGTM: Encoding resolution with appropriate BOM handling.The differentiation between file and stream encoding is well-designed:
- Files default to UTF-8 with BOM, but correctly suppress BOM when appending to avoid mid-file markers
- Streams expose explicit BOM control via the
writeBomparameter
140-149: LGTM: Dispose correctly handles stream wrapper reference.The
_streamWrapperis properly nulled without disposal (sinceleaveOpen: trueis used when creating theStreamWriter), maintaining correct ownership semantics.
151-163: No issues found with the StreamWriter initialization. The bufferSize parameter value of -1 is explicitly allowed by .NET's StreamWriter constructor to indicate the default buffer size, and the current implementation is correct.Likely an incorrect or invalid review comment.
src/OneScript.DebugServices/DefaultDebugger.cs (1)
22-23: Protocol version bump to 4 is correctly synchronized with the adapter.The version increment in
SUPPORTED_FORMAT_VERSIONmatches the correspondingVersion4 = 4constant insrc/VSCode.DebugAdapter/Transport/ProtocolVersions.cs, andLatestKnownVersionhas been updated to referenceVersion4. The synchronization requirement noted in the comment is satisfied.src/OneScript.DebugProtocol/StackFrame.cs (1)
13-29: Breaking change: IVariableLocator interface removed from StackFrame.The removal of
IVariableLocatorand associated members (Variables,Hydrate,CreateChildLocator, etc.) aligns with the PR's shift to a provider-based variable access model. This is a clean simplification of the data transfer object with no remaining dependencies in the codebase.tests/text-write.os (3)
9-35: LGTM! Clear test organization.The test list is well-organized with clear comments separating file tests, stream tests, and combined file/stream tests. The naming convention is consistent across all test procedures.
282-453: Good test coverage for stream operations.The stream tests are well-structured and cover important scenarios including:
- Basic stream writing with various encodings and BOM
- Error handling for closed and read-only streams
- Reopening scenarios between file and stream
Note: The stream position reset (
Поток.Перейти(0, ПозицияВПотоке.Начало)) after writing is correctly applied before reading, which is essential for in-memory streams.
455-532: LGTM! Comprehensive line separator testing.The separator tests validate proper handling of different line endings (ПС, ВК+ПС, ВК) for both files and streams. The helper functions improve test maintainability and provide better error messages through escaped line break visualization.
src/VSCode.DebugAdapter/Transport/ProtocolVersions.cs (2)
44-48: LGTM! Protocol version addition is correct.The Version4 constant is properly added to support module variables. The constant value and XML documentation are consistent with the existing pattern.
58-58: LGTM! Version update is correct.LatestKnownVersion is correctly updated to Version4. This ensures the validation logic in IsValid and Adjust methods will work with the new protocol version.
src/VSCode.DebugAdapter/OneScriptDebuggerClient.cs (1)
136-146: LGTM! Consistent implementation.The GetModuleVariables method follows the same pattern as GetVariables and correctly implements the RPC call to retrieve module-scoped variables.
src/VSCode.DebugAdapter/DebugeeProcess.cs (1)
365-368: LGTM! Improved architecture with provider pattern.The change from FillVariables to FetchVariables with a return value is a positive architectural improvement. The provider pattern is more flexible and functional compared to the previous locator-based approach.
src/OneScript.DebugProtocol/IDebuggerService.cs (1)
55-63: LGTM! Interface addition is clear and consistent.The GetModuleVariables method is properly documented and follows the same parameter pattern as GetVariables, making it intuitive to understand and use.
src/OneScript.DebugServices/Internal/DebuggerServiceImpl.cs (1)
142-155: LGTM! Implementation mirrors GetVariables correctly.The GetModuleVariables implementation follows the same pattern as GetVariables, with the key difference being the use of
machine.GetModuleVariables(frameIndex)to retrieve module-scoped variables instead of frame locals. The path traversal and variable conversion logic is correctly reused.src/ScriptEngine/Machine/MachineInstance.cs (1)
2580-2596: LGTM! Correct implementation for module variable retrieval.The GetModuleVariables method correctly retrieves variables from the frame's ThisScope, which represents the module/class instance scope. The bounds checking and iteration logic are sound. The approach differs appropriately from GetFrameLocals, which returns frame locals directly.
src/VSCode.DebugAdapter/Variables/IVariablesProvider.cs (1)
12-27: LGTM! Clean provider interface design.The IVariablesProvider interface establishes a clean, stateless pattern for variable retrieval:
FetchVariablesretrieves variables on-demand from the runtimeCreateChildProviderenables hierarchical navigation into nested objectsThis design supports lazy loading and avoids caching stale data, which is appropriate for a debugger adapter.
src/VSCode.DebugAdapter/Variables/ChildVariablesProvider.cs (1)
40-46: LGTM!The path extension logic is correct: allocating a new array, copying existing elements, and appending the new index. This immutable approach avoids side effects on the original path array.
src/VSCode.DebugAdapter/Variables/LocalScopeProvider.cs (1)
16-41: LGTM!Clean implementation of
IVariablesProviderfor local scope variables. The use ofArray.Empty<int>()avoids unnecessary allocations, and the lambda correctly delegates toservice.GetVariablesfor child variable retrieval.src/VSCode.DebugAdapter/OscriptDebugSession.cs (3)
375-390: LGTM!The scope creation logic correctly instantiates
LocalScopeProviderand conditionally createsModuleScopeProviderwhen the protocol version supports module variables (>= Version4). The provider registration via_threadState.RegisterVariablesProvideraligns with the new provider-based architecture.
397-424: LGTM!The variables retrieval flow correctly:
- Retrieves the provider by handle
- Returns an appropriate error for invalid references
- Fetches variables through the provider abstraction
- Creates and registers child providers for structured (expandable) variables
The error message change from "No active stackframe" to "Invalid variables reference" is appropriate since this now validates the provider handle rather than a frame.
459-468: LGTM!Correctly initializes
idto 0 (no expandable reference) and creates anEvaluatedExpressionProviderfor structured evaluation results. This integrates properly with the provider-based variable hierarchy.src/VSCode.DebugAdapter/Variables/EvaluatedExpressionProvider.cs (1)
44-54: LGTM!The
CreateChildProvidercorrectly captures_expressionin the lambda closure, ensuring that nested variable lookups maintain the original expression context throughGetEvaluatedVariables. The path extension logic is correct.src/VSCode.DebugAdapter/Variables/ModuleScopeProvider.cs (1)
16-41: LGTM!The implementation correctly mirrors
LocalScopeProviderbut delegates toservice.GetModuleVariablesfor module-scoped variable retrieval. Consistent with the provider pattern used throughout the codebase.src/VSCode.DebugAdapter/ThreadStateContainer.cs (2)
19-19: Well-implemented provider registration and retrieval methods.The updated
Reset()call and new provider methods follow the established patterns in this class:
RegisterVariablesProvidermirrorsRegisterFrame(line 22)GetVariablesProviderByIdmirrorsGetFrameById(line 24)- Null handling for invalid IDs is consistent across both frame and provider retrieval
The implementation is clean and maintains consistency with the existing codebase patterns.
Also applies to: 26-28
14-14: Clean refactoring to provider-based pattern.The field type and name change from
IVariableLocatortoIVariablesProvider(with pluralization of_variablesHandles) aligns well with the architectural shift described in the PR objectives. All call sites across the codebase have been properly updated to use the new provider-based methods, with no remaining references to the old interface or method names.src/ScriptEngine/Machine/MachineStopManager.cs (2)
88-88: The reference equality comparison forExecutionFrameobjects is correct and appropriate.ExecutionFrameis a reference type with no customEqualsoverride, and frame instances are created fresh for each execution context (not pooled or reused). The comparison correctly identifies whether the debugger is at the same frame object, which is the intended behavior for tracking execution state in the debugger.Likely an incorrect or invalid review comment.
61-132: Remove the thread safety concern—this is not applicable to the single-threaded execution model.
ShouldStopAtThisLineis only called fromEmitStopEventIfNecessary()within the main synchronous execution loop ofMachineInstance. EachMachineInstanceis tied to oneBslProcesswith a singleVirtualThreadIdand executes code sequentially through its main command loop. State modifications to_lastStopPoint,_currentState,LastStopReason, andLastStopErrorMessageoccur only within this single execution thread. TheSetDebugModemethod is properly synchronized using[MethodImpl(MethodImplOptions.Synchronized)]to protect initialization from concurrent calls, which is the appropriate design for this architecture.Likely an incorrect or invalid review comment.
| if (_currentState == DebugState.Running) | ||
| { | ||
| LastStopReason = MachineStopReason.Breakpoint; | ||
|
|
||
| // Проверим существование условия остановки | ||
| var condition = Breakpoints.GetCondition(module, currentFrame.LineNumber); | ||
| // Проверим существование условия остановки | ||
| var condition = Breakpoints.GetCondition(module, currentFrame.LineNumber); | ||
|
|
||
| if (!string.IsNullOrEmpty(condition)) | ||
| { | ||
| try | ||
| { | ||
| mustStop = _machine.EvaluateInFrame(condition, currentFrame).AsBoolean(); | ||
| } | ||
| catch (Exception ex) | ||
| if (!string.IsNullOrEmpty(condition)) | ||
| { | ||
| // Остановим и сообщим, что остановка произошла не по условию, а в результате ошибки вычисления | ||
| mustStop = true; | ||
| LastStopReason = MachineStopReason.BreakpointConditionError; | ||
| LastStopErrorMessage = $"Не удалось выполнить условие точки останова: {ex.Message}"; | ||
| try | ||
| { | ||
| mustStop = _machine.EvaluateInFrame(condition, currentFrame).AsBoolean(); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| // Остановим и сообщим, что остановка произошла не по условию, а в результате ошибки вычисления | ||
| mustStop = true; | ||
| LastStopReason = MachineStopReason.BreakpointConditionError; | ||
| LastStopErrorMessage = $"Не удалось выполнить условие точки останова: {ex.Message}"; | ||
| } | ||
| } | ||
| } | ||
| else | ||
| { | ||
| LastStopReason = MachineStopReason.Step; | ||
| } | ||
|
|
||
| _lastStopPoint = new StopPoint() | ||
| { | ||
| frame = currentFrame, | ||
| line = currentFrame.LineNumber | ||
| }; | ||
| _currentState = DebugState.Running; | ||
| } | ||
| else | ||
| LastStopReason = MachineStopReason.Step; | ||
|
|
||
| _currentState = DebugState.Running; | ||
| { | ||
| mustStop = false; | ||
| } |
There was a problem hiding this comment.
Critical: Stop point tracking breaks conditional breakpoints.
The logic updates _lastStopPoint (lines 117-122) even when a breakpoint condition evaluates to false. This creates a critical bug:
- Breakpoint condition evaluates to false →
mustStop = false(line 101) _lastStopPointis still updated with current frame/line (lines 117-122)- Next time this line is hit, the check on line 88 matches →
mustStop = false(line 126) - The condition is never evaluated again—the breakpoint is permanently disabled
Additionally, line 122 resets _currentState to Running even when mustStop is false, which exits stepping mode without actually stopping.
🔎 Proposed fix
Move the _lastStopPoint update and state reset inside a conditional that ensures we're actually stopping:
if (_lastStopPoint.frame != currentFrame || _lastStopPoint.line != currentFrame.LineNumber)
{
if (_currentState == DebugState.Running)
{
LastStopReason = MachineStopReason.Breakpoint;
// Проверим существование условия остановки
var condition = Breakpoints.GetCondition(module, currentFrame.LineNumber);
if (!string.IsNullOrEmpty(condition))
{
try
{
mustStop = _machine.EvaluateInFrame(condition, currentFrame).AsBoolean();
}
catch (Exception ex)
{
// Остановим и сообщим, что остановка произошла не по условию, а в результате ошибки вычисления
mustStop = true;
LastStopReason = MachineStopReason.BreakpointConditionError;
LastStopErrorMessage = $"Не удалось выполнить условие точки останова: {ex.Message}";
}
}
}
else
{
LastStopReason = MachineStopReason.Step;
}
- _lastStopPoint = new StopPoint()
- {
- frame = currentFrame,
- line = currentFrame.LineNumber
- };
- _currentState = DebugState.Running;
+ // Only update last stop point if we're actually stopping
+ if (mustStop)
+ {
+ _lastStopPoint = new StopPoint()
+ {
+ frame = currentFrame,
+ line = currentFrame.LineNumber
+ };
+ _currentState = DebugState.Running;
+ }
}
else
{
mustStop = false;
}📝 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.
| if (_currentState == DebugState.Running) | |
| { | |
| LastStopReason = MachineStopReason.Breakpoint; | |
| // Проверим существование условия остановки | |
| var condition = Breakpoints.GetCondition(module, currentFrame.LineNumber); | |
| // Проверим существование условия остановки | |
| var condition = Breakpoints.GetCondition(module, currentFrame.LineNumber); | |
| if (!string.IsNullOrEmpty(condition)) | |
| { | |
| try | |
| { | |
| mustStop = _machine.EvaluateInFrame(condition, currentFrame).AsBoolean(); | |
| } | |
| catch (Exception ex) | |
| if (!string.IsNullOrEmpty(condition)) | |
| { | |
| // Остановим и сообщим, что остановка произошла не по условию, а в результате ошибки вычисления | |
| mustStop = true; | |
| LastStopReason = MachineStopReason.BreakpointConditionError; | |
| LastStopErrorMessage = $"Не удалось выполнить условие точки останова: {ex.Message}"; | |
| try | |
| { | |
| mustStop = _machine.EvaluateInFrame(condition, currentFrame).AsBoolean(); | |
| } | |
| catch (Exception ex) | |
| { | |
| // Остановим и сообщим, что остановка произошла не по условию, а в результате ошибки вычисления | |
| mustStop = true; | |
| LastStopReason = MachineStopReason.BreakpointConditionError; | |
| LastStopErrorMessage = $"Не удалось выполнить условие точки останова: {ex.Message}"; | |
| } | |
| } | |
| } | |
| else | |
| { | |
| LastStopReason = MachineStopReason.Step; | |
| } | |
| _lastStopPoint = new StopPoint() | |
| { | |
| frame = currentFrame, | |
| line = currentFrame.LineNumber | |
| }; | |
| _currentState = DebugState.Running; | |
| } | |
| else | |
| LastStopReason = MachineStopReason.Step; | |
| _currentState = DebugState.Running; | |
| { | |
| mustStop = false; | |
| } | |
| if (_currentState == DebugState.Running) | |
| { | |
| LastStopReason = MachineStopReason.Breakpoint; | |
| // Проверим существование условия остановки | |
| var condition = Breakpoints.GetCondition(module, currentFrame.LineNumber); | |
| if (!string.IsNullOrEmpty(condition)) | |
| { | |
| try | |
| { | |
| mustStop = _machine.EvaluateInFrame(condition, currentFrame).AsBoolean(); | |
| } | |
| catch (Exception ex) | |
| { | |
| // Остановим и сообщим, что остановка произошла не по условию, а в результате ошибки вычисления | |
| mustStop = true; | |
| LastStopReason = MachineStopReason.BreakpointConditionError; | |
| LastStopErrorMessage = $"Не удалось выполнить условие точки останова: {ex.Message}"; | |
| } | |
| } | |
| } | |
| else | |
| { | |
| LastStopReason = MachineStopReason.Step; | |
| } | |
| // Only update last stop point if we're actually stopping | |
| if (mustStop) | |
| { | |
| _lastStopPoint = new StopPoint() | |
| { | |
| frame = currentFrame, | |
| line = currentFrame.LineNumber | |
| }; | |
| _currentState = DebugState.Running; | |
| } | |
| } | |
| else | |
| { | |
| mustStop = false; | |
| } |
🤖 Prompt for AI Agents
In src/ScriptEngine/Machine/MachineStopManager.cs around lines 90 to 127, the
code updates _lastStopPoint and resets _currentState to DebugState.Running even
when a conditional breakpoint evaluates to false, which prevents the condition
from being re-evaluated and incorrectly exits stepping mode; fix this by only
assigning _lastStopPoint and setting _currentState = DebugState.Running when
mustStop is true (i.e., inside the branch where you actually stop for a
breakpoint or step), leaving them untouched when mustStop is false so
conditional breakpoints and stepping continue to function correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OneScript.Language/ScriptException.cs (1)
104-112: Critical: Null reference exception and state mutation in getter.Three issues with the new clamping logic:
Null reference exception: If
Codeis null (line 101),codeLinewill be null, causingcodeLine.Length(line 105) and the range operators (lines 109, 111) to throwNullReferenceException.State mutation in getter: Mutating
ColumnNumber(line 107) inside theMessagegetter violates best practices:
- Getters should be side-effect free
- Creates race conditions in concurrent scenarios
- Causes unexpected behavior if
Messageis accessed multiple timesIncomplete bounds checking: The check only handles values exceeding the length, but negative
ColumnNumbervalues would still cause exceptions on lines 109-111.🔎 Proposed fix
if (ColumnNumber != ErrorPositionInfo.OUT_OF_TEXT) { - if (ColumnNumber > codeLine.Length) + if (codeLine == null) { - ColumnNumber = codeLine.Length; + sb.AppendLine(); + } + else + { + var clampedColumn = Math.Max(0, Math.Min(ColumnNumber, codeLine.Length)); + sb.Append(codeLine[..clampedColumn]); + sb.Append("<<?>>"); + sb.AppendLine(codeLine[clampedColumn..]); - sb.Append(codeLine[..ColumnNumber]); - sb.Append("<<?>>"); - sb.AppendLine(codeLine[ColumnNumber..]); + } }This fix:
- Handles null codeLine gracefully
- Clamps without mutating the property
- Protects against both negative values and values exceeding length
🧹 Nitpick comments (1)
src/OneScript.Language/LexicalAnalysis/WordLexerState.cs (1)
8-8: Remove unused import.The
using System.Linq.Expressions;directive is not used anywhere in this file.🔎 Proposed fix
-using System.Linq.Expressions; - namespace OneScript.Language.LexicalAnalysis
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/OneScript.Language/IdentifiersTrie.cs(1 hunks)src/OneScript.Language/LanguageDef.cs(9 hunks)src/OneScript.Language/LexicalAnalysis/StringLexerState.cs(1 hunks)src/OneScript.Language/LexicalAnalysis/WordLexerState.cs(1 hunks)src/OneScript.Language/ScriptException.cs(1 hunks)src/ScriptEngine.HostedScript/oscript.cfg(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/OneScript.Language/LexicalAnalysis/StringLexerState.cs
🧰 Additional context used
🧬 Code graph analysis (1)
src/OneScript.Language/LanguageDef.cs (2)
src/OneScript.Language/IdentifiersTrie.cs (3)
IdentifiersTrie(14-203)Add(53-75)Add(176-179)src/OneScript.Language/LexicalAnalysis/WordLexerState.cs (1)
Lexem(14-89)
🔇 Additional comments (3)
src/OneScript.Language/LexicalAnalysis/WordLexerState.cs (1)
14-89: Well-structured refactor with clear classification logic.The rewrite streamlines lexem reading by:
- Capturing location once at the current position
- Using delimiter-based content extraction
- Delegating word-type classification to
LanguageDef.GetWordType- Preserving built-in function lookahead logic (lines 69-76)
The switch-based dispatch is cleaner and more maintainable than the previous approach.
src/OneScript.Language/LanguageDef.cs (1)
27-482: Excellent consolidation of word classification logic.This refactoring successfully consolidates language definition logic:
- Introduces
WordTypeenum for clear categorization (lines 27-35)- Replaces multiple separate tries with a unified
_specwordstrie (line 37)- Refactors multiple methods to use idiomatic switch statements for better performance and readability
- Adds public
GetWordTypeAPI (lines 449-451) for external classification- Updates helper methods to leverage the consolidated
_specwordsstructureThe changes improve maintainability while preserving existing behavior.
src/ScriptEngine.HostedScript/oscript.cfg (1)
4-4: The hardcoded path concern requires context clarificationThe review comment claims the path was "changed from a relative path" to an absolute Windows path. However, git history shows the absolute path
c:\Program Files\OneScript\libhas existed since the first commit of this file. There is no evidence of a prior relative path../libin this repository's history.That said, the underlying concern about hardcoded machine-specific paths in source control remains valid: this path may not match all developer installations or deployment environments. Whether using a relative path, environment variables, or removing environment-specific configuration from source control requires clarification of the file's intended purpose (local development config vs. template/example).
Likely an incorrect or invalid review comment.
| public bool Contains(string str) | ||
| { | ||
| var node = _root.sibl; | ||
| foreach (char ch in str) | ||
| { | ||
| var key = node.Find(ch); | ||
| if (key == null) | ||
| return false; | ||
|
|
||
| node = key.next; | ||
| } | ||
|
|
||
| return node.HasValue; | ||
| } |
There was a problem hiding this comment.
Critical: Contains method has incorrect trie traversal logic.
The Contains method has two significant issues:
-
Wrong starting point: Line 100 starts traversal from
_root.siblinstead of_root. This causes the firstFindcall to search_root.sibl.sibl, effectively skipping the first character level of the trie. Compare withContainsKey(line 79) andGet(line 116) which correctly start from_root. -
Incomplete terminal validation: Line 110 only checks
node.HasValue, butContainsKey(line 90) checks bothnode.next == null && node.HasValue. The missingnode.next == nullcheck meansContainscould return true for intermediate nodes that have values but aren't proper terminal entries.
🔎 Proposed fix
public bool Contains(string str)
{
- var node = _root.sibl;
+ var node = _root;
foreach (char ch in str)
{
var key = node.Find(ch);
if (key == null)
return false;
node = key.next;
}
- return node.HasValue;
+ return node.next == null && node.HasValue;
}🤖 Prompt for AI Agents
In src/OneScript.Language/IdentifiersTrie.cs around lines 98 to 111, the
Contains method starts traversal at _root.sibl (should start at _root) and only
checks node.HasValue when deciding terminal membership; change traversal to
begin from _root and follow each char with node.Find, returning false on nulls,
and at the end require the same terminal condition used in ContainsKey
(node.next == null && node.HasValue) so only proper terminal entries are
considered contained.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/OneScript.Language/LanguageDef.cs (2)
62-81: Consider case consistency for Russian keywords.The logical operators are registered in
_specwordswith all-uppercase Russian text (lines 74-76: "И", "ИЛИ", "НЕ"), but later registered viaAddKeywordwith mixed-case (lines 117-119: "И", "Или", "Не"). While the case-insensitive trie handles both correctly, using consistent casing would improve code clarity.🔎 Proposed fix for consistency
-_specwords.Add("И", WordType.Logical); -_specwords.Add("ИЛИ", WordType.Logical); -_specwords.Add("НЕ", WordType.Logical); +_specwords.Add("И", WordType.Logical); +_specwords.Add("Или", WordType.Logical); +_specwords.Add("Не", WordType.Logical);
467-470: Consider using_specwordsfor consistency.
IsNullStringuses direct string comparison while other similar methods (IsBooleanLiteralString,IsUndefinedString,IsLogicalOperatorString,IsImportDirective) use_specwords. For consistency and maintainability, consider refactoring to:🔎 Proposed refactor
[MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool IsNullString(string value) { - return string.Compare(value, "Null", StringComparison.OrdinalIgnoreCase) == 0; + return _specwords.TryGetValue(value, out var wordType) && wordType == WordType.Null; }This aligns with the pattern used by other helper methods and centralizes keyword management in
_specwords.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/OneScript.Language/LanguageDef.cs(9 hunks)src/OneScript.Language/LexicalAnalysis/WordLexerState.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/OneScript.Language/LexicalAnalysis/WordLexerState.cs (5)
src/OneScript.Language/LexicalAnalysis/CodeRange.cs (2)
CodeRange(12-16)CodeRange(18-21)src/OneScript.Language/SpecialChars.cs (2)
SpecialChars(12-54)IsDelimiter(49-52)src/OneScript.Language/LexicalAnalysis/SourceCodeIterator.cs (2)
GetContents(228-231)SkipSpaces(137-166)src/OneScript.Language/LexicalAnalysis/Lexem.cs (2)
Lexem(21-24)Lexem(26-29)src/OneScript.Language/LanguageDef.cs (5)
LanguageDef(16-483)LanguageDef(39-231)WordType(449-452)Token(271-281)Token(400-410)
src/OneScript.Language/LanguageDef.cs (4)
src/OneScript.Language/IdentifiersTrie.cs (1)
IdentifiersTrie(14-187)src/OneScript.Language/LexicalAnalysis/WordLexerState.cs (1)
Lexem(12-87)src/OneScript.Language/LexicalAnalysis/LabelLexerState.cs (1)
Lexem(27-58)src/OneScript.Language/LexicalAnalysis/FullSourceLexer.cs (1)
Lexem(41-82)
🔇 Additional comments (12)
src/OneScript.Language/LexicalAnalysis/WordLexerState.cs (5)
14-14: LGTM! Correct location capture.The location is correctly captured at the start of the token before content extraction begins.
26-36: LGTM! Correct logical operator handling.Logical operators are properly classified and mapped to
LexemType.Operatorwith the token resolved viaLanguageDef.GetToken().
38-63: LGTM! Correct literal handling.Boolean, Undefined, and Null literals are properly classified with their respective
LexemTypevalues. The absence ofTokenassignment is appropriate for these literal types.
16-23: No changes needed. The code correctly handles content extraction and all edge cases including end-of-text scenarios.
65-84: Iterator state is properly managed—no inconsistency issue.The calling pattern in
FullSourceLexer.NextLexem()always invokesMoveToContent()before eachReadNextLexem()call. SinceMoveToContent()normalizes the iterator position by skipping whitespace and marking the start position, any position changes fromSkipSpaces()inside the built-in function check are automatically reset on the next iteration. The iterator state management is consistent and correct.Likely an incorrect or invalid review comment.
src/OneScript.Language/LanguageDef.cs (7)
27-35: LGTM! Well-designed enum.The
WordTypeenum provides a clear classification system for language keywords. TheNonevalue appropriately serves as a default for unrecognized words.
37-37: LGTM! Good consolidation.The unified
_specwordstrie replaces multiple specialized dictionaries, simplifying the architecture.
271-281: LGTM! Cleaner syntax.The refactor to use inline
outparameter withTryGetValueis more idiomatic and readable.
296-316: LGTM! Complete and clear binary operator check.The switch-based implementation correctly covers all binary operators: arithmetic, comparison, and logical. Unary operators (Not) are appropriately excluded.
331-345: LGTM! Comprehensive literal type checking.The switch-based implementation cleanly covers all literal types including the newly added
UndefinedLiteralandNullLiteral.
429-446: LGTM! Complete block terminator checking.The switch correctly identifies all block-ending tokens, including explicit ends (
EndIf,EndLoop, etc.), branch transitions (Else,ElseIf,Exception), and end-of-file (EndOfText).
348-369: The allowance ofAnd,Or, andNottokens as valid property names (lines 361-363) is intentional language design for 1C:Enterprise compatibility. These are bilingual keywords (Russian: И, Или, Не) that commonly appear as property names in business domain models. Unlike statement-level keywords, logical operators can reasonably be accessed as properties via dot notation (e.g.,object.And). This behavior is correct and intentional.
| using System.Collections.Generic; | ||
| using System.Runtime.CompilerServices; | ||
| using OneScript.Language.LexicalAnalysis; | ||
| using static OneScript.Language.LanguageDef; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unnecessary static using directive.
The static using of LanguageDef within the LanguageDef class itself is unnecessary. Static members are already accessible without qualification within the same class. This circular self-reference is confusing and should be removed.
🔎 Proposed fix
-using static OneScript.Language.LanguageDef;🤖 Prompt for AI Agents
In src/OneScript.Language/LanguageDef.cs around line 12, remove the unnecessary
circular directive "using static OneScript.Language.LanguageDef;": delete that
line so the class no longer statically imports itself, then rebuild to ensure no
other references rely on that using (adjust references to static members if any
fully-qualified or direct access is required).
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OneScript.Language/IdentifiersTrie.cs (1)
173-174:Countproperty always returns 0.The
Countproperty is never updated when items are added or removed, so it will always return 0. This violates theIDictionary<string, T>contract and could cause issues for consumers relying on it (e.g., LINQ's.Count()optimization, serialization, debugging).🔎 Proposed fix
+ private int _count; + - public int Count { get; } + public int Count => _count; public bool IsReadOnly { get; }Then update
Addto increment the count:key.value = val; - key.hasValue = true; + if (!key.hasValue) + { + key.hasValue = true; + _count++; + }
🧹 Nitpick comments (1)
src/OneScript.Language/IdentifiersTrie.cs (1)
33-43: Remove unusedFindmethod from TrieNode class (lines 33-43).The
Findmethod is dead code—it's not called anywhere in the codebase. Both theAddandTryGetValuemethods implement the same sibling-search logic inline rather than delegating toFind. Removing it will reduce maintenance burden.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/OneScript.Language/IdentifiersTrie.cs(4 hunks)src/OneScript.Language/LexicalAnalysis/StringLexerState.cs(1 hunks)src/OneScript.Language/ScriptException.cs(1 hunks)src/ScriptEngine.HostedScript/oscript.cfg(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/OneScript.Language/LexicalAnalysis/StringLexerState.cs
🔇 Additional comments (4)
src/ScriptEngine.HostedScript/oscript.cfg (1)
14-14: Clarify intent of commentedruntime.defaultsetting.The
runtime.default = nativesetting is commented out. If this is meant to be a new feature or default behavior, uncomment it; if it's just a configuration example, add clarifying documentation.src/OneScript.Language/IdentifiersTrie.cs (3)
46-79: LGTM!The
Addmethod correctly traverses the trie structure, creating new nodes as siblings when characters don't exist and properly setting the value andhasValueflag at the terminal node. The use of_rootas a sentinel with actual data starting at_root.siblis consistent throughout.
81-95: LGTM - Clean refactor addressing past concerns.The previous
Containsmethod (which had traversal bugs) has been removed. BothContainsKeyandGetnow correctly delegate toTryGetValue, eliminating code duplication and ensuring consistent behavior across all lookup operations.
106-136: LGTM!The
TryGetValueimplementation correctly traverses the trie starting from_root.sibl(consistent with howAddstores data), uses case-insensitive character matching, and properly validates the terminal node viahasValuebefore returning the value.
| { | ||
| if (ColumnNumber > codeLine.Length) | ||
| { | ||
| ColumnNumber = codeLine.Length; | ||
| } |
There was a problem hiding this comment.
Avoid mutating ColumnNumber inside a property getter.
Line 107 mutates the ColumnNumber property within the Message getter. This creates problematic side effects:
- Non-idempotent: Calling
Messagemultiple times can produce different results if the originalColumnNumberwas out of bounds. - Thread-safety: Concurrent access to
Messagecould cause race conditions. - State inconsistency: The exception's
ColumnNumberchanges when inspecting itsMessage.
Use a local variable instead to clamp the column position for display purposes.
🔎 Proposed fix using a local variable
if (ColumnNumber != ErrorPositionInfo.OUT_OF_TEXT)
{
- if (ColumnNumber > codeLine.Length)
- {
- ColumnNumber = codeLine.Length;
- }
- sb.Append(codeLine[..ColumnNumber]);
+ var displayColumn = Math.Min(ColumnNumber, codeLine.Length);
+ sb.Append(codeLine[..displayColumn]);
sb.Append("<<?>>");
- sb.AppendLine(codeLine[ColumnNumber..]);
+ sb.AppendLine(codeLine[displayColumn..]);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/OneScript.Language/ScriptException.cs around lines 104 to 108, the
Message getter currently mutates the ColumnNumber property when clamping it to
the code line length; change this so you do not modify the property: copy
ColumnNumber to a local variable (e.g. clampedColumn), clamp that local to
codeLine.Length, and use the local for formatting the message instead of
assigning back to ColumnNumber, preserving thread-safety and immutability of the
exception state.
| { | ||
| if (ColumnNumber > codeLine.Length) | ||
| { | ||
| ColumnNumber = codeLine.Length; | ||
| } | ||
| sb.Append(codeLine[..ColumnNumber]); | ||
| sb.Append("<<?>>"); | ||
| sb.AppendLine(codeLine[ColumnNumber..]); |
There was a problem hiding this comment.
Add null safety check for codeLine.
If Code is null, codeLine will be null (line 101), causing a NullReferenceException on line 105 (codeLine.Length) and lines 109-111 (range slicing). Add a null or empty check before accessing codeLine.Length.
🔎 Proposed fix with null safety
if (ColumnNumber != ErrorPositionInfo.OUT_OF_TEXT)
{
+ if (string.IsNullOrEmpty(codeLine))
+ {
+ sb.AppendLine(codeLine);
+ }
+ else
+ {
- if (ColumnNumber > codeLine.Length)
- {
- ColumnNumber = codeLine.Length;
- }
- sb.Append(codeLine[..ColumnNumber]);
+ var displayColumn = Math.Min(ColumnNumber, codeLine.Length);
+ sb.Append(codeLine[..displayColumn]);
- sb.Append("<<?>>");
- sb.AppendLine(codeLine[ColumnNumber..]);
+ sb.Append("<<?>>");
+ sb.AppendLine(codeLine[displayColumn..]);
+ }
}Note: This fix also incorporates the mutation fix from the previous comment.
🤖 Prompt for AI Agents
In src/OneScript.Language/ScriptException.cs around lines 104 to 111, add a
null/empty guard for codeLine before accessing its Length or slicing: if
codeLine is null or empty, set it to string.Empty (or skip insertion) and ensure
ColumnNumber is clamped to the valid range (0..codeLine.Length) to avoid
NullReferenceException and out-of-range slicing; then proceed to append the
pre-slice, marker, and post-slice using the safe codeLine and adjusted
ColumnNumber.
| #Конфигурационный файл OneScript | ||
|
|
||
| # Корневой каталог системных библиотек | ||
| lib.system = c:\Program Files\OneScript\lib |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the oscript.cfg file and examine its current state
find . -name "oscript.cfg" -type f 2>/dev/null | head -20Repository: Mr-Rm/OneScript
Length of output: 102
🏁 Script executed:
# Examine the oscript.cfg file
cat -n ./src/ScriptEngine.HostedScript/oscript.cfgRepository: Mr-Rm/OneScript
Length of output: 564
🏁 Script executed:
# Search for other oscript.cfg files or config-related files
find . -name "*.cfg" -o -name "*config*" | grep -E "(oscript|config)" | head -20Repository: Mr-Rm/OneScript
Length of output: 192
🏁 Script executed:
# Look for code that reads or processes lib.system path
rg "lib\.system|oscript\.cfg" -t cs -t csharp -A 3 -B 1 | head -100Repository: Mr-Rm/OneScript
Length of output: 2872
🏁 Script executed:
# Examine how lib.system is used and whether there's environment variable substitution
rg "SYSTEM_LIBRARY_DIR|lib\.system" -A 5 -B 2 | head -150Repository: Mr-Rm/OneScript
Length of output: 5021
🏁 Script executed:
# Check git history for this specific file
cd src/ScriptEngine.HostedScript && git log --oneline -20 oscript.cfg 2>/dev/null || echo "Git history not available"Repository: Mr-Rm/OneScript
Length of output: 131
🏁 Script executed:
# Look for environment variable handling in config processing
rg "Environment\.|ExpandEnvironment" -t cs -A 3 -B 1 | head -100Repository: Mr-Rm/OneScript
Length of output: 7024
🏁 Script executed:
# Get full git history for the oscript.cfg file with diffs
cd src/ScriptEngine.HostedScript && git log --follow -p oscript.cfg 2>/dev/null | head -200Repository: Mr-Rm/OneScript
Length of output: 897
🏁 Script executed:
# Check if this config file is copied/used in the build or distribution
rg "oscript\.cfg" -t cs -t xml -A 2 -B 2 | grep -v "Binary"Repository: Mr-Rm/OneScript
Length of output: 2264
🏁 Script executed:
# Look at the CfgFileConfigProvider to understand how paths are resolved
cat -n src/ScriptEngine.HostedScript/CfgFileConfigProvider.cs | head -50Repository: Mr-Rm/OneScript
Length of output: 1925
🏁 Script executed:
# Check if this file was renamed or moved from somewhere else
git log --all --full-history -- "*oscript.cfg" 2>/dev/null || echo "No history found"Repository: Mr-Rm/OneScript
Length of output: 6784
🏁 Script executed:
# Check .gitignore to see if oscript.cfg should be ignored
cat .gitignore 2>/dev/null | grep -E "cfg|config|oscript"Repository: Mr-Rm/OneScript
Length of output: 102
🏁 Script executed:
# Check the rest of CfgFileConfigProvider to see path resolution logic
cat -n src/ScriptEngine.HostedScript/CfgFileConfigProvider.csRepository: Mr-Rm/OneScript
Length of output: 3577
🏁 Script executed:
# Look for environment variable override mechanism
rg "OSCRIPT_CONFIG" -t cs -t sh -t cmd -B 2 -A 2Repository: Mr-Rm/OneScript
Length of output: 86
🏁 Script executed:
# Check if this file should be a template or example
head -20 src/ScriptEngine.HostedScript/oscript.cfgRepository: Mr-Rm/OneScript
Length of output: 466
🏁 Script executed:
# Verify the commit message and see what else changed in that commit
git show --stat 294c23976f8a20d00e8a358a08db75a46e1662c2 | head -40Repository: Mr-Rm/OneScript
Length of output: 2356
🏁 Script executed:
# Check if there's any documentation about this config file
find . -name "*.md" -o -name "*.txt" | xargs grep -l "oscript.cfg" 2>/dev/null | head -5Repository: Mr-Rm/OneScript
Length of output: 83
🏁 Script executed:
# Check the developer documentation for oscript.cfg context
grep -A 10 -B 10 "oscript.cfg" ./docs/developer_docs.mdRepository: Mr-Rm/OneScript
Length of output: 1897
🏁 Script executed:
# Check if there are any README files that explain the configuration
find . -name "README*" -exec grep -l "oscript.cfg\|lib.system" {} \;Repository: Mr-Rm/OneScript
Length of output: 94
🏁 Script executed:
# Look for how this file is actually used during build/test
rg "oscript.cfg" --type-list | head -5
rg "FilePath.*=.*oscript" -A 2 -B 2Repository: Mr-Rm/OneScript
Length of output: 448
🏁 Script executed:
# Check VSCode README to see the context for OSCRIPT_CONFIG environment variable
cat ./src/VSCode.DebugAdapter/README.md | grep -A 5 -B 5 "OSCRIPT_CONFIG"Repository: Mr-Rm/OneScript
Length of output: 332
🏁 Script executed:
# Look for where this config file location is referenced in code
rg "src/ScriptEngine.HostedScript" | grep -E "path|location|config"Repository: Mr-Rm/OneScript
Length of output: 41
🏁 Script executed:
# Check Build.csproj to understand how the library path is passed during build
cat Build.csproj | grep -A 5 -B 5 "LibDir\|OSCRIPT_CONFIG"Repository: Mr-Rm/OneScript
Length of output: 3807
Use a relative path or environment variable for lib.system configuration.
This newly added configuration file contains a hardcoded absolute Windows path (c:\Program Files\OneScript\lib) that breaks cross-platform compatibility. The codebase already supports:
- Relative path resolution (relative to the config file directory)
- Environment variable override via
OSCRIPT_CONFIG(used in Build.csproj and test scripts) - Cross-platform builds for Windows, Linux, and macOS
Replace the hardcoded path with a relative path like ../lib, or leave it commented and rely on the OSCRIPT_CONFIG environment variable for configuration—which is already the standard practice in the build system and test infrastructure.
🤖 Prompt for AI Agents
In src/ScriptEngine.HostedScript/oscript.cfg around line 4, the config currently
hardcodes lib.system to an absolute Windows path which breaks cross-platform
builds; replace that value with a relative path (for example ../lib) or
remove/comment the line so the build/test system can use the OSCRIPT_CONFIG
environment variable instead, ensuring the path resolution remains relative to
the config file directory and works on Windows, Linux and macOS.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/OneScript.Language/IdentifiersTrie.cs (3)
46-79: Add method silently overwrites existing keys.The implementation allows overwriting existing entries without throwing, which differs from standard
IDictionary<K,V>.Addbehavior that throwsArgumentExceptionfor duplicate keys. This matches the indexer setter semantics but may surprise callers expecting dictionary-like behavior.If this is intentional (e.g., for upsert semantics), consider documenting it. Otherwise, you may want to check
hasValuebefore assignment:🔎 Optional fix to throw on duplicate key
+ if (key.hasValue) + throw new ArgumentException($"Key '{str}' already exists"); key.value = val; key.hasValue = true;
173-174: Count and IsReadOnly are not properly implemented.
Countalways returns 0 (default int) andIsReadOnlyalways returns false (default bool). This breaks theIDictionary<K,V>contract if callers rely on these properties.Consider either:
- Tracking count during
Addoperations- Throwing
NotSupportedExceptionfor consistency withKeys/Values🔎 Option: Throw NotSupportedException for consistency
-public int Count { get; } -public bool IsReadOnly { get; } +public int Count => throw new NotSupportedException(); +public bool IsReadOnly => throw new NotSupportedException();🔎 Option: Track count properly
+private int _count; + public void Add(string str, T val) { var node = _root; // ... existing logic ... + if (!key.hasValue) + _count++; key.value = val; key.hasValue = true; } -public int Count { get; } +public int Count => _count; public bool IsReadOnly => false;
86-89: Unimplemented IDictionary methods are acceptable if intentional.The
Remove,Clear,GetEnumerator, etc. throwingNotImplementedExceptionis fine if this trie is used only for read-after-build scenarios (like language keyword lookup). Consider adding a brief XML doc comment noting the partial implementation scope.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/OneScript.Language/IdentifiersTrie.cs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/OneScript.Language/IdentifiersTrie.cs (1)
src/OneScript.Core/Commons/IndexedNameValueCollection.cs (3)
Add(24-33)Add(35-44)TryGetValue(81-94)
🔇 Additional comments (3)
src/OneScript.Language/IdentifiersTrie.cs (3)
18-44: LGTM - TrieNode structure is well-designed for case-insensitive trie.The separation of
charL/charUfor case-insensitive matching andsibl/nextfor horizontal/vertical traversal is clear. The default constructor appropriately creates the sentinel root node.
81-95: LGTM - Clean delegation pattern.
ContainsKeyandGetcorrectly delegate toTryGetValue, reducing code duplication and ensuring consistent lookup behavior.
106-136: LGTM - TryGetValue correctly implements case-insensitive lookup.The traversal logic properly:
- Starts from
_root.sibl(where first-level characters are stored)- Uses case-insensitive comparison (
charL/charU)- Distinguishes complete entries from mere prefixes via
hasValueThis addresses the previous review concerns about the now-removed
Containsmethod.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.