Feature/run precompiled #1645
Feature/run precompiled #1645
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces a comprehensive compiled script packaging and execution pipeline. It adds serializable data transfer objects for compiled modules, builders to create bundles and libraries from source code, loaders to deserialize and execute packages, and new CLI commands to compile scripts. It also expands public APIs on several existing classes and promotes interface visibility. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Developer
participant CLI as oscript CLI<br/>BehaviorSelector
participant Compiler as Compiler<br/>Frontend
participant Builder as BundleBuilder
participant Packager as Compiled<br/>ModulePackager
participant MsgPack as MessagePack
participant FileSystem as File System
User->>CLI: -build script.os
CLI->>Builder: new BundleBuilder()
Builder->>Compiler: Compile(entrySource)
Compiler-->>Builder: StackRuntimeModule
Builder->>Builder: CollectLibraryModules()
Builder->>Packager: ConvertToDto(module)
Packager-->>Builder: CompiledPackageDto
Builder->>MsgPack: Serialize(package)
MsgPack-->>Builder: byte[]
Builder->>FileSystem: Write script.osc
FileSystem-->>CLI: ✓
CLI-->>User: Bundle saved to: script.osc
sequenceDiagram
participant User as Runtime
participant BehaviorSelector as BehaviorSelector
participant Loader as BundleLoader
participant MsgPack as MessagePack
participant Deserializer as Compiled<br/>ModulePackager
participant Engine as ScriptingEngine
participant Process as Process
User->>BehaviorSelector: Execute compiled.osc
BehaviorSelector->>Loader: new BundleLoader()
Loader->>MsgPack: Load(stream)
MsgPack-->>Loader: CompiledPackageDto
Loader->>Deserializer: ConvertFromDto(dto)
Deserializer-->>Loader: StackRuntimeModule[]
Loader->>Engine: RegisterModule(symbol, module)
Engine-->>Loader: UserScriptContextInstance
Loader->>Engine: RegisterClass(symbol, module)
Engine-->>Loader: ✓
Loader->>Process: Create & Run(entryModule)
Process-->>User: ✓ / Exit Code
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ScriptEngine/Machine/MachineMethodInfo.cs (1)
23-31: Add defensive null check for thelocalsparameter.The
localsparameter is stored directly without validation. Since this is now a public method, add a defensive null check to prevent potentialNullReferenceExceptiondownstream.🔎 Proposed fix to add null validation
public void SetRuntimeParameters(int entryPoint, string[] locals) { + if (locals == null) + throw new System.ArgumentNullException(nameof(locals)); + _method = new MachineMethod { EntryPoint = entryPoint, LocalVariables = locals, Signature = this.MakeSignature() }; }Alternatively, if
nullis a valid value, normalize it to an empty array:public void SetRuntimeParameters(int entryPoint, string[] locals) { _method = new MachineMethod { EntryPoint = entryPoint, - LocalVariables = locals, + LocalVariables = locals ?? System.Array.Empty<string>(), Signature = this.MakeSignature() }; }Based on learnings, public methods in OneScript should implement defensive checks for parameters that could be null.
🧹 Nitpick comments (13)
src/OneScript.Core/Contexts/Internal/IBuildableMember.cs (1)
11-33: Public interfaces inInternalnamespace – acceptable but worth noting.The visibility promotion is necessary for cross-assembly usage (e.g.,
CompiledModulePackagercasting toIBuildableMember). However, havingpublicinterfaces in a namespace namedInternalcreates a semantic inconsistency that may confuse API consumers.Consider either:
- Keeping as-is with XML doc comments clarifying these are implementation details not meant for general consumption, or
- Moving to a non-
Internalnamespace if these are now part of the stable public API.This is a minor organizational concern and doesn't affect functionality.
src/OneScript.Core/Contexts/BslParameterInfo.cs (1)
93-96: Consider consolidatingByValueandSetByValue.Both methods have identical logic. The internal
ByValue(bool)method at lines 93-96 could delegate to the new publicSetByValue(bool)to reduce duplication:internal void ByValue(bool byVal) { - ExplicitByVal = byVal; + SetByValue(byVal); }Alternatively, if
ByValueis no longer needed internally, it could be removed.Also applies to: 110-113
src/ScriptEngine/Machine/MachineMethodInfo.cs (2)
13-13: Consider adding XML documentation for the new public API.The newly exposed public class, factory method, and
SetRuntimeParametersmethod lack XML documentation comments. Adding documentation would improve maintainability and help consumers understand the intended usage pattern.📝 Example documentation
+/// <summary> +/// Represents runtime method information for compiled scripts. +/// Use <see cref="Create"/> to instantiate and <see cref="SetRuntimeParameters"/> to configure. +/// </summary> public class MachineMethodInfo : BslScriptMethodInfo { private MachineMethod _method; internal MachineMethodInfo() { } + /// <summary> + /// Creates a new instance of <see cref="MachineMethodInfo"/> for builder pattern usage. + /// </summary> + /// <returns>A new <see cref="MachineMethodInfo"/> instance.</returns> public static MachineMethodInfo Create() => new MachineMethodInfo(); + /// <summary> + /// Sets the runtime parameters for the method including entry point and local variables. + /// </summary> + /// <param name="entryPoint">The entry point index for the method.</param> + /// <param name="locals">The array of local variable names.</param> public void SetRuntimeParameters(int entryPoint, string[] locals) {Also applies to: 21-21, 23-31
13-13: Public API expansion is intentional and justified for compiled script deserialization.The
Create()factory and publicSetRuntimeParameters()are actively used inCompiledModulePackagerandStackMachineCodeGeneratorto deserialize and construct runtime method information. The builder pattern is appropriate for this use case.No defensive null checks are needed—
GetVariableNames()returns an array viaToArray()which never produces null, and the DTO path also callsToArray().Consider adding XML documentation for the new public members to clarify the builder pattern API.
src/ScriptEngine/Compiler/Packaged/FieldDto.cs (1)
28-29: InitializeAnnotationsto avoid null reference issues.Other DTOs in this namespace (e.g.,
CompiledModuleDto) initialize theirList<>properties withnew List<...>(). LeavingAnnotationsuninitialized may causeNullReferenceExceptionwhen iterating without null checks.🔎 Proposed fix
[Key(3)] - public List<AnnotationDto> Annotations { get; set; } + public List<AnnotationDto> Annotations { get; set; } = new List<AnnotationDto>();src/ScriptEngine/Compiler/Packaged/AnnotationDto.cs (1)
22-23: InitializeParametersto avoid null reference issues.For consistency with
CompiledModuleDtolist properties and to prevent potentialNullReferenceExceptionduring iteration, initialize the list.🔎 Proposed fix
[Key(1)] - public List<AnnotationParameterDto> Parameters { get; set; } + public List<AnnotationParameterDto> Parameters { get; set; } = new List<AnnotationParameterDto>();src/oscript/BuildLibraryBehavior.cs (1)
38-75: Consider refining exception handling.The generic
catch (Exception e)at Line 71 may obscure unexpected failures (e.g., I/O errors, serialization issues, or runtime bugs). While catchingScriptExceptionseparately is good, the fallback handler loses diagnostic context by only printinge.Message.🔎 Recommendation: Log stack trace for unexpected exceptions
catch (Exception e) { - Output.WriteLine($"Error: {e.Message}"); + Output.WriteLine($"Error: {e.Message}"); + Output.WriteLine(e.ToString()); return 1; }Or consider whether certain exception types (e.g.,
IOException,UnauthorizedAccessException) warrant specific messages.src/oscript/SaveCompiledBehavior.cs (1)
78-87: Exception handling mirrors BuildLibraryBehavior.The pattern of catching
ScriptExceptionseparately and falling back to genericExceptionis consistent withBuildLibraryBehavior.cs. The same recommendation applies: consider logging full exception details for unexpected errors.src/ScriptEngine.HostedScript/FileSystemDependencyResolver.cs (1)
330-330: Consider adding a using statement for LibraryLoader.Line 330 uses the fully qualified name
Compiler.Packaged.LibraryLoader. Sinceusing ScriptEngine.Compiler.Packaged;is already present at line 18, this could be simplified to justLibraryLoader. However, this may be intentional disambiguation if there's anotherLibraryLoadertype in scope (from line 28's private field_defaultLoaderof typeLibraryLoader).Verify whether there's a naming conflict:
#!/bin/bash # Description: Check if there are multiple LibraryLoader types that could conflict rg -n "class LibraryLoader" --type cssrc/ScriptEngine/Compiler/Packaged/LibraryLoader.cs (1)
113-127: Missing null check forprocessparameter.
RegisterModulepassesprocessdirectly toInitializeSDO. If a caller passesnull, this could cause unexpected behavior downstream. Based on learnings, defensive null checks are recommended for public-facing methods in OneScript.🔎 Proposed fix
private UserScriptContextInstance RegisterModule(string symbol, StackRuntimeModule module, IBslProcess process) { if (string.IsNullOrEmpty(symbol)) return null; + + if (process == null) + throw new ArgumentNullException(nameof(process)); var instance = _engine.CreateUninitializedSDO(module);src/ScriptEngine/Compiler/Packaged/BundleBuilder.cs (1)
135-174: Good deduplication logic, but consider edge case with emptysourceLocation.The deduplication check on lines 148-153 handles null correctly, but an empty string would pass the
!string.IsNullOrEmptycheck and be added to_collectedSources, potentially causing issues if multiple modules have empty locations.🔎 Proposed fix
var sourceLocation = stackModule.Source?.Location; // Пропускаем если уже собрали (по пути к исходнику) - if (!string.IsNullOrEmpty(sourceLocation)) + if (!string.IsNullOrWhiteSpace(sourceLocation)) { if (_collectedSources.Contains(sourceLocation)) return; _collectedSources.Add(sourceLocation); }src/ScriptEngine/Compiler/Packaged/LibraryBuilder.cs (1)
150-175: Simplified dependency parsing may miss edge cases.The comment acknowledges this is a simplified implementation. The current approach won't handle:
- Multi-line directives
- Comments before the directive on the same line
- String literals containing
//This is acceptable for an initial implementation, but consider adding a TODO to use the proper lexer/parser in the future.
src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.cs (1)
532-552: Unusedmethodparameter inConvertParameterFromDto.The
methodparameter is declared but never used in the method body. Consider removing it if not needed for future functionality.🔎 Proposed fix
- private BslParameterInfo ConvertParameterFromDto(ParameterDto dto, int position, BslScriptMethodInfo method) + private BslParameterInfo ConvertParameterFromDto(ParameterDto dto, int position)And update the call site on line 517:
- var parameters = dto.Parameters.Select((p, idx) => ConvertParameterFromDto(p, idx, builder)).ToList(); + var parameters = dto.Parameters.Select((p, idx) => ConvertParameterFromDto(p, idx)).ToList();
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
src/OneScript.Core/Contexts/BslParameterInfo.cssrc/OneScript.Core/Contexts/BslScriptFieldInfo.cssrc/OneScript.Core/Contexts/Internal/IBuildableMember.cssrc/ScriptEngine.HostedScript/FileSystemDependencyResolver.cssrc/ScriptEngine/Compiler/Packaged/AnnotationDto.cssrc/ScriptEngine/Compiler/Packaged/BundleBuilder.cssrc/ScriptEngine/Compiler/Packaged/BundleLoader.cssrc/ScriptEngine/Compiler/Packaged/CommandDto.cssrc/ScriptEngine/Compiler/Packaged/CompiledModuleDto.cssrc/ScriptEngine/Compiler/Packaged/CompiledModulePackager.cssrc/ScriptEngine/Compiler/Packaged/CompiledPackageDto.cssrc/ScriptEngine/Compiler/Packaged/ConstantDto.cssrc/ScriptEngine/Compiler/Packaged/FieldDto.cssrc/ScriptEngine/Compiler/Packaged/LibraryBuilder.cssrc/ScriptEngine/Compiler/Packaged/LibraryLoader.cssrc/ScriptEngine/Compiler/Packaged/MethodDto.cssrc/ScriptEngine/Compiler/Packaged/SymbolBindingDto.cssrc/ScriptEngine/Machine/MachineMethodInfo.cssrc/ScriptEngine/ScriptEngine.csprojsrc/oscript/BehaviorSelector.cssrc/oscript/BuildLibraryBehavior.cssrc/oscript/ExecuteCompiledBehavior.cssrc/oscript/SaveCompiledBehavior.cssrc/oscript/ShowUsageBehavior.cs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.cursor/rules/langversion.mdc)
Do not use C# language features that are not available in C# 8 when generating code for projects using .NET 8.0 with LangVersion 8
Files:
src/oscript/ShowUsageBehavior.cssrc/OneScript.Core/Contexts/BslScriptFieldInfo.cssrc/OneScript.Core/Contexts/Internal/IBuildableMember.cssrc/oscript/BuildLibraryBehavior.cssrc/ScriptEngine/Compiler/Packaged/CompiledPackageDto.cssrc/ScriptEngine/Compiler/Packaged/SymbolBindingDto.cssrc/ScriptEngine/Compiler/Packaged/BundleLoader.cssrc/ScriptEngine/Compiler/Packaged/ConstantDto.cssrc/ScriptEngine/Compiler/Packaged/FieldDto.cssrc/ScriptEngine/Compiler/Packaged/LibraryBuilder.cssrc/ScriptEngine/Compiler/Packaged/CompiledModulePackager.cssrc/ScriptEngine/Compiler/Packaged/CommandDto.cssrc/ScriptEngine/Compiler/Packaged/AnnotationDto.cssrc/ScriptEngine.HostedScript/FileSystemDependencyResolver.cssrc/ScriptEngine/Compiler/Packaged/LibraryLoader.cssrc/oscript/SaveCompiledBehavior.cssrc/ScriptEngine/Compiler/Packaged/CompiledModuleDto.cssrc/ScriptEngine/Compiler/Packaged/BundleBuilder.cssrc/ScriptEngine/Compiler/Packaged/MethodDto.cssrc/oscript/ExecuteCompiledBehavior.cssrc/oscript/BehaviorSelector.cssrc/ScriptEngine/Machine/MachineMethodInfo.cssrc/OneScript.Core/Contexts/BslParameterInfo.cs
🧠 Learnings (3)
📚 Learning: 2025-12-18T16:13:05.448Z
Learnt from: Mr-Rm
Repo: EvilBeaver/OneScript PR: 1636
File: src/OneScript.StandardLibrary/StringOperations.cs:155-157
Timestamp: 2025-12-18T16:13:05.448Z
Learning: Guideline: In OneScript, when a method parameter has a concrete type (e.g., string) and is called from scripts with omitted arguments, the runtime passes an empty string "" rather than null. Direct (non-script) calls to the method may still pass null, so implement defensive null checks for these parameters in public methods that can be called from scripts. Treat empty string as a legitimate value for script calls, and explicitly handle null for direct calls (e.g., fail fast, throw ArgumentNullException, or normalize to "" where appropriate). This should apply to all C# methods that may be invoked from OneScript with optional string parameters across the codebase.
Applied to files:
src/oscript/ShowUsageBehavior.cssrc/OneScript.Core/Contexts/BslScriptFieldInfo.cssrc/OneScript.Core/Contexts/Internal/IBuildableMember.cssrc/oscript/BuildLibraryBehavior.cssrc/ScriptEngine/Compiler/Packaged/CompiledPackageDto.cssrc/ScriptEngine/Compiler/Packaged/SymbolBindingDto.cssrc/ScriptEngine/Compiler/Packaged/BundleLoader.cssrc/ScriptEngine/Compiler/Packaged/ConstantDto.cssrc/ScriptEngine/Compiler/Packaged/FieldDto.cssrc/ScriptEngine/Compiler/Packaged/LibraryBuilder.cssrc/ScriptEngine/Compiler/Packaged/CompiledModulePackager.cssrc/ScriptEngine/Compiler/Packaged/CommandDto.cssrc/ScriptEngine/Compiler/Packaged/AnnotationDto.cssrc/ScriptEngine.HostedScript/FileSystemDependencyResolver.cssrc/ScriptEngine/Compiler/Packaged/LibraryLoader.cssrc/oscript/SaveCompiledBehavior.cssrc/ScriptEngine/Compiler/Packaged/CompiledModuleDto.cssrc/ScriptEngine/Compiler/Packaged/BundleBuilder.cssrc/ScriptEngine/Compiler/Packaged/MethodDto.cssrc/oscript/ExecuteCompiledBehavior.cssrc/oscript/BehaviorSelector.cssrc/ScriptEngine/Machine/MachineMethodInfo.cssrc/OneScript.Core/Contexts/BslParameterInfo.cs
📚 Learning: 2025-12-01T15:13:19.064Z
Learnt from: CR
Repo: EvilBeaver/OneScript PR: 0
File: .cursor/rules/runbsltests.mdc:0-0
Timestamp: 2025-12-01T15:13:19.064Z
Learning: Build the oscript console application from sources with latest changes using `dotnet build src/oscript/oscript.csproj` before running BSL tests
Applied to files:
src/oscript/BuildLibraryBehavior.cssrc/oscript/ExecuteCompiledBehavior.cs
📚 Learning: 2024-10-01T08:10:34.187Z
Learnt from: Mr-Rm
Repo: EvilBeaver/OneScript PR: 1456
File: src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs:308-308
Timestamp: 2024-10-01T08:10:34.187Z
Learning: Если метод `ToString()` обеспечивает достаточное представление константы, возможно, хранение поля `presentation` не требуется.
Applied to files:
src/ScriptEngine/Compiler/Packaged/ConstantDto.cs
🧬 Code graph analysis (16)
src/oscript/ShowUsageBehavior.cs (2)
src/oscript/CgiBehavior.cs (1)
Output(179-186)src/oscript/Output.cs (4)
Output(13-73)Output(17-20)WriteLine(48-52)WriteLine(54-57)
src/OneScript.Core/Contexts/BslScriptFieldInfo.cs (2)
src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.cs (1)
BslScriptFieldInfo(479-494)src/OneScript.Core/Contexts/BslFieldBuilder.cs (1)
BslScriptFieldInfo(66-76)
src/OneScript.Core/Contexts/Internal/IBuildableMember.cs (4)
src/OneScript.Core/Contexts/BslParameterInfo.cs (10)
IBuildableMember(58-61)IBuildableMember(63-66)IBuildableMember(68-71)IBuildableMember(73-76)IBuildableMember(78-81)IBuildableMember(83-86)IBuildableMember(88-91)BslParameterInfo(17-124)BslParameterInfo(21-25)BslParameterInfo(27-27)src/OneScript.Core/Contexts/BslScriptFieldInfo.cs (7)
IBuildableMember(56-59)IBuildableMember(61-64)IBuildableMember(66-68)IBuildableMember(70-73)IBuildableMember(75-78)IBuildableMember(80-83)IBuildableMember(85-88)src/OneScript.Core/Contexts/BslScriptMethodInfo.cs (4)
IBuildableMember(38-41)IBuildableMember(43-46)BslParameterInfo(94-94)BslParameterInfo(96-104)src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.cs (1)
BslParameterInfo(532-552)
src/oscript/BuildLibraryBehavior.cs (2)
src/oscript/ConsoleHostBuilder.cs (1)
ConsoleHostBuilder(16-56)src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.cs (2)
Save(44-48)Save(53-57)
src/ScriptEngine/Compiler/Packaged/CompiledPackageDto.cs (2)
src/ScriptEngine/Compiler/Packaged/BundleBuilder.cs (1)
CompiledPackageDto(46-83)src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.cs (1)
CompiledModuleDto(80-134)
src/ScriptEngine/Compiler/Packaged/BundleLoader.cs (1)
src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.cs (5)
CompiledModulePackager(28-567)Dictionary(414-447)StackRuntimeModule(62-66)StackRuntimeModule(71-75)StackRuntimeModule(319-398)
src/ScriptEngine/Compiler/Packaged/ConstantDto.cs (2)
src/ScriptEngine/Compiler/Packaged/CompiledModuleDto.cs (1)
MessagePackObject(16-57)src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.cs (1)
ConstantDto(136-169)
src/ScriptEngine/Compiler/Packaged/CommandDto.cs (1)
src/ScriptEngine/Compiler/Packaged/CompiledModuleDto.cs (1)
MessagePackObject(16-57)
src/ScriptEngine.HostedScript/FileSystemDependencyResolver.cs (2)
src/OneScript.Core/Compilation/PackageInfo.cs (2)
PackageInfo(13-30)PackageInfo(15-19)src/OneScript.Language/Localization/BilingualString.cs (4)
BilingualString(13-76)BilingualString(17-26)BilingualString(38-42)BilingualString(44-48)
src/ScriptEngine/Compiler/Packaged/LibraryLoader.cs (1)
src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.cs (1)
CompiledModulePackager(28-567)
src/oscript/SaveCompiledBehavior.cs (6)
src/oscript/ExecuteCompiledBehavior.cs (1)
Execute(34-89)src/oscript/ConsoleHostBuilder.cs (1)
ConsoleHostBuilder(16-56)src/ScriptEngine.HostedScript/HostedScriptEngine.cs (1)
SetGlobalEnvironment(135-140)src/oscript/DoNothingHost.cs (1)
DoNothingHost(14-37)src/ScriptEngine/Compiler/Packaged/BundleBuilder.cs (4)
BundleBuilder(26-198)BundleBuilder(34-41)Save(88-91)Save(96-99)src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.cs (2)
Save(44-48)Save(53-57)
src/ScriptEngine/Compiler/Packaged/CompiledModuleDto.cs (6)
src/ScriptEngine/Compiler/Packaged/SymbolBindingDto.cs (1)
MessagePackObject(16-50)src/ScriptEngine/Compiler/Packaged/AnnotationDto.cs (2)
MessagePackObject(16-24)MessagePackObject(29-37)src/ScriptEngine/Compiler/Packaged/CommandDto.cs (1)
MessagePackObject(15-23)src/ScriptEngine/Compiler/Packaged/ConstantDto.cs (1)
MessagePackObject(15-32)src/ScriptEngine/Compiler/Packaged/FieldDto.cs (1)
MessagePackObject(16-30)src/ScriptEngine/Compiler/Packaged/MethodDto.cs (2)
MessagePackObject(16-51)MessagePackObject(56-73)
src/ScriptEngine/Compiler/Packaged/MethodDto.cs (3)
src/ScriptEngine/Compiler/Packaged/AnnotationDto.cs (2)
MessagePackObject(16-24)MessagePackObject(29-37)src/ScriptEngine/Compiler/Packaged/CompiledModuleDto.cs (1)
MessagePackObject(16-57)src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.cs (4)
MethodDto(171-206)ParameterDto(208-224)AnnotationDto(226-240)AnnotationDto(242-256)
src/oscript/BehaviorSelector.cs (3)
src/oscript/CmdLineHelper.cs (1)
Tail(39-42)src/oscript/SaveCompiledBehavior.cs (2)
SaveCompiledBehavior(20-109)SaveCompiledBehavior(25-29)src/oscript/BuildLibraryBehavior.cs (2)
BuildLibraryBehavior(19-100)BuildLibraryBehavior(24-28)
src/ScriptEngine/Machine/MachineMethodInfo.cs (1)
src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.cs (1)
BslScriptMethodInfo(496-530)
src/OneScript.Core/Contexts/BslParameterInfo.cs (2)
src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.cs (1)
BslParameterInfo(532-552)src/OneScript.Core/Contexts/BslParameterBuilder.cs (1)
BslParameterInfo(65-68)
🔇 Additional comments (38)
src/OneScript.Core/Contexts/BslScriptFieldInfo.cs (1)
34-35: LGTM – Factory method aligns with the builder pattern.The factory method correctly delegates to the internal constructor and supports the established pattern where callers can either provide the name directly or set it later via
IBuildableMember.SetName(). The usage inCompiledModulePackager.ConvertFieldFromDtoconfirms this design.src/OneScript.Core/Contexts/BslParameterInfo.cs (2)
26-27: LGTM – Factory method provides consistent API entry point.The parameterless factory mirrors the pattern established for
BslScriptFieldInfo.Create()and enables external assembly usage while keeping the constructor internal.
104-118: New public setters look correct.The setters follow the established patterns:
SetDefaultValueIndexcorrectly mirrorsSetDefaultValuebut uses an index for deferred resolutionSetPositiondirectly setsPositionImpl- All are used appropriately by
CompiledModulePackager.ConvertParameterFromDtosrc/ScriptEngine/ScriptEngine.csproj (1)
49-51: MessagePack 2.5.187 is a valid and appropriately patched version.Version 2.5.187 is confirmed as a released and patched version (Oct 17, 2025) that addresses CVE-2024-48924 (hash collision DoS vulnerability in MessagePack for C#). This is the version recommended by the project maintainers. Ensure that serialization/deserialization operations follow MessagePack security best practices when handling data sources, particularly by configuring appropriate
MessagePackSerializerOptionsif processing untrusted input.src/ScriptEngine/Compiler/Packaged/ConstantDto.cs (1)
15-42: Well-structured DTO for constant serialization.The discriminated union pattern with
ConstantTypeand nullable value properties is appropriate for MessagePack serialization. Key indices are sequential, and the design aligns with the usage inCompiledModulePackager.ConvertConstant.src/oscript/ShowUsageBehavior.cs (2)
27-30: LGTM: Help text for new build commands.The usage instructions for
-buildand-buildlibare clear and follow the existing formatting conventions.
52-56: LGTM: Documentation for compiled module execution.The new section clearly explains how to run pre-compiled
.oscfiles directly.src/ScriptEngine/Compiler/Packaged/CommandDto.cs (1)
15-23: LGTM: Simple and correct bytecode command DTO.The structure with
CodeandArgumentinteger properties is appropriate for bytecode serialization and integrates correctly withCompiledModuleDto.Code.src/oscript/BehaviorSelector.cs (3)
16-16: LGTM: Constant for compiled extension.Using a constant improves maintainability and consistency.
29-36: LGTM: Auto-detection of compiled script files.The case-insensitive extension check with
StringComparison.OrdinalIgnoreCasecorrectly routes.oscfiles toExecuteCompiledBehaviorwhile preserving existing behavior for source scripts.
53-54: LGTM: New build command mappings.The
-buildand-buildlibcommand registrations follow the existing pattern and correctly delegate toSaveCompiledBehavior.CreateandBuildLibraryBehavior.Create.src/ScriptEngine/Compiler/Packaged/AnnotationDto.cs (1)
29-37: LGTM: AnnotationParameterDto structure.Simple and correct DTO for annotation parameter serialization.
src/ScriptEngine/Compiler/Packaged/SymbolBindingDto.cs (1)
16-50: Well-documented DTO with clear field semantics.The XML documentation clearly explains the purpose of each field. The structure appropriately captures symbol binding information for both static and scoped references.
Verify that
ScopeBindingKindenum serializes correctly with MessagePack. Typically enums serialize as their underlying integer type, but confirm this works as expected in the serialization/deserialization round-trip tests.src/oscript/BuildLibraryBehavior.cs (2)
32-36: LGTM: Directory validation is appropriate.The check ensures the library directory exists before attempting compilation, with a clear error message and appropriate exit code.
87-88: LGTM: Path normalization prevents issues.Trimming trailing separators ensures consistent path handling and avoids potential issues with path concatenation.
src/oscript/SaveCompiledBehavior.cs (2)
33-37: LGTM: File validation with clear error messaging.The existence check prevents proceeding with invalid input and provides a descriptive error message.
64-74: The module count calculation is safe and requires no changes.The assumption that the entry point is always included in
package.Scripts.Countis correct.BundleBuilder.Build()unconditionally adds the entry module at line 80 ofsrc/ScriptEngine/Compiler/Packaged/BundleBuilder.cs, ensuring that any successfully returned package contains at least one script. Therefore,moduleCountwill always be ≥ 1, andlibModules = moduleCount - 1will never be negative.src/ScriptEngine.HostedScript/FileSystemDependencyResolver.cs (3)
25-25: LGTM: Public constant for compiled library extension.The constant provides a single source of truth for the
.oslibextension and is appropriately named.
100-127: LGTM: Prioritizes compiled libraries over directory-based libraries.The search logic correctly checks for
.oslibfiles in each search directory and the root directory before falling back to directory-based libraries. This allows precompiled libraries to override source libraries when available.
305-357: LGTM: Compiled library loading mirrors existing patterns.The implementation:
- Correctly handles circular dependency detection (lines 308-321) using the same pattern as
LoadLibraryInternal- Uses bilingual error messages consistently (lines 315-317, 340-342)
- Manages library state transitions properly (Discovered → Processed)
- Cleans up on failure by removing from
_libslist (line 354)- Recursively resolves dependencies before marking as processed
src/oscript/ExecuteCompiledBehavior.cs (3)
36-40: LGTM: File existence validation with descriptive error.The check prevents attempting to execute a non-existent bundle and returns an appropriate exit code (2).
58-71: LGTM: Bundle loading with proper validation.The code correctly:
- Uses a
usingstatement for the file stream (line 62)- Validates that the bundle contains an entry point (line 67)
- Returns a clear error message if the entry point is missing
121-134: LGTM: CompiledCodeSource appropriately represents compiled modules.Returning a fixed message from
GetSourceCode()is correct for compiled modules where source code is not available. TheLocationproperty still provides the file path for context and error reporting.src/ScriptEngine/Compiler/Packaged/CompiledPackageDto.cs (3)
16-27: LGTM: Well-designed enum with explicit values.The
PackageTypeenum:
- Uses explicit
bytevalues for serialization stability- Provides clear documentation for Bundle vs Library
- Follows C# naming conventions
53-87: LGTM: Robust DTO design with versioning support.The
CompiledPackageDtoclass demonstrates good practices:
- Magic header (
"OSCP") for file format validation- Version field (
FormatVersion = 1) enables future format evolution- MessagePack
[Key]attributes ensure stable serialization- Collections initialized to empty lists (lines 80, 86) prevent null reference issues
- Clear XML documentation
92-124: LGTM: Well-structured script DTO with proper metadata.The
PackagedScriptDtoclass appropriately captures:
- Script type (Entry/Module/Class)
- Symbolic name for modules and classes
- Owner library information for dependency tracking
- Load order for proper initialization sequence
- The compiled module data
The
[Key]attributes ensure deterministic serialization order.src/ScriptEngine/Compiler/Packaged/LibraryLoader.cs (1)
73-82: Consider adding the module toresult.Modulesregardless ofinstancebeing non-null.The condition
if (instance != null)is always true whensymbolis non-empty (sinceRegisterModulereturnsnullonly whensymbolis empty). However, ifRegisterModulefails silently for another reason in the future, the module won't be tracked. The current logic is correct but tightly coupled toRegisterModule's implementation.src/ScriptEngine/Compiler/Packaged/BundleBuilder.cs (2)
46-53: LGTM - Bundle building initialization.The package initialization correctly sets the type and derives the name from the source location with a sensible fallback to "bundle".
101-120: Solid context traversal for collecting library modules.The iteration over
AttachedContextscorrectly identifiesUserScriptContextInstanceandPropertyBagcontexts. The mapping tocontextSymbolsenables proper serialization of bindings.src/ScriptEngine/Compiler/Packaged/LibraryBuilder.cs (1)
55-64: Good validation for empty library.The check ensures at least one valid script file exists before proceeding, providing a clear error message.
src/ScriptEngine/Compiler/Packaged/BundleLoader.cs (2)
54-83: Loading logic is well-structured.The two-phase loading (library scripts first, then entry) correctly handles dependency ordering. The use of
LoadOrderfor sorting ensures deterministic load sequence.
139-154: Clarify why bundle modules receive isolated processes while library modules share a process.
BundleLoader.RegisterModulecreates a new process for each module, whileLibraryLoader.RegisterModulereuses a single process passed as a parameter. This isolation is architecturally consistent with the different use cases (standalone bundles vs. interdependent libraries), but the design choice lacks explicit documentation. Consider adding a comment explaining the rationale for per-module process isolation in bundles, or verify that the passed-process approach is intentional.src/ScriptEngine/Compiler/Packaged/MethodDto.cs (1)
16-51: MethodDto structure is well-defined.All properties have appropriate MessagePack keys, and collection properties are properly initialized to avoid null references during serialization/deserialization.
src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.cs (4)
400-412: Switch expression usage is appropriate for C# 8.The switch expression for constant type conversion is idiomatic and handles all cases with a default throw for unknown types. This aligns with the coding guidelines allowing C# 8 features.
276-298: Good handling of static bindings with PropertyBag special case.The serialization correctly identifies static bindings and preserves both the context name and member name for
PropertyBagcontexts, enabling proper restoration during deserialization.
449-477: Robust binding restoration with clear error on unresolved context.The deserialization properly handles the lookup and throws a meaningful exception when a context cannot be resolved. The
PropertyBagmember number restoration by name is a nice touch for handling dynamic property indices.
572-588:CompiledCodeSourcecorrectly implementsICodeSourcefor compiled modules.Returning empty string from
GetSourceCode()is appropriate since source code is unavailable for compiled modules. The location is preserved for debugging/error reporting purposes.src/ScriptEngine/Compiler/Packaged/CompiledModuleDto.cs (1)
16-57: Well-structured DTO with proper versioning and defaults.The
CompiledModuleDtofollows MessagePack best practices:
- Magic header and version enable format validation and future compatibility
- All collection properties are initialized to avoid null references
- Keys are sequential and well-organized
| { | ||
| private readonly ScriptingEngine _engine; | ||
| private readonly CompiledModulePackager _packager; | ||
| private readonly Dictionary<string, IAttachableContext> _loadedModules; |
There was a problem hiding this comment.
_loadedModules is populated but never consumed.
The dictionary _loadedModules is populated in LoadLibraryScript (line 99) but is never used for context resolution during entry module loading. The comment on line 87 mentions "расширенный lookup", but ConvertFromDto uses BuildContextLookup from the environment, not from _loadedModules.
Either remove the unused field or integrate it into the lookup mechanism:
#!/bin/bash
# Verify _loadedModules usage
rg -n "_loadedModules" --type=cs src/ScriptEngine/Compiler/Packaged/BundleLoader.csAlso applies to: 93-100
🤖 Prompt for AI Agents
In src/ScriptEngine/Compiler/Packaged/BundleLoader.cs around lines 27 and
87-100, the _loadedModules dictionary is populated in LoadLibraryScript but
never used when loading the entry module; you should either remove the unused
field and its population code, or integrate it into the context lookup used by
ConvertFromDto: modify the entry-module loading path to build a composite
IContextLookup (or equivalent) that queries _loadedModules first and falls back
to Environment.BuildContextLookup(), and pass that composite lookup into
ConvertFromDto/BuildContextLookup so the previously loaded modules are resolved
during entry module conversion; if you keep _loadedModules ensure its population
and lookups are thread-safe and remove any dead code if you choose deletion.
| private void RegisterClass(string symbol, StackRuntimeModule module) | ||
| { | ||
| if (string.IsNullOrEmpty(symbol)) | ||
| return; | ||
|
|
||
| // Регистрируем класс через фабрику | ||
| _engine.AttachedScriptsFactory?.RegisterTypeModule(symbol, module); | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling compared to LibraryLoader.RegisterClass.
LibraryLoader.RegisterClass throws InvalidOperationException if factory is null, while this method silently ignores the condition using ?.. This inconsistency could lead to silent failures during bundle loading.
🔎 Proposed fix for consistent error handling
private void RegisterClass(string symbol, StackRuntimeModule module)
{
if (string.IsNullOrEmpty(symbol))
return;
// Регистрируем класс через фабрику
- _engine.AttachedScriptsFactory?.RegisterTypeModule(symbol, module);
+ var factory = _engine.AttachedScriptsFactory;
+ if (factory == null)
+ {
+ throw new InvalidOperationException($"AttachedScriptsFactory is not initialized. Cannot register class '{symbol}'");
+ }
+
+ factory.RegisterTypeModule(symbol, module);
}📝 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.
| private void RegisterClass(string symbol, StackRuntimeModule module) | |
| { | |
| if (string.IsNullOrEmpty(symbol)) | |
| return; | |
| // Регистрируем класс через фабрику | |
| _engine.AttachedScriptsFactory?.RegisterTypeModule(symbol, module); | |
| } | |
| private void RegisterClass(string symbol, StackRuntimeModule module) | |
| { | |
| if (string.IsNullOrEmpty(symbol)) | |
| return; | |
| // Регистрируем класс через фабрику | |
| var factory = _engine.AttachedScriptsFactory; | |
| if (factory == null) | |
| { | |
| throw new InvalidOperationException($"AttachedScriptsFactory is not initialized. Cannot register class '{symbol}'"); | |
| } | |
| factory.RegisterTypeModule(symbol, module); | |
| } |
🤖 Prompt for AI Agents
In src/ScriptEngine/Compiler/Packaged/BundleLoader.cs around lines 156 to 163,
the method currently uses a null-conditional call on
_engine.AttachedScriptsFactory which silently ignores a missing factory; change
it to obtain the factory into a local variable, throw an
InvalidOperationException with a clear message if the factory is null (matching
LibraryLoader.RegisterClass behavior), and otherwise call
factory.RegisterTypeModule(symbol, module); keep the existing early return for
null or empty symbol.
| // Methods | ||
| foreach (var method in module.Methods.Cast<MachineMethodInfo>()) | ||
| { | ||
| dto.Methods.Add(ConvertMethod(method)); | ||
| } | ||
|
|
||
| // Fields | ||
| foreach (var field in module.Fields.Cast<BslScriptFieldInfo>()) | ||
| { | ||
| dto.Fields.Add(ConvertField(field)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's look at the file structure and context
head -150 src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.cs | tail -50Repository: EvilBeaver/OneScript
Length of output: 184
🏁 Script executed:
#!/bin/bash
# Check what type module is and what Methods/Fields property types are
rg -n "module\.Methods|module\.Fields" src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.cs -B3 -A3Repository: EvilBeaver/OneScript
Length of output: 969
🏁 Script executed:
#!/bin/bash
# Find MachineMethodInfo and BslScriptFieldInfo definitions
fd . src/ -name "*.cs" | xargs rg -l "class MachineMethodInfo|class BslScriptFieldInfo"Repository: EvilBeaver/OneScript
Length of output: 407
🏁 Script executed:
#!/bin/bash
# Check for other implementations of method/field info interfaces
rg "class.*:.*MethodInfo|class.*:.*FieldInfo" src/ -t csRepository: EvilBeaver/OneScript
Length of output: 1290
🏁 Script executed:
#!/bin/bash
# Find the type of 'module' parameter in the ConvertToDto method
rg -n "ConvertToDto|StackRuntimeModule" src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.cs -B5 -A2 | head -40Repository: EvilBeaver/OneScript
Length of output: 1391
🏁 Script executed:
#!/bin/bash
# Look for StackRuntimeModule definition to understand what Methods/Fields collections contain
fd . src/ -name "*.cs" | xargs rg -l "class StackRuntimeModule"Repository: EvilBeaver/OneScript
Length of output: 358
🏁 Script executed:
#!/bin/bash
# Find ConvertMethodFromDto and ConvertFieldFromDto implementations
rg -n "ConvertMethodFromDto|ConvertFieldFromDto" src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.cs -A8Repository: EvilBeaver/OneScript
Length of output: 1500
🏁 Script executed:
#!/bin/bash
# Check StackRuntimeModule definition and its Methods/Fields collections
cat src/ScriptEngine/Machine/StackRuntimeModule.csRepository: EvilBeaver/OneScript
Length of output: 2197
🏁 Script executed:
#!/bin/bash
# Check if there are other ways to create/populate StackRuntimeModule besides ConvertFromDto
rg -n "new StackRuntimeModule|\.Methods\.Add|\.Fields\.Add" src/ScriptEngine/ -t cs | head -50Repository: EvilBeaver/OneScript
Length of output: 1372
🏁 Script executed:
#!/bin/bash
# Check the inheritance hierarchy and if BslNativeMethodInfo is used anywhere
rg -n "BslNativeMethodInfo" src/ -t cs | head -20Repository: EvilBeaver/OneScript
Length of output: 2755
🏁 Script executed:
#!/bin/bash
# Check what type of methodInfo is added to module.Methods in StackMachineCodeGenerator
rg -n "methodInfo|_module.Methods.Add" src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs -B10 | grep -A10 "Methods.Add"Repository: EvilBeaver/OneScript
Length of output: 426
🏁 Script executed:
#!/bin/bash
# Get more context around line 238 in StackMachineCodeGenerator to see what methodInfo is
sed -n '225,245p' src/ScriptEngine/Compiler/StackMachineCodeGenerator.csRepository: EvilBeaver/OneScript
Length of output: 886
🏁 Script executed:
#!/bin/bash
# Find where methodInfo is created/assigned in StackMachineCodeGenerator
sed -n '200,240p' src/ScriptEngine/Compiler/StackMachineCodeGenerator.csRepository: EvilBeaver/OneScript
Length of output: 1546
🏁 Script executed:
#!/bin/bash
# Look for the full context around the methodInfo creation
sed -n '310,335p' src/ScriptEngine/Compiler/StackMachineCodeGenerator.csRepository: EvilBeaver/OneScript
Length of output: 1009
🏁 Script executed:
#!/bin/bash
# Find the NewMethod() method definition
rg -n "NewMethod\(\)|private.*NewMethod" src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs -A5Repository: EvilBeaver/OneScript
Length of output: 1096
🏁 Script executed:
#!/bin/bash
# Look at the beginning of StackMachineCodeGenerator to understand the class hierarchy
head -60 src/ScriptEngine/Compiler/StackMachineCodeGenerator.csRepository: EvilBeaver/OneScript
Length of output: 2416
🏁 Script executed:
#!/bin/bash
# Check ConvertFieldFromDto more completely to see what it returns
sed -n '479,495p' src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.csRepository: EvilBeaver/OneScript
Length of output: 646
🏁 Script executed:
#!/bin/bash
# Check if there are any other places where module.Methods/Fields are populated
rg -n "\.Methods\.Add|\.Fields\.Add" src/ScriptEngine/Compiler/ -t csRepository: EvilBeaver/OneScript
Length of output: 921
🏁 Script executed:
#!/bin/bash
# Verify what field type is created in StackMachineCodeGenerator
sed -n '175,190p' src/ScriptEngine/Compiler/StackMachineCodeGenerator.csRepository: EvilBeaver/OneScript
Length of output: 733
🏁 Script executed:
#!/bin/bash
# Check if BslScriptFieldInfo has subclasses other than itself
rg "class.*:.*BslScriptFieldInfo" src/ -t csRepository: EvilBeaver/OneScript
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Check the full field creation in StackMachineCodeGenerator around line 182
sed -n '160,185p' src/ScriptEngine/Compiler/StackMachineCodeGenerator.csRepository: EvilBeaver/OneScript
Length of output: 1153
🏁 Script executed:
#!/bin/bash
# Look at line 104-110 context more carefully - check ConvertMethod to see what it expects
sed -n '100,120p' src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.csRepository: EvilBeaver/OneScript
Length of output: 717
🏁 Script executed:
#!/bin/bash
# Find BslFieldBuilder definition and its Build method
fd . src/ -name "*.cs" | xargs rg -l "class BslFieldBuilder"Repository: EvilBeaver/OneScript
Length of output: 358
🏁 Script executed:
#!/bin/bash
# Check what BslFieldBuilder.Build() returns
rg -n "class BslFieldBuilder|public.*Build\(\)" src/OneScript.Core/Contexts/BslFieldBuilder.cs -A2Repository: EvilBeaver/OneScript
Length of output: 287
🏁 Script executed:
#!/bin/bash
# Get the full BslScriptFieldInfo Build method
sed -n '66,75p' src/OneScript.Core/Contexts/BslFieldBuilder.csRepository: EvilBeaver/OneScript
Length of output: 403
🏁 Script executed:
#!/bin/bash
# Check the IList<BslScriptFieldInfo> on StackRuntimeModule more carefully - what's actually in it?
rg -n "class BslScriptFieldInfo" src/ -t csRepository: EvilBeaver/OneScript
Length of output: 185
🏁 Script executed:
#!/bin/bash
# Check the full context of ConvertToDto - is this handling potentially untrusted serialized data?
sed -n '80,115p' src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.csRepository: EvilBeaver/OneScript
Length of output: 1178
🏁 Script executed:
#!/bin/bash
# Check if there are any tests that might show if casts ever fail or if the assumption is guaranteed
fd . src/ -name "*Test*.cs" | xargs rg -l "CompiledModulePackager|Cast.*Method" | head -5Repository: EvilBeaver/OneScript
Length of output: 559
🏁 Script executed:
#!/bin/bash
# Check the ConvertMethod signature - what does it expect and do?
sed -n '430,460p' src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.csRepository: EvilBeaver/OneScript
Length of output: 1305
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or comments explaining why the cast is necessary
sed -n '95,125p' src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.csRepository: EvilBeaver/OneScript
Length of output: 1026
🏁 Script executed:
#!/bin/bash
# Double-check what ConvertMethod expects as parameter
rg -n "private.*ConvertMethod" src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.cs -A3Repository: EvilBeaver/OneScript
Length of output: 486
🏁 Script executed:
#!/bin/bash
# Final verification: confirm ConvertField signature to understand line 110
rg -n "private.*ConvertField" src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.cs -A3Repository: EvilBeaver/OneScript
Length of output: 434
Line 110's cast to BslScriptFieldInfo is redundant.
The cast on line 110 (module.Fields.Cast<BslScriptFieldInfo>()) is unnecessary because module.Fields is already typed as IList<BslScriptFieldInfo>. This is a self-cast that has no effect.
Line 104's cast to MachineMethodInfo is safe in this codebase—module.Methods only ever contains MachineMethodInfo instances (created via StackMachineCodeGenerator.NewMethod() or ConvertMethodFromDto()). However, the cast could be removed since ConvertMethod expects MachineMethodInfo and the collection is guaranteed to contain only that type.
🤖 Prompt for AI Agents
In src/ScriptEngine/Compiler/Packaged/CompiledModulePackager.cs around lines
103-113, remove the redundant LINQ casts: drop the .Cast<BslScriptFieldInfo>()
on line 110 and iterate directly over module.Fields, and likewise remove the
unnecessary .Cast<MachineMethodInfo>() on line 104 (iterate directly over
module.Methods) so the foreach loops use the concrete element types
(MachineMethodInfo and BslScriptFieldInfo) expected by
ConvertMethod/ConvertField.
| // Проверяем папку Классы | ||
| var classesPath = Path.Combine(libraryPath, "Классы"); | ||
| if (Directory.Exists(classesPath)) | ||
| { | ||
| CompileClasses(classesPath, libraryName, package, process); | ||
| } | ||
|
|
||
| // Альтернативное имя папки | ||
| classesPath = Path.Combine(libraryPath, "Classes"); | ||
| if (Directory.Exists(classesPath)) | ||
| { | ||
| CompileClasses(classesPath, libraryName, package, process); | ||
| } |
There was a problem hiding this comment.
Potential duplicate class registration if both "Классы" and "Classes" folders exist.
If a library has both folders with overlapping file names, the same class could be added twice to the package. Consider tracking processed class names or merging the folder contents before processing.
🔎 Proposed fix
// Проверяем папку Классы
var classesPath = Path.Combine(libraryPath, "Классы");
+ var processedClasses = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
if (Directory.Exists(classesPath))
{
- CompileClasses(classesPath, libraryName, package, process);
+ CompileClasses(classesPath, libraryName, package, process, processedClasses);
}
// Альтернативное имя папки
classesPath = Path.Combine(libraryPath, "Classes");
if (Directory.Exists(classesPath))
{
- CompileClasses(classesPath, libraryName, package, process);
+ CompileClasses(classesPath, libraryName, package, process, processedClasses);
}Then update CompileClasses to skip already-processed class names.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/ScriptEngine/Compiler/Packaged/LibraryBuilder.cs around lines 96 to 108,
calling CompileClasses twice for "Классы" and "Classes" can register the same
class twice; change the flow to avoid duplicate registration by maintaining a
HashSet (or similar) of processed class names/paths before calling
CompileClasses (or by merging the two directories into a single enumeration),
and update CompileClasses to accept that set (or check it internally) and skip
any class already processed (optionally emit a warning log when a duplicate is
skipped) so identical class files from both folders are not added twice.
| private LoadedLibrary LoadPackage(CompiledPackageDto package, IBslProcess process) | ||
| { | ||
| ValidatePackage(package); | ||
|
|
||
| var result = new LoadedLibrary | ||
| { | ||
| Name = package.Name, | ||
| Dependencies = package.Dependencies.ToList() | ||
| }; |
There was a problem hiding this comment.
Potential NullReferenceException if Dependencies is null.
If package.Dependencies is null (e.g., due to deserialization of older/malformed data), calling ToList() will throw. Consider defensive handling.
🔎 Proposed fix
var result = new LoadedLibrary
{
Name = package.Name,
- Dependencies = package.Dependencies.ToList()
+ Dependencies = package.Dependencies?.ToList() ?? new List<string>()
};🤖 Prompt for AI Agents
In src/ScriptEngine/Compiler/Packaged/LibraryLoader.cs around lines 56 to 64,
the code calls package.Dependencies.ToList() which can throw a
NullReferenceException if Dependencies is null; change this to defensively
handle null by creating an empty list when Dependencies is null (e.g. use the
null-coalescing or null-conditional pattern to produce a new List<string>() when
package.Dependencies is null) so LoadedLibrary.Dependencies always gets a safe
list instance.
| [MessagePackObject] | ||
| public class ParameterDto | ||
| { | ||
| [Key(0)] | ||
| public string Name { get; set; } | ||
|
|
||
| [Key(1)] | ||
| public bool IsByValue { get; set; } | ||
|
|
||
| [Key(2)] | ||
| public bool HasDefaultValue { get; set; } | ||
|
|
||
| [Key(3)] | ||
| public int DefaultValueIndex { get; set; } | ||
|
|
||
| [Key(4)] | ||
| public List<AnnotationDto> Annotations { get; set; } | ||
| } |
There was a problem hiding this comment.
Inconsistent default initialization for ParameterDto.Annotations.
MethodDto initializes Annotations to new List<AnnotationDto>() (line 50), but ParameterDto.Annotations (line 72) lacks a default initializer. This inconsistency could cause NullReferenceException when accessing Annotations on a deserialized ParameterDto without annotations.
Based on CompiledModulePackager.ConvertParameter (lines 218-221 in relevant snippets), annotations are only set when param.Annotations != null, meaning deserialized DTOs may have null annotations.
🔎 Proposed fix
[Key(4)]
- public List<AnnotationDto> Annotations { get; set; }
+ public List<AnnotationDto> Annotations { get; set; } = new List<AnnotationDto>();📝 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.
| [MessagePackObject] | |
| public class ParameterDto | |
| { | |
| [Key(0)] | |
| public string Name { get; set; } | |
| [Key(1)] | |
| public bool IsByValue { get; set; } | |
| [Key(2)] | |
| public bool HasDefaultValue { get; set; } | |
| [Key(3)] | |
| public int DefaultValueIndex { get; set; } | |
| [Key(4)] | |
| public List<AnnotationDto> Annotations { get; set; } | |
| } | |
| [MessagePackObject] | |
| public class ParameterDto | |
| { | |
| [Key(0)] | |
| public string Name { get; set; } | |
| [Key(1)] | |
| public bool IsByValue { get; set; } | |
| [Key(2)] | |
| public bool HasDefaultValue { get; set; } | |
| [Key(3)] | |
| public int DefaultValueIndex { get; set; } | |
| [Key(4)] | |
| public List<AnnotationDto> Annotations { get; set; } = new List<AnnotationDto>(); | |
| } |
🤖 Prompt for AI Agents
In src/ScriptEngine/Compiler/Packaged/MethodDto.cs around lines 56 to 73,
ParameterDto.Annotations is not default-initialized (unlike MethodDto), which
can lead to null references after deserialization; fix this by giving the
Annotations property a default value (initialize it to a new
List<AnnotationDto>()), keeping the MessagePack attributes intact so
deserialized instances always have a non-null annotations list.
1 participant
Опять прекомпиляция скриптов. Хочу собрать для тестирования
Summary by CodeRabbit
.oscformat via-buildcommand.oslibformat via-buildlibcommand.oscfiles).oslibfiles) can be loaded as dependencies✏️ Tip: You can customize this high-level summary in your review settings.